Closed
Bug 1036737
Opened 10 years ago
Closed 10 years ago
Add support for draft-ietf-tls-downgrade-scsv to Gecko/Firefox
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
People
(Reporter: briansmith, Assigned: mt)
References
(Blocks 1 open bug, )
Details
Attachments
(2 files, 2 obsolete files)
613 bytes,
application/javascript
|
Details | |
2.09 KB,
patch
|
mt
:
review+
bkerensa
:
approval-mozilla-esr31+
|
Details | Diff | Splinter Review |
We should add support for this draft to Gecko so we can get implementation experience to verify that it makes sense, and to encourage server-side adoption of the mechanism.
Assignee | ||
Comment 1•10 years ago
|
||
Looks easy. Chrome already uses it and have reported no ill effects, so I see no reason not to ship it. I have a few patches already; testing in progress.
Assignee: nobody → martin.thomson
Assignee | ||
Comment 2•10 years ago
|
||
I had a good day yesterday. I implemented this, then this morning realized that Google already have an implementation for NSS in Chrome.
What would it going take to get the Chrome implementation upstream into NSS so that we can use it?
I do have one concern with the implementation; since we do version intolerance checking outside of NSS, I'm concerned that we won't create the SCSV properly when we do fallback.
BTW, if you are interested, I can upload my patches. The SCSV handling is completely different to what is in Chrome; it may be too hacky though. I think that my error message text is better. The rest is almost identical.
Flags: needinfo?(wtc)
Assignee | ||
Comment 3•10 years ago
|
||
Wah-Teh, have you had a chance to look at this?
ekr and I have looked at the NSS code in Chrome: https://code.google.com/p/chromium/codesearch#chromium/src/net/third_party/nss/patches/fallbackscsv.patch&sq=package:chromium
And that looks like it would work for us. If you could land that in NSS, we will write some unit tests and make the necessary Firefox-side changes.
Comment 4•10 years ago
|
||
Martin:
I'm sorry I forgot to attach this patch. Please review and test it. Thanks.
Attachment #8470320 -
Flags: review?(martin.thomson)
Flags: needinfo?(wtc)
Comment 5•10 years ago
|
||
(In reply to Martin Thomson [:mt] from comment #2)
>
> I do have one concern with [the Chrome implementation]; since we do version
> intolerance checking outside of NSS, I'm concerned that we won't create the
> SCSV properly when we do fallback.
Are you still concerned about this?
> BTW, if you are interested, I can upload my patches. The SCSV handling is
> completely different to what is in Chrome; it may be too hacky though. I
> think that my error message text is better. The rest is almost identical.
Yes, I'm interested in using the better error message, at least. If you think
you can improve any of the code, please suggest changes. Thanks!
Comment 6•10 years ago
|
||
Comment on attachment 8470320 [details] [diff] [review]
Patch by Adam Langley
I moved this NSS patch to the NSS bug 1036735.
Attachment #8470320 -
Attachment is obsolete: true
Attachment #8470320 -
Flags: review?(martin.thomson)
Assignee | ||
Comment 7•10 years ago
|
||
If this makes the next train, that would be great. But the window is short, so...
Attachment #8481527 -
Flags: review?(dkeeler)
Assignee | ||
Comment 8•10 years ago
|
||
Here's how I tested this. I edited /etc/hosts to point www.google.com at 127.0.0.1. Then, I edit the attached relay script to include the current IP address of www.google.com. I then run the attached script and attempted to visit https://www.google.com/. Google respect the fallback SCSV and therefore trigger the error.
Then I have to remember to revert the change to /etc/hosts. Usually that takes a few days.
Assignee | ||
Comment 9•10 years ago
|
||
Comment 10•10 years ago
|
||
Can we add a host to BadCertServer that does what Google does here? (i.e. that way we can have an automated test for this)
Comment 11•10 years ago
|
||
Comment on attachment 8481527 [details] [diff] [review]
0001-Bug-1036737-Adding-fallback-SCSV-use.patch
Review of attachment 8481527 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with comments addressed (in particular the one about session resumption). Also, we should definitely see if we can get some automated tests for this. Finally, is this the kind of thing where it would be good to have a pref so it can be disabled if it's problematic?
::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +2394,4 @@
> return NS_ERROR_FAILURE;
> }
>
> + PRUint16 oldMaxVer = range.max;
nit: uint16_t
@@ +2410,5 @@
> infoObject->SetTLSVersionRange(range);
>
> + // when adjustForTLSIntolerance tweaks the maximum version downward,
> + // we tell the server using this SCSV so they can detect a downgrade attack
> + if (oldMaxVer > range.max) {
nit: to me, this would make more sense as range.max < oldMaxVer (also, maybe oldMaxVer should be something like maxEnabledVersion?)
@@ +2413,5 @@
> + // we tell the server using this SCSV so they can detect a downgrade attack
> + if (oldMaxVer > range.max) {
> + PR_LOG(gPIPNSSLog, PR_LOG_DEBUG,
> + ("[%p] nsSSLIOLayerSetOptions: enabling TLS_FALLBACK_SCSV\n", fd));
> + if (SECSuccess != SSL_OptionSet(fd, SSL_ENABLE_FALLBACK_SCSV, PR_TRUE)) {
Would this happen for session resumption as well? (My understanding from a quick scan of the spec is that we don't want to specify this if we're resuming a session.)
Also, nit: true instead of PR_TRUE
Attachment #8481527 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 12•10 years ago
|
||
r=keeler
(In reply to David Keeler (:keeler) [use needinfo?] from comment #11)
> Would this happen for session resumption as well? (My understanding from a
> quick scan of the spec is that we don't want to specify this if we're
> resuming a session.)
The spec doesn't really say anything about it, but the NSS code suppresses the SCSV when resuming:
http://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c#5144
I think that there was a comment on the TLS mailing list about the resumption use of the SCSV. The SCSV - if ignored - will appear during renegotiation, which is arguably wasteful, but it's also harmless.
Attachment #8481527 -
Attachment is obsolete: true
Attachment #8481621 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 13•10 years ago
|
||
As for tests (I forgot this), this is something that we have planned for the NSS unit tests. Camilo is currently bashing those into some sort of reasonable shape. Unit tests of the sort that appear in security/manager/ssl/tests/gtest/TLSIntoleranceTest.cpp are interesting, but I'd want to have something a little more robust as an integration test.
Comment 14•10 years ago
|
||
Keywords: checkin-needed
Comment 15•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 16•10 years ago
|
||
Bug 1053565 intends to add the associated nss version test to configure.in.
Depends on: 1053565
Comment 17•10 years ago
|
||
I suggest to add this patch to the ESR 31.x branch.
For the justification, please refer to bug 1084005.
Comment 19•10 years ago
|
||
Martin suggested to use this bug for the entire ESR 31.x backport decision.
Setting the flag here.
If you agreed, it would require:
- landing the patch from this bug
- in addition, upgrade NSS to e.g. 3.16.2.3 (being considered, not yet done)
tracking-firefox-esr31:
--- → ?
Comment 20•10 years ago
|
||
Comment on attachment 8481621 [details] [diff] [review]
0001-Bug-1036737-Adding-fallback-SCSV-use.patch
Requesting approval for ESR 31. It depends on the NSS upgrade suggested in bug 1084005.
An esr-31 try build with both changes has been started here:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f69e43df260f
Attachment #8481621 -
Flags: approval-mozilla-esr31?
Updated•10 years ago
|
Attachment #8481621 -
Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Updated•10 years ago
|
status-firefox-esr31:
--- → affected
Comment 21•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-esr31/rev/08a13694dde6
Do we need this on Beta34 as well? Also, I think we want this for B2G v1.4 and v2.0 if we're taking bug 1084005.
status-b2g-v1.4:
--- → affected
status-b2g-v2.0:
--- → affected
status-b2g-v2.0M:
--- → affected
status-b2g-v2.1:
--- → affected
status-b2g-v2.2:
--- → fixed
status-firefox34:
--- → ?
status-firefox35:
--- → fixed
Flags: needinfo?(martin.thomson)
Assignee | ||
Comment 22•10 years ago
|
||
Honestly, it wasn't my plan to push this into earlier releases. This requires server support, which diminishes its value significantly. I can understand the desire to push this into an ESR, because those users will be exposed until we get to the next ESR, but this isn't going to be good if we just land this.
I've been at a meeting today, I didn't see the approval. If you take this, then you are going to want to take a few more patches: primarily bug 1072382 and probably also bug 1088950. That is, unless you hate gmail users.
(I apologize for missing the flag change :kaie made, I should have added this information at that time.)
Flags: needinfo?(martin.thomson)
Comment 23•10 years ago
|
||
Sounds like we may want to consider backing this out from esr31 then? Kai/David, any opinions?
Flags: needinfo?(kaie)
Flags: needinfo?(dkeeler)
Comment 24•10 years ago
|
||
Is there any reason to avoid uplifting this patch to 34beta?
Comment 25•10 years ago
|
||
We should probably either take the additional fixes mentioned in comment 22 or back this out of ESR31.
Flags: needinfo?(dkeeler)
Comment 26•10 years ago
|
||
I don't have a strong opinion, because I haven't looked at the required remedy patches mentioned in comment 22.
I'll let you make the decision.
Updated•10 years ago
|
Flags: needinfo?(kaie)
Comment 27•10 years ago
|
||
Regarding comment 22 and comment 25:
In the meantime, triggered by bug 1107578, the patch from bug 1072382 has been uplifted to esr31, and mt recommended not to uplift bug 1088950 (see bug 1072382 comment 20).
You need to log in
before you can comment on or make changes to this bug.
Description
•