* [PATCH 1/1] NetworkPkg: Fix possible infinite loop in HTTP msg body parser @ 2020-08-28 18:17 Vladimir Olovyannikov 2020-09-24 21:57 ` Vladimir Olovyannikov 0 siblings, 1 reply; 7+ messages in thread From: Vladimir Olovyannikov @ 2020-08-28 18:17 UTC (permalink / raw) To: devel; +Cc: Vladimir Olovyannikov, Maciej Rabeda, Jiaxin Wu, Siyuan Fu 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 <vladimir.olovyannikov@broadcom.com> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com> Cc: Jiaxin Wu <jiaxin.wu@intel.com> Cc: Siyuan Fu <siyuan.fu@intel.com> --- 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] NetworkPkg: Fix possible infinite loop in HTTP msg body parser 2020-08-28 18:17 [PATCH 1/1] NetworkPkg: Fix possible infinite loop in HTTP msg body parser Vladimir Olovyannikov @ 2020-09-24 21:57 ` Vladimir Olovyannikov 2020-09-30 9:56 ` [edk2-devel] " Maciej Rabeda 0 siblings, 1 reply; 7+ messages in thread From: Vladimir Olovyannikov @ 2020-09-24 21:57 UTC (permalink / raw) To: devel; +Cc: Maciej Rabeda, Jiaxin Wu, Siyuan Fu, Laszlo Ersek [-- Attachment #1: Type: text/plain, Size: 3444 bytes --] 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 <vladimir.olovyannikov@broadcom.com> > Sent: Friday, August 28, 2020 11:17 AM > To: devel@edk2.groups.io > Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>; Maciej > Rabeda <maciej.rabeda@linux.intel.com>; Jiaxin Wu <jiaxin.wu@intel.com>; > Siyuan Fu <siyuan.fu@intel.com> > 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 > <vladimir.olovyannikov@broadcom.com> > Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com> > Cc: Jiaxin Wu <jiaxin.wu@intel.com> > Cc: Siyuan Fu <siyuan.fu@intel.com> > --- > 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 [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 4193 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] NetworkPkg: Fix possible infinite loop in HTTP msg body parser 2020-09-24 21:57 ` Vladimir Olovyannikov @ 2020-09-30 9:56 ` Maciej Rabeda 2020-10-01 15:25 ` Vladimir Olovyannikov 0 siblings, 1 reply; 7+ messages in thread From: Maciej Rabeda @ 2020-09-30 9:56 UTC (permalink / raw) To: devel, vladimir.olovyannikov; +Cc: Jiaxin Wu, Siyuan Fu, Laszlo Ersek 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 <vladimir.olovyannikov@broadcom.com> >> Sent: Friday, August 28, 2020 11:17 AM >> To: devel@edk2.groups.io >> Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>; Maciej >> Rabeda <maciej.rabeda@linux.intel.com>; Jiaxin Wu <jiaxin.wu@intel.com>; >> Siyuan Fu <siyuan.fu@intel.com> >> 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 >> <vladimir.olovyannikov@broadcom.com> >> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com> >> Cc: Jiaxin Wu <jiaxin.wu@intel.com> >> Cc: Siyuan Fu <siyuan.fu@intel.com> >> --- >> 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 > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] NetworkPkg: Fix possible infinite loop in HTTP msg body parser 2020-09-30 9:56 ` [edk2-devel] " Maciej Rabeda @ 2020-10-01 15:25 ` Vladimir Olovyannikov 2020-10-02 12:02 ` Maciej Rabeda 0 siblings, 1 reply; 7+ messages in thread From: Vladimir Olovyannikov @ 2020-10-01 15:25 UTC (permalink / raw) To: Rabeda, Maciej, devel; +Cc: Jiaxin Wu, Siyuan Fu, Laszlo Ersek [-- Attachment #1: Type: text/plain, Size: 4490 bytes --] Hi Maciej, Thank you for looking into this. Vladimir > -----Original Message----- > From: Rabeda, Maciej <maciej.rabeda@linux.intel.com> > Sent: Wednesday, September 30, 2020 2:57 AM > To: devel@edk2.groups.io; vladimir.olovyannikov@broadcom.com > Cc: Jiaxin Wu <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>; > Laszlo Ersek <lersek@redhat.com> > 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 <vladimir.olovyannikov@broadcom.com> > >> Sent: Friday, August 28, 2020 11:17 AM > >> To: devel@edk2.groups.io > >> Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>; > >> Maciej Rabeda <maciej.rabeda@linux.intel.com>; Jiaxin Wu > >> <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com> > >> 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 > >> <vladimir.olovyannikov@broadcom.com> > >> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com> > >> Cc: Jiaxin Wu <jiaxin.wu@intel.com> > >> Cc: Siyuan Fu <siyuan.fu@intel.com> > >> --- > >> 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 > > > > > > > > [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 4193 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] NetworkPkg: Fix possible infinite loop in HTTP msg body parser 2020-10-01 15:25 ` Vladimir Olovyannikov @ 2020-10-02 12:02 ` Maciej Rabeda 2020-10-02 15:08 ` Vladimir Olovyannikov 0 siblings, 1 reply; 7+ messages in thread From: Maciej Rabeda @ 2020-10-02 12:02 UTC (permalink / raw) To: Vladimir Olovyannikov, devel; +Cc: Jiaxin Wu, Siyuan Fu, Laszlo Ersek 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 Do not submit v2, I will correct that upon merging. On terms CS issue is addressed, I am giving: Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com> 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 <maciej.rabeda@linux.intel.com> >> Sent: Wednesday, September 30, 2020 2:57 AM >> To: devel@edk2.groups.io; vladimir.olovyannikov@broadcom.com >> Cc: Jiaxin Wu <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>; >> Laszlo Ersek <lersek@redhat.com> >> 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 <vladimir.olovyannikov@broadcom.com> >>>> Sent: Friday, August 28, 2020 11:17 AM >>>> To: devel@edk2.groups.io >>>> Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>; >>>> Maciej Rabeda <maciej.rabeda@linux.intel.com>; Jiaxin Wu >>>> <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com> >>>> 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 >>>> <vladimir.olovyannikov@broadcom.com> >>>> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com> >>>> Cc: Jiaxin Wu <jiaxin.wu@intel.com> >>>> Cc: Siyuan Fu <siyuan.fu@intel.com> >>>> --- >>>> 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 >>> >>> >>> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] NetworkPkg: Fix possible infinite loop in HTTP msg body parser 2020-10-02 12:02 ` Maciej Rabeda @ 2020-10-02 15:08 ` Vladimir Olovyannikov 2020-10-09 11:02 ` Maciej Rabeda 0 siblings, 1 reply; 7+ messages in thread From: Vladimir Olovyannikov @ 2020-10-02 15:08 UTC (permalink / raw) To: Rabeda, Maciej, devel; +Cc: Jiaxin Wu, Siyuan Fu, Laszlo Ersek [-- Attachment #1: Type: text/plain, Size: 6003 bytes --] Hi Maciej, Thank you for reviewing the patch. > -----Original Message----- > From: Rabeda, Maciej <maciej.rabeda@linux.intel.com> > Sent: Friday, October 2, 2020 5:02 AM > To: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>; > devel@edk2.groups.io > Cc: Jiaxin Wu <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>; > Laszlo Ersek <lersek@redhat.com> > 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 <maciej.rabeda@linux.intel.com> 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 <maciej.rabeda@linux.intel.com> > >> Sent: Wednesday, September 30, 2020 2:57 AM > >> To: devel@edk2.groups.io; vladimir.olovyannikov@broadcom.com > >> Cc: Jiaxin Wu <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>; > >> Laszlo Ersek <lersek@redhat.com> > >> 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 <vladimir.olovyannikov@broadcom.com> > >>>> Sent: Friday, August 28, 2020 11:17 AM > >>>> To: devel@edk2.groups.io > >>>> Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>; > >>>> Maciej Rabeda <maciej.rabeda@linux.intel.com>; Jiaxin Wu > >>>> <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com> > >>>> 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 > >>>> <vladimir.olovyannikov@broadcom.com> > >>>> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com> > >>>> Cc: Jiaxin Wu <jiaxin.wu@intel.com> > >>>> Cc: Siyuan Fu <siyuan.fu@intel.com> > >>>> --- > >>>> 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 > >>> > >>> > >>> [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 4193 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] NetworkPkg: Fix possible infinite loop in HTTP msg body parser 2020-10-02 15:08 ` Vladimir Olovyannikov @ 2020-10-09 11:02 ` Maciej Rabeda 0 siblings, 0 replies; 7+ messages in thread From: Maciej Rabeda @ 2020-10-09 11:02 UTC (permalink / raw) To: devel, vladimir.olovyannikov; +Cc: Jiaxin Wu, Siyuan Fu, Laszlo Ersek 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 <maciej.rabeda@linux.intel.com> >> Sent: Friday, October 2, 2020 5:02 AM >> To: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>; >> devel@edk2.groups.io >> Cc: Jiaxin Wu <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>; >> Laszlo Ersek <lersek@redhat.com> >> 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 <maciej.rabeda@linux.intel.com> > 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 <maciej.rabeda@linux.intel.com> >>>> Sent: Wednesday, September 30, 2020 2:57 AM >>>> To: devel@edk2.groups.io; vladimir.olovyannikov@broadcom.com >>>> Cc: Jiaxin Wu <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>; >>>> Laszlo Ersek <lersek@redhat.com> >>>> 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 <vladimir.olovyannikov@broadcom.com> >>>>>> Sent: Friday, August 28, 2020 11:17 AM >>>>>> To: devel@edk2.groups.io >>>>>> Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>; >>>>>> Maciej Rabeda <maciej.rabeda@linux.intel.com>; Jiaxin Wu >>>>>> <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com> >>>>>> 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 >>>>>> <vladimir.olovyannikov@broadcom.com> >>>>>> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com> >>>>>> Cc: Jiaxin Wu <jiaxin.wu@intel.com> >>>>>> Cc: Siyuan Fu <siyuan.fu@intel.com> >>>>>> --- >>>>>> 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 >>>>> >>>>> > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-10-09 11:02 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-08-28 18:17 [PATCH 1/1] NetworkPkg: Fix possible infinite loop in HTTP msg body parser Vladimir Olovyannikov 2020-09-24 21:57 ` Vladimir Olovyannikov 2020-09-30 9:56 ` [edk2-devel] " Maciej Rabeda 2020-10-01 15:25 ` Vladimir Olovyannikov 2020-10-02 12:02 ` Maciej Rabeda 2020-10-02 15:08 ` Vladimir Olovyannikov 2020-10-09 11:02 ` Maciej Rabeda
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox