From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by mx.groups.io with SMTP id smtpd.web11.1844.1570808168163836851 for ; Fri, 11 Oct 2019 08:36:08 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id A17943024562; Fri, 11 Oct 2019 15:36:07 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-177.rdu2.redhat.com [10.10.120.177]) by smtp.corp.redhat.com (Postfix) with ESMTP id E13D75C1B2; Fri, 11 Oct 2019 15:36:05 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553) To: David Woodhouse , "Wu, Jiaxin" , "devel@edk2.groups.io" , "Wang, Jian J" , Bret Barkelew Cc: Richard Levitte References: <20190927034441.3096-1-Jiaxin.wu@intel.com> <69774fe6-ea00-44b9-5468-c092dea6cd36@redhat.com> <8106467c9f4132c831d0aa604e897fe9d4dda12a.camel@infradead.org> <895558F6EA4E3B41AC93A00D163B727416F5D921@SHSMSX107.ccr.corp.intel.com> <777053db79600eb90a19945700293d14f4978344.camel@infradead.org> <6bb5d2f6-ec6f-1766-e19b-03fd45c1bc12@redhat.com> <9A4966EE-76CD-465C-A6CA-70DD9E38D834@infradead.org> <850a81a8-2cdc-0708-4ff7-db9825fdaedc@redhat.com> <23699ae3-10c2-037c-b3f5-ac8f5bea1fb7@redhat.com> <895558F6EA4E3B41AC93A00D163B727416F7E4AB@SHSMSX107.ccr.corp.intel.com> <6939ba4e-6c77-0769-4ac2-c3ba1ea9a0b7@redhat.com> <44468659be80e9bf1886e7b6f8f3aa77044b5fd6.camel@infradead.org> From: "Laszlo Ersek" Message-ID: <5bbadb29-36f2-1054-fd41-5577d59c9290@redhat.com> Date: Fri, 11 Oct 2019 17:36:04 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <44468659be80e9bf1886e7b6f8f3aa77044b5fd6.camel@infradead.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.49]); Fri, 11 Oct 2019 15:36:07 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/11/19 13:16, David Woodhouse wrote: > On Fri, 2019-10-11 at 12:55 +0200, Laszlo Ersek wrote: >> On 10/11/19 04:24, Wu, Jiaxin wrote: >>> Hi Laszlo & David, >>> >>> I think I have *repeated* several times that we are targeting to fix the HostName validation issue, not the IP or email address. *But* even so, the series patches for UEFI TLS is also allowable to specify IP as host name for CN or dNSName of SAN in the certificate. That's why I said "if the CN or SAN in the certificate are set correctly, it should be OK to pass the verification". The failure you mentioned here is to set the IP in iPAddress of SAN, I agree it's the routine and suggested setting, *but* obviously, it's not the target we are supported according the implementation/description of TlsSetVerifyHost. We are targeting to the hostname verification, and meanwhile compatible with the IP in the URI (But need the *correct* certificate setting). >>> >>> IP addresses stored in the DNS names and CN are of cause ignored by X509_check_ip & X509_check_ip_asc(). >>> >>> Post my explain again: >>>> UEFI TLS only provides the *HostName* verification interface to upper driver (HttpDxe), >>>> not the IP/email verification capability. Please refer to UEFI Spec2.8 section 28.10.2: >>>> "...TLS session hostname for validation which is used to verify whether the name >>>> within the peer certificate matches a given host name..." >>>> In upper UEFI HTTP driver, we get the hostname from URI directly no matter it's the real >>>> FQDN (www.xxx.com) or IP address format string (1.2.3.4 or 2001:8b0:10b::5 (not "[2001:8b0:10b::5])), >>>> and set it to the TLS hostname filed via the interface -- EFI_TLS_VERIFY_HOST. >>>> That's implementation choice for HttpDxe to achieve the HTTPS HostName validation feature >>>> by following the standard TLS HostName verification capability. >>> >>> Now, let's talking the iPAddress setting in SAN (same as email address), if you do need such feature that verify the IP in X509_check_ip & X509_check_ip_asc , please report new Bugzilla (need TLS Spec update the expose the setting interface), don't mix up the HTTPS hostname verification here. >> >> (To clarify my stance.) >> >> Given the above statement of scope, and given the test env details that >> I included in my previous message: >> >> series >> Tested-by: Laszlo Ersek >> >> I understand that my testing does not cover the use case described by David. >> >> So my Tested-by is certainly *not* intended as the "last word" in this >> thread, or anything like that. I'm only saying this patch set is good >> enough for me, not that everyone should find it good enough for them. > > > As you wish. > > Note for the record that that this is a functional regression. We go > from accepting all certs regardless of the target/subject, to rejecting > some valid certificates. Indeed that is the case; I'm aware of it. Thanks for pointing it out. This functional regression does not translate to an actual end-user regression, under my circumstances. From my perspective, UEFI HTTPS boot is a feature that cannot be enabled *at all* in a production environment, due to the possible MITM. In other words, I cannot build OVMF with -D NETWORK_HTTP_BOOT_ENABLE -D NETWORK_TLS_ENABLE and give the binary to users with a clear conscience. They'd see UEFI HTTPS boot, say "hey this is secure!", and use it. That would be a false sense of security. With the patches applied, yes, some valid certificates would be rejected. That would be a normal bug report, or a known limitation, or even a "layered" feature request. Not a security bug. It would not prevent the basic -- or let's say, "somewhat restricted" -- enablement of UEFI HTTPS boot. Not a deal-breaker. It would be possible for us to announce, "and now we're giving you UEFI HTTPS boot that is secure to the best of our knowledge, only it does not *yet* support GEN_IP in SAN. Stay tuned for that." This would not be a regression, from an end-user perspective. Again, I'm not attempting to "override" or "suppress" your (implied) NACK. If the agreement turns out to be that we should *first* add support for GEN_IP in the SAN, I will not campaign for merging the v1 patches *upstream*, right now. I respect your feedback. No patch should be merged while there are outstanding questions, let alone well-founded NACKs. > I first started looking at this when it was > reported as such, on the list. I believe you. Can you somehow find that thread? I tried, but I couldn't find it. My mailbox (going back 9 years) is indexed, but my searches have failed. I must be using the wrong search terms. If I try "GEN_IP" or "Subject Alternative Name", I only get this thread. > Given the length of time it's taken to fix this CVE already, I > understand that Laszlo is keen to get *something* committed, at last. More precisely, I'd be happy to see patches committed that plug the security hole. Which has been *completely* preventing us from enabling this feature. I agree that, while working towards a feature or a bugfix, already functional things should *not* be regressed -- even temporarily, even if the regression lasts only from patch#n to patch#(n+1) in a single series. This is a very strong guideline, minimally for bisectability. But to me, this feature has never existed in practice. I found and reported the security issue (BZ#960) while working on the downstream enablement of UEFI HTTPS boot. The problem caused me to postpone the downstream enablement. So for me there's nothing to regress. Similarly, knowledge of this issue prevented me from porting NETWORK_TLS_ENABLE from OvmfPkg to ArmVirtPkg, up-stream: https://bugzilla.tianocore.org/show_bug.cgi?id=1007 https://bugzilla.tianocore.org/show_bug.cgi?id=1009 For a long time, I couldn't publicly tell the truth about splitting BZ#1009 off of BZ#1007. My *real* incentive was to save at least ArmVirtPkg users from exposure. I called Ard on the phone, and we agreed to postpone TLS in ArmVirPkg. (I couldn't do the same for OVMF users, as I would have had to back out TLS settings from OvmfPkg, and that would have drawn stares.) I went on to intentionally ignore BZ#1009, hoping no-one would ask me about it. Then Guillaume did. See my answer here: . Therefore, UEFI HTTPS boot has not existed for me, in practice, even in the *upstream* project, to the extent that I was able to block the "spreading" of the feature. > But I stand by the assertion that this could be done properly, without > the regression, with much less additional typing even than we've > already done in this thread the last couple of days. It just isn't that > hard. David: it *is* hard! It is hard for me. I wouldn't know where to begin. I assume it may be hard for Jiaxin too. If it's not hard for you, can you please post the patches? Feel free to pick up this series as a starting point, rework it, and then post an updated variant. If it takes two more weeks and you can solve the problem to everyone's satisfaction (including your own!), I'll be overjoyed. I'll be happy to wait! If it takes four more months... then I might apply the present patchset down-stream only. As always, I strongly favor "upstream first". Show us the code, please? > Ultimately, though, do whatever you like. I am not the final arbiter of > engineering sanity and common sense for the EDK2 project. No well-founded NACK or pending question should be ignored in patch review. > > If I were, the world would be a very different place. No doubt! ;) Laszlo