Closed Bug 920248 Opened 6 years ago Closed 6 years ago

Disable TLS False Start

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox25 + fixed
firefox26 + fixed
firefox27 + fixed

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(2 files, 5 obsolete files)

Attached patch disable TLS false start (obsolete) — Splinter Review
Based on recent news about governments exploiting web servers to obtain private keys, speculation about even worse attacks against RC4, and recent adoption of ephemeral-key-exchange AES-based cipher suites by popular websites, we should reconsider our criteria for TLS false start. Until we have a new plan, let's disable false start.
Attached patch disable-false-start.patch (obsolete) — Splinter Review
[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 658222
See the release-drivers discussion. I am going to uplift this before it gets approval. The approval flags are just so it stays on peoples' radar.
Attachment #809442 - Flags: review?(mcmanus)
Attachment #809442 - Flags: approval-mozilla-beta?
Attachment #809442 - Flags: approval-mozilla-aurora?
Attachment #809435 - Attachment is obsolete: true
Whiteboard: [leave open]
Comment on attachment 809442 [details] [diff] [review]
disable-false-start.patch

Hmm...looks like mcmanus is offline. Will move review to keeler.
Attachment #809442 - Flags: review?(mcmanus) → review?(dkeeler)
Attachment #809442 - Flags: review?(dkeeler) → review+
Attached patch remove-false-start-beta.patch (obsolete) — Splinter Review
Kai, this patch removes the private patch for TLS False Start from mozilla-beta and removes the parts of the PSM code that used the removed functionality. After this patch, mozilla-beta should be compatible with NSS_3_15_1_RTM according to security/nss/TAG-INFO.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 658222
User impact if declined: See release-drivers thread. Also, the removal of the private patch to NSS is needed to support the --use-system-nss build option for Linux distros like Red Hat and Debian.

Testing completed (on m-c, etc.): Tested locally. This patch shouldn't be applied to mozilla-central or mozilla-aurora.

Risk to taking this patch (and alternatives if risky): Basically none. We already connect to basically every TLS server using a non-false-start connection at least once. There will be undoing a performance win, returning us to the point where we were with the Firefox 24 release.

String or IDL/UUID changes made by this patch: none
Assignee: nobody → brian
Status: NEW → ASSIGNED
Attachment #809575 - Flags: review?(kaie)
Attachment #809575 - Flags: approval-mozilla-beta?
Comment on attachment 809442 [details] [diff] [review]
disable-false-start.patch

Only the second patch is needed for mozilla-beta.
Attachment #809442 - Flags: approval-mozilla-beta?
Attachment #809442 - Flags: review+
Comment on attachment 809575 [details] [diff] [review]
remove-false-start-beta.patch

Review of attachment 809575 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ -2562,5 @@
>      return nullptr;
>    }
>    SSL_SetPKCS11PinArg(sslSock, (nsIInterfaceRequestor*)infoObject);
>    SSL_HandshakeCallback(sslSock, HandshakeCallback, infoObject);
> -  SSL_SetCanFalseStartCallback(sslSock, CanFalseStartCallback, infoObject);

Just wanted to verify that you did not delete CanFalseStartCallback
because you wanted to keep the patch small. Correct?
Attachment #809575 - Flags: review+
(In reply to Wan-Teh Chang from comment #5)
> Just wanted to verify that you did not delete CanFalseStartCallback
> because you wanted to keep the patch small. Correct?

Exactly. I did not revert the entire false start patch for PSM/Necko because I wanted to minimize disruption.
Comment on attachment 809575 [details] [diff] [review]
remove-false-start-beta.patch

I confirm this patch reverts mozilla-beta back to unmodified/released NSS 3.15.1, thanks for cleaning up.

Please take this patch for mozilla-beta 25
Attachment #809575 - Flags: review?(kaie) → review+
Attached patch incremental-beta-remove.patch (obsolete) — Splinter Review
Brian, I found another place in PSM that checks for the pref.

I agree it's not something that many users would do, however, if a user looked at about:config, and changed the pref twice, it would execute code that enables false start.

You might consider to remove that observer code and set the pref to disabled for clarity.
Attachment #809858 - Flags: review?(brian)
Attachment #809858 - Flags: approval-mozilla-beta?
Approval comment:

- attachment 809575 [details] [diff] [review] is required

- attachment 809858 [details] [diff] [review] is addressing an unlikely edge case, only
Comment on attachment 809858 [details] [diff] [review]
incremental-beta-remove.patch

Review of attachment 809858 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Kai.
Attachment #809858 - Flags: review?(brian) → review+
Once this is fixed on central we can look at uplifts (even though different patches) so that we ensure this is fixed across the board.
This is basically a combination of the relevant parts of kai's patch for mozilla-beta with my original patch, with the goal to make the mozilla-central, mozilla-aurora, and mozilla-beta patches as similar as possible.
Attachment #809442 - Attachment is obsolete: true
Attachment #809442 - Flags: approval-mozilla-aurora?
Attachment #810077 - Flags: review+
Comment on attachment 810077 [details] [diff] [review]
disable-false-start-central.patch

Review of attachment 810077 [details] [diff] [review]:
-----------------------------------------------------------------

https://hg.mozilla.org/integration/mozilla-inbound/rev/c42bd1756a79
Attachment #810077 - Flags: approval-mozilla-aurora?
disable-false-start-central.patch (which includes Kai's additions, slightly modified) combined with my original remove-false-start-beta.patch.
Attachment #809575 - Attachment is obsolete: true
Attachment #809858 - Attachment is obsolete: true
Attachment #809575 - Flags: approval-mozilla-beta?
Attachment #809858 - Flags: approval-mozilla-beta?
Attachment #810086 - Flags: review+
Attachment #810086 - Flags: approval-mozilla-beta?
Attachment #810077 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #810086 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Brian Smith (:briansmith, was :bsmith@mozilla.com) from comment #0)
> Based on recent news about governments exploiting web servers to obtain
> private keys, speculation about even worse attacks against RC4, and recent
> adoption of ephemeral-key-exchange AES-based cipher suites by popular
> websites, we should reconsider our criteria for TLS false start. Until we
> have a new plan, let's disable false start.

Is there a specific security concern with TLS False Start that motivates disabling it in particular, or is this just speculation?  It's not obvious how any of the items you mentioned specifically relates to TLS False Start more than SSL/TLS in general.  Is there a discussion of this somewhere?

Is there a reason to completely remove the functionality, rather than preffing it off by default (for instance, so that people can continue to use the code for security and performance analyses)?
Summary: Reconsider TLS False Start criteria → Re-enable TLS False Start
Attached patch re-enable-false-start.patch (obsolete) — Splinter Review
Note that in this patch I changed the default to require perfect forward secrecy, and I removed false start support for RC4.

I verified that we negotiate AES-GCM cipher suites when connecting to Twitter, Facebook, CloudFlare, and Google.

I also modified the telemetry so that we have more precise measurements for why we don't false start. So far in my limited testing, I've found that requiring forward secrecy and not allowing false start for RC4 have no effect on how often we false start. There appear to be two primary reasons: (1) The NPN requirement seems to subsume these requirements; that is, sites that support NPN seem to negotiate non-RC4 cipher suites with ephemeral key exchange already, and (2) The false start callback isn't called in many cases, presumably because the Finished message arrived before the certificate was successfully verified. Consequently, based on my limited testing, it seems like the primary improvements we need to make to improve the frequency of false starting are to remove the NPN requirement and to make certificate verification faster (i.e. stop doing OCSP requests).

Let's see if the telemetry we gather on Nightly and Aurora users matches my personal measurements.
Attachment #8337598 - Flags: review?(mcmanus)
Comment on attachment 8337598 [details] [diff] [review]
re-enable-false-start.patch

Never mind, I am going to do the re-enabling in a new bug, because I'm going to morph this back to "disable false start," because all the tracking flags are already marked "fixed" for the purpose of disabling false start.
Attachment #8337598 - Attachment is obsolete: true
Attachment #8337598 - Flags: review?(mcmanus)
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Summary: Re-enable TLS False Start → Disable TLS False Start
Whiteboard: [leave open]
The bug to re-enable false start is now bug 942729.
No longer depends on: 898431, 942728
See Also: → 942729
Target Milestone: --- → mozilla27
(In reply to Josh Triplett from comment #17)
> Is there a specific security concern with TLS False Start that motivates
> disabling it in particular, or is this just speculation?

Josh, the issue was bug 919877, which was a security issue in the NSS implementation of TLS False Start. That bug was fixed and TLS False Start is now enabled in Firefox 28 (beta).
What's the status on this? There were numerous papers which indicated that falsestart could be used for downgrade attacks... had all these been fixed to allow keeping it enabled?
You need to log in before you can comment on or make changes to this bug.