Bugzilla – Bug 4206
Unbound does not verify IP SAN entries during the TLS handshake
Last modified: 2019-02-07 14:41:21 CET
While working on running unbound as a local DoT server on my macOS machine, I came across:
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:
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:
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_verify_cert.html )
If you're okay with targeting the 1.1.0 API, there's a verify callback that can help:
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
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.
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
> 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:
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.
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
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
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 126.96.36.199@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.
Reopening since the fix was not completely implemented.
Thank you for the test and fix. That makes sense to change those defines.
Integrated the patch into the code repository.
Best regards, Wouter
Great, thank you!