From 555d93f2daa551dc2311b15210a918aa79ed18ff Mon Sep 17 00:00:00 2001

From: Laszlo Ersek <lersek@redhat.com>

Date: Tue, 14 Jan 2020 12:39:06 +0100

Subject: [PATCH 2/2] NetworkPkg/HttpDxe: fix 32-bit truncation in HTTPS

 download

MIME-Version: 1.0

Content-Type: text/plain; charset=UTF-8

Content-Transfer-Encoding: 8bit

RH-Author: Laszlo Ersek <lersek@redhat.com>

Message-id: <20200114123906.8547-3-lersek@redhat.com>

Patchwork-id: 93340

O-Subject: [RHEL-8.2.0 edk2 PATCH 2/2] NetworkPkg/HttpDxe: fix 32-bit truncation in HTTPS download

Bugzilla: 1789797

RH-Acked-by: Vitaly Kuznetsov <vkuznets@redhat.com>

RH-Acked-by: Philippe Mathieu-Daudé <philmd@redhat.com>

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>

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

Reviewed-by: Siyuan Fu <siyuan.fu@intel.com>

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

(cherry picked from commit 4cca7923992a13f6b753782f469ee944da2db796)

Signed-off-by: Miroslav Rezanina <mrezanin@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 6b87731..1acbb60 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);

-- 

1.8.3.1