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.2619.1571211421596961945 for ; Wed, 16 Oct 2019 00:37:01 -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-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E391410CC207; Wed, 16 Oct 2019 07:37:00 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-232.ams2.redhat.com [10.36.116.232]) by smtp.corp.redhat.com (Postfix) with ESMTP id CCA9E1001938; Wed, 16 Oct 2019 07:36:58 +0000 (UTC) Subject: Re: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses To: "Wu, Jiaxin" , "devel@edk2.groups.io" Cc: Bret Barkelew , David Woodhouse , "Wang, Jian J" , Richard Levitte , Sivaraman Nainar References: <20190927034441.3096-1-Jiaxin.wu@intel.com> <20191015230839.27708-1-lersek@redhat.com> <895558F6EA4E3B41AC93A00D163B727416F81251@SHSMSX107.ccr.corp.intel.com> From: "Laszlo Ersek" Message-ID: <56d17f5f-8433-2ec5-924c-bade642ac5a7@redhat.com> Date: Wed, 16 Oct 2019 09:36:57 +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: <895558F6EA4E3B41AC93A00D163B727416F81251@SHSMSX107.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.6.2 (mx1.redhat.com [10.5.110.65]); Wed, 16 Oct 2019 07:37:01 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Jiaxin, On 10/16/19 07:18, Wu, Jiaxin wrote: > These days I'm too busy with other things. Just take the time to > review the email discussion & researched the correct behavior of HTTPS > cert verification. > > I did never though my patch caused the function regression, and I'm > also not loop in the long ago email discussion reported from AMI. > > Now, I understand the testing *gap* between us is that we set the IP > address string in the CN or dNSName filed of SAN when creating the > certificate, while you don't prefer an IP address string setting in > dNSName but setting the OCTET string to iPAddress of SAN. That's why > the failure happen with different certificates setting (I've explained > it many times). You have misunderstood. It's not my *preference* to set an "OCTET string in the SAN iPAddress". It is so much *not* my preference that I didn't even know how to generate such a server certificate until David showed me! I gave Tested-by for your patch set because my personal validation mapped cleanly to your validation. In fact I think your validation was more extensive than mine, and so my testing was a *proper subset* of yours. But, that's not the whole story. When these patches were first exposed to the infosec group in TianoCore#960, in June, AMI reported a practical regression with them, to the list. Unfortunately, at that time, that regression report didn't reach you (it should have been made in the BZ, not on the list, because the patches were also attached to the BZ -- at that time, anyway). Therefore, we have to figure out what to do about the regression encountered by AMI. We have two options: - We can call their use case "invalid", assuming we can prove this from the UEFI spec. - We can attempt to accommodate their use case. The present patch is an *attempt* to accommodate their use case, based on David's guidance. Unfortunately, it does not work. (See the "Notes" section in the patch for the symptoms.) So we can decide to stop pursuing this use case, or we can continue researching it. We can *also* call AMI's use case invalid still, if we can prove that from the UEFI spec. To me, AMI's use case looked valid (= covered under the UEFI spec), but that's just me. We can contact the USWG for help with the spec interpretation. > With my patches to UEFI part, I believe you can pass the test if > setting the IP in CN or dNSName of SAN, Yes, I fully agree about that. That's why I gave Tested-by earlier in this thread, for your patches 1-4. Please see: http://mid.mail-archive.com/6939ba4e-6c77-0769-4ac2-c3ba1ea9a0b7@redhat.com https://edk2.groups.io/g/devel/message/48813 > but no one shared me any significant evidence to indicate it's the > mistake idea if URI is specified as an IP address rather than a > Hostname. I have not shared such evidence because I do not *believe* that storing the IP address in the Common Name of the certificate is a mistake. The "genkey" utility that Red Hat recommends (in the official docs) for setting up mod_ssl with Apache places the IP address in the Common Name, if the user enters an IP address. David argues that this is wrong, but I'm not convinced just yet. Therefore, I agree that your v1 patches 1-4 cover a valid use case. The issue is that they break a *different* use case, which *may* also be valid. (Dependent on how we interpret the UEFI spec.) > On the contrary, my explains to my thought/viewpoint when creating the > patch was treated as poor excuse:(. Not by me, for sure. > Now, I post my own investigation here to correct my mistake thought: > > According my previous experiment and RFC5280, the dNSName of > subjectAltName is IA5String, which means the string of an IPv4 or IPv6 > address is also legal to be included in the dNSName: > SubjectAltName ::= GeneralNames > GeneralName ::= CHOICE { > otherName [0] OtherName, > rfc822Name [1] IA5String, > dNSName [2] IA5String, > x400Address [3] ORAddress, > directoryName [4] Name, > ediPartyName [5] EDIPartyName, > uniformResourceIdentifier [6] IA5String, > iPAddress [7] OCTET STRING, > registeredID [8] OBJECT IDENTIFIER } > > If so, I thought it should be doable and make sense to treat the IP in > URI as hostname in CN or dNSName of SAN. Besides, it can also pass the > verification if the certificate only has the CN (NO SAN case, I think > it's also the failure case Laszlo met in Note(2)). I believed it's the > implementation choice before if no below finding in RFC 6125. But with > the series discussion here, I'm starting to wonder whether there is > any RFC or document to restrict the TLS & certificate checking roles , > for example: > 1) IP in the URI can be treated as Hostname or it only can be used for > the iPAddress of SAN verification? > 2) Certificate must have the iPAddress or dNSName of SAN? > > Fortunately, I get my wanted answer in RFC6125, SECTION 3.1 : > > If a subjectAltName extension of type dNSName is present, that MUST > be used as the identity. Otherwise, the (most specific) Common > Name field in the Subject field of the certificate MUST be used. > Although the use of the Common Name is existing practice, it is > deprecated and Certification Authorities are encouraged to use the > dNSName instead. > ... > In some cases, the URI is specified as an IP address rather than a > Hostname . In this case, the iPAddress subjectAltName must be > present in the certificate and must exactly match the IP in the > URI. Wow! This seems to prove David right, and it suggests that symptom (2) encountered with my patch is actually *valid* behavior -- the certificate I generated with "genkey" is *not* valid for a URI that specifies an IP address! This is not good news: the "curl" utility also accepts such a certificate as valid (IP address in URL, but the certificate only contains a CN, matching the IP address in textual form). Is that a bug in "curl" then? > So, the question is more clear here: > 1) IP in the URI is used for the iPAddress of SAN verification, but > *not say no* if upper driver set to dNSName of SAN. I'm not sure what you mean by "not say no if upper driver set to dNSName of SAN". Do you mean that we can ignore the IP address in the SAN if the URL contains a hostname? (And then the hostname matches either the dNSName in the SAN (high prio), or the CN (low prio)?) I agree about that. > 2) iPAddress of SAN MUST be present in the certificate and must > exactly match the IP in the URI. If not, we can treat as failure. Yes, I'm compelled to agree. > Ok, with above declaim in RFC, I agree there is the function issue in > my series patches to treat IP address in URI as string setting to > hostname, which breaks the above 2 rules defined in RFC 6125. > > Now, to resolve the problem, I think the best compatibility can > actually be reached by setting the IP address both as iPAddress and > dNSName in SAN. Wait, I don't understand the word "setting". "Setting" is an activity that the server admin performs. The UEFI TLS code is on the client side, and has no control over the elements of the certificate. Do you mean: the best compatibility can be reached by *matching* the IP address (from the URL) in both SAN.iPAddress and SAN.dNSName? I think I disagree with that. The part you quoted above from the RFC states that an IP-based URL can only be accepted via SAN.iPAddress. > To achieve that, we have two methods: > 1) TLS provides the separate interfaces to upper driver to set the > iPAddress & dNSName (even email address), which means HTTPS driver > need follow RFC6125. But that will need spec changes. I think this is not a good approach. David's pull request for OpenSSL, namely , makes the argument that recognizing and prioritizing an IP address over a DNS hostname should be *internal* to OpenSSL itself. https://github.com/openssl/openssl/pull/9201/commits/da280b92520f5a0e41efb777a4d39bc907c42ecf > diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c > index c481e292fc88..393e1a704d2e 100644 > --- a/ssl/ssl_lib.c > +++ b/ssl/ssl_lib.c > @@ -965,11 +965,21 @@ int SSL_set_trust(SSL *s, int trust) > > int SSL_set1_host(SSL *s, const char *hostname) > { > + /* If a hostname is provided and parses as an IP address, > + * treat it as such. */ > + if (hostname && X509_VERIFY_PARAM_set1_ip_asc(s->param, hostname) == 1) > + return 1; > + > return X509_VERIFY_PARAM_set1_host(s->param, hostname, 0); > } > > int SSL_add1_host(SSL *s, const char *hostname) > { > + /* If a hostname is provided and parses as an IP address, > + * treat it as such. */ > + if (hostname && X509_VERIFY_PARAM_set1_ip_asc(s->param, hostname) == 1) > + return 1; > + > return X509_VERIFY_PARAM_add1_host(s->param, hostname, 0); > } > Therefore we should not burden the HTTPS driver in edk2 with this; and we should even less complicate the UEFI spec with it. Back to your email: On 10/16/19 07:18, Wu, Jiaxin wrote: > 2) Just as David's suggestion and the patch send out by Laszlo, fix it > in UEFI TLS part. I reviewed the existing UEFI 2.8, looks we can treat > the HostName in Spec as the widely name (can be IP, DNS or email > address name). I don't understand how email enters the picture, but otherwise I agree. > To make things simple & easier, I also prefer 2). So, I reviewed the > patch from Laszlo: > > Comment1: How about setting the IP address both as iPAddress and > dNSName in SAN for the compatibility, if so, I think below note2 > failure will be gone. See above -- failure (2) appears valid, based on the part you quoted from the RFC. I'll have to generate a better certificate. Also, curl appears wrong to accept the certificate. (I'm stunned by this!) > Comment2: do we really need the app_verify_callback function setting? > Why not call X509_VERIFY_PARAM_set1_ip_asc (TlsConn->Ssl->param, > HostName) in TlsSetVerifyHost directly? anything I missed in the > discussion? I don't think client code should access "Ssl->param" directly. SSL should be treated as an opaque data structure. However, I think you may have a point. Formally, the SSL_get0_param() function could be called to retrieve X509_VERIFY_PARAM. https://www.openssl.org/docs/man1.1.1/man3/SSL_get0_param.html And then we could call X509_VERIFY_PARAM_set1_ip_asc() on that, perhaps. This would make both the ExData stuff and the custom certificate verification procedure unnecessary. TBH, I'm quite confused by the OpenSSL API: after all, just *what* is the object that X509_VERIFY_PARAM is associated with? Does it belong to the SSL object, or to the peer certificate (X509_STORE_CTX)? Thank you Jiaxin for following up! Laszlo