From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=195.135.221.5; helo=smtp.nue.novell.com; envelope-from=glin@suse.com; receiver=edk2-devel@lists.01.org Received: from smtp.nue.novell.com (smtp.nue.novell.com [195.135.221.5]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id CA29022225BFA for ; Mon, 25 Dec 2017 17:51:36 -0800 (PST) Received: from emea4-mta.ukb.novell.com ([10.120.13.87]) by smtp.nue.novell.com with ESMTP (TLS encrypted); Tue, 26 Dec 2017 02:56:28 +0100 Received: from GaryWorkstation (nwb-a10-snat.microfocus.com [10.120.13.202]) by emea4-mta.ukb.novell.com with ESMTP (TLS encrypted); Tue, 26 Dec 2017 01:56:09 +0000 Date: Tue, 26 Dec 2017 09:56:03 +0800 From: Gary Lin To: Jiaxin Wu Cc: edk2-devel@lists.01.org, Ye Ting , Wang Fan , Fu Siyuan Message-ID: <20171226015603.c22tfqq6vj5oesc7@GaryWorkstation> References: <1514252029-12720-1-git-send-email-jiaxin.wu@intel.com> <1514252029-12720-2-git-send-email-jiaxin.wu@intel.com> MIME-Version: 1.0 In-Reply-To: <1514252029-12720-2-git-send-email-jiaxin.wu@intel.com> User-Agent: NeoMutt/20170912 (1.9.0) Subject: Re: [Patch 1/5] MdeModulePkg/DxeHttpLib: Add boundary condition check. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 26 Dec 2017 01:51:37 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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_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 >