public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [Patch 0/5] MdeModulePkg/DxeHttpLib: Fix series issues in DxeHttpLib.
@ 2017-12-26  1:33 Jiaxin Wu
  2017-12-26  1:33 ` [Patch 1/5] MdeModulePkg/DxeHttpLib: Add boundary condition check Jiaxin Wu
                   ` (5 more replies)
  0 siblings, 6 replies; 9+ messages in thread
From: Jiaxin Wu @ 2017-12-26  1:33 UTC (permalink / raw)
  To: edk2-devel; +Cc: Ye Ting, Fu Siyuan, Wang Fan, Wu Jiaxin

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>

Jiaxin Wu (5):
  MdeModulePkg/DxeHttpLib: Add boundary condition check.
  MdeModulePkg/DxeHttpLib: Avoid the potential memory leak when error
    happen.
  MdeModulePkg/DxeHttpLib: Check the input parameters for some APIs.
  MdeModulePkg/DxeHttpLib: Correct some return Status.
  MdeModulePkg/DxeHttpLib: Refine some coding style.

 MdeModulePkg/Include/Library/HttpLib.h       |  10 +-
 MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 190 +++++++++++++++++----------
 2 files changed, 125 insertions(+), 75 deletions(-)

-- 
1.9.5.msysgit.1



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Patch 1/5] MdeModulePkg/DxeHttpLib: Add boundary condition check.
  2017-12-26  1:33 [Patch 0/5] MdeModulePkg/DxeHttpLib: Fix series issues in DxeHttpLib Jiaxin Wu
@ 2017-12-26  1:33 ` Jiaxin Wu
  2017-12-26  1:56   ` Gary Lin
  2017-12-26  1:33 ` [Patch 2/5] MdeModulePkg/DxeHttpLib: Avoid the potential memory leak when error happen Jiaxin Wu
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 9+ messages in thread
From: Jiaxin Wu @ 2017-12-26  1:33 UTC (permalink / raw)
  To: edk2-devel; +Cc: Ye Ting, Fu Siyuan, Wang Fan, Wu Jiaxin

This patch is to add the boundary condition check to make sure
the accessed buffer is valid.

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 | 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_SUCCESS            Successfully decoded the URI.
   @retval EFI_INVALID_PARAMETER  Buffer is not a valid percent-encoded string.
   
 **/
 EFI_STATUS
-EFIAPI
 UriPercentDecode (
   IN      CHAR8            *Buffer,
   IN      UINT32            BufferLength,
      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



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Patch 2/5] MdeModulePkg/DxeHttpLib: Avoid the potential memory leak when error happen.
  2017-12-26  1:33 [Patch 0/5] MdeModulePkg/DxeHttpLib: Fix series issues in DxeHttpLib Jiaxin Wu
  2017-12-26  1:33 ` [Patch 1/5] MdeModulePkg/DxeHttpLib: Add boundary condition check Jiaxin Wu
@ 2017-12-26  1:33 ` Jiaxin Wu
  2017-12-26  1:33 ` [Patch 3/5] MdeModulePkg/DxeHttpLib: Check the input parameters for some APIs Jiaxin Wu
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 9+ messages in thread
From: Jiaxin Wu @ 2017-12-26  1:33 UTC (permalink / raw)
  To: edk2-devel; +Cc: Ye Ting, Fu Siyuan, Wang Fan, Wu Jiaxin

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 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
index 4d353d7..27b94e3 100644
--- a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
+++ b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
@@ -369,10 +369,12 @@ HttpParseUrl (
   UINT32                Field;
   UINT32                OldField;
   BOOLEAN               FoundAt;
   EFI_STATUS            Status;
   HTTP_URL_PARSER       *Parser;
+
+  Parser = NULL;
   
   if (Url == NULL || Length == 0 || UrlParser == NULL) {
     return EFI_INVALID_PARAMETER;
   }
 
@@ -399,10 +401,11 @@ HttpParseUrl (
     //
     State = NetHttpParseUrlChar (*Char, State);
 
     switch (State) {
     case UrlParserStateMax:
+      FreePool (Parser);
       return EFI_INVALID_PARAMETER;
       
     case UrlParserSchemeColon:
     case UrlParserSchemeColonSlash:
     case UrlParserSchemeColonSlashSlash:
@@ -461,10 +464,11 @@ HttpParseUrl (
   // If has authority component, continue to parse the username, host and port.
   //
   if ((Parser->FieldBitMap & BIT (HTTP_URI_FIELD_AUTHORITY)) != 0) {
     Status = NetHttpParseAuthority (Url, FoundAt, Parser);
     if (EFI_ERROR (Status)) {
+      FreePool (Parser);
       return Status;
     }
   }
 
   *UrlParser = Parser;
@@ -1525,10 +1529,11 @@ HttpSetFieldNameAndValue (
   HttpHeader->FieldName[FieldNameSize - 1] = 0;
 
   FieldValueSize = AsciiStrSize (FieldValue);
   HttpHeader->FieldValue = AllocateZeroPool (FieldValueSize);
   if (HttpHeader->FieldValue == NULL) {
+    FreePool (HttpHeader->FieldName);
     return EFI_OUT_OF_RESOURCES;
   }
   CopyMem (HttpHeader->FieldValue, FieldValue, FieldValueSize);
   HttpHeader->FieldValue[FieldValueSize - 1] = 0;
 
-- 
1.9.5.msysgit.1



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Patch 3/5] MdeModulePkg/DxeHttpLib: Check the input parameters for some APIs.
  2017-12-26  1:33 [Patch 0/5] MdeModulePkg/DxeHttpLib: Fix series issues in DxeHttpLib Jiaxin Wu
  2017-12-26  1:33 ` [Patch 1/5] MdeModulePkg/DxeHttpLib: Add boundary condition check Jiaxin Wu
  2017-12-26  1:33 ` [Patch 2/5] MdeModulePkg/DxeHttpLib: Avoid the potential memory leak when error happen Jiaxin Wu
@ 2017-12-26  1:33 ` Jiaxin Wu
  2017-12-26  1:33 ` [Patch 4/5] MdeModulePkg/DxeHttpLib: Correct some return Status Jiaxin Wu
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 9+ messages in thread
From: Jiaxin Wu @ 2017-12-26  1:33 UTC (permalink / raw)
  To: edk2-devel; +Cc: Ye Ting, Fu Siyuan, Wang Fan, Wu Jiaxin

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/Include/Library/HttpLib.h       |  1 +
 MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 25 ++++++++++++++++++++-----
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Include/Library/HttpLib.h b/MdeModulePkg/Include/Library/HttpLib.h
index 8539820..88b56ae 100644
--- a/MdeModulePkg/Include/Library/HttpLib.h
+++ b/MdeModulePkg/Include/Library/HttpLib.h
@@ -370,10 +370,11 @@ HttpFindHeader (
   @param[in]      FieldName           FieldName of this HttpHeader, a NULL terminated ASCII string.
   @param[in]      FieldValue          FieldValue of this HttpHeader, a NULL terminated ASCII string.
 
 
   @retval EFI_SUCCESS             The FieldName and FieldValue are set into HttpHeader successfully.
+  @retval EFI_INVALID_PARAMETER   The parameter is invalid.
   @retval EFI_OUT_OF_RESOURCES    Failed to allocate resources.
 
 **/
 EFI_STATUS
 EFIAPI
diff --git a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
index 27b94e3..38ded5d 100644
--- a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
+++ b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
@@ -1396,10 +1396,14 @@ HttpIsMessageComplete (
   IN VOID              *MsgParser
   )
 {
   HTTP_BODY_PARSER      *Parser;
 
+  if (MsgParser == NULL) {
+    return FALSE;
+  }
+
   Parser = (HTTP_BODY_PARSER*) MsgParser;
 
   if (Parser->State == BodyParserComplete) {
     return TRUE;
   }
@@ -1497,10 +1501,11 @@ AsciiStrGetNextToken (
   @param[in]  FieldName           FieldName of this HttpHeader, a NULL terminated ASCII string.
   @param[in]  FieldValue          FieldValue of this HttpHeader, a NULL terminated ASCII string.
 
 
   @retval EFI_SUCCESS             The FieldName and FieldValue are set into HttpHeader successfully.
+  @retval EFI_INVALID_PARAMETER   The parameter is invalid.
   @retval EFI_OUT_OF_RESOURCES    Failed to allocate resources.
 
 **/
 EFI_STATUS
 EFIAPI
@@ -1511,10 +1516,14 @@ HttpSetFieldNameAndValue (
   )
 {
   UINTN                       FieldNameSize;
   UINTN                       FieldValueSize;
 
+  if (HttpHeader == NULL || FieldName == NULL || FieldValue == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
   if (HttpHeader->FieldName != NULL) {
     FreePool (HttpHeader->FieldName);
   }
   if (HttpHeader->FieldValue != NULL) {
     FreePool (HttpHeader->FieldValue);
@@ -1728,14 +1737,10 @@ HttpGenRequestMessage (
   VOID                             *HttpHdr;
   EFI_HTTP_HEADER                  **AppendList;
   UINTN                            Index;
   EFI_HTTP_UTILITIES_PROTOCOL      *HttpUtilitiesProtocol;
 
-
-  ASSERT (Message != NULL);
-
-  *RequestMsg           = NULL;
   Status                = EFI_SUCCESS;
   HttpHdrSize           = 0;
   MsgSize               = 0;
   Success               = FALSE;
   HttpHdr               = NULL;
@@ -1746,11 +1751,12 @@ HttpGenRequestMessage (
   // 1. If we have a Request, we cannot have a NULL Url
   // 2. If we have a Request, HeaderCount can not be non-zero
   // 3. If we do not have a Request, HeaderCount should be zero
   // 4. If we do not have Request and Headers, we need at least a message-body
   //
-  if ((Message->Data.Request != NULL && Url == NULL) ||
+  if ((Message == NULL || RequestMsg == NULL || RequestMsgSize == NULL) || 
+      (Message->Data.Request != NULL && Url == NULL) ||
       (Message->Data.Request != NULL && Message->HeaderCount == 0) ||
       (Message->Data.Request == NULL && Message->HeaderCount != 0) ||
       (Message->Data.Request == NULL && Message->HeaderCount == 0 && Message->BodyLength == 0)) {
     return EFI_INVALID_PARAMETER;
   }
@@ -1827,10 +1833,11 @@ HttpGenRequestMessage (
   MsgSize += Message->BodyLength;
 
   //
   // memory for the string that needs to be sent to TCP
   //
+  *RequestMsg           = NULL;
   *RequestMsg = AllocateZeroPool (MsgSize);
   if (*RequestMsg == NULL) {
     Status = EFI_OUT_OF_RESOURCES;
     goto Exit;
   }
@@ -2052,11 +2059,19 @@ HttpIsValidHttpHeader (
   IN  CHAR8            *FieldName
   )
 {
   UINTN                       Index;
 
+  if (FieldName == NULL) {
+    return FALSE;
+  }
+
   for (Index = 0; Index < DeleteCount; Index++) {
+    if (DeleteList[Index] == NULL) {
+      continue;
+    }
+    
     if (AsciiStrCmp (FieldName, DeleteList[Index]) == 0) {
       return FALSE;
     }
   }
 
-- 
1.9.5.msysgit.1



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Patch 4/5] MdeModulePkg/DxeHttpLib: Correct some return Status.
  2017-12-26  1:33 [Patch 0/5] MdeModulePkg/DxeHttpLib: Fix series issues in DxeHttpLib Jiaxin Wu
                   ` (2 preceding siblings ...)
  2017-12-26  1:33 ` [Patch 3/5] MdeModulePkg/DxeHttpLib: Check the input parameters for some APIs Jiaxin Wu
@ 2017-12-26  1:33 ` Jiaxin Wu
  2017-12-26  1:33 ` [Patch 5/5] MdeModulePkg/DxeHttpLib: Refine some coding style Jiaxin Wu
  2017-12-26  1:40 ` [Patch 0/5] MdeModulePkg/DxeHttpLib: Fix series issues in DxeHttpLib Fu, Siyuan
  5 siblings, 0 replies; 9+ messages in thread
From: Jiaxin Wu @ 2017-12-26  1:33 UTC (permalink / raw)
  To: edk2-devel; +Cc: Ye Ting, Fu Siyuan, Wang Fan, Wu Jiaxin

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/Include/Library/HttpLib.h       |  5 +++--
 MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 11 ++++++-----
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/MdeModulePkg/Include/Library/HttpLib.h b/MdeModulePkg/Include/Library/HttpLib.h
index 88b56ae..285a831 100644
--- a/MdeModulePkg/Include/Library/HttpLib.h
+++ b/MdeModulePkg/Include/Library/HttpLib.h
@@ -284,12 +284,13 @@ HttpInitMsgParser (
   @param[in]         BodyLength           Length in bytes of the Body.
   @param[in]         Body                 Pointer to the buffer of the message-body to be parsed.
 
   @retval EFI_SUCCESS                Successfully parse the message-body.
   @retval EFI_INVALID_PARAMETER      MsgParser is NULL or Body is NULL or BodyLength is 0.
-  @retval Others                     Operation aborted.
-
+  @retval EFI_ABORTED                Operation aborted.
+  @retval Other                      Error happened while parsing message body.
+  
 **/
 EFI_STATUS
 EFIAPI
 HttpParseMessageBody (
   IN OUT VOID              *MsgParser,
diff --git a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
index 38ded5d..327ca9e 100644
--- a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
+++ b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
@@ -152,11 +152,11 @@ NetHttpParseAuthorityChar (
   @param[in]       Url            The pointer to a HTTP URL string.
   @param[in]       FoundAt        TRUE if there is an at sign ('@') in the authority component.
   @param[in, out]  UrlParser      Pointer to the buffer of the parse result.
 
   @retval EFI_SUCCESS             Successfully parse the authority.
-  @retval Other                   Error happened.
+  @retval EFI_INVALID_PARAMETER   The Url is invalid to parse the authority component.
 
 **/
 EFI_STATUS
 NetHttpParseAuthority (
   IN      CHAR8              *Url,
@@ -569,11 +569,11 @@ HttpUrlGetIp4 (
   }
 
   Parser = (HTTP_URL_PARSER*) UrlParser;
 
   if ((Parser->FieldBitMap & BIT (HTTP_URI_FIELD_HOST)) == 0) {
-    return EFI_INVALID_PARAMETER;
+    return EFI_NOT_FOUND;
   }
 
   Ip4String = AllocatePool (Parser->FieldData[HTTP_URI_FIELD_HOST].Length + 1);
   if (Ip4String == NULL) {
     return EFI_OUT_OF_RESOURCES;
@@ -632,11 +632,11 @@ HttpUrlGetIp6 (
   }
 
   Parser = (HTTP_URL_PARSER*) UrlParser;
 
   if ((Parser->FieldBitMap & BIT (HTTP_URI_FIELD_HOST)) == 0) {
-    return EFI_INVALID_PARAMETER;
+    return EFI_NOT_FOUND;
   }
 
   //
   // IP-literal = "[" ( IPv6address / IPvFuture  ) "]"
   //
@@ -711,11 +711,11 @@ HttpUrlGetPort (
   Index = 0;
 
   Parser = (HTTP_URL_PARSER*) UrlParser;
 
   if ((Parser->FieldBitMap & BIT (HTTP_URI_FIELD_PORT)) == 0) {
-    return EFI_INVALID_PARAMETER;
+    return EFI_NOT_FOUND;
   }
 
   PortString = AllocatePool (Parser->FieldData[HTTP_URI_FIELD_PORT].Length + 1);
   if (PortString == NULL) {
     return EFI_OUT_OF_RESOURCES;
@@ -1130,11 +1130,12 @@ HttpInitMsgParser (
   @param[in]         BodyLength           Length in bytes of the Body.
   @param[in]         Body                 Pointer to the buffer of the message-body to be parsed.
 
   @retval EFI_SUCCESS                Successfully parse the message-body.
   @retval EFI_INVALID_PARAMETER      MsgParser is NULL or Body is NULL or BodyLength is 0.
-  @retval Others                     Operation aborted.
+  @retval EFI_ABORTED                Operation aborted.
+  @retval Other                      Error happened while parsing message body.
 
 **/
 EFI_STATUS
 EFIAPI
 HttpParseMessageBody (
-- 
1.9.5.msysgit.1



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Patch 5/5] MdeModulePkg/DxeHttpLib: Refine some coding style.
  2017-12-26  1:33 [Patch 0/5] MdeModulePkg/DxeHttpLib: Fix series issues in DxeHttpLib Jiaxin Wu
                   ` (3 preceding siblings ...)
  2017-12-26  1:33 ` [Patch 4/5] MdeModulePkg/DxeHttpLib: Correct some return Status Jiaxin Wu
@ 2017-12-26  1:33 ` Jiaxin Wu
  2017-12-26  1:40 ` [Patch 0/5] MdeModulePkg/DxeHttpLib: Fix series issues in DxeHttpLib Fu, Siyuan
  5 siblings, 0 replies; 9+ messages in thread
From: Jiaxin Wu @ 2017-12-26  1:33 UTC (permalink / raw)
  To: edk2-devel; +Cc: Ye Ting, Fu Siyuan, Wang Fan, Wu Jiaxin

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/Include/Library/HttpLib.h       |   4 +-
 MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 118 +++++++++++++--------------
 2 files changed, 60 insertions(+), 62 deletions(-)

diff --git a/MdeModulePkg/Include/Library/HttpLib.h b/MdeModulePkg/Include/Library/HttpLib.h
index 285a831..9975aea 100644
--- a/MdeModulePkg/Include/Library/HttpLib.h
+++ b/MdeModulePkg/Include/Library/HttpLib.h
@@ -432,13 +432,13 @@ HttpFreeHeaderFields (
   @param[in]   Url                The URL of a remote host.
   @param[out]  RequestMsg         Pointer to the created HTTP request message.
                                   NULL if any error occured.
   @param[out]  RequestMsgSize     Size of the RequestMsg (in bytes).
 
-  @return EFI_SUCCESS             If HTTP request string was created successfully
+  @retval EFI_SUCCESS             If HTTP request string was created successfully.
   @retval EFI_OUT_OF_RESOURCES    Failed to allocate resources.
-  @retval EFI_INVALID_PARAMETER   The input arguments are invalid
+  @retval EFI_INVALID_PARAMETER   The input arguments are invalid.
 
 **/
 EFI_STATUS
 EFIAPI
 HttpGenRequestMessage (
diff --git a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
index 327ca9e..4e5cf84 100644
--- a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
+++ b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
@@ -53,11 +53,12 @@ UriPercentDecode (
   Index = 0;
   Offset = 0;
   HexStr[2] = '\0';
   while (Index < BufferLength) {
     if (Buffer[Index] == '%') {
-      if (Index + 1 >= BufferLength || Index + 2 >= BufferLength || !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);
@@ -506,11 +507,11 @@ HttpUrlGetHostName (
 
   if (Url == NULL || UrlParser == NULL || HostName == NULL) {
     return EFI_INVALID_PARAMETER;
   }
 
-  Parser = (HTTP_URL_PARSER*) UrlParser;
+  Parser = (HTTP_URL_PARSER *) UrlParser;
 
   if ((Parser->FieldBitMap & BIT (HTTP_URI_FIELD_HOST)) == 0) {
     return EFI_NOT_FOUND;
   }
 
@@ -566,11 +567,11 @@ HttpUrlGetIp4 (
   
   if (Url == NULL || UrlParser == NULL || Ip4Address == NULL) {
     return EFI_INVALID_PARAMETER;
   }
 
-  Parser = (HTTP_URL_PARSER*) UrlParser;
+  Parser = (HTTP_URL_PARSER *) UrlParser;
 
   if ((Parser->FieldBitMap & BIT (HTTP_URI_FIELD_HOST)) == 0) {
     return EFI_NOT_FOUND;
   }
 
@@ -629,11 +630,11 @@ HttpUrlGetIp6 (
   
   if (Url == NULL || UrlParser == NULL || Ip6Address == NULL) {
     return EFI_INVALID_PARAMETER;
   }
 
-  Parser = (HTTP_URL_PARSER*) UrlParser;
+  Parser = (HTTP_URL_PARSER *) UrlParser;
 
   if ((Parser->FieldBitMap & BIT (HTTP_URI_FIELD_HOST)) == 0) {
     return EFI_NOT_FOUND;
   }
 
@@ -694,25 +695,25 @@ HttpUrlGetPort (
   IN      CHAR8              *Url,
   IN      VOID               *UrlParser,
      OUT  UINT16             *Port
   )
 {
-  CHAR8         *PortString;
-  EFI_STATUS    Status;
-  UINTN         Index;
-  UINTN         Data;
-  UINT32        ResultLength;
+  CHAR8                *PortString;
+  EFI_STATUS           Status;
+  UINTN                Index;
+  UINTN                Data;
+  UINT32               ResultLength;
   HTTP_URL_PARSER      *Parser;
 
   if (Url == NULL || UrlParser == NULL || Port == NULL) {
     return EFI_INVALID_PARAMETER;
   }
 
   *Port = 0;
   Index = 0;
 
-  Parser = (HTTP_URL_PARSER*) UrlParser;
+  Parser = (HTTP_URL_PARSER *) UrlParser;
 
   if ((Parser->FieldBitMap & BIT (HTTP_URI_FIELD_PORT)) == 0) {
     return EFI_NOT_FOUND;
   }
 
@@ -786,11 +787,11 @@ HttpUrlGetPath (
 
   if (Url == NULL || UrlParser == NULL || Path == NULL) {
     return EFI_INVALID_PARAMETER;
   }
 
-  Parser = (HTTP_URL_PARSER*) UrlParser;
+  Parser = (HTTP_URL_PARSER *) UrlParser;
 
   if ((Parser->FieldBitMap & BIT (HTTP_URI_FIELD_PATH)) == 0) {
     return EFI_NOT_FOUND;
   }
 
@@ -1156,21 +1157,21 @@ HttpParseMessageBody (
 
   if (MsgParser == NULL) {
     return EFI_INVALID_PARAMETER;
   }
 
-  Parser = (HTTP_BODY_PARSER*) MsgParser;
+  Parser = (HTTP_BODY_PARSER *) MsgParser;
 
   if (Parser->IgnoreBody) {
     Parser->State = BodyParserComplete;
     if (Parser->Callback != NULL) {
       Status = Parser->Callback (
-                 BodyParseEventOnComplete,
-                 Body,
-                 0,
-                 Parser->Context
-                 );
+                         BodyParseEventOnComplete,
+                         Body,
+                         0,
+                         Parser->Context
+                         );
       if (EFI_ERROR (Status)) {
         return Status;
       }
     }
     return EFI_SUCCESS;
@@ -1198,30 +1199,30 @@ HttpParseMessageBody (
       //
       // Identity transfer-coding, just notify user to save the body data.
       //
       if (Parser->Callback != NULL) {
         Status = Parser->Callback (
-                   BodyParseEventOnData,
-                   Char,
-                   MIN (BodyLength, Parser->ContentLength - Parser->ParsedBodyLength),
-                   Parser->Context
-                   );
+                           BodyParseEventOnData,
+                           Char,
+                           MIN (BodyLength, Parser->ContentLength - Parser->ParsedBodyLength),
+                           Parser->Context
+                           );
         if (EFI_ERROR (Status)) {
           return Status;
         }
       }
       Char += MIN (BodyLength, Parser->ContentLength - Parser->ParsedBodyLength);
       Parser->ParsedBodyLength += MIN (BodyLength, Parser->ContentLength - Parser->ParsedBodyLength);
       if (Parser->ParsedBodyLength == Parser->ContentLength) {
         Parser->State = BodyParserComplete;
         if (Parser->Callback != NULL) {
           Status = Parser->Callback (
-                     BodyParseEventOnComplete,
-                     Char,
-                     0,
-                     Parser->Context
-                     );
+                             BodyParseEventOnComplete,
+                             Char,
+                             0,
+                             Parser->Context
+                             );
           if (EFI_ERROR (Status)) {
             return Status;
           }
         }
       }
@@ -1302,15 +1303,15 @@ HttpParseMessageBody (
       if (*Char == '\n') {
         Parser->State = BodyParserComplete;
         Char++;
         if (Parser->Callback != NULL) {
           Status = Parser->Callback (
-                     BodyParseEventOnComplete,
-                     Char,
-                     0,
-                     Parser->Context
-                     );
+                             BodyParseEventOnComplete,
+                             Char,
+                             0,
+                             Parser->Context
+                             );
           if (EFI_ERROR (Status)) {
             return Status;
           }
         }
         break;
@@ -1332,15 +1333,15 @@ HttpParseMessageBody (
       //
       RemainderLengthInThis = BodyLength - (Char - Body);
       LengthForCallback = MIN (Parser->CurrentChunkSize - Parser->CurrentChunkParsedSize, RemainderLengthInThis);
       if (Parser->Callback != NULL) {
         Status = Parser->Callback (
-                   BodyParseEventOnData,
-                   Char,
-                   LengthForCallback,
-                   Parser->Context
-                   );
+                           BodyParseEventOnData,
+                           Char,
+                           LengthForCallback,
+                           Parser->Context
+                           );
         if (EFI_ERROR (Status)) {
           return Status;
         }
       }
       Char += LengthForCallback;
@@ -1401,11 +1402,11 @@ HttpIsMessageComplete (
 
   if (MsgParser == NULL) {
     return FALSE;
   }
 
-  Parser = (HTTP_BODY_PARSER*) MsgParser;
+  Parser = (HTTP_BODY_PARSER *) MsgParser;
 
   if (Parser->State == BodyParserComplete) {
     return TRUE;
   }
   return FALSE;
@@ -1435,11 +1436,11 @@ HttpGetEntityLength (
 
   if (MsgParser == NULL || ContentLength == NULL) {
     return EFI_INVALID_PARAMETER;
   }
 
-  Parser = (HTTP_BODY_PARSER*) MsgParser;
+  Parser = (HTTP_BODY_PARSER *) MsgParser;
 
   if (!Parser->ContentLengthIsValid) {
     return EFI_NOT_READY;
   }
 
@@ -1473,11 +1474,10 @@ HttpFreeMsgParser (
   @return     Pointer to the next string.
   @return     NULL if not find or String is NULL.
 
 **/
 CHAR8 *
-EFIAPI
 AsciiStrGetNextToken (
   IN CONST CHAR8 *String,
   IN       CHAR8 Separator
   )
 {
@@ -1618,20 +1618,20 @@ HttpGetFieldNameAndValue (
   while (TRUE) {
     if (*FieldValueStr == ' ' || *FieldValueStr == '\t') {
       //
       // Boundary condition check. 
       //
-      if ((UINTN)EndofHeader - (UINTN)(FieldValueStr) < 1) {
+      if ((UINTN) EndofHeader - (UINTN) FieldValueStr < 1) {
         return NULL;  
       }
       
       FieldValueStr ++;
     } else if (*FieldValueStr == '\r') {
       //
       // Boundary condition check. 
       //
-      if ((UINTN)EndofHeader - (UINTN)(FieldValueStr) < 3) {
+      if ((UINTN) EndofHeader - (UINTN) FieldValueStr < 3) {
         return NULL;  
       }
 
       if (*(FieldValueStr + 1) == '\n' && (*(FieldValueStr + 2) == ' ' || *(FieldValueStr + 2) == '\t')) {
         FieldValueStr = FieldValueStr + 3;
@@ -1713,13 +1713,13 @@ HttpFreeHeaderFields (
   @param[in]   Url                The URL of a remote host.
   @param[out]  RequestMsg         Pointer to the created HTTP request message.
                                   NULL if any error occured.
   @param[out]  RequestMsgSize     Size of the RequestMsg (in bytes).
 
-  @return EFI_SUCCESS             If HTTP request string was created successfully
+  @retval EFI_SUCCESS             If HTTP request string was created successfully.
   @retval EFI_OUT_OF_RESOURCES    Failed to allocate resources.
-  @retval EFI_INVALID_PARAMETER   The input arguments are invalid
+  @retval EFI_INVALID_PARAMETER   The input arguments are invalid.
 
 **/
 EFI_STATUS
 EFIAPI
 HttpGenRequestMessage (
@@ -1767,11 +1767,11 @@ HttpGenRequestMessage (
     // Locate the HTTP_UTILITIES protocol.
     //
     Status = gBS->LocateProtocol (
                     &gEfiHttpUtilitiesProtocolGuid,
                     NULL,
-                    (VOID **)&HttpUtilitiesProtocol
+                    (VOID **) &HttpUtilitiesProtocol
                     );
 
     if (EFI_ERROR (Status)) {
       DEBUG ((DEBUG_ERROR,"Failed to locate Http Utilities protocol. Status = %r.\n", Status));
       return Status;
@@ -1791,24 +1791,22 @@ HttpGenRequestMessage (
 
     //
     // Build raw HTTP Headers
     //
     Status = HttpUtilitiesProtocol->Build (
-                HttpUtilitiesProtocol,
-                0,
-                NULL,
-                0,
-                NULL,
-                Message->HeaderCount,
-                AppendList,
-                &HttpHdrSize,
-                &HttpHdr
-                );
-
-    if (AppendList != NULL) {
-      FreePool (AppendList);
-    }
+                                      HttpUtilitiesProtocol,
+                                      0,
+                                      NULL,
+                                      0,
+                                      NULL,
+                                      Message->HeaderCount,
+                                      AppendList,
+                                      &HttpHdrSize,
+                                      &HttpHdr
+                                      );
+
+    FreePool (AppendList);
 
     if (EFI_ERROR (Status) || HttpHdr == NULL){
       return Status;
     }
   }
@@ -1834,11 +1832,11 @@ HttpGenRequestMessage (
   MsgSize += Message->BodyLength;
 
   //
   // memory for the string that needs to be sent to TCP
   //
-  *RequestMsg           = NULL;
+  *RequestMsg = NULL;
   *RequestMsg = AllocateZeroPool (MsgSize);
   if (*RequestMsg == NULL) {
     Status = EFI_OUT_OF_RESOURCES;
     goto Exit;
   }
-- 
1.9.5.msysgit.1



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [Patch 0/5] MdeModulePkg/DxeHttpLib: Fix series issues in DxeHttpLib.
  2017-12-26  1:33 [Patch 0/5] MdeModulePkg/DxeHttpLib: Fix series issues in DxeHttpLib Jiaxin Wu
                   ` (4 preceding siblings ...)
  2017-12-26  1:33 ` [Patch 5/5] MdeModulePkg/DxeHttpLib: Refine some coding style Jiaxin Wu
@ 2017-12-26  1:40 ` Fu, Siyuan
  5 siblings, 0 replies; 9+ messages in thread
From: Fu, Siyuan @ 2017-12-26  1:40 UTC (permalink / raw)
  To: Wu, Jiaxin, edk2-devel@lists.01.org; +Cc: Ye, Ting, Wang, Fan

Reviewed-by: Fu Siyuan <siyuan.fu@intel.com>

> -----Original Message-----
> From: Wu, Jiaxin
> Sent: Tuesday, December 26, 2017 9:34 AM
> To: edk2-devel@lists.01.org
> Cc: Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Wang,
> Fan <fan.wang@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: [Patch 0/5] MdeModulePkg/DxeHttpLib: Fix series issues in
> DxeHttpLib.
> 
> 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>
> 
> Jiaxin Wu (5):
>   MdeModulePkg/DxeHttpLib: Add boundary condition check.
>   MdeModulePkg/DxeHttpLib: Avoid the potential memory leak when error
>     happen.
>   MdeModulePkg/DxeHttpLib: Check the input parameters for some APIs.
>   MdeModulePkg/DxeHttpLib: Correct some return Status.
>   MdeModulePkg/DxeHttpLib: Refine some coding style.
> 
>  MdeModulePkg/Include/Library/HttpLib.h       |  10 +-
>  MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 190 +++++++++++++++++-----
> -----
>  2 files changed, 125 insertions(+), 75 deletions(-)
> 
> --
> 1.9.5.msysgit.1



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Patch 1/5] MdeModulePkg/DxeHttpLib: Add boundary condition check.
  2017-12-26  1:33 ` [Patch 1/5] MdeModulePkg/DxeHttpLib: Add boundary condition check Jiaxin Wu
@ 2017-12-26  1:56   ` Gary Lin
  2017-12-26  2:21     ` Wu, Jiaxin
  0 siblings, 1 reply; 9+ messages in thread
From: Gary Lin @ 2017-12-26  1:56 UTC (permalink / raw)
  To: Jiaxin Wu; +Cc: edk2-devel, Ye Ting, Wang Fan, Fu Siyuan

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.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 | 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_SUCCESS            Successfully decoded the URI.
>    @retval EFI_INVALID_PARAMETER  Buffer is not a valid percent-encoded string.
>    
>  **/
>  EFI_STATUS
> -EFIAPI
>  UriPercentDecode (
>    IN      CHAR8            *Buffer,
>    IN      UINT32            BufferLength,
>       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
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Patch 1/5] MdeModulePkg/DxeHttpLib: Add boundary condition check.
  2017-12-26  1:56   ` Gary Lin
@ 2017-12-26  2:21     ` Wu, Jiaxin
  0 siblings, 0 replies; 9+ messages in thread
From: Wu, Jiaxin @ 2017-12-26  2:21 UTC (permalink / raw)
  To: Gary Lin; +Cc: edk2-devel@lists.01.org, Ye, Ting, Wang, Fan, Fu, Siyuan

Thanks to catch that.

Best Regards!
Jiaxin

> -----Original Message-----
> From: Gary Lin [mailto:glin@suse.com]
> Sent: Tuesday, December 26, 2017 9:56 AM
> To: Wu, Jiaxin <jiaxin.wu@intel.com>
> Cc: edk2-devel@lists.01.org; Ye, Ting <ting.ye@intel.com>; Wang, Fan
> <fan.wang@intel.com>; Fu, Siyuan <siyuan.fu@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.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 | 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_SUCCESS            Successfully decoded the URI.
> >    @retval EFI_INVALID_PARAMETER  Buffer is not a valid percent-encoded
> string.
> >
> >  **/
> >  EFI_STATUS
> > -EFIAPI
> >  UriPercentDecode (
> >    IN      CHAR8            *Buffer,
> >    IN      UINT32            BufferLength,
> >       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
> >


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2017-12-26  2:16 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-26  1:33 [Patch 0/5] MdeModulePkg/DxeHttpLib: Fix series issues in DxeHttpLib Jiaxin Wu
2017-12-26  1:33 ` [Patch 1/5] MdeModulePkg/DxeHttpLib: Add boundary condition check Jiaxin Wu
2017-12-26  1:56   ` Gary Lin
2017-12-26  2:21     ` Wu, Jiaxin
2017-12-26  1:33 ` [Patch 2/5] MdeModulePkg/DxeHttpLib: Avoid the potential memory leak when error happen Jiaxin Wu
2017-12-26  1:33 ` [Patch 3/5] MdeModulePkg/DxeHttpLib: Check the input parameters for some APIs Jiaxin Wu
2017-12-26  1:33 ` [Patch 4/5] MdeModulePkg/DxeHttpLib: Correct some return Status Jiaxin Wu
2017-12-26  1:33 ` [Patch 5/5] MdeModulePkg/DxeHttpLib: Refine some coding style Jiaxin Wu
2017-12-26  1:40 ` [Patch 0/5] MdeModulePkg/DxeHttpLib: Fix series issues in DxeHttpLib Fu, Siyuan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox