Reviewed-by: Siyuan Fu <siyuan...@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...@intel.com>; Maciej Rabeda
> <maciej.rab...@linux.intel.com>; Fu, Siyuan <siyuan...@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...@intel.com>
> Cc: Maciej Rabeda <maciej.rab...@linux.intel.com>
> Cc: Siyuan Fu <siyuan...@intel.com>
> Signed-off-by: Laszlo Ersek <ler...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
