From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by mx.groups.io with SMTP id smtpd.web11.11059.1602241353472444969 for ; Fri, 09 Oct 2020 04:02:33 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: linux.intel.com, ip: 192.55.52.136, mailfrom: maciej.rabeda@linux.intel.com) IronPort-SDR: cjc4hcgJbpWXBrMlqxy7DgyHqLNYA/y+W+nvjReZNHP5D4gpvgZhMAPEVBwZYGJvz5BS/fWnBA EaztGA7BFzoQ== X-IronPort-AV: E=McAfee;i="6000,8403,9768"; a="144791330" X-IronPort-AV: E=Sophos;i="5.77,354,1596524400"; d="scan'208";a="144791330" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Oct 2020 04:02:32 -0700 IronPort-SDR: 4ocT3ukWyKx7HJjackJvC/bDJ+bnEu94CPWFz1PODq57yBKeYWyrX3on8jJjmgNkxVMoopZEG2 17eTPSCu53JQ== X-IronPort-AV: E=Sophos;i="5.77,354,1596524400"; d="scan'208";a="312512066" Received: from mrabeda-mobl.ger.corp.intel.com (HELO [10.213.19.174]) ([10.213.19.174]) by orsmga003-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Oct 2020 04:02:31 -0700 Subject: Re: [edk2-devel] [PATCH 1/1] NetworkPkg: Fix possible infinite loop in HTTP msg body parser To: devel@edk2.groups.io, vladimir.olovyannikov@broadcom.com Cc: Jiaxin Wu , Siyuan Fu , Laszlo Ersek References: <20200828181706.25296-1-vladimir.olovyannikov@broadcom.com> <2d7b8b14f01cc630017e3e1134f17585@mail.gmail.com> <4b4d9ed6f95926f5029beb97fbf8f47a@mail.gmail.com> <4f0f875d-865e-cc4b-ae21-21a422b4b0b9@linux.intel.com> <7f0c8a9bcb51af3d5f974e5fd2abc334@mail.gmail.com> From: "Maciej Rabeda" Message-ID: Date: Fri, 9 Oct 2020 13:02:28 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.12.1 MIME-Version: 1.0 In-Reply-To: <7f0c8a9bcb51af3d5f974e5fd2abc334@mail.gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: pl Merged. https://github.com/tianocore/edk2/commit/70c2f10fde5b67b0d7d62ba7ea3271fc514ebcc4 https://github.com/tianocore/edk2/pull/997 On 02-Oct-20 17:08, Vladimir Olovyannikov via groups.io wrote: > Hi Maciej, > > Thank you for reviewing the patch. > >> -----Original Message----- >> From: Rabeda, Maciej >> Sent: Friday, October 2, 2020 5:02 AM >> To: Vladimir Olovyannikov ; >> devel@edk2.groups.io >> Cc: Jiaxin Wu ; Siyuan Fu ; >> Laszlo Ersek >> Subject: Re: [edk2-devel] [PATCH 1/1] NetworkPkg: Fix possible infinite >> loop >> in HTTP msg body parser >> >> Hi Vladimir, >> >> Functionally the patch is fine. >> However, from coding standard perspective, !PortionLen is not allowed - >> such structure is used only for BOOLEAN type values. >> Reference: Table 10, >> https://edk2-docs.gitbook.io/edk-ii-c-coding-standards- >> specification/5_source_files/57_c_programming#5-7-2-2-a-comparison-of- >> any-pointer-to-zero-must-be-done-via-the-null-type > Sorry, my bad. That's a result of switching between edk2 and Linux > developments. >> Do not submit v2, I will correct that upon merging. On terms CS issue is >> addressed, I am giving: >> Reviewed-by: Maciej Rabeda > Thank you Maciej, > > Vladimir >> Thanks, >> Maciej >> >> On 01-Oct-20 17:25, Vladimir Olovyannikov wrote: >>> Hi Maciej, >>> >>> Thank you for looking into this. >>> >>> Vladimir >>>> -----Original Message----- >>>> From: Rabeda, Maciej >>>> Sent: Wednesday, September 30, 2020 2:57 AM >>>> To: devel@edk2.groups.io; vladimir.olovyannikov@broadcom.com >>>> Cc: Jiaxin Wu ; Siyuan Fu ; >>>> Laszlo Ersek >>>> Subject: Re: [edk2-devel] [PATCH 1/1] NetworkPkg: Fix possible >>>> infinite loop in HTTP msg body parser >>>> >>>> Hi Vladimir, >>>> >>>> Yes, this must have go past my radar, sorry. Things are becoming more >>>> and more busy out here :/ I will take a look at it by the end of week. >>>> >>>> On 24-Sep-20 23:57, Vladimir Olovyannikov via groups.io wrote: >>>>> Hi Maciej, >>>>> >>>>> Can you please review this patch? >>>>> It is sitting there for a while, looks like it slipped through the >>>>> cracks. >>>>> >>>>> Thank you, >>>>> Vladimir >>>>>> -----Original Message----- >>>>>> From: Vladimir Olovyannikov >>>>>> Sent: Friday, August 28, 2020 11:17 AM >>>>>> To: devel@edk2.groups.io >>>>>> Cc: Vladimir Olovyannikov ; >>>>>> Maciej Rabeda ; Jiaxin Wu >>>>>> ; Siyuan Fu >>>>>> Subject: [PATCH 1/1] NetworkPkg: Fix possible infinite loop in HTTP >>>>>> msg >>>>> body >>>>>> parser >>>>>> >>>>>> When an HTTP server sends a non-chunked body data with no >> Content- >>>>>> Length header, the HttpParserMessageBody in DxeHttpLib gets >>>>>> confused and never sets the Char pointer beyond the body start. >>>>>> This causes "for" loop to never break because the condition of >>>>>> "Char >>>>>>> = >>>>> Body >>>>>> + BodyLength" is never satisfied. >>>>>> Use BodyLength as the ContentLength for the parser when >>>> ContentLength >>>>>> is absent in HTTP response headers. >>>>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2941 >>>>>> >>>>>> Signed-off-by: Vladimir Olovyannikov >>>>>> >>>>>> Cc: Maciej Rabeda >>>>>> Cc: Jiaxin Wu >>>>>> Cc: Siyuan Fu >>>>>> --- >>>>>> NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c | 19 >>>>>> ++++++++++++++++- >>>> -- >>>>>> 1 file changed, 16 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c >>>>>> b/NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c >>>>>> index 180d9321025a..e550c9962dc1 100644 >>>>>> --- a/NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c >>>>>> +++ b/NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c >>>>>> @@ -1122,6 +1122,7 @@ HttpParseMessageBody ( >>>>>> CHAR8 *Char; >>>>>> UINTN RemainderLengthInThis; >>>>>> UINTN LengthForCallback; >>>>>> + UINTN PortionLength; >>>>>> EFI_STATUS Status; >>>>>> HTTP_BODY_PARSER *Parser; >>>>>> >>>>>> @@ -1173,19 +1174,31 @@ HttpParseMessageBody ( >>>>>> // >>>>>> // Identity transfer-coding, just notify user to save the >>>>>> body >>>>> data. >>>>>> // >>>>>> + PortionLength = MIN ( >>>>>> + BodyLength, >>>>>> + Parser->ContentLength - >>>>> Parser->ParsedBodyLength >>>>>> + ); >>>>>> + if (!PortionLength) { >>>>>> + // >>>>>> + // Got BodyLength, but no ContentLength. Use BodyLength. >>>>>> + // >>>>>> + PortionLength = BodyLength; >>>>>> + Parser->ContentLength = PortionLength; >>>>>> + } >>>>>> + >>>>>> if (Parser->Callback != NULL) { >>>>>> Status = Parser->Callback ( >>>>>> BodyParseEventOnData, >>>>>> Char, >>>>>> - MIN (BodyLength, Parser->ContentLength - >>>>> Parser- >>>>>>> ParsedBodyLength), >>>>>> + PortionLength, >>>>>> Parser->Context >>>>>> ); >>>>>> if (EFI_ERROR (Status)) { >>>>>> return Status; >>>>>> } >>>>>> } >>>>>> - Char += MIN (BodyLength, Parser->ContentLength - Parser- >>>>>>> ParsedBodyLength); >>>>>> - Parser->ParsedBodyLength += MIN (BodyLength, Parser- >>>>>>> ContentLength - Parser->ParsedBodyLength); >>>>>> + Char += PortionLength; >>>>>> + Parser->ParsedBodyLength += PortionLength; >>>>>> if (Parser->ParsedBodyLength == Parser->ContentLength) { >>>>>> Parser->State = BodyParserComplete; >>>>>> if (Parser->Callback != NULL) { >>>>>> -- >>>>>> 2.26.2.266.ge870325ee8 >>>>> >>>>> > > > >