Bug 4206

Summary: Unbound does not verify IP SAN entries during the TLS handshake
Product: unbound Reporter: David <dcow>
Component: serverAssignee: unbound team <unbound-team>
Status: RESOLVED FIXED    
Severity: major CC: cathya, jimp, wouter
Priority: P5    
Version: 1.8.1   
Hardware: All   
OS: All   
Attachments: OpenSSL 1.0.2 host verify fix

Description David 2018-11-17 10:43:52 CET
While working on running unbound as a local DoT server on my macOS machine, I came across: 

    https://www.nlnetlabs.nl/bugs-script/show_bug.cgi?id=658

I was intrigued so I decided to audit the code. I came across a few things:

 1. The code does not verify (or otherwise configure openssl to verify) the IP address when establishing a TLS connection in order to forward requests.

    If you inspect the TLS certs being presented by DoT servers, you'll notice they have IP entries in their SAN. They are provided as a way to have TLS work with bare IP addresses, which is exactly the use case here. In fact, the hostname check is rather superfluous once you've verified the IP. Further, the hostname check is almost non-nonsensical in this case: 

    "The DNS system says the person that has been issued this certificate is associated with this domain that the user configured, but I'll skip the DNS system to initiate this connection because it is in fact a connection to the DNS system itself, and just use the IP the user has configured."

    What you're actually trying to do is connect to an IP and verify that the person serving the request is to be trusted, so what better way than to compare the IP entries in the SAN to the IP address the user has configured?

    There a function added in verrsion 1.0.2b that allows you to configure openssl to verify the IP address field in the cert the remote dns server presents:
    
    https://www.openssl.org/docs/man1.0.2/crypto/X509_VERIFY_PARAM_set1_ip.html

 2. Instead of using SSL_CTX_set1_host (v1.1.0), there is similarly a version 1.0.2b compatible way to verify the hostname using VERIFY_PARAM_set1_host. See:
    
    https://www.openssl.org/docs/man1.0.2/crypto/X509_VERIFY_PARAM_set1_host.html

    If setting the params on a new context for every new connection is annoying from an implementation standpoint, something you can do is SSL_get_peer_certificate and call the X509_check functions yourself:

  * https://www.openssl.org/docs/man1.0.2/crypto/X509_check_host.html
  * https://www.openssl.org/docs/man1.0.2/crypto/X509_check_ip.html
 (* https://www.openssl.org/docs/man1.0.2/crypto/X509_verify_cert.html )

    If you're okay with targeting the 1.1.0 API, there's a verify callback that can help:

  * https://www.openssl.org/docs/man1.1.0/ssl/SSL_CTX_set_cert_verify_callback.html

---

Practically it probably makes sense to keep hostname verification so as not to break current setups. My recommendation would be logic like:

    configure to verify chain

    IF forward-addr HAS hostname
        configure to verify hostname
    ELSE
    IF config.verify_IPs
        configure to verify_IP

That way you always verify the chain and then you can let the user decide which SAN entry they care about (although  you might suggest in documentation that the IP entry is most logical). If you don't want to break setups that currently don't verify anything other than the chain, you can add a config option to enable IP verification that defaults to false. However I don't like the idea of failing open. Regardless, the config could eventually default to true in future versions once people have had fair warning.

Thoughts?
Comment 1 Wouter Wijngaards 2018-12-03 17:13:11 CET
Hi David,

No.  I am not going to implement X509 verification, that is what OpenSSL does.  And does best, because I don't want bugs in my x509 certificate chains and fallback and so on.  This is why I use the 1.1 ssl_ctx_set_hostname function.  Other than that, it would be a nice idea, which I believe already works, but you configure the ip address after the # in the config file, instead of the domain name to verify.  And I believe that works too.  So it already works, eg., users can configure this behaviour if they want.  If there was an API call to do it that does not involve writing OpenSSL but then part of the application, then I would be interested in the feature, actually.

Right now, since the TLS forwarders are configured, the talk about defaults is not really something that has to be looked at.  There can be config options.  Because everything has to be configured anyway.

So it could be nice, but there is no API call, and there is already, I believe, a way to config it if you want it, by using the IP address as the host name to check for.

Best regards, Wouter
Comment 2 David 2018-12-09 12:00:31 CET
> No.  I am not going to implement X509 verification, that is what OpenSSL does.  And does best, because I don't want bugs in my x509 certificate chains and fallback and so on.

I'm not suggesting you do this. All you have to do is perform a few checks yourself (which is why I used the word "configure")--not implement any of the logic. It's all explained on the wiki:

https://wiki.openssl.org/index.php/SSL/TLS_Client#OpenSSL_1.0.2

It's pretty standard practice when using OpenSSL 1.0.2. I suggested this approach because it is compatible with 1.0.2 meaning your could drop down to that version. It's unfortunate that unbound requires a newer openssl than most systems today have. If you're not interested in supporting back to 1.0.2, of course, that's up to you.
Comment 3 Wouter Wijngaards 2018-12-10 15:33:27 CET
Hi David,

That is not too bad, for 1.0.2, I see, added those lines to the code, so Unbound can support hostname verification with openssl 1.0.2.  Before that the name matching is manual as well, that is maybe a bit too much backwards compatibility.

Code is the code repository.

I also check some other certificate code, do you think I should enable X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS also for the host1_check in 1.1.1 openssl? Because I guess the defaults are probably best for 1.1.0 and later?

Best regards, Wouter
Comment 4 Wouter Wijngaards 2018-12-11 14:49:02 CET
Hi,

The bug report gets closed because code is committed for it; not in 1.8.3 that is a bugfix release, but it is available in the code repository for the next release.  Let me know what other openssl improvements we can pick up, that would be nice to have.

Best regards, Wouter
Comment 5 Jim P. 2019-02-06 20:03:03 CET
Created attachment 558 [details]
OpenSSL 1.0.2 host verify fix

This is not fixed completely. The code change to support host validation on OpenSSL 1.0.2 was done in outside_network.c but the checks in daemon/remote.c, iterator/iter_fwd.c, and iterator/iter_hints.c were not fixed. As such, attempting to validate hosts with Unbound 1.9.0 still fails with OpenSSL 1.0.2. Specifically, on FreeBSD 11.2 it does not function:

unbound: [45843:0] error: no name verification functionality in ssl library, ignored name for 9.9.9.9@853#blah.example.com

The attached patch fixes the checks so it works as intended. I have tested the patch on FreeBSD 11.2 and pfSense 2.4.5 snapshots and it works. The logs indicate successes and failures of validation as expected when testing various scenarios.
Comment 6 Jim P. 2019-02-06 20:03:52 CET
Reopening since the fix was not completely implemented.
Comment 7 Wouter Wijngaards 2019-02-07 09:35:25 CET
Hi Jim,

Thank you for the test and fix.  That makes sense to change those defines.

Integrated the patch into the code repository.

Best regards, Wouter
Comment 8 Jim P. 2019-02-07 14:41:21 CET
Great, thank you!