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.799.1570652677370400010 for ; Wed, 09 Oct 2019 13:24:37 -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-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id EC46818C4283; Wed, 9 Oct 2019 20:24:36 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-11.rdu2.redhat.com [10.10.120.11]) by smtp.corp.redhat.com (Postfix) with ESMTP id 46F6E60BF4; Wed, 9 Oct 2019 20:24:34 +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> From: "Laszlo Ersek" Message-ID: <6bb5d2f6-ec6f-1766-e19b-03fd45c1bc12@redhat.com> Date: Wed, 9 Oct 2019 22:24:34 +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: <777053db79600eb90a19945700293d14f4978344.camel@infradead.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.6.2 (mx1.redhat.com [10.5.110.62]); Wed, 09 Oct 2019 20:24:37 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi All, (multi-hour composition ahead...) On 10/09/19 09:53, David Woodhouse wrote: > On Tue, 2019-10-08 at 06:19 +0000, Wu, Jiaxin wrote: >> Hi David, >> >> I just realized you have the comments on Bugzilla 960: >> >>> "...given that testing is failing and code inspection shows it >>> would never have been expected to work." >> >> Do you mean you didn't pass the verification if URLs with IPv6 >> literals (https://[2001:8b0:10b:1236::1]/)? Can you also show me >> where the code inspection indicated it would never have been expected >> to work? We do pass the testing for the URLs with IPv6 if the CN or >> SAN in certificate has the corresponding IPv6 address (at least >> working with openssl 1.1.0). > > I have not tested this, but I started looking when there was a message > on the edk2 list from someone who was reporting that it didn't work > for IPv6 URIs, IIRC. > > You are using SSL_set1_host(), and I believe you're just passing in > the bare hostname part of the URI, be it "1.2.3.4" or > "[2001:8b0:10b::5]". > > That just adds it to the 'hosts' list in the X509_VERIFY_PARAM for the > SSL connection. > > In the check_hosts() function in openssl/crypto/x509/v509_vfy.c, the > code simply iterates over the members of that list, calling > X509_check_host() for each one. It never calls X509_check_ip(). > > If you look in openssl/crypto/x509/v3_utl.c you can see the > X509_check_host() really does only check hostnames. You'd need to call > X509_check_ip_asc() to check hostnames. And something would need to > have stripped the [] which surround an IPv6 literal. > > I can't see how this can work. Have you tested it since the report on > the list that it wasn't working? > > cf. https://github.com/openssl/openssl/pull/9201 which is being > ignored by the OpenSSL developers  OpenSSL really doesn't make > life easy for you here, which is a shame. > > >> For the series patches here, we are intending to support the host >> name validation, I think we can commit the series patches since we >> pass the verification of IPV6 URL, what do you think? > > If it passes the verification of IPv6 literals, then all my analysis > is broken and so was the report on the list that prompted me to start > looking (or I'm misremembering that report). In that case, sure, go > ahead and commit. Here's a summary of my setup. * I've generated a brand new CA certificate, and two HTTP server certificates, signed by the CA. * One HTTP server certificate is for Common Name = 192.168.124.2 * The other HTTP server certificate is for Common Name = fd33:eb1b:9b36::2 * I have a "net-server" virtual machine that runs Apache on the above IP addresses (TCP port 443). - This virtual machine also runs DHCP (v4) and DHCP (v6) daemons. - The DHCP servers send the following boot file names: - "https://192.168.124.2/RHEL-7.4-20170711.0-Server-x86_64-boot.iso" [IPv4] - "https://[fd33:eb1b:9b36::2]/RHEL-7.4-20170711.0-Server-x86_64-boot.iso" [IPv6] * For sanity-checking the environment, I run the following two commands on the *host* (connecting to the "net-server" virtual machine): - curl -I 'https://192.168.124.2/RHEL-7.4-20170711.0-Server-x86_64-boot.iso' - curl --globoff -I 'https://[fd33:eb1b:9b36::2]/RHEL-7.4-20170711.0-Server-x86_64-boot.iso' - The host is configured to trust the brand new test CA certificate (see near the top). - When the certificates are assigned *correctly* to the IP addresses in the Apache configuration, the above "curl" commands complete just fine. If I add the "-v" option to "curl", it confirms the right certificates are used, and it confirms the test CA as issuer too. - When the certificates are (intentionally) *cross-assigned* to the IP addresses in the Apache configuration, then both "curl" commands break with the following error message: > curl: (51) Unable to communicate securely with peer: requested domain > name does not match the server's certificate. - If I add the "-v" option, I also see > NSS error -12276 (SSL_ERROR_BAD_CERT_DOMAIN) - As a side comment: Apache itself warns about the misconfig, in "/var/log/httpd/ssl_error_log": > ... [ssl:warn] ... AH01909: RSA certificate configured for ...:443 > does NOT include an ID which matches the server name * I have a "net-client" virtual machine, running OVMF. - The edk2 HTTPS/TLS client booting in this virtual machine is configured to trust the exact same set of CA certificates that the host trusts too. - In other words, HTTPS boot in the "net-client" VM accepts server certificates signed by the new test CA. * The following is the test plan. 1. The patch set is *not* applied (that is, OVMF is built at current master, commit 976d0353a6ce). 1. Properly assigned certificates: 1. HTTPSv4 boot --> expect success (correct behavior, establishes baseline) 2. HTTPSv6 boot --> expect success (correct behavior, establishes baseline) 2. Cross-assigned certificates: 1. HTTPSv4 boot --> expect success (for reproducing the bug) 2. HTTPSv6 boot --> expect success (for reproducing the bug) 2. With the patch set applied: 1. Properly assigned certificates: 1. HTTPSv4 boot --> expect success (failure means a regression) 2. HTTPSv6 boot --> expect success (failure means a regression) 2. Cross-assigned certificates: 1. HTTPSv4 boot --> expect failure (for verifying the bugfix) 2. HTTPSv6 boot --> expect failure (for verifying the bugfix) * Results: - 1.1.1. as expected (HTTPSv4 baseline established) - 1.1.2. as expected (HTTPSv6 baseline established) - 1.2.1. as expected (HTTPSv4 MITM bug reproduced) - 1.2.2. as expected (HTTPSv6 MITM bug reproduced) - 2.1.1. as expected (HTTPSv4 not regressed by series) - 2.1.2. as expected (HTTPSv6 not regressed by series) - 2.2.1. as expected (HTTPSv4 MITM averted) - 2.2.2. as expected (HTTPSv6 MITM averted) * In cases 2.2.1. and 2.2.2.: - The UEFI console contains, respectively: > >>Start HTTP Boot over IPv4.... > Station IP address is 192.168.124.106 > > URI: https://192.168.124.2/RHEL-7.4-20170711.0-Server-x86_64-boot.iso > > Error: Could not retrieve NBP file size from HTTP server. > > Error: Unexpected network error. > >>Start HTTP Boot over IPv6.... > Station IPv6 address is FD33:EB1B:9B36:0:0:0:0:C8 > > URI: https://[fd33:eb1b:9b36::2]/RHEL-7.4-20170711.0-Server-x86_64-boot.iso > > Error: Could not retrieve NBP file size from HTTP server. > > Error: Unexpected network error. - The OVMF log contains (in both cases): > TlsDoHandshake SSL_HANDSHAKE_ERROR State=0x4 SSL_ERROR_SSL > TlsDoHandshake ERROR 0x1416F086=L14:F16F:R86 - Decoding: - Library 0x14 -> ERR_LIB_SSL - Function 0x16F -> SSL_F_TLS_PROCESS_SERVER_CERTIFICATE - Reason 0x86 -> SSL_R_CERTIFICATE_VERIFY_FAILED - So this means that the ssl_verify_cert_chain() call fails in the tls_process_server_certificate() function, in "CryptoPkg/Library/OpensslLib/openssl/ssl/statem/statem_clnt.c". Normally the above would be sufficient for me to give a "Tested-by" for this patch set. But now I'm uncertain whether (a) my results contradict David's analysis, or (b) I tested something that David's analysis doesn't *apply* to. (IOW if my test plan doesn't actually verify "IPv6 literals".) FWIW, the brackets in the IPv6 notation are stripped in EfiHttpRequest() [NetworkPkg/HttpDxe/HttpImpl.c], using the "HostName" local variable. (The stripping comes from earlier commit 7191827f90b4 ("NetworkPkg/HttpDxe: Strip square brackets in IPv6 expressed HostName.", 2018-08-03).) Later in EfiHttpRequest(), "HttpInstance->RemoteHost" is assigned "HostName". Further, in TlsConfigureSession() [NetworkPkg/HttpDxe/HttpsSupport.c], we set "HttpInstance->TlsConfigData.VerifyHost.HostName" to "HttpInstance->RemoteHost". This is done in patch#4. Also in patch#4, in the same function, we pass "HttpInstance->TlsConfigData.VerifyHost" to SetSessionData(), from patch#3. There we pass "TlsVerifyHost->HostName" to TlsSetVerifyHost(), which resides in patch#2. At that point, we pass the hostname -- the IPv6 address, with the brackets stripped -- to SSL_set1_host(). So, my take is that the comparison is done simply on the textual representation (with the IPv6 brackets stripped), not the numerical value. Is that bad? The textual comparison may certainly report a mismatch when the numerical values actually match (for an IPv4 example, "192.168.0.1" would not match "192.168.000.001"). But that errs in the safe direction, does it not? Thanks! Laszlo