caddytls: Raise TLS alert if no certificate matches SAN (closes #1303) #2339
Conversation
I don't love this half-baked solution to the issue raised in #1303 way more than a year after the original issue was closed (the necro comments are about an issue separate from the original issue that started it), but I do like TLS alerts more than wrong certificates.
|
Checking |
|
I did basic test and recieved a self signed cert on a non existent domain for a caddy instance. Its a little confusing but will solve the issue. |
Thanks for the test. Can you elaborate? What was your Caddyfile, what was your request, and which cert exactly did you get back? |
|
This seems to resolve the issue for me, though I don't really understand how the fix works (mostly because I don't really understand how the issue arises in the first place). Caddyfile: /etc/hosts: Test command: for i in {1..10}; do echo | openssl s_client -showcerts -servername localhost -connect localhost:4443 2>/dev/null | openssl x509 -inform pem -noout -text | grep -F DNS; doneOutput, master: Output, norandomcert: |
|
Nothing revealing showing up in a packet capture, either. |
|
That's good to hear. Thank you for your testing. This is interesting:
Notice that the host names are not round-robined, and Even with this patch, you can still determine which hosts are being served from a machine, in roughly the same time. And I might be breaking any number of legitimate use cases by accident by doing this. But, like you, I prefer TLS alerts over mismatched certificates.
Except that SNI is not encrypted, so hostnames from legitimate visitors are leaked on the wire anyway. I will give a little more time for review and testing from anyone else who would like to participate, before merging this in. |
I'll explain. This is the code that was introduced a few months ago (less than a year ago, far less than 2 years ago as originally claimed) - view it on Sourcegraph: // if nothing matches, use a random certificate
// TODO: This is not my favorite behavior; I would rather serve
// no certificate if SNI is provided and cause a TLS alert, than
// serve the wrong certificate (but sometimes the 'wrong' cert
// is what is wanted, but in those cases I would prefer that the
// site owner explicitly configure a "default" certificate).
// (See issue 2035; any change to this behavior must account for
// hosts defined like ":443" or "0.0.0.0:443" where the hostname
// is empty or a catch-all IP or something.)
for _, certKey := range cfg.Certificates {
cert = cfg.certCache.cache[certKey]
defaulted = true
return
}The comment explains the issue, and you can look back at the commit histories to see why this was done and for further discussion. As you can see, we iterated the map of certificates and pulled out the first one in the map. In Go, map traversal is random -- not necessarily round-robin -- resulting in a random certificate to satisfy a request that doesn't match one. The "workaround" worked by satisfying a precursory condition here: https://sourcegraph.com/github.com/mholt/caddy@6e2de19/-/blob/caddytls/handshake.go#L62-67 // try a config that serves all names (the above
// loop doesn't try empty string; for hosts defined
// with only a port, for instance, like ":443")
if config, ok := cg[""]; ok {
return config
}Before a certificate can be selected, first the matching TLS configuration must be obtained. This is similarly done using SNI. Each config has its own map of certificates to use. Each recognized hostname is mapped to a hash of the certificate, and that hash is used to look up and access the singular copy of the certificate in the cache. The config condition mentioned above is for matching sites configured like Of course that itself reveals, especially with time, what hosts Caddy does and does not serve. This patch works roughly the same way, but instead of serving a self-signed certificate, it issues a TLS alert with no certificate instead. In this change we remove the code that pulls a random certificate as the default: https://github.com/mholt/caddy/pull/2339/files#diff-178ab7d13385d45b8abb244fda5a215fL190 Hopefully that helps explain the issue. Again, I'm not sure it is correct for every case but apparently it does the job you're looking for. |
|
Thanks for the explanation. That sounds like it should be sufficient. The reason I became aware of this--and the reason I conflated it with #1303--is that I had abused #1303 while pentesting to glean information I wasn't supposed to know and wasn't able to obtain through OSINT (e.g., CT logs). In particular, I was able to determine that a server had formerly (briefly) hosted a particular domain name and that the two domains were therefore controlled by the same entity. The issue resurfaced recently during pentesting, but this time I noticed that subsequent requests returned random certificates. It wasn't clear to me that this was a separate issue, as I was able to use it to the same effect, though this particular behave seemed especially buggy to me. The first time around I attributed it to a misconfiguration, but the random certificate issue was nastier since it exposed more information, and I figured it deserved to be reported. A few days ago, I was informed that someone had decided to write a blog post on the issue, and they sent it to me to review. That prompted me to revisit the issue--I wasn't keen on them going out of their way to publicize an issue that I deemed to be a vulnerability without a proper CVE and fix. I've requested that they contact you directly via email before going ahead with the post. I have little to no say in the matter, but I'm hoping they'll allow you to review it and make any necessary corrections before publishing it. |
|
If I visit https://sdfsl.azure.cookingisfun.ie on my server I get This site can’t provide a secure connection sdfsl.azure.cookingisfun.ie sent an invalid response. |
|
This solves the issue i originally reported. Thank you @mholt and team! |
I don't love this half-baked solution to the issue raised in #1303 way
more than a year after the original issue was closed (the necro comments
are about an issue separate from the original issue that started it),
but I do like TLS alerts more than wrong certificates.
1. What does this change do, exactly?
Raises a TLS alert on mismatching (or missing) SNI if no site is configured with a matching certificate, rather than serving a random certificate.
2. Please link to the relevant issues.
/cc @Zenexer @ghoeffner @orenyomtov @mxlje - Please determine whether this addresses your concerns. This is on you now. This is not entirely satisfying to me, but if it makes you happy, let's merge it.