Re: [edk2] [PATCH] NetworkPkg: fix read memory access overflow in HTTPBoot.
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 ; edk2-devel@lists.01.org
> Cc: Wu, Jiaxin
> 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 ; Wu, Jiaxin
> > 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
> > Cc: Wu Jiaxin
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1204
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Songpeng Li
> > ---
> > 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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [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 ; Wu, Jiaxin
> 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
> Cc: Wu Jiaxin
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1204
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Songpeng Li
> ---
> 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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [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
Cc: Wu Jiaxin
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1204
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Songpeng Li
---
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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
