Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

["Laszlo Ersek" <lersek@redhat.com>]

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 \x14 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