Add support for draft-ietf-tls-downgrade-scsv to Gecko/Firefox

RESOLVED FIXED in Firefox 35, Firefox OS v2.2

Status

()

Core
Security: PSM
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: briansmith, Assigned: mt)

Tracking

(Blocks: 1 bug)

Trunk
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox34 ?, firefox35 fixed, firefox-esr3134+ fixed, b2g-v1.4 affected, b2g-v2.0 affected, b2g-v2.0M affected, b2g-v2.1 affected, b2g-v2.2 fixed)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

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

4 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

4 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

4 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

4 years ago
Created attachment 8470320 [details] [diff] [review]
Patch by Adam Langley

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

4 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

4 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

4 years ago
Created attachment 8481527 [details] [diff] [review]
0001-Bug-1036737-Adding-fallback-SCSV-use.patch

If this makes the next train, that would be great.  But the window is short, so...
Attachment #8481527 - Flags: review?(dkeeler)
(Assignee)

Comment 8

4 years ago
Created attachment 8481529 [details]
relay.js

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

4 years ago
Created attachment 8481621 [details] [diff] [review]
0001-Bug-1036737-Adding-fallback-SCSV-use.patch

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

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 13

4 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.
https://hg.mozilla.org/mozilla-central/rev/6f76904f5567
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Bug 1053565 intends to add the associated nss version test to configure.in.
Depends on: 1053565
Depends on: 1072382
(Assignee)

Updated

3 years ago
Blocks: 1075167
(Assignee)

Updated

3 years ago
Blocks: 1075991
(Assignee)

Updated

3 years ago
Blocks: 1076983

Updated

3 years ago
See Also: → bug 1084005

Comment 17

3 years ago
I suggest to add this patch to the ESR 31.x branch.

For the justification, please refer to bug 1084005.

Updated

3 years ago
Duplicate of this bug: 1084005

Comment 19

3 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

3 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?
Attachment #8481621 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
status-firefox-esr31: --- → affected
tracking-firefox-esr31: ? → 34+
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
status-firefox-esr31: affected → fixed
Flags: needinfo?(martin.thomson)
(Assignee)

Comment 22

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

3 years ago
Is there any reason to avoid uplifting this patch to 34beta?
We should probably either take the additional fixes mentioned in comment 22 or back this out of ESR31.
Flags: needinfo?(dkeeler)

Comment 26

3 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

3 years ago
Flags: needinfo?(kaie)

Comment 27

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