Re: [Patch 1/5 v2] MdeModulePkg/DxeHttpLib: Add boundary condition check.

[Gary Lin <glin@suse.com>]

On Tue, Dec 26, 2017 at 10:36:47AM +0800, Jiaxin Wu wrote:
> v2:
> * Fix GCC the build error.
> 
> This patch is to add the boundary condition check to make sure
> the accessed buffer is valid.
> 
> Cc: Gary Lin <glin@suse.com>
> Cc: Ye Ting <ting.ye@intel.com>
> Cc: Fu Siyuan <siyuan.fu@intel.com>
> Cc: Wang Fan <fan.wang@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
> ---
>  MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 38 +++++++++++++++++++++++++---
>  1 file changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> index caddbb7..915b81d 100644
> --- a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> +++ b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> @@ -54,11 +54,11 @@ UriPercentDecode (
>    Index = 0;
>    Offset = 0;
>    HexStr[2] = '\0';
>    while (Index < BufferLength) {
>      if (Buffer[Index] == '%') {
> -      if (!NET_IS_HEX_CHAR (Buffer[Index+1]) || !NET_IS_HEX_CHAR (Buffer[Index+2])) {
> +      if (Index + 1 >= BufferLength || Index + 2 >= BufferLength || !NET_IS_HEX_CHAR (Buffer[Index+1]) || !NET_IS_HEX_CHAR (Buffer[Index+2])) {
>          return EFI_INVALID_PARAMETER;
>        }
>        HexStr[0] = Buffer[Index+1];
>        HexStr[1] = Buffer[Index+2];
>        ResultBuffer[Offset] = (CHAR8) AsciiStrHexToUintn (HexStr);
> @@ -1556,20 +1556,31 @@ HttpGetFieldNameAndValue (
>    )
>  {
>    CHAR8  *FieldNameStr;
>    CHAR8  *FieldValueStr;
>    CHAR8  *StrPtr;
> +  CHAR8  *EndofHeader;
>  
>    if (String == NULL || FieldName == NULL || FieldValue == NULL) {
>      return NULL;
>    }
>  
>    *FieldName    = NULL;
>    *FieldValue   = NULL;
>    FieldNameStr  = NULL;
>    FieldValueStr = NULL;
>    StrPtr        = NULL;
> +  EndofHeader   = NULL;
> +
> +
> +  //
> +  // Check whether the raw HTTP header string is valid or not.
> +  //
> +  EndofHeader = AsciiStrStr (String, "\r\n\r\n");
> +  if (EndofHeader == NULL) {
> +    return NULL;
> +  }
>  
>    //
>    // Each header field consists of a name followed by a colon (":") and the field value.
>    //
>    FieldNameStr = String;
> @@ -1583,17 +1594,36 @@ HttpGetFieldNameAndValue (
>    //
>    *(FieldValueStr - 1) = 0;
>  
>    //
>    // The field value MAY be preceded by any amount of LWS, though a single SP is preferred.
> +  // Note: LWS  = [CRLF] 1*(SP|HT), it can be '\r\n ' or '\r\n\t' or ' ' or '\t'.
> +  //       CRLF = '\r\n'.
> +  //       SP   = ' '.
> +  //       HT   = '\t' (Tab).
>    //
>    while (TRUE) {
>      if (*FieldValueStr == ' ' || *FieldValueStr == '\t') {
> +      //
> +      // Boundary condition check. 
Trailing whitespaces (nitpicking :-)

> +      //
> +      if ((UINTN)EndofHeader - (UINTN)(FieldValueStr) < 1) {
> +        return NULL;  
ditto

> +      }
> +      
ditto

>        FieldValueStr ++;
> -    } else if (*FieldValueStr == '\r' && *(FieldValueStr + 1) == '\n' &&
> -               (*(FieldValueStr + 2) == ' ' || *(FieldValueStr + 2) == '\t')) {
> -      FieldValueStr = FieldValueStr + 3;
> +    } else if (*FieldValueStr == '\r') {
> +      //
> +      // Boundary condition check. 
ditto

> +      //
> +      if ((UINTN)EndofHeader - (UINTN)(FieldValueStr) < 3) {
> +        return NULL;  
ditto

> +      }
> +
> +      if (*(FieldValueStr + 1) == '\n' && (*(FieldValueStr + 2) == ' ' || *(FieldValueStr + 2) == '\t')) {
> +        FieldValueStr = FieldValueStr + 3;
> +      }
>      } else {
>        break;
>      }
>    }
>  
This patch fixes the gcc build.

Tested-by: Gary Lin <glin@suse.com>

Gary Lin

> -- 
> 1.9.5.msysgit.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>