Closed Bug 1036735 Opened 6 years ago Closed 6 years ago

Add support for draft-ietf-tls-downgrade-scsv to NSS

Categories

(NSS :: Libraries, enhancement, P1)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED
3.17.1

People

(Reporter: briansmith, Assigned: agl)

References

()

Details

Attachments

(2 files, 1 obsolete file)

In the Google Chrome tree, there is already a patch that implements support for the fallback SCSV:
http://src.chromium.org/viewvc/chrome/trunk/src/net/third_party/nss/patches/fallbackscsv.patch?view=log

I am not sure what the state of that implementation is, with respect to the specification. Wan-Teh and Adam, are there changes to that patch that would be needed to bring it in sync with the current draft? Is there any significant reason why we should wait to add support for this, off by default, to NSS?
Attached patch Patch by Adam Langley (obsolete) — Splinter Review
Attachment #8470326 - Flags: review?(martin.thomson)
Assignee: nobody → wtc
Severity: normal → enhancement
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → 3.17
Comment on attachment 8470326 [details] [diff] [review]
Patch by Adam Langley

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

Thanks Wan-Teh.

::: lib/ssl/SSLerrs.h
@@ +421,5 @@
> +
> +ER3(SSL_ERROR_INAPPROPRIATE_FALLBACK_ALERT, (SSL_ERROR_BASE + 131),
> +"The connection was using a lesser TLS version as a result of a previous"
> +" handshake failure, but the server indicated that it should not have been"
> +" needed.")

I had different text here:

The server rejected the handshake because the client downgraded to a lower TLS version than the server supports.

::: lib/ssl/sslproto.h
@@ +209,5 @@
>  #define TLS_EMPTY_RENEGOTIATION_INFO_SCSV       0x00FF
>  
> +/* TLS_FALLBACK_SCSV is a signaling cipher suite value that indicates that a
> + * handshake is the result of TLS version fallback. This value is not IANA
> + * assigned. */

This is now IANA assigned.
Attachment #8470326 - Flags: review?(martin.thomson) → review+
Martin: I made the changes you suggested. I won't be able to
check this in until next Monday because the NSS tree is closed
in preparation for the NSS 3.17 release.
Attachment #8470326 - Attachment is obsolete: true
Attachment #8470345 - Flags: review?(martin.thomson)
Attachment #8470345 - Flags: review?(martin.thomson) → review+
I take it then that this is going to miss 3.17.  When is this likely to land Firefox side?
Flags: needinfo?(dkeeler)
I imagine this will land in NSS 3.17.1. mozilla-central can take betas of 3.17.1 as soon as they're ready, really (see for example bug 1020695).
Flags: needinfo?(dkeeler)
Comment on attachment 8470345 [details] [diff] [review]
Patch by Adam Langley, v2

Patch checked in: https://hg.mozilla.org/projects/nss/rev/45cb71fd7bca
Attachment #8470345 - Flags: checked-in+
Assignee: wtc → agl
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [3.17.1]
Whiteboard: [3.17.1]
Target Milestone: 3.17 → 3.17.1
Comment on attachment 8470345 [details] [diff] [review]
Patch by Adam Langley, v2

Patch pushed to mozilla-inbound as part of NSS_3_17_1_BETA1:
https://hg.mozilla.org/integration/mozilla-inbound/rev/102eaae71580
Wan-Teh, does this look right? I have added the dummy bit as discussed, for full binary compatibility (even if it were unnecessary, it shouldn't hurt).
Attachment #8506359 - Flags: review?(wtc)
Blocks: 1084005
Comment on attachment 8506359 [details] [diff] [review]
patch v2 backported to the NSS_3_16_2_BRANCH

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

r=wtc. Thanks!
Attachment #8506359 - Flags: review?(wtc) → review+
backported patch checked in to NSS_3_16_2_BRANCH
https://hg.mozilla.org/projects/nss/rev/53506619e81a
to be released as part of 3.16.2.3
You need to log in before you can comment on or make changes to this bug.