Closed
Bug 920248
Opened 11 years ago
Closed 11 years ago
Disable TLS False Start
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: briansmith, Assigned: briansmith)
References
Details
Attachments
(2 files, 5 obsolete files)
(deleted),
patch
|
briansmith
:
review+
akeybl
:
approval-mozilla-aurora+
briansmith
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
briansmith
:
review+
akeybl
:
approval-mozilla-beta+
briansmith
:
checkin+
|
Details | Diff | 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.
Assignee | ||
Comment 1•11 years ago
|
||
[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?
Assignee | ||
Updated•11 years ago
|
Attachment #809435 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Whiteboard: [leave open]
Assignee | ||
Comment 2•11 years ago
|
||
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+
Assignee | ||
Comment 3•11 years ago
|
||
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?
Assignee | ||
Comment 4•11 years ago
|
||
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?
Updated•11 years ago
|
Attachment #809442 -
Flags: review+
Comment 5•11 years ago
|
||
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+
Assignee | ||
Comment 6•11 years ago
|
||
(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 7•11 years ago
|
||
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+
Comment 8•11 years ago
|
||
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?
Comment 9•11 years ago
|
||
Approval comment:
- attachment 809575 [details] [diff] [review] is required
- attachment 809858 [details] [diff] [review] is addressing an unlikely edge case, only
Assignee | ||
Comment 10•11 years ago
|
||
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+
Updated•11 years ago
|
status-firefox25:
--- → affected
tracking-firefox25:
--- → ?
Updated•11 years ago
|
status-firefox26:
--- → affected
tracking-firefox26:
--- → ?
Comment 11•11 years ago
|
||
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.
status-firefox27:
--- → affected
tracking-firefox27:
--- → +
Assignee | ||
Comment 12•11 years ago
|
||
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+
Assignee | ||
Comment 13•11 years ago
|
||
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?
Assignee | ||
Comment 14•11 years ago
|
||
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?
Updated•11 years ago
|
Attachment #810077 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 years ago
|
Attachment #810086 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 16•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #810077 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #810086 -
Flags: checkin+
Comment 17•11 years ago
|
||
(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)?
Comment 18•11 years ago
|
||
Brian, is this fixed?
Assignee | ||
Updated•11 years ago
|
Summary: Reconsider TLS False Start criteria → Re-enable TLS False Start
Assignee | ||
Comment 19•11 years ago
|
||
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)
Assignee | ||
Comment 20•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Summary: Re-enable TLS False Start → Disable TLS False Start
Whiteboard: [leave open]
Updated•11 years ago
|
Target Milestone: --- → mozilla27
Assignee | ||
Comment 22•11 years ago
|
||
(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).
Comment 23•9 years ago
|
||
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.
Description
•