Re: [PATCH] NetworkPkg: fix read memory access overflow in HTTPBoot.

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

Besides, I recommend to separate the patch for HttpDxe and HttpUtilitiesDxe.

Thanks,
Jiaxin

> -----Original Message-----
> From: Fu, Siyuan
> Sent: Tuesday, September 25, 2018 11:43 AM
> To: Li, Songpeng <songpeng.li@intel.com>; edk2-devel@lists.01.org
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: RE: [PATCH] NetworkPkg: fix read memory access overflow in
> HTTPBoot.
> 
> Hi, Songpeng
> 
> The change is ok with me while I have one comment for the original
> AllocateZeroPool() in these places. Since there will be always a CopyMem()
> to fill up data content to the new allocated buffer, there is no need to use
> AllocateZeroPool(), just AllocatePool() and adding null terminator should be
> enough. This will save the unnecessary ZeroMem() time cost for better
> performance. Thanks.
> 
> 
> BestRegards
> Fu Siyuan
> 
> 
> > -----Original Message-----
> > From: Li, Songpeng
> > Sent: Tuesday, September 25, 2018 11:29 AM
> > To: edk2-devel@lists.01.org
> > Cc: Fu, Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> > Subject: [PATCH] NetworkPkg: fix read memory access overflow in
> HTTPBoot.
> >
> > The input param String of AsciiStrStr() requires a pointer to
> >  Null-terminated string, however in HttpTcpReceiveHeader() and
> >  HttpUtilitiesParse(), the Buffersize before AllocateZeroPool()
> >  is equal to the size of TCP header, after the CopyMem(), it
> >  might not end with Null-terminator. It might cause memory
> >  access overflow.
> >
> > Cc: Fu Siyuan <siyuan.fu@intel.com>
> > Cc: Wu Jiaxin <jiaxin.wu@intel.com>
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1204
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Songpeng Li <songpeng.li@intel.com>
> > ---
> >  NetworkPkg/HttpDxe/HttpProto.c                      | 4 ++--
> >  NetworkPkg/HttpUtilitiesDxe/HttpUtilitiesProtocol.c | 4 +++-
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/NetworkPkg/HttpDxe/HttpProto.c
> > b/NetworkPkg/HttpDxe/HttpProto.c
> > index 94f89f5665..c729f76eff 100644
> > --- a/NetworkPkg/HttpDxe/HttpProto.c
> > +++ b/NetworkPkg/HttpDxe/HttpProto.c
> > @@ -1917,7 +1917,7 @@ HttpTcpReceiveHeader (
> >        // Append the response string.
> >        //
> >        *BufferSize = *SizeofHeaders + Fragment.Len;
> > -      Buffer      = AllocateZeroPool (*BufferSize);
> > +      Buffer      = AllocateZeroPool (*BufferSize + 1);
> >        if (Buffer == NULL) {
> >          Status = EFI_OUT_OF_RESOURCES;
> >          return Status;
> > @@ -2016,7 +2016,7 @@ HttpTcpReceiveHeader (
> >        // Append the response string.
> >        //
> >        *BufferSize = *SizeofHeaders + Fragment.Len;
> > -      Buffer      = AllocateZeroPool (*BufferSize);
> > +      Buffer      = AllocateZeroPool (*BufferSize + 1);
> >        if (Buffer == NULL) {
> >          Status = EFI_OUT_OF_RESOURCES;
> >          return Status;
> > diff --git a/NetworkPkg/HttpUtilitiesDxe/HttpUtilitiesProtocol.c
> > b/NetworkPkg/HttpUtilitiesDxe/HttpUtilitiesProtocol.c
> > index a9a1c7c586..2292b52537 100644
> > --- a/NetworkPkg/HttpUtilitiesDxe/HttpUtilitiesProtocol.c
> > +++ b/NetworkPkg/HttpUtilitiesDxe/HttpUtilitiesProtocol.c
> > @@ -298,6 +298,7 @@ HttpUtilitiesParse (
> >    CHAR8                     *FieldName;
> >    CHAR8                     *FieldValue;
> >    UINTN                     Index;
> > +  UINTN                     HttpBufferSize;
> >
> >    Status          = EFI_SUCCESS;
> >    TempHttpMessage = NULL;
> > @@ -311,7 +312,8 @@ HttpUtilitiesParse (
> >      return EFI_INVALID_PARAMETER;
> >    }
> >
> > -  TempHttpMessage = AllocateZeroPool (HttpMessageSize);
> > +  HttpBufferSize = HttpMessageSize + 1;
> > +  TempHttpMessage = AllocateZeroPool (HttpBufferSize);
> >    if (TempHttpMessage == NULL) {
> >      return EFI_OUT_OF_RESOURCES;
> >    }
> > --
> > 2.18.0.windows.1