952863 - Disable TLS False Start for non-ECDHE key exchange

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.)

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 2

[Masatoshi Kimura [:emk]]

Bug 1109766 will effectively enforce this because we do not (and will not) enable non-ECDHE AES-GCM cipher suites.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I thought I'd already attached a patch to this bug, bug I guess I was mistaken. I'll do so now.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 1: Require ECDHE for False Start

This patch doesn't require any compatibility-breaking changes, so it can potentially be uplifted all the way  to beta/release.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 2: Remove dead code for non-ECDHE False Start

This patch has non-backward-compatible interface changes, so it probably shouldn't be uplifted past Aurora.

Comment 7

[Dana Keeler (she/her) [:keeler]]

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.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 10

[Patrick McManus [:mcmanus]]

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

[Patrick McManus [:mcmanus]]

Attachment: DEADCODE

Comment 12

[Patrick McManus [:mcmanus]]

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.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fbd204c775be

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/019b0263187c

Is Part 2 still landing in this bug?

Comment 15

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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!

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Parts 2 and 3:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b496822d9ff4
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e5aea4e121c

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b496822d9ff4
https://hg.mozilla.org/mozilla-central/rev/9e5aea4e121c