Closed
Bug 952863
Opened 10 years ago
Closed 9 years ago
Disable TLS False Start for non-ECDHE key exchange
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: briansmith, Assigned: briansmith)
References
Details
Attachments
(3 files)
3.99 KB,
patch
|
keeler
:
review+
Sylvestre
:
approval-mozilla-aurora+
briansmith
:
checkin+
|
Details | Diff | Splinter Review |
13.13 KB,
patch
|
keeler
:
review+
mcmanus
:
review+
|
Details | Diff | Splinter Review |
8.32 KB,
patch
|
briansmith
:
review+
|
Details | Diff | Splinter Review |
Because some clients (Java 7 and earlier, in particular) cannot deal with DHE keys larger than 1024 bits, many servers choose DHE keys that are 1024 bits. Also, the default DHE key size in Java was 768 bits for a long time. If the server would normally negotiate ECDHE but a MitM tricks it into negotiating DHE, the MitM may successfully significantly degrade the security of the connection in the False Start case. According to the last telemetry I looked at, DHE key exchange is used in approximately 6% of full handshakes. Unfortunately, almost all of them are using 1024-bit keys and the second-most-common size is 512 bits. When we do this, we need to check to make sure that 2048-bit keys are not getting measured as 2041-2047 bits and/or we should include 2041-2047 bit keys in the allowed range. (I see in the telemetry there are as many 1537-2047 bit entries as there are 2048-bit entries.)
Assignee | ||
Comment 1•9 years ago
|
||
The SSL_KEA_DHE_KEY_SIZE_FULL telemetry shows that requiring 2048-bit DHE keys for False Start would disable false start for almost all DHE handshakes, because almost all DHE handshakes (over 99%) use 1024-bit keys or smaller. Further, in Firefox 24, 9% of handshakes used DHE, but now only 5% do. Discussion in other bugs has shown that it is problematic that we don't validate the DH parameters we receive, but it is also problematic to validate them because the performance would be too terrible. Finally, not everybody agrees that 2048-bit DHE is sufficient; some people think that 2432-bit DHE is the minimum acceptable, and some think only 3072-bit or higher is acceptable. Consequently, we might as well just disable False Start for DHE altogether.
Summary: Require DHE keys to be at least 2048 bits for TLS False Start → Disable TLS False Start for non-ECDHE key exchange
Comment 2•9 years ago
|
||
Bug 1109766 will effectively enforce this because we do not (and will not) enable non-ECDHE AES-GCM cipher suites.
Assignee | ||
Comment 3•9 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #2) > Bug 1109766 will effectively enforce this because we do not (and will not) > enable non-ECDHE AES-GCM cipher suites. That's correct. Similarly, bug 1109766 will enforce what bug 861310 does too. However, it is important for all of these bugs to be fixed in order for the SSL_REASONS_FOR_NOT_FALSE_STARTING telemetry to be correct, so that we can more precisely measure the impact of these changes.
Assignee | ||
Comment 4•9 years ago
|
||
I thought I'd already attached a patch to this bug, bug I guess I was mistaken. I'll do so now.
Assignee: nobody → brian
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla37
Assignee | ||
Comment 5•9 years ago
|
||
This patch doesn't require any compatibility-breaking changes, so it can potentially be uplifted all the way to beta/release.
Attachment #8535775 -
Flags: review?(dkeeler)
Assignee | ||
Comment 6•9 years ago
|
||
This patch has non-backward-compatible interface changes, so it probably shouldn't be uplifted past Aurora.
Attachment #8535777 -
Flags: review?(dkeeler)
Attachment #8535775 -
Flags: review?(dkeeler) → review+
Comment on attachment 8535777 [details] [diff] [review] Part 2: Remove dead code for non-ECDHE False Start Review of attachment 8535777 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, but I imagine you should get sign-off from a Necko peer for the netwerk/ parts.
Attachment #8535777 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8535777 [details] [diff] [review] Part 2: Remove dead code for non-ECDHE False Start This is all dead code, due to the other patch in this bug, and due to bug 1109766 having already landed, since we don't support any non-ECDHE AEAD (AES-GCM) cipher suites.
Attachment #8535777 -
Flags: review?(mcmanus)
Assignee | ||
Comment 9•9 years ago
|
||
Comment on attachment 8535775 [details] [diff] [review] Part 1: Require ECDHE for False Start Review of attachment 8535775 [details] [diff] [review]: ----------------------------------------------------------------- https://hg.mozilla.org/integration/mozilla-inbound/rev/fbd204c775be Approval Request Comment: See bug 1109766 comment 4. Because bug 1109766's patch was uplifted, this is effectively a no-op patch, EXCEPT that it makes the telemetry more accurate. So, there should be NO user impact to this patch, but having accurate telemetry is important.
Attachment #8535775 -
Flags: checkin+
Attachment #8535775 -
Flags: approval-mozilla-aurora?
Comment 10•9 years ago
|
||
thanks for doing this - I'm excited about it. There is more to prune in protocol/http based on what you've done. I'll attach a suggested diff for your integration.
Comment 11•9 years ago
|
||
Comment 12•9 years ago
|
||
Comment on attachment 8535777 [details] [diff] [review] Part 2: Remove dead code for non-ECDHE False Start Review of attachment 8535777 [details] [diff] [review]: ----------------------------------------------------------------- I hope you'll choose to integrate comments 10 and 11.. but r=mcmanus in any event. I can land it separately if need be without a problem.
Attachment #8535777 -
Flags: review?(mcmanus) → review+
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fbd204c775be
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
Attachment #8535775 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 14•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/019b0263187c Is Part 2 still landing in this bug?
status-firefox36:
--- → fixed
status-firefox37:
--- → fixed
Assignee | ||
Comment 15•9 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #14) > Is Part 2 still landing in this bug? Re-opening so I can land Part 2, after reviewing Patrick's changes. Patrick, thanks for the quick review turnaround!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•9 years ago
|
Attachment #8537568 -
Flags: review+
Assignee | ||
Comment 16•9 years ago
|
||
Parts 2 and 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/b496822d9ff4 https://hg.mozilla.org/integration/mozilla-inbound/rev/9e5aea4e121c
Comment 17•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b496822d9ff4 https://hg.mozilla.org/mozilla-central/rev/9e5aea4e121c
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•