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

2017-12-25 Thread Wu, Jiaxin 
Thanks to catch that.

Best Regards!
Jiaxin

> -Original Message-
> From: Gary Lin [mailto:g...@suse.com]
> Sent: Tuesday, December 26, 2017 9:56 AM
> To: Wu, Jiaxin <jiaxin...@intel.com>
> Cc: edk2-devel@lists.01.org; Ye, Ting <ting...@intel.com>; Wang, Fan
> <fan.w...@intel.com>; Fu, Siyuan <siyuan...@intel.com>
> Subject: Re: [edk2] [Patch 1/5] MdeModulePkg/DxeHttpLib: Add boundary
> condition check.
> 
> On Tue, Dec 26, 2017 at 09:33:45AM +0800, Jiaxin Wu wrote:
> > This patch is to add the boundary condition check to make sure
> > the accessed buffer is valid.
> >
> > Cc: Ye Ting <ting...@intel.com>
> > Cc: Fu Siyuan <siyuan...@intel.com>
> > Cc: Wang Fan <fan.w...@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Wu Jiaxin <jiaxin...@intel.com>
> > ---
> >  MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 39
> 
> >  1 file changed, 34 insertions(+), 5 deletions(-)
> >
> > diff --git a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> > index caddbb7..4d353d7 100644
> > --- a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> > +++ b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> > @@ -33,11 +33,10 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF
> ANY KIND, EITHER EXPRESS OR IMPLIED.
> >@retval EFI_SUCCESSSuccessfully decoded the URI.
> >@retval EFI_INVALID_PARAMETER  Buffer is not a valid percent-encoded
> string.
> >
> >  **/
> >  EFI_STATUS
> > -EFIAPI
> >  UriPercentDecode (
> >IN  CHAR8*Buffer,
> >IN  UINT32BufferLength,
> >   OUT  CHAR8*ResultBuffer,
> >   OUT  UINT32   *ResultLength
> This change will break gcc build since EFIAPI is still declared in
> HttpLib.h.
> 
> Gary Lin
> 
> > @@ -54,11 +53,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 +1555,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 +1593,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.
> > +  //
> > +  if ((UINTN)EndofHeader - (UINTN)(FieldValueStr) < 1) {
> > +return NULL;
> > +  }
> > +
> >FieldValueStr ++;
> > -} else if (*FieldValueStr == '\r' && *(FieldValueStr + 1) == '\n' &&
> > -   (*(FieldValueStr + 2) == ' ' || *(FieldValueStr + 2) == 
> > '\t')) {
> > -  FieldValueStr = FieldValueStr + 3;
> > +} else if (*Field

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

2017-12-25 Thread Gary Lin 
On Tue, Dec 26, 2017 at 09:33:45AM +0800, Jiaxin Wu wrote:
> This patch is to add the boundary condition check to make sure
> the accessed buffer is valid.
> 
> Cc: Ye Ting 
> Cc: Fu Siyuan 
> Cc: Wang Fan 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Wu Jiaxin 
> ---
>  MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 39 
> 
>  1 file changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c 
> b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> index caddbb7..4d353d7 100644
> --- a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> +++ b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> @@ -33,11 +33,10 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
> EXPRESS OR IMPLIED.
>@retval EFI_SUCCESSSuccessfully decoded the URI.
>@retval EFI_INVALID_PARAMETER  Buffer is not a valid percent-encoded 
> string.
>
>  **/
>  EFI_STATUS
> -EFIAPI
>  UriPercentDecode (
>IN  CHAR8*Buffer,
>IN  UINT32BufferLength,
>   OUT  CHAR8*ResultBuffer,
>   OUT  UINT32   *ResultLength
This change will break gcc build since EFIAPI is still declared in
HttpLib.h.

Gary Lin

> @@ -54,11 +53,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 +1555,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 +1593,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. 
> +  //
> +  if ((UINTN)EndofHeader - (UINTN)(FieldValueStr) < 1) {
> +return NULL;  
> +  }
> +  
>FieldValueStr ++;
> -} else if (*FieldValueStr == '\r' && *(FieldValueStr + 1) == '\n' &&
> -   (*(FieldValueStr + 2) == ' ' || *(FieldValueStr + 2) == 
> '\t')) {
> -  FieldValueStr = FieldValueStr + 3;
> +} else if (*FieldValueStr == '\r') {
> +  //
> +  // Boundary condition check. 
> +  //
> +  if ((UINTN)EndofHeader - (UINTN)(FieldValueStr) < 3) {
> +return NULL;  
> +  }
> +
> +  if (*(FieldValueStr + 1) == '\n' && (*(FieldValueStr + 2) == ' ' || 
> *(FieldValueStr + 2) == '\t')) {
> +FieldValueStr = FieldValueStr + 3;
> +  }
>  } else {
>break;
>  }
>}
>  
> -- 
> 1.9.5.msysgit.1
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2017-12-25 Thread Jiaxin Wu 
This patch is to add the boundary condition check to make sure
the accessed buffer is valid.

Cc: Ye Ting 
Cc: Fu Siyuan 
Cc: Wang Fan 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Wu Jiaxin 
---
 MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 39 
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c 
b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
index caddbb7..4d353d7 100644
--- a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
+++ b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
@@ -33,11 +33,10 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
   @retval EFI_SUCCESSSuccessfully decoded the URI.
   @retval EFI_INVALID_PARAMETER  Buffer is not a valid percent-encoded string.
   
 **/
 EFI_STATUS
-EFIAPI
 UriPercentDecode (
   IN  CHAR8*Buffer,
   IN  UINT32BufferLength,
  OUT  CHAR8*ResultBuffer,
  OUT  UINT32   *ResultLength
@@ -54,11 +53,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 +1555,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 +1593,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. 
+  //
+  if ((UINTN)EndofHeader - (UINTN)(FieldValueStr) < 1) {
+return NULL;  
+  }
+  
   FieldValueStr ++;
-} else if (*FieldValueStr == '\r' && *(FieldValueStr + 1) == '\n' &&
-   (*(FieldValueStr + 2) == ' ' || *(FieldValueStr + 2) == '\t')) {
-  FieldValueStr = FieldValueStr + 3;
+} else if (*FieldValueStr == '\r') {
+  //
+  // Boundary condition check. 
+  //
+  if ((UINTN)EndofHeader - (UINTN)(FieldValueStr) < 3) {
+return NULL;  
+  }
+
+  if (*(FieldValueStr + 1) == '\n' && (*(FieldValueStr + 2) == ' ' || 
*(FieldValueStr + 2) == '\t')) {
+FieldValueStr = FieldValueStr + 3;
+  }
 } else {
   break;
 }
   }
 
-- 
1.9.5.msysgit.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel