Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFE: Certificate checking for Resolveds DNS over TLS feature #9397

Closed
shibumi opened this issue Jun 24, 2018 · 13 comments
Closed

RFE: Certificate checking for Resolveds DNS over TLS feature #9397

shibumi opened this issue Jun 24, 2018 · 13 comments
Labels
resolve RFE 🎁 Request for Enhancement, i.e. a feature request

Comments

@shibumi
Copy link
Contributor

shibumi commented Jun 24, 2018

Since systemd 239 systemd-resolved supports DNS over TLS. Currently (systemd version 239) systemd-resolved does not certificate checking for DNS Servers as covered in this PR: #8849

This issue is for keeping track of certificate checking for DNS over TLS.
One possible solution could be to add a new format for the DNS-Servers in the /etc/systemd/resolved.conf file. A possible solution would be an IP/hostname Tuple. This would allow hostname based certificate validation. More Details about this idea here: #8849 (comment)

@yuwata yuwata added RFE 🎁 Request for Enhancement, i.e. a feature request resolve labels Jun 25, 2018
@ott
Copy link
Contributor

ott commented Aug 31, 2018

I would recommend to not a IP address hostname tuples.

I think there are two possibilities: either an IP address of the DNS server or the hostname of the upstream resolver are provided. The Baseline Requirements of the CA/Browser Forum allow certificates to be issued for IP addresses and hostnames, so both are possible. For example, Cloudflare's DNS resolvers have an X.509 certificate with the following Subject Alternative Name: DNS:*.cloudflare-dns.com, IP Address:1.1.1.1, IP Address:1.0.0.1, DNS:cloudflare-dns.com, IP Address:2606:4700:4700:0:0:0:0:1111, IP Address:2606:4700:4700:0:0:0:0:1001.

If just the hostname is known, it might be required to resolve it first over unencrypted DNS or some form of unencrypted multicast DNS. Nonetheless, I don't think that it will cause major problems.

However, it might be harder to operate a DNS resolver that supports DNS over TLS and which has IP addresses which are made public over DHCP or IPv6 RA Options. It seems that not all CAs will issue certificates for IP addresses, in particular Let's Encrypt does not. RFC 8106 and RFC 3646 also just allow IP addresses and not hostnames. It gets particularly bad if the DNS resolvers just have link-local or private IP addresses. No CA will issue certificates for them.

@ott
Copy link
Contributor

ott commented Aug 31, 2018

It seems that it also has implications for #5873.

@Lekensteyn
Copy link
Contributor

Lekensteyn commented Oct 29, 2019

Certificate validation is still incomplete up to at least v243-425-g09ee387e08, even with the PR from @irtimmer in #12815. "certificate verification" in GnuTLS:

if (server->manager->dns_over_tls_mode == DNS_OVER_TLS_YES)
gnutls_session_set_verify_cert(gs, NULL, 0);

However, it only checks whether it uses a valid certificate chain. I did an experiment (redirect to any TCP/443 server with a valid certificate) and found that systemd-resolved would still accept certificates for other domains (iptables -t nat -A OUTPUT -p tcp --dport 853 -d 1.1.1.1 -j DNAT --to-destination $https_host:443).

To solve this, setting the IP address in place of the host (NULL above) would probably not work. You probably have to use gnutls_x509_trust_list_verify_crt2.

On the other hand, systemd-resolved built with OpenSSL should do proper certificate validation, but I have not verified this:

if (server->manager->dns_over_tls_mode == DNS_OVER_TLS_YES) {
X509_VERIFY_PARAM *v;
const unsigned char *ip;
SSL_set_verify(s, SSL_VERIFY_PEER, NULL);
v = SSL_get0_param(s);
ip = server->family == AF_INET ? (const unsigned char*) &server->address.in.s_addr : server->address.in6.s6_addr;
if (!X509_VERIFY_PARAM_set1_ip(v, ip, FAMILY_ADDRESS_SIZE(server->family)))
return -ECONNREFUSED;
}

At minimum it would be great to have a configurable host name used for validation purposes. This would be similar to how Android implemented DoT.

A secondary mechanism is public key pinning (SPKI) as described in RFC 7858 (Section 4.2). This should permit multiple potential keys. It will probably not be good for the majority of users. They would either have to regularly update their pinned keys, or use a resolver that uses long-lived certificates, or a resolver that reuses their private keys for new certificates.

Before adding too many new TLS or trust verification-related options, keep in mind that doing so can result in terrible user interfaces that will likely result in an insecure configuration. See for example Network Manager that has to configure wpa_supplicant behind the scenes. It might make sense just to add an optional DNSOverTLSHostname option that will be used in place of IP address for certificate validation.

@irtimmer
Copy link
Contributor

@Lekensteyn I was already made aware of the mistake (I forgot some code in my original pull request) when I read the blog post from Cloudflare ;). So I have post a new pull request #13870 to fix the issue when using DNS-over-TLS in combination with GnuTLS.

I have some POC code for using hostname and SPKI validation using the suggested tuples in the DNS= option. But I haven't made time yet to cleanup this code. Adding an additional option could be better, but currently you can't set options for a single DNS server and to change that seems like a lot of work (which I want to prevent ;) ).

@Lekensteyn
Copy link
Contributor

The gravatar of the blog post author and me look very similar :)

Thanks for the quick fix, it looks good. Unfortunately it might break users who use their own resolver which use a DNSName-only certificate in strict mode. For good reasons, previously any other cert would be accepted.

While at it, maybe handshake failure messages could be logged with higher severity, especially in struct mode? That should help users who do not set Environment=SYSTEMD_LOG_LEVEL=debug.

@nluedtke
Copy link

nluedtke commented Nov 7, 2019

Being tracked as CVE-2018-21029.

@Mynacol
Copy link

Mynacol commented Nov 12, 2019

Being tracked as CVE-2018-21029.

At the provided CVE it says that the problem is affected by version up to (excluding) 243. Now I wanted to recheck: The pull request with the needed fixes got merged 13 days ago, but the last release, version 243, was on September 3rd.

Is this an error in the CVE record? Is version 243 affected by this or not?

@shibumi
Copy link
Contributor Author

shibumi commented Nov 12, 2019

@Mynacol Looks like an error, The fix for it will be in version 244. Note: There is still no real hostname validation, only IP address validation for certificates with IP addresses.

@ret2libc
Copy link
Contributor

Also, until v243 the strict mode (DNSOverTLS=yes) was not implemented, so there was only the opportunistic mode. In opportunistic mode, according to section 4.1 in RFC7858, the client might or might not validate the resolver, so we can't really call it a vulnerability if the spec itself allows for not checking the validity of the resolver. Opportunistic can be easily downgraded as well.

To me, it seems the real problem is only in systemd v243, where strict mode was implemented and it was supposed to verify the certificate, but it does not do it well.

@ott
Copy link
Contributor

ott commented Mar 1, 2020

RFC 8738 extends the ACME protocol to IP addresses. So perhaps Let's Encrypt will issue certificates for IP addresses in the future and strict certificate verification becomes feasible.

@nl6720
Copy link
Contributor

nl6720 commented Mar 11, 2020

With #15012 merged, is this issue now fixed?

@shibumi
Copy link
Contributor Author

shibumi commented Mar 11, 2020

Interesting. Is this already in systemd 245? CC @yuwata

@yuwata
Copy link
Member

yuwata commented Mar 11, 2020

No, the commit was merged after v245 was released. Please wait for v246 or try to use the current git master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resolve RFE 🎁 Request for Enhancement, i.e. a feature request
Development

No branches or pull requests

9 participants