Bug 4206 - Unbound does not verify IP SAN entries during the TLS handshake
Unbound does not verify IP SAN entries during the TLS handshake
Status: RESOLVED FIXED
Product: unbound
Classification: Unclassified
Component: server
1.8.1
All All
: P5 major
Assigned To: unbound team
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2018-11-17 10:43 CET by David
Modified: 2018-12-11 14:49 CET (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
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