From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) by mx.groups.io with SMTP id smtpd.web11.3103.1571217557980102769 for ; Wed, 16 Oct 2019 02:19:20 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@infradead.org header.s=merlin.20170209 header.b=ueJrf5wq; spf=none, err=permanent DNS error (domain: merlin.srs.infradead.org, ip: 205.233.59.134, mailfrom: batv+ff4cde649f6f2a81df6b+5897+infradead.org+dwmw2@merlin.srs.infradead.org) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=merlin.20170209; h=Mime-Version:Content-Type:References: In-Reply-To:Date:Cc:To:From:Subject:Message-ID:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=OBJk6zL/T6L013FPdbsnBz3z7k9jRX/V2WPK+XRT7lE=; b=ueJrf5wq0by/c4nOHE35Uyquw 40LM9izZSeAzv6j7oqQUsm42ANwcFdvgn9gaBfGejL7x7wmkParKzfBVbGL4PfjXM62EOkheHY/vg dK/73sskNGs57Kgp/AV6wT9aRgX+p/q7/aY3oIKGmR0lFZ3dfwzOm860Qu7cpoAOcNTm+CCrTvJvz As4dRZyQPLjIl1pwCYlYnJ5Ox2oDMDxDFDNU3Jc86tTTUR1Yi8d5bb9LlUgcBGEOIWWB9QHdtzm6W FAmk+p+Ktr8VOX4H+VbAhWPotZXa3QD6Lwz2ODkGo0bjmy4ktOVNFiROltxlHHhIc4O5jKMqJw6D7 9JdFGzTqg==; Received: from [54.239.6.185] (helo=freeip.amazon.com) by merlin.infradead.org with esmtpsa (Exim 4.92.3 #3 (Red Hat Linux)) id 1iKfSX-0004kv-9N; Wed, 16 Oct 2019 09:19:09 +0000 Message-ID: <139da0c5a4684b76809fa19acc007f4699e3eb28.camel@infradead.org> Subject: Re: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses From: "David Woodhouse" To: Laszlo Ersek , "Wu, Jiaxin" , "devel@edk2.groups.io" Cc: Bret Barkelew , "Wang, Jian J" , Richard Levitte , Sivaraman Nainar Date: Wed, 16 Oct 2019 10:19:06 +0100 In-Reply-To: <56d17f5f-8433-2ec5-924c-bade642ac5a7@redhat.com> References: <20190927034441.3096-1-Jiaxin.wu@intel.com> <20191015230839.27708-1-lersek@redhat.com> <895558F6EA4E3B41AC93A00D163B727416F81251@SHSMSX107.ccr.corp.intel.com> <56d17f5f-8433-2ec5-924c-bade642ac5a7@redhat.com> X-Mailer: Evolution 3.28.5-0ubuntu0.18.04.1 Mime-Version: 1.0 X-SRS-Rewrite: SMTP reverse-path rewritten from by merlin.infradead.org. See http://www.infradead.org/rpr.html X-Groupsio-MsgNum: 49082 Content-Type: multipart/signed; micalg="sha-256"; protocol="application/x-pkcs7-signature"; boundary="=-5XHVqKwiFQOxBDAA7FQb" --=-5XHVqKwiFQOxBDAA7FQb Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 2019-10-16 at 09:36 +0200, Laszlo Ersek wrote: > On 10/16/19 07:18, Wu, Jiaxin wrote: > > 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. >=20 > Wow! >=20 > 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! >=20 > 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? Yes, I believe so. Please report it at https://github.com/curl/curl/issues > > 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. >=20 > 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. >=20 > https://github.com/openssl/openssl/pull/9201/commits/da280b92520f5a0e41= efb777a4d39bc907c42ecf Indeed; I think we all agree here. I believe it was presented as a straw man option just for completeness and even Jiaxin didn't seriously intend that we'd do it. But for the record, let me make it abundantly clear how wrong this option would be... I will *always* make the argument that crypto libraries should make it trivial for users to get things like this right. I'm dismayed that we even have to have this discussion. This stuff should Just Work=E2=84=A2. That applies just as much to the API that you present from the TlsLib. If you give your users a function that takes a hostname and silently does a completely bogus wrong thing when it's passed an IP address instead, that's a *bad* API design. Forcing users to work it out for themselves and call a *different* function if they happen to have an IP address, and still doing that bogus wrong thing if they call the original function, is just compounding the error. And yes, I'll argue quite strongly that OpenSSL shouldn't do that. OpenSSL did it because of the way its API evolved, and I will continue to have the discussion with them upstream about not designing APIs that set their users up for failure. It is an ongoing theme. But for UEFI? To even *propose* that we make such an appalling design choice in TlsLib? Just NO. Don't even contemplate it. As noted above, I choose to believe that it was only being suggested as a straw man, and only for completeness. I just wanted to make it *absolutely* clear that it was entirely unacceptable. We should apply that "don't set your users up for failure" principle everywhere. I will strongly assert, for example, that TlsLib shouldn't make the same mistake in its API design that led to CVE-2019-14553 in the first place. If a caller "forgets" to specify the hostname (or IP) that they're connecting to, the connection should FAIL. It shouldn't default to just connecting to anything. That's just batshit insane. If you have to fix the UEFI spec to make setting the hostname mandatory, then do so. Don't set your users up for failure by design. > > 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? >=20 > I don't think client code should access "Ssl->param" directly. SSL > should be treated as an opaque data structure. >=20 > However, I think you may have a point. Formally, the SSL_get0_param() > function could be called to retrieve X509_VERIFY_PARAM. >=20 > https://www.openssl.org/docs/man1.1.1/man3/SSL_get0_param.html >=20 > 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. Haha, I had *looked* for that but somehow failed to find it. I can't for the life of me work out how I missed it, now. Yes, that would be a whole lot easier than the callback and ex_data nonsense. It's really just as simple as X509_VERIFY_PARAM *vpm =3D SSL_get0_param(ssl); if (!X509_VERIFY_PARAM_set_ip_asc(vpm, hostname)) /* Didn't like that, must be an actual hostname then */ X509_VERIFY_PARAM_set1_host(vpm, hostname, 0); So what did we say about false modesty, Laszlo? In the end you did actually solve it all for yourself =E2=80=94 based on th= e pointer I'd given in bugzilla, and then ignoring my subsequent misdirection about callbacks and my overly complex attempt at doing it myself :) --=-5XHVqKwiFQOxBDAA7FQb Content-Type: application/x-pkcs7-signature; name="smime.p7s" Content-Disposition: attachment; filename="smime.p7s" Content-Transfer-Encoding: base64 MIAGCSqGSIb3DQEHAqCAMIACAQExDzANBglghkgBZQMEAgEFADCABgkqhkiG9w0BBwEAAKCCECow ggUcMIIEBKADAgECAhEA4rtJSHkq7AnpxKUY8ZlYZjANBgkqhkiG9w0BAQsFADCBlzELMAkGA1UE BhMCR0IxGzAZBgNVBAgTEkdyZWF0ZXIgTWFuY2hlc3RlcjEQMA4GA1UEBxMHU2FsZm9yZDEaMBgG A1UEChMRQ09NT0RPIENBIExpbWl0ZWQxPTA7BgNVBAMTNENPTU9ETyBSU0EgQ2xpZW50IEF1dGhl bnRpY2F0aW9uIGFuZCBTZWN1cmUgRW1haWwgQ0EwHhcNMTkwMTAyMDAwMDAwWhcNMjIwMTAxMjM1 OTU5WjAkMSIwIAYJKoZIhvcNAQkBFhNkd213MkBpbmZyYWRlYWQub3JnMIIBIjANBgkqhkiG9w0B AQEFAAOCAQ8AMIIBCgKCAQEAsv3wObLTCbUA7GJqKj9vHGf+Fa+tpkO+ZRVve9EpNsMsfXhvFpb8 RgL8vD+L133wK6csYoDU7zKiAo92FMUWaY1Hy6HqvVr9oevfTV3xhB5rQO1RHJoAfkvhy+wpjo7Q cXuzkOpibq2YurVStHAiGqAOMGMXhcVGqPuGhcVcVzVUjsvEzAV9Po9K2rpZ52FE4rDkpDK1pBK+ uOAyOkgIg/cD8Kugav5tyapydeWMZRJQH1vMQ6OVT24CyAn2yXm2NgTQMS1mpzStP2ioPtTnszIQ Ih7ASVzhV6csHb8Yrkx8mgllOyrt9Y2kWRRJFm/FPRNEurOeNV6lnYAXOymVJwIDAQABo4IB0zCC Ac8wHwYDVR0jBBgwFoAUgq9sjPjF/pZhfOgfPStxSF7Ei8AwHQYDVR0OBBYEFLfuNf820LvaT4AK xrGK3EKx1DE7MA4GA1UdDwEB/wQEAwIFoDAMBgNVHRMBAf8EAjAAMB0GA1UdJQQWMBQGCCsGAQUF BwMEBggrBgEFBQcDAjBGBgNVHSAEPzA9MDsGDCsGAQQBsjEBAgEDBTArMCkGCCsGAQUFBwIBFh1o dHRwczovL3NlY3VyZS5jb21vZG8ubmV0L0NQUzBaBgNVHR8EUzBRME+gTaBLhklodHRwOi8vY3Js LmNvbW9kb2NhLmNvbS9DT01PRE9SU0FDbGllbnRBdXRoZW50aWNhdGlvbmFuZFNlY3VyZUVtYWls Q0EuY3JsMIGLBggrBgEFBQcBAQR/MH0wVQYIKwYBBQUHMAKGSWh0dHA6Ly9jcnQuY29tb2RvY2Eu Y29tL0NPTU9ET1JTQUNsaWVudEF1dGhlbnRpY2F0aW9uYW5kU2VjdXJlRW1haWxDQS5jcnQwJAYI KwYBBQUHMAGGGGh0dHA6Ly9vY3NwLmNvbW9kb2NhLmNvbTAeBgNVHREEFzAVgRNkd213MkBpbmZy YWRlYWQub3JnMA0GCSqGSIb3DQEBCwUAA4IBAQALbSykFusvvVkSIWttcEeifOGGKs7Wx2f5f45b nv2ghcxK5URjUvCnJhg+soxOMoQLG6+nbhzzb2rLTdRVGbvjZH0fOOzq0LShq0EXsqnJbbuwJhK+ PnBtqX5O23PMHutP1l88AtVN+Rb72oSvnD+dK6708JqqUx2MAFLMevrhJRXLjKb2Mm+/8XBpEw+B 7DisN4TMlLB/d55WnT9UPNHmQ+3KFL7QrTO8hYExkU849g58Dn3Nw3oCbMUgny81ocrLlB2Z5fFG Qu1AdNiBA+kg/UxzyJZpFbKfCITd5yX49bOriL692aMVDyqUvh8fP+T99PqorH4cIJP6OxSTdxKM MIIFHDCCBASgAwIBAgIRAOK7SUh5KuwJ6cSlGPGZWGYwDQYJKoZIhvcNAQELBQAwgZcxCzAJBgNV BAYTAkdCMRswGQYDVQQIExJHcmVhdGVyIE1hbmNoZXN0ZXIxEDAOBgNVBAcTB1NhbGZvcmQxGjAY BgNVBAoTEUNPTU9ETyBDQSBMaW1pdGVkMT0wOwYDVQQDEzRDT01PRE8gUlNBIENsaWVudCBBdXRo ZW50aWNhdGlvbiBhbmQgU2VjdXJlIEVtYWlsIENBMB4XDTE5MDEwMjAwMDAwMFoXDTIyMDEwMTIz NTk1OVowJDEiMCAGCSqGSIb3DQEJARYTZHdtdzJAaW5mcmFkZWFkLm9yZzCCASIwDQYJKoZIhvcN AQEBBQADggEPADCCAQoCggEBALL98Dmy0wm1AOxiaio/bxxn/hWvraZDvmUVb3vRKTbDLH14bxaW /EYC/Lw/i9d98CunLGKA1O8yogKPdhTFFmmNR8uh6r1a/aHr301d8YQea0DtURyaAH5L4cvsKY6O 0HF7s5DqYm6tmLq1UrRwIhqgDjBjF4XFRqj7hoXFXFc1VI7LxMwFfT6PStq6WedhROKw5KQytaQS vrjgMjpICIP3A/CroGr+bcmqcnXljGUSUB9bzEOjlU9uAsgJ9sl5tjYE0DEtZqc0rT9oqD7U57My ECIewElc4VenLB2/GK5MfJoJZTsq7fWNpFkUSRZvxT0TRLqznjVepZ2AFzsplScCAwEAAaOCAdMw ggHPMB8GA1UdIwQYMBaAFIKvbIz4xf6WYXzoHz0rcUhexIvAMB0GA1UdDgQWBBS37jX/NtC72k+A CsaxitxCsdQxOzAOBgNVHQ8BAf8EBAMCBaAwDAYDVR0TAQH/BAIwADAdBgNVHSUEFjAUBggrBgEF BQcDBAYIKwYBBQUHAwIwRgYDVR0gBD8wPTA7BgwrBgEEAbIxAQIBAwUwKzApBggrBgEFBQcCARYd aHR0cHM6Ly9zZWN1cmUuY29tb2RvLm5ldC9DUFMwWgYDVR0fBFMwUTBPoE2gS4ZJaHR0cDovL2Ny bC5jb21vZG9jYS5jb20vQ09NT0RPUlNBQ2xpZW50QXV0aGVudGljYXRpb25hbmRTZWN1cmVFbWFp bENBLmNybDCBiwYIKwYBBQUHAQEEfzB9MFUGCCsGAQUFBzAChklodHRwOi8vY3J0LmNvbW9kb2Nh LmNvbS9DT01PRE9SU0FDbGllbnRBdXRoZW50aWNhdGlvbmFuZFNlY3VyZUVtYWlsQ0EuY3J0MCQG CCsGAQUFBzABhhhodHRwOi8vb2NzcC5jb21vZG9jYS5jb20wHgYDVR0RBBcwFYETZHdtdzJAaW5m cmFkZWFkLm9yZzANBgkqhkiG9w0BAQsFAAOCAQEAC20spBbrL71ZEiFrbXBHonzhhirO1sdn+X+O W579oIXMSuVEY1LwpyYYPrKMTjKECxuvp24c829qy03UVRm742R9Hzjs6tC0oatBF7KpyW27sCYS vj5wbal+TttzzB7rT9ZfPALVTfkW+9qEr5w/nSuu9PCaqlMdjABSzHr64SUVy4ym9jJvv/FwaRMP gew4rDeEzJSwf3eeVp0/VDzR5kPtyhS+0K0zvIWBMZFPOPYOfA59zcN6AmzFIJ8vNaHKy5QdmeXx RkLtQHTYgQPpIP1Mc8iWaRWynwiE3ecl+PWzq4i+vdmjFQ8qlL4fHz/k/fT6qKx+HCCT+jsUk3cS jDCCBeYwggPOoAMCAQICEGqb4Tg7/ytrnwHV2binUlYwDQYJKoZIhvcNAQEMBQAwgYUxCzAJBgNV BAYTAkdCMRswGQYDVQQIExJHcmVhdGVyIE1hbmNoZXN0ZXIxEDAOBgNVBAcTB1NhbGZvcmQxGjAY BgNVBAoTEUNPTU9ETyBDQSBMaW1pdGVkMSswKQYDVQQDEyJDT01PRE8gUlNBIENlcnRpZmljYXRp b24gQXV0aG9yaXR5MB4XDTEzMDExMDAwMDAwMFoXDTI4MDEwOTIzNTk1OVowgZcxCzAJBgNVBAYT AkdCMRswGQYDVQQIExJHcmVhdGVyIE1hbmNoZXN0ZXIxEDAOBgNVBAcTB1NhbGZvcmQxGjAYBgNV BAoTEUNPTU9ETyBDQSBMaW1pdGVkMT0wOwYDVQQDEzRDT01PRE8gUlNBIENsaWVudCBBdXRoZW50 aWNhdGlvbiBhbmQgU2VjdXJlIEVtYWlsIENBMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKC AQEAvrOeV6wodnVAFsc4A5jTxhh2IVDzJXkLTLWg0X06WD6cpzEup/Y0dtmEatrQPTRI5Or1u6zf +bGBSyD9aH95dDSmeny1nxdlYCeXIoymMv6pQHJGNcIDpFDIMypVpVSRsivlJTRENf+RKwrB6vcf WlP8dSsE3Rfywq09N0ZfxcBa39V0wsGtkGWC+eQKiz4pBZYKjrc5NOpG9qrxpZxyb4o4yNNwTqza aPpGRqXB7IMjtf7tTmU2jqPMLxFNe1VXj9XB1rHvbRikw8lBoNoSWY66nJN/VCJv5ym6Q0mdCbDK CMPybTjoNCQuelc0IAaO4nLUXk0BOSxSxt8kCvsUtQIDAQABo4IBPDCCATgwHwYDVR0jBBgwFoAU u69+Aj36pvE8hI6t7jiY7NkyMtQwHQYDVR0OBBYEFIKvbIz4xf6WYXzoHz0rcUhexIvAMA4GA1Ud DwEB/wQEAwIBhjASBgNVHRMBAf8ECDAGAQH/AgEAMBEGA1UdIAQKMAgwBgYEVR0gADBMBgNVHR8E RTBDMEGgP6A9hjtodHRwOi8vY3JsLmNvbW9kb2NhLmNvbS9DT01PRE9SU0FDZXJ0aWZpY2F0aW9u QXV0aG9yaXR5LmNybDBxBggrBgEFBQcBAQRlMGMwOwYIKwYBBQUHMAKGL2h0dHA6Ly9jcnQuY29t b2RvY2EuY29tL0NPTU9ET1JTQUFkZFRydXN0Q0EuY3J0MCQGCCsGAQUFBzABhhhodHRwOi8vb2Nz cC5jb21vZG9jYS5jb20wDQYJKoZIhvcNAQEMBQADggIBAHhcsoEoNE887l9Wzp+XVuyPomsX9vP2 SQgG1NgvNc3fQP7TcePo7EIMERoh42awGGsma65u/ITse2hKZHzT0CBxhuhb6txM1n/y78e/4ZOs 0j8CGpfb+SJA3GaBQ+394k+z3ZByWPQedXLL1OdK8aRINTsjk/H5Ns77zwbjOKkDamxlpZ4TKSDM KVmU/PUWNMKSTvtlenlxBhh7ETrN543j/Q6qqgCWgWuMAXijnRglp9fyadqGOncjZjaaSOGTTFB+ E2pvOUtY+hPebuPtTbq7vODqzCM6ryEhNhzf+enm0zlpXK7q332nXttNtjv7VFNYG+I31gnMrwfH M5tdhYF/8v5UY5g2xANPECTQdu9vWPoqNSGDt87b3gXb1AiGGaI06vzgkejL580ul+9hz9D0S0U4 jkhJiA7EuTecP/CFtR72uYRBcunwwH3fciPjviDDAI9SnC/2aPY8ydehzuZutLbZdRJ5PDEJM/1t yZR2niOYihZ+FCbtf3D9mB12D4ln9icgc7CwaxpNSCPt8i/GqK2HsOgkL3VYnwtx7cJUmpvVdZ4o gnzgXtgtdk3ShrtOS1iAN2ZBXFiRmjVzmehoMof06r1xub+85hFQzVxZx5/bRaTKTlL8YXLI8nAb R9HWdFqzcOoB/hxfEyIQpx9/s81rgzdEZOofSlZHynoSMYIDyjCCA8YCAQEwga0wgZcxCzAJBgNV BAYTAkdCMRswGQYDVQQIExJHcmVhdGVyIE1hbmNoZXN0ZXIxEDAOBgNVBAcTB1NhbGZvcmQxGjAY BgNVBAoTEUNPTU9ETyBDQSBMaW1pdGVkMT0wOwYDVQQDEzRDT01PRE8gUlNBIENsaWVudCBBdXRo ZW50aWNhdGlvbiBhbmQgU2VjdXJlIEVtYWlsIENBAhEA4rtJSHkq7AnpxKUY8ZlYZjANBglghkgB ZQMEAgEFAKCCAe0wGAYJKoZIhvcNAQkDMQsGCSqGSIb3DQEHATAcBgkqhkiG9w0BCQUxDxcNMTkx MDE2MDkxOTA3WjAvBgkqhkiG9w0BCQQxIgQg64E+MFdHN0K12ELluUFv76kDaeTgTJGalSgUw/cV QzUwgb4GCSsGAQQBgjcQBDGBsDCBrTCBlzELMAkGA1UEBhMCR0IxGzAZBgNVBAgTEkdyZWF0ZXIg TWFuY2hlc3RlcjEQMA4GA1UEBxMHU2FsZm9yZDEaMBgGA1UEChMRQ09NT0RPIENBIExpbWl0ZWQx PTA7BgNVBAMTNENPTU9ETyBSU0EgQ2xpZW50IEF1dGhlbnRpY2F0aW9uIGFuZCBTZWN1cmUgRW1h aWwgQ0ECEQDiu0lIeSrsCenEpRjxmVhmMIHABgsqhkiG9w0BCRACCzGBsKCBrTCBlzELMAkGA1UE BhMCR0IxGzAZBgNVBAgTEkdyZWF0ZXIgTWFuY2hlc3RlcjEQMA4GA1UEBxMHU2FsZm9yZDEaMBgG A1UEChMRQ09NT0RPIENBIExpbWl0ZWQxPTA7BgNVBAMTNENPTU9ETyBSU0EgQ2xpZW50IEF1dGhl bnRpY2F0aW9uIGFuZCBTZWN1cmUgRW1haWwgQ0ECEQDiu0lIeSrsCenEpRjxmVhmMA0GCSqGSIb3 DQEBAQUABIIBAA+nsCBQKDqAEVbfKZL8A++j2tkorpCFYZpUL+wH7Ltzo6QYkf+SfEax90dk5A6k 6Myw85JOEWdXlboD2ZmkgyrHPf92wD8+Iner+t7jk+Z/wrlUkTMP1Ai7i6HzEkqmVj0yJgN6KCCe g+7/vIulwIzlXD6XchFpCqzPbng5mx9wcgSq1b2nSqa2AryTQXOE2saq80RtQjjw9NI6DD53kVnz S4RK380p4RPd6wy3xZ3zW5LfzaNVieotr3IrNBdoTSZJndp2/qRddNuTDM465YdQ/yc0HQEQug0E nyBhLA7ahk6q+HE/Fg0SVai6GxWj9gsLp+9n3M7CNxCTgrsPRZkAAAAAAAA= --=-5XHVqKwiFQOxBDAA7FQb--