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

caddytls: Fix handling of IP-only TLS configs and empty-SNI handshakes #2452

Merged
merged 5 commits into from Feb 5, 2019

Conversation

Projects
None yet
7 participants
@mholt
Copy link
Owner

mholt commented Feb 2, 2019

1. What does this change do, exactly?

  • Always choose a TLS config during a handshake, even if there is no match. Random selection (as a last resort) is OK; any config should allow the TLS-ALPN challenge to be completed.

  • Add -default-sni flag. If a ClientHello is received with no ServerName (SNI), then this value will be assumed when matching TLS configs and choosing a certificate.

  • When no ServerName is given, certificates may be chosen based on a matching IP address of the listener.

  • Update CertMagic dependency, which supports this new matching logic with a default ServerName.

  • If SNI is present in a ClientHello but does not match any certificates, a TLS alert is raised (in keeping with previous behavior before this change and other recent changes)

2. Please link to the relevant issues.

Hopefully fixes #2451, fixes #2438, fixes #2414, and fixes #2407

3. Which documentation changes (if any) need to be made because of this PR?

Add note about the -default-sni flag.

4. Checklist

  • I have written tests and verified that they fail without my change
  • I have squashed any insignificant commits
  • This change has comments for package types, values, functions, and non-obvious lines of code
  • I am willing to help maintain this change if there are issues with it later

Calling on: @fr33tux, @oscartbeaumont, @rmoriz, @whitestrake, and @magikstm if you could help test this please, and ensure it works for you, since I was not able to reproduce all the various issues myself (like the Docker stuff).

mholt added some commits Feb 2, 2019

caddytls: Fix empty SNI handling (new -default-sni flag)
vendor: update certmagic, needed to support this

Hopefully fixes #2451, fixes #2438, and fixes #2414
@mholt

This comment has been minimized.

Copy link
Owner Author

mholt commented Feb 2, 2019

Better get @ghoeffner @orenyomtov @Zenexer to try this out while they still have a chance, before it lands -- since they apparently have opinions on the matter. And if not, they would not have made such a stink about it before. So I expect to hear from them, but if not it's on them.

Show resolved Hide resolved caddy/caddymain/run.go Outdated
@tobya

tobya approved these changes Feb 5, 2019

@elcore

elcore approved these changes Feb 5, 2019

@mholt mholt merged commit 9e4a291 into master Feb 5, 2019

4 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@mholt mholt deleted the sni-fixes branch Feb 5, 2019

@xombiemp

This comment has been minimized.

Copy link

xombiemp commented Feb 9, 2019

@mholt I can confirm that this change has fixed #2438
Thank you!

@Zenexer

This comment has been minimized.

Copy link

Zenexer commented Feb 14, 2019

@mholt None of this is on me. You're responsible for the security of your own product. I reported the vulnerability and provided a fix. What you do from here is on you. I don't use Caddy and I'm not going to spend any more time testing and coding for someone who's just going to get angry at me. If I see another vulnerability while pentesting, I'll report it, but don't expect me to contribute beyond that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment