From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by mx.groups.io with SMTP id smtpd.web10.11618.1578670284155107733 for ; Fri, 10 Jan 2020 07:31:24 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: linux.intel.com, ip: 134.134.136.126, mailfrom: maciej.rabeda@linux.intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Jan 2020 07:31:23 -0800 X-IronPort-AV: E=Sophos;i="5.69,417,1571727600"; d="scan'208";a="223796550" Received: from mrabeda-mobl.ger.corp.intel.com (HELO [10.102.8.43]) ([10.102.8.43]) by orsmga006-auth.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-SHA; 10 Jan 2020 07:31:22 -0800 Subject: Re: [edk2-devel] [PATCH 2/2] NetworkPkg/HttpDxe: fix 32-bit truncation in HTTPS download To: devel@edk2.groups.io, lersek@redhat.com Cc: Jiaxin Wu , Siyuan Fu References: <20200108234313.28510-1-lersek@redhat.com> <20200108234313.28510-3-lersek@redhat.com> From: "Maciej Rabeda" Message-ID: Date: Fri, 10 Jan 2020 16:31:16 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.3.0 MIME-Version: 1.0 In-Reply-To: <20200108234313.28510-3-lersek@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: pl Thanks for a very thorough explanation! Reviewed-by: Maciej Rabeda 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 > Cc: Maciej Rabeda > Cc: Siyuan Fu > Signed-off-by: Laszlo Ersek > --- > 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); >