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.

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.

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.

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.

Martin: I'm sorry I forgot to attach this patch. Please review and test it. Thanks.

(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 on attachment 8470320 [details] [diff] [review] Patch by Adam Langley I moved this NSS patch to the NSS bug 1036735.

If this makes the next train, that would be great. But the window is short, so...

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

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.

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.

Bug 1053565 intends to add the associated nss version test to configure.in.

I suggest to add this patch to the ESR 31.x branch. For the justification, please refer to bug 1084005.

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)

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

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.

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

Sounds like we may want to consider backing this out from esr31 then? Kai/David, any opinions?

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.

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.

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