Skip to content

/ servo

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

Jump to bottom

TLS connections should disallow handshakes which use small DH groups #8581

Closed
alex opened this issue Nov 18, 2015 · 10 comments
Closed

TLS connections should disallow handshakes which use small DH groups #8581

alex opened this issue Nov 18, 2015 · 10 comments
Assignees
@avadacatavra
Labels
A-network A-security

Comments

@alex
Copy link
Contributor

@alex alex commented Nov 18, 2015

See for example:

which should be blocked. (https://dh1024.badssl.com/ should also be blocked eventually IMO, but is not yet in many major browsers)
@jdm jdm added the A-network label Nov 18, 2015
@frewsxcv frewsxcv added the A-security label Nov 19, 2015
@alex alex mentioned this issue Nov 20, 2015
Merged
@nox
Copy link
Member

@nox nox commented Apr 8, 2017

The first two are now blocked, but https://dh1024.badssl.com/ isn't. Assigning to @avadacatavra to decide.
@avadacatavra
Copy link
Contributor

@avadacatavra avadacatavra commented Apr 19, 2017

@nox I think we should block it
@avadacatavra
Copy link
Contributor

@avadacatavra avadacatavra commented Apr 19, 2017

Once I've blocked it, I think it would be good to make a more usable error message ("obsolete/insecure cipher--handshake failed" maybe). Currently, when we block these, the only message displayed is 'the handshake failed'
@avadacatavra
Copy link
Contributor

@avadacatavra avadacatavra commented Apr 19, 2017 •
edited

Also, how do we feel about switching to the modern cipher list (less backward compatibility) in https://wiki.mozilla.org/Security/Server_Side_TLS?

Major differences:

  • Sha1 removed (yay!)
  • DHE removed (slower than ECDHE)

Based on my recent-ish cipherscan results, it looks like we shouldn't take too much of a hit on web compat

CC @metajack @jdm
@alex
Copy link
Contributor Author

@alex alex commented Apr 19, 2017

Modern also drops non-AEAD suites, which probably breaks substantial portions of the web :-(

+1 on removing DHE though, Chrome has already done this, and because the TLS handshake doesn't negotiate FFDH-params, there's probably no way to bump the minimum DH without also breaking stuff.
@nox
Copy link
Member

@nox nox commented Apr 19, 2017

"HTTPS state" in the Fetch spec can be either "modern" or "deprecated", so we can have two lists, one for the ones we prefer, and one for the ones which are bad but which we need to support for webcompat.
@nox
Copy link
Member

@nox nox commented Apr 19, 2017

See #16357.
@avadacatavra avadacatavra mentioned this issue Apr 19, 2017
Closed
3 of 5 tasks complete
@metajack
Copy link
Contributor

@metajack metajack commented Apr 20, 2017

@avadacatavra if you want to switch modern on, you'll probably need to provide data on how bad the breakage is so we can make a decision. Feel free to remove DHE though, since it seems like other browsers are doing that.
@avadacatavra
Copy link
Contributor

@avadacatavra avadacatavra commented Apr 20, 2017

@metajack @nox in #16535 i changed our default to modern with a switch for intermediate (which does keep DHE). does that sound like it will solve this?
bors-servo added a commit that referenced this issue May 9, 2017
@bors-servo
Auto merge of #16535 - avadacatavra:modern-cipher, r=jdm
Loading status checks…
4928a94 
changed default ciphers to modern ones

<!-- Please describe your changes on the following line: -->
I changed the default ciphers to the modern set (ref: https://wiki.mozilla.org/Security/Server_Side_TLS), but added an option to use the intermediate set for web compatibility

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #8581 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16535)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jun 21, 2017
@bors-servo
Auto merge of #16535 - avadacatavra:modern-cipher, r=jdm
Loading status checks…
aecc3cc 
changed default ciphers to modern ones

<!-- Please describe your changes on the following line: -->
I changed the default ciphers to the modern set (ref: https://wiki.mozilla.org/Security/Server_Side_TLS), but added an option to use the intermediate set for web compatibility

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #8581 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16535)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jun 30, 2017
@bors-servo
Auto merge of #16535 - avadacatavra:modern-cipher, r=jdm
Loading status checks…
2125c95 
changed default ciphers to modern ones

<!-- Please describe your changes on the following line: -->
I changed the default ciphers to the modern set (ref: https://wiki.mozilla.org/Security/Server_Side_TLS), but added an option to use the intermediate set for web compatibility

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #8581 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16535)
<!-- Reviewable:end -->
@Darkspirit
Copy link
Contributor

@Darkspirit Darkspirit commented May 9, 2019

This can be closed. Support for DHE has been removed with #22243.
@alex alex closed this May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@avadacatavra avadacatavra

Labels
A-network A-security
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

changed default ciphers to modern ones
7 participants
@alex @jdm @metajack @nox @frewsxcv @avadacatavra @Darkspirit
You can’t perform that action at this time.