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.web10.15.1571069752894943700 for ; Mon, 14 Oct 2019 09:15:53 -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-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 6519C307CDC0; Mon, 14 Oct 2019 16:15:52 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-254.ams2.redhat.com [10.36.117.254]) by smtp.corp.redhat.com (Postfix) with ESMTP id 84DB1194B6; Mon, 14 Oct 2019 16:15:50 +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> <5bbadb29-36f2-1054-fd41-5577d59c9290@redhat.com> From: "Laszlo Ersek" Message-ID: <5c33b6c2-c8b0-aa64-a85f-06bdc3c69843@redhat.com> Date: Mon, 14 Oct 2019 18:15:49 +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: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.49]); Mon, 14 Oct 2019 16:15:52 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/11/19 18:01, David Woodhouse wrote: > On Fri, 2019-10-11 at 17:36 +0200, Laszlo Ersek wrote: >> On 10/11/19 13:16, David Woodhouse wrote: >>> 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. > > https://www.mail-archive.com/devel@edk2.groups.io/msg03339.html Thank you. IIUC, Sivaraman attempted to integrate the patches from TianoCore#960, and then they ran into the SAN/GEN_IP regression *in practice*. Unfortunately, in retrospect, that has been a *complete* communication fiasco. - Sivaraman tested the patches from TianoCore#960, but he did not report his findings (the regression) in the Bugzilla entry. - Conversely, the discussion on the list was missed by Jiaxin. - I participated in both places, and I failed to realize we were talking about any kind of regression. (To be honest, even if I re-read the initial message from Sivaraman, I still find it very difficult to understand: When we create certificates with CN as Host Name and SAN as IP TLS Handshake works only for Host Name and it provides Handshake Error when the request are IP Based. Even a *modicum* of punctuation would have helped with that *soup* of acronyms. Such as: When we create certificates with CN as Host Name, and SAN as IP, TLS Handshake works only for Host Name, and it provides Handshake Error when the request are IP Based. "SAN as IP" is the expression with 90% of the information content, and it received a fraction of the space / focus. Not to mention, back then I had zero idea about "Subject Alternative Name" and "GEN_IP".) > In that thread you pointed me at the bug, and I immediately pointed out > the error in the patch series: > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c31 That's correct. The word "regression" was not uttered however. From the rest of the discussion in the BZ, I gather that Jiaxin never treated the problem as a regression. If this were not a practical regression, I'd agree with Jiaxin that we should consider the SAN/GEN_IP use case on top of the present patch set, optionally, separately, incrementally. But, with evidence that AMI reported their practical regression back in June -- even though they failed to (a) describe their use case clearly, (b) state that it was a regression, (c) make the statement in the same place where the patches existed --, I'm now inclined to reverse my position. *Unless* Jiaxin can show that what AMI is trying to do is outside of the UEFI spec (v2.8), I agree we should fix the SAN/GEN_IP case in *this* series. (The spec writes, EfiTlsVerifyHost -- TLS session hostname for validation which is used to verify whether the name within the peer certificate matches a given host name. [...] The "given host name" expression (EFI_TLS_VERIFY_HOST.HostName) seems to map to "URL used in request". The question is whether the "name within the peer certificate" part includes GEN_IP in the Subject Alternative Name. If it does, then this series is not complete.) > Followed by a bit more detail on how to fix it, with examples to look > at: > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32 > >> David: it *is* hard! It is hard for me. I wouldn't know where to begin. > > I suspect this is false modesty on your part. Nope. > Given the pointers and > the examples above, I have lots of confidence that if this were the > task on your plate, you would accomplish it with ease. Definitely not with ease. > > I would, of course, be happy to provide further pointers, and even work > with upstream OpenSSL to make this even easier. Crypto libraries should > make it hard for application developers to get things wrong, and they > often let us down in that respect. > > In fact, I did that last bit already: > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c33 > >> As always, I strongly favor "upstream first". Show us the code, please? > > It's already linked from that Bugzilla comment I referenced: > https://github.com/openssl/openssl/pull/9201 > > Pull that into your OpenSSL tree, then make a trivial change following > the example in that PR, to do > > if (SSL_set1_ip_asc(ssl, hostname) < 0) > SSL_set1_host(ssl, hostname); > > instead of just the SSL_set1_host() call. > > That way, *if* the string happens to be a valid IPv6 or Legacy IP > address, the SSL_set1_ip_asc() call will work; otherwise it's treated > as a hostname. My understanding is that a fix purely in edk2 -- that is, without advancing our openssl submodule reference at once -- is possible, based on your comment https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32 Namely, edk2 commit 9396cdfeaa7a ("CryptoPkg: Add new TlsLib library", 2016-12-22) added a SSL_set_verify() call (in function TlsSetVerify()). The last argument of that call is currently NULL. We should change that, to a callback function that implements what ssl_app_verify_callback() and match_cert_hostname() do, in your source file http://git.infradead.org/users/dwmw2/openconnect.git/blob/HEAD:/openssl.c There seems to be a GEN_* switch inside a loop in there. Jiaxin: I think David has a point. AMI's use case appears covered by the UEFI-2.8 spec, to me anyway (though I'm not an OpenSSL expert!!!), and they've encountered a practical regression with this patch set. Do you think you can implement the verify callback outlined by David above, or do you prefer if we wait until we can consume David's OpenSSL fix through the next openssl submodule rebase (and then the edk2 change is minimal)? David: another way to prevent the regression is to commit the current patches, but disable them with a BOOLEAN PCD, by default. (This need not be a feature PCD; it could even be dynamic.) Then platforms accepting the SAN/GEN_IP regression temporarily could enable the PCD. This solution would permit a separate (follow-up) series for the SAN/GEN_IP case. We could file a reminder BZ now, and implement the "easy" solution when we next rebase the openssl submodule. Would that be tolerable? Thanks, Laszlo