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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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);
>   
>