From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web10.8335.1578576563910913997 for ; Thu, 09 Jan 2020 05:29:24 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=LpSIDL98; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: philmd@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1578576563; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=wyU6CQGlGu73KcvMPmJHsuaZSZz1w4dxcErTNignN+k=; b=LpSIDL9862YY7Nhdbv7zZeBk+BIjJXdKBVLp7QR0fdVZERCFHgsb+8derYsKVtzM7Yir1E Jz25/l9W6aLQqf6Vm9Igk3DHEmi/kZ/OgjDSwFExe87nAUt68QGomZZ8tFlYaM9LKvzAPm grbowWE2PXVEr28tLHKWDI7zYck3mL0= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-211-GjpSO1Y9MvOFEoeMDWMNJA-1; Thu, 09 Jan 2020 08:29:19 -0500 Received: by mail-wm1-f69.google.com with SMTP id w205so919632wmb.5 for ; Thu, 09 Jan 2020 05:29:19 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=wyU6CQGlGu73KcvMPmJHsuaZSZz1w4dxcErTNignN+k=; b=PcFb8V/Y+J2vM5GTgd4wSn2V4yjMJFe41NW2wygehgZjJjYSXpUobKZ9BhaehYd4zf garTob7JLIhMWCjFHHHcaNFusIUqHWF6gO8reUOs57G32BpK+xksDoNLGrGeS9mlI3dn 4NsvhFnm/dEqnJ6kxvzvlK90uHc2ZeEo+rdYBYcSjSfqnp+5RJUlkuVfjVmLEIuzyoIz fDFBsQ8cTCrVfNjDAwopVz0MPKqUOlMaZzq+6Gv0ja4cQ5JIWxBwPY85+FVCfQ8hk93/ hXQAn6ShiLWrVpPBk+Ux7QKkClq1p931g7FNaLiBQHSQkFh3neKRWCAL1HJSM6oq2uz4 x6bQ== X-Gm-Message-State: APjAAAU7reCgEzt14xxnF3EVp85h+T9jJ1klFf3q8LgTRZSHY+L98osP SPuxD2rW9otrZsAO51GzrI9ZtSMsCEc2E8zeg435Qfdg54vZan+lREgmuG7o47ueB71mmRBU2L1 /K+YB73J+O+nRJg== X-Received: by 2002:a05:6000:11c9:: with SMTP id i9mr10739318wrx.164.1578576558602; Thu, 09 Jan 2020 05:29:18 -0800 (PST) X-Google-Smtp-Source: APXvYqw77SYPV8K4ZBsG5gO6equiNnjzFreNiyNLA77SBAi66GhEdnRct4/sshOb/xU1NPHitwfp3w== X-Received: by 2002:a05:6000:11c9:: with SMTP id i9mr10739295wrx.164.1578576558340; Thu, 09 Jan 2020 05:29:18 -0800 (PST) Return-Path: Received: from [192.168.1.35] (113.red-83-57-172.dynamicip.rima-tde.net. [83.57.172.113]) by smtp.gmail.com with ESMTPSA id m10sm8152890wrx.19.2020.01.09.05.29.17 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 09 Jan 2020 05:29:17 -0800 (PST) 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 , Maciej Rabeda , Siyuan Fu References: <20200108234313.28510-1-lersek@redhat.com> <20200108234313.28510-3-lersek@redhat.com> From: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= Message-ID: <092026eb-df04-8c50-af51-b9e1874a4b68@redhat.com> Date: Thu, 9 Jan 2020 14:29:16 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <20200108234313.28510-3-lersek@redhat.com> X-MC-Unique: GjpSO1Y9MvOFEoeMDWMNJA-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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 > > 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); > >