1. Patchew
  2. EDK2
  3. [edk2-devel] [PATCH 0/2] UefiBootManagerLib, HttpDxe: tweaks for large HTTP(S) downloads
  4. View patch

[edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix 32-bit truncation in HTTPS download

Laszlo Ersek posted 2 patches 6 years, 1 month ago
[edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix 32-bit truncation in HTTPS download
Posted by Laszlo Ersek 6 years, 1 month ago 
When downloading over TLS, each TLS message ("APP packet") is returned as
a (decrypted) fragment table by EFI_TLS_PROTOCOL.ProcessPacket().

The TlsProcessMessage() function in "NetworkPkg/HttpDxe/HttpsSupport.c"
linearizes the fragment table into a single contiguous data block. The
resultant flat data block contains both TLS headers and data.

The HttpsReceive() function parses the actual application data -- in this
case: decrypted HTTP data -- out of the flattened TLS data block, peeling
off the TLS headers.

The HttpResponseWorker() function in "NetworkPkg/HttpDxe/HttpImpl.c"
propagates this HTTP data outwards, implementing the
EFI_HTTP_PROTOCOL.Response() function.

Now consider the following documentation for EFI_HTTP_PROTOCOL.Response(),
quoted from "MdePkg/Include/Protocol/Http.h":

> It is the responsibility of the caller to allocate a buffer for Body and
> specify the size in BodyLength. If the remote host provides a response
> that contains a content body, up to BodyLength bytes will be copied from
> the receive buffer into Body and BodyLength will be updated with the
> amount of bytes received and copied to Body. This allows the client to
> download a large file in chunks instead of into one contiguous block of
> memory.

Note that, if the caller-allocated buffer is larger than the
server-provided chunk, then the transfer length is limited by the latter.
This is in fact the dominant case when downloading a huge file (for which
UefiBootManagerLib allocated a huge contiguous RAM Disk buffer) in small
TLS messages.

For adjusting BodyLength as described above -- i.e., to the application
data chunk that has been extracted from the TLS message --, the
HttpResponseWorker() function employs the following assignment:

    HttpMsg->BodyLength = MIN (Fragment.Len, (UINT32) HttpMsg->BodyLength);

The (UINT32) cast is motivated by the MIN() requirement -- in
"MdePkg/Include/Base.h" -- that both arguments be of the same type.

"Fragment.Len" (NET_FRAGMENT.Len) has type UINT32, and
"HttpMsg->BodyLength" (EFI_HTTP_MESSAGE.BodyLength) has type UINTN.
Therefore a cast is indeed necessary.

Unfortunately, the cast is done in the wrong direction. Consider the
following circumstances:

- "Fragment.Len" happens to be consistently 16KiB, dictated by the HTTPS
  Server's TLS stack,

- the size of the file to download is 4GiB + N*16KiB, where N is a
  positive integer.

As the download progresses, each received 16KiB application data chunk
brings the *next* input value of BodyLength closer down to 4GiB. The cast
in MIN() always masks off the high-order bits from the input value of
BodyLength, but this is no problem because the low-order bits are nonzero,
therefore the MIN() always permits progress.

However, once BodyLength reaches 4GiB exactly on input, the MIN()
invocation produces a zero value. HttpResponseWorker() adjusts the output
value of BodyLength to zero, and then passes it to HttpParseMessageBody().

HttpParseMessageBody() (in "NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c")
rejects the zero BodyLength with EFI_INVALID_PARAMETER, which is fully
propagated outwards, and aborts the HTTPS download. HttpBootDxe writes the
message "Error: Unexpected network error" to the UEFI console.

For example, a file with size (4GiB + 197MiB) terminates after downloading
just 197MiB.

Invert the direction of the cast: widen "Fragment.Len" to UINTN.

Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 NetworkPkg/HttpDxe/HttpImpl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c
index 6b877314bd57..1acbb60d1014 100644
--- a/NetworkPkg/HttpDxe/HttpImpl.c
+++ b/NetworkPkg/HttpDxe/HttpImpl.c
@@ -1348,7 +1348,7 @@ HttpResponseWorker (
     //
     // Process the received the body packet.
     //
-    HttpMsg->BodyLength = MIN (Fragment.Len, (UINT32) HttpMsg->BodyLength);
+    HttpMsg->BodyLength = MIN ((UINTN) Fragment.Len, HttpMsg->BodyLength);
 
     CopyMem (HttpMsg->Body, Fragment.Bulk, HttpMsg->BodyLength);
 
-- 
2.19.1.3.g30247aa5d201


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53036): https://edk2.groups.io/g/devel/message/53036
Mute This Topic: https://groups.io/mt/69550085/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix 32-bit truncation in HTTPS download
Posted by Philippe Mathieu-Daudé 6 years, 1 month ago 
On 1/9/20 12:43 AM, Laszlo Ersek wrote:
> When downloading over TLS, each TLS message ("APP packet") is returned as
> a (decrypted) fragment table by EFI_TLS_PROTOCOL.ProcessPacket().
> 
> The TlsProcessMessage() function in "NetworkPkg/HttpDxe/HttpsSupport.c"
> linearizes the fragment table into a single contiguous data block. The
> resultant flat data block contains both TLS headers and data.
> 
> The HttpsReceive() function parses the actual application data -- in this
> case: decrypted HTTP data -- out of the flattened TLS data block, peeling
> off the TLS headers.
> 
> The HttpResponseWorker() function in "NetworkPkg/HttpDxe/HttpImpl.c"
> propagates this HTTP data outwards, implementing the
> EFI_HTTP_PROTOCOL.Response() function.
> 
> Now consider the following documentation for EFI_HTTP_PROTOCOL.Response(),
> quoted from "MdePkg/Include/Protocol/Http.h":
> 
>> It is the responsibility of the caller to allocate a buffer for Body and
>> specify the size in BodyLength. If the remote host provides a response
>> that contains a content body, up to BodyLength bytes will be copied from
>> the receive buffer into Body and BodyLength will be updated with the
>> amount of bytes received and copied to Body. This allows the client to
>> download a large file in chunks instead of into one contiguous block of
>> memory.
> 
> Note that, if the caller-allocated buffer is larger than the
> server-provided chunk, then the transfer length is limited by the latter.
> This is in fact the dominant case when downloading a huge file (for which
> UefiBootManagerLib allocated a huge contiguous RAM Disk buffer) in small
> TLS messages.
> 
> For adjusting BodyLength as described above -- i.e., to the application
> data chunk that has been extracted from the TLS message --, the
> HttpResponseWorker() function employs the following assignment:
> 
>      HttpMsg->BodyLength = MIN (Fragment.Len, (UINT32) HttpMsg->BodyLength);
> 
> The (UINT32) cast is motivated by the MIN() requirement -- in
> "MdePkg/Include/Base.h" -- that both arguments be of the same type.
> 
> "Fragment.Len" (NET_FRAGMENT.Len) has type UINT32, and
> "HttpMsg->BodyLength" (EFI_HTTP_MESSAGE.BodyLength) has type UINTN.
> Therefore a cast is indeed necessary.
> 
> Unfortunately, the cast is done in the wrong direction. Consider the
> following circumstances:
> 
> - "Fragment.Len" happens to be consistently 16KiB, dictated by the HTTPS
>    Server's TLS stack,
> 
> - the size of the file to download is 4GiB + N*16KiB, where N is a
>    positive integer.
> 
> As the download progresses, each received 16KiB application data chunk
> brings the *next* input value of BodyLength closer down to 4GiB. The cast
> in MIN() always masks off the high-order bits from the input value of
> BodyLength, but this is no problem because the low-order bits are nonzero,
> therefore the MIN() always permits progress.
> 
> However, once BodyLength reaches 4GiB exactly on input, the MIN()
> invocation produces a zero value. HttpResponseWorker() adjusts the output
> value of BodyLength to zero, and then passes it to HttpParseMessageBody().
> 
> HttpParseMessageBody() (in "NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c")
> rejects the zero BodyLength with EFI_INVALID_PARAMETER, which is fully
> propagated outwards, and aborts the HTTPS download. HttpBootDxe writes the
> message "Error: Unexpected network error" to the UEFI console.

Thanks for the very detailed explanation.

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

> 
> For example, a file with size (4GiB + 197MiB) terminates after downloading
> just 197MiB.
> 
> Invert the direction of the cast: widen "Fragment.Len" to UINTN.
> 
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>   NetworkPkg/HttpDxe/HttpImpl.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c
> index 6b877314bd57..1acbb60d1014 100644
> --- a/NetworkPkg/HttpDxe/HttpImpl.c
> +++ b/NetworkPkg/HttpDxe/HttpImpl.c
> @@ -1348,7 +1348,7 @@ HttpResponseWorker (
>       //
>       // Process the received the body packet.
>       //
> -    HttpMsg->BodyLength = MIN (Fragment.Len, (UINT32) HttpMsg->BodyLength);
> +    HttpMsg->BodyLength = MIN ((UINTN) Fragment.Len, HttpMsg->BodyLength);
>   
>       CopyMem (HttpMsg->Body, Fragment.Bulk, HttpMsg->BodyLength);
>   
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53081): https://edk2.groups.io/g/devel/message/53081
Mute This Topic: https://groups.io/mt/69550085/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix 32-bit truncation in HTTPS download
Posted by Siyuan, Fu 6 years, 1 month ago 
Reviewed-by: Siyuan Fu <siyuan.fu@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> Ersek
> Sent: 2020年1月9日 7:43
> To: edk2-devel-groups-io <devel@edk2.groups.io>
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; Maciej Rabeda
> <maciej.rabeda@linux.intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> Subject: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix 32-bit truncation
> in HTTPS download
> 
> When downloading over TLS, each TLS message ("APP packet") is returned as
> a (decrypted) fragment table by EFI_TLS_PROTOCOL.ProcessPacket().
> 
> The TlsProcessMessage() function in "NetworkPkg/HttpDxe/HttpsSupport.c"
> linearizes the fragment table into a single contiguous data block. The
> resultant flat data block contains both TLS headers and data.
> 
> The HttpsReceive() function parses the actual application data -- in this
> case: decrypted HTTP data -- out of the flattened TLS data block, peeling
> off the TLS headers.
> 
> The HttpResponseWorker() function in "NetworkPkg/HttpDxe/HttpImpl.c"
> propagates this HTTP data outwards, implementing the
> EFI_HTTP_PROTOCOL.Response() function.
> 
> Now consider the following documentation for
> EFI_HTTP_PROTOCOL.Response(),
> quoted from "MdePkg/Include/Protocol/Http.h":
> 
> > It is the responsibility of the caller to allocate a buffer for Body and
> > specify the size in BodyLength. If the remote host provides a response
> > that contains a content body, up to BodyLength bytes will be copied from
> > the receive buffer into Body and BodyLength will be updated with the
> > amount of bytes received and copied to Body. This allows the client to
> > download a large file in chunks instead of into one contiguous block of
> > memory.
> 
> Note that, if the caller-allocated buffer is larger than the
> server-provided chunk, then the transfer length is limited by the latter.
> This is in fact the dominant case when downloading a huge file (for which
> UefiBootManagerLib allocated a huge contiguous RAM Disk buffer) in small
> TLS messages.
> 
> For adjusting BodyLength as described above -- i.e., to the application
> data chunk that has been extracted from the TLS message --, the
> HttpResponseWorker() function employs the following assignment:
> 
>     HttpMsg->BodyLength = MIN (Fragment.Len, (UINT32) HttpMsg-
> >BodyLength);
> 
> The (UINT32) cast is motivated by the MIN() requirement -- in
> "MdePkg/Include/Base.h" -- that both arguments be of the same type.
> 
> "Fragment.Len" (NET_FRAGMENT.Len) has type UINT32, and
> "HttpMsg->BodyLength" (EFI_HTTP_MESSAGE.BodyLength) has type UINTN.
> Therefore a cast is indeed necessary.
> 
> Unfortunately, the cast is done in the wrong direction. Consider the
> following circumstances:
> 
> - "Fragment.Len" happens to be consistently 16KiB, dictated by the HTTPS
>   Server's TLS stack,
> 
> - the size of the file to download is 4GiB + N*16KiB, where N is a
>   positive integer.
> 
> As the download progresses, each received 16KiB application data chunk
> brings the *next* input value of BodyLength closer down to 4GiB. The cast
> in MIN() always masks off the high-order bits from the input value of
> BodyLength, but this is no problem because the low-order bits are nonzero,
> therefore the MIN() always permits progress.
> 
> However, once BodyLength reaches 4GiB exactly on input, the MIN()
> invocation produces a zero value. HttpResponseWorker() adjusts the output
> value of BodyLength to zero, and then passes it to HttpParseMessageBody().
> 
> HttpParseMessageBody() (in "NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c")
> rejects the zero BodyLength with EFI_INVALID_PARAMETER, which is fully
> propagated outwards, and aborts the HTTPS download. HttpBootDxe writes
> the
> message "Error: Unexpected network error" to the UEFI console.
> 
> For example, a file with size (4GiB + 197MiB) terminates after downloading
> just 197MiB.
> 
> Invert the direction of the cast: widen "Fragment.Len" to UINTN.
> 
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  NetworkPkg/HttpDxe/HttpImpl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/NetworkPkg/HttpDxe/HttpImpl.c
> b/NetworkPkg/HttpDxe/HttpImpl.c
> index 6b877314bd57..1acbb60d1014 100644
> --- a/NetworkPkg/HttpDxe/HttpImpl.c
> +++ b/NetworkPkg/HttpDxe/HttpImpl.c
> @@ -1348,7 +1348,7 @@ HttpResponseWorker (
>      //
>      // Process the received the body packet.
>      //
> -    HttpMsg->BodyLength = MIN (Fragment.Len, (UINT32) HttpMsg-
> >BodyLength);
> +    HttpMsg->BodyLength = MIN ((UINTN) Fragment.Len, HttpMsg-
> >BodyLength);
> 
>      CopyMem (HttpMsg->Body, Fragment.Bulk, HttpMsg->BodyLength);
> 
> --
> 2.19.1.3.g30247aa5d201
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53103): https://edk2.groups.io/g/devel/message/53103
Mute This Topic: https://groups.io/mt/69550085/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix 32-bit truncation in HTTPS download
Posted by Laszlo Ersek 6 years, 1 month ago 
On 01/10/20 02:01, Siyuan, Fu wrote:
> Reviewed-by: Siyuan Fu <siyuan.fu@intel.com>

Thanks, Siyuan! Can you please review patch#1 too in this series? (See
Ray's request: <https://edk2.groups.io/g/devel/message/53044>.)

Thanks!
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53155): https://edk2.groups.io/g/devel/message/53155
Mute This Topic: https://groups.io/mt/69550085/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix 32-bit truncation in HTTPS download
Posted by Maciej Rabeda 6 years, 1 month ago 
Thanks for a very thorough explanation!

Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>

On 09-Jan-20 00:43, Laszlo Ersek wrote:
> When downloading over TLS, each TLS message ("APP packet") is returned as
> a (decrypted) fragment table by EFI_TLS_PROTOCOL.ProcessPacket().
>
> The TlsProcessMessage() function in "NetworkPkg/HttpDxe/HttpsSupport.c"
> linearizes the fragment table into a single contiguous data block. The
> resultant flat data block contains both TLS headers and data.
>
> The HttpsReceive() function parses the actual application data -- in this
> case: decrypted HTTP data -- out of the flattened TLS data block, peeling
> off the TLS headers.
>
> The HttpResponseWorker() function in "NetworkPkg/HttpDxe/HttpImpl.c"
> propagates this HTTP data outwards, implementing the
> EFI_HTTP_PROTOCOL.Response() function.
>
> Now consider the following documentation for EFI_HTTP_PROTOCOL.Response(),
> quoted from "MdePkg/Include/Protocol/Http.h":
>
>> It is the responsibility of the caller to allocate a buffer for Body and
>> specify the size in BodyLength. If the remote host provides a response
>> that contains a content body, up to BodyLength bytes will be copied from
>> the receive buffer into Body and BodyLength will be updated with the
>> amount of bytes received and copied to Body. This allows the client to
>> download a large file in chunks instead of into one contiguous block of
>> memory.
> Note that, if the caller-allocated buffer is larger than the
> server-provided chunk, then the transfer length is limited by the latter.
> This is in fact the dominant case when downloading a huge file (for which
> UefiBootManagerLib allocated a huge contiguous RAM Disk buffer) in small
> TLS messages.
>
> For adjusting BodyLength as described above -- i.e., to the application
> data chunk that has been extracted from the TLS message --, the
> HttpResponseWorker() function employs the following assignment:
>
>      HttpMsg->BodyLength = MIN (Fragment.Len, (UINT32) HttpMsg->BodyLength);
>
> The (UINT32) cast is motivated by the MIN() requirement -- in
> "MdePkg/Include/Base.h" -- that both arguments be of the same type.
>
> "Fragment.Len" (NET_FRAGMENT.Len) has type UINT32, and
> "HttpMsg->BodyLength" (EFI_HTTP_MESSAGE.BodyLength) has type UINTN.
> Therefore a cast is indeed necessary.
>
> Unfortunately, the cast is done in the wrong direction. Consider the
> following circumstances:
>
> - "Fragment.Len" happens to be consistently 16KiB, dictated by the HTTPS
>    Server's TLS stack,
>
> - the size of the file to download is 4GiB + N*16KiB, where N is a
>    positive integer.
>
> As the download progresses, each received 16KiB application data chunk
> brings the *next* input value of BodyLength closer down to 4GiB. The cast
> in MIN() always masks off the high-order bits from the input value of
> BodyLength, but this is no problem because the low-order bits are nonzero,
> therefore the MIN() always permits progress.
>
> However, once BodyLength reaches 4GiB exactly on input, the MIN()
> invocation produces a zero value. HttpResponseWorker() adjusts the output
> value of BodyLength to zero, and then passes it to HttpParseMessageBody().
>
> HttpParseMessageBody() (in "NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c")
> rejects the zero BodyLength with EFI_INVALID_PARAMETER, which is fully
> propagated outwards, and aborts the HTTPS download. HttpBootDxe writes the
> message "Error: Unexpected network error" to the UEFI console.
>
> For example, a file with size (4GiB + 197MiB) terminates after downloading
> just 197MiB.
>
> Invert the direction of the cast: widen "Fragment.Len" to UINTN.
>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>   NetworkPkg/HttpDxe/HttpImpl.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c
> index 6b877314bd57..1acbb60d1014 100644
> --- a/NetworkPkg/HttpDxe/HttpImpl.c
> +++ b/NetworkPkg/HttpDxe/HttpImpl.c
> @@ -1348,7 +1348,7 @@ HttpResponseWorker (
>       //
>       // Process the received the body packet.
>       //
> -    HttpMsg->BodyLength = MIN (Fragment.Len, (UINT32) HttpMsg->BodyLength);
> +    HttpMsg->BodyLength = MIN ((UINTN) Fragment.Len, HttpMsg->BodyLength);
>   
>       CopyMem (HttpMsg->Body, Fragment.Bulk, HttpMsg->BodyLength);
>   

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53162): https://edk2.groups.io/g/devel/message/53162
Mute This Topic: https://groups.io/mt/69550085/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.