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

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

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
> >