* [PATCH] NetworkPkg: fix read memory access overflow in HTTPBoot.
@ 2018-09-25 3:28 Songpeng Li
2018-09-25 3:43 ` Fu, Siyuan
0 siblings, 1 reply; 3+ messages in thread
From: Songpeng Li @ 2018-09-25 3:28 UTC (permalink / raw)
To: edk2-devel; +Cc: Fu Siyuan, Wu Jiaxin
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] NetworkPkg: fix read memory access overflow in HTTPBoot.
2018-09-25 3:28 [PATCH] NetworkPkg: fix read memory access overflow in HTTPBoot Songpeng Li
@ 2018-09-25 3:43 ` Fu, Siyuan
2018-09-25 3:48 ` Wu, Jiaxin
0 siblings, 1 reply; 3+ messages in thread
From: Fu, Siyuan @ 2018-09-25 3:43 UTC (permalink / raw)
To: Li, Songpeng, edk2-devel@lists.01.org; +Cc: Wu, Jiaxin
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] NetworkPkg: fix read memory access overflow in HTTPBoot.
2018-09-25 3:43 ` Fu, Siyuan
@ 2018-09-25 3:48 ` Wu, Jiaxin
0 siblings, 0 replies; 3+ messages in thread
From: Wu, Jiaxin @ 2018-09-25 3:48 UTC (permalink / raw)
To: Fu, Siyuan, Li, Songpeng, edk2-devel@lists.01.org
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-09-25 3:48 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-09-25 3:28 [PATCH] NetworkPkg: fix read memory access overflow in HTTPBoot Songpeng Li
2018-09-25 3:43 ` Fu, Siyuan
2018-09-25 3:48 ` Wu, Jiaxin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox