[edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix 32-bit truncation in HTTPS download
Philippe Mathieu-Daudé
philmd at redhat.com
Thu Jan 9 13:29:16 UTC 2020
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 at 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 at intel.com>
> Cc: Maciej Rabeda <maciej.rabeda at linux.intel.com>
> Cc: Siyuan Fu <siyuan.fu at intel.com>
> Signed-off-by: Laszlo Ersek <lersek at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list