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

["Fu, Siyuan" <siyuan.fu@intel.com>]

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