Closed Bug 1247860 Opened 4 years ago Closed 4 years ago

Enable ChaCha20/Poly1305 cipher suites for Firefox

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed
relnote-firefox --- 47+

People

(Reporter: ttaubert, Assigned: ttaubert)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 2 obsolete files)

We're not there yet but I wanted to get this on file once we have NSS 3.23 in Firefox. The available cipher suites are:

TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256   0xCCA8
TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256 0xCCA9
TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256     0xCCAA

There are a few questions:

1) Do we want to enable TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256? I think we might not actually want to.

2) Will we keep GCM prioritized over ChaCha/Poly?

3) Will we prioritize ChaCha/Poly on CPUs without AES-NI?

4) If we can't/don't want to detect AES-NI, do we at least prioritize ChaCha on Firefox for Android?
(In reply to Tim Taubert [:ttaubert] from comment #0)
> We're not there yet but I wanted to get this on file once we have NSS 3.23
> in Firefox.

... so we're ready to land this once we have NSS 3.23 in Firefox.
Here's a WIP patch if someone wants to play around with this. Needs NSS trunk revision from ~today.
(In reply to Tim Taubert [:ttaubert] from comment #0)
> 1) Do we want to enable TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256? I think
> we might not actually want to.

I also think we do not. We do not enable TLS_DHE_RSA_WITH_AES_128_GCM_SHA256 either.

> 2) Will we keep GCM prioritized over ChaCha/Poly?
> 
> 3) Will we prioritize ChaCha/Poly on CPUs without AES-NI?
> 
> 4) If we can't/don't want to detect AES-NI, do we at least prioritize ChaCha
> on Firefox for Android?

Does NSS support prioritizing cipher suites at all?
Probably you should change
  if (cipherInfo.symCipher != ssl_calg_aes_gcm) {
to
  if (cipherInfo.macAlgorithm != ssl_mac_aead) {
in CanFalseStartCallback() to allow TLS False Start for ChaCha20/Poly1305 (and other future AEAD cipher suites).
I agree, no to DHE.

We can't prioritize.  Feel free to open a bug on that.

And yes, enabling false start is good.  I'm OK with the change to all AEAD, we shouldn't be adding a new cipher suite that isn't at least as good as what we have.  (Generally, we shouldn't be adding a new cipher suite, but here we are...)
(In reply to Martin Thomson [:mt:] from comment #5)
> We can't prioritize.  Feel free to open a bug on that.

We already had a bug: bug 1126830 :)
Bug 1126830 doesn't count, that's UI for this.  That's the last thing I want.  An interface to NSS whereby we can reorder ciphers might be handy, to enable the sorts of things Tim mentions.
Bug 1126830 changed the product to NSS because it was WONTFIX as a Firefox bug (Firefox will not add such UI). The summary should have been changed.
You should also update security-prefs.js.
(In reply to Masatoshi Kimura [:emk] from comment #4)
> Probably you should change
>   if (cipherInfo.symCipher != ssl_calg_aes_gcm) {
> to
>   if (cipherInfo.macAlgorithm != ssl_mac_aead) {
> in CanFalseStartCallback() to allow TLS False Start for ChaCha20/Poly1305
> (and other future AEAD cipher suites).

Good idea.

(In reply to Masatoshi Kimura [:emk] from comment #9)
> You should also update security-prefs.js.

Done.
NSS_3_23_BETA5 will bring ChaCha20-Poly1305.
Depends on: 1245053
Comment on attachment 8719286 [details] [diff] [review]
0001-Bug-1247860-Enable-ChaCha20-Poly1305-cipher-suites.patch, v2

NSS with ChaCha20/Poly1305 cipher suites landed on inbound, would be great to get this landed before the next merge in two weeks.
Attachment #8719286 - Flags: review?(dkeeler)
Comment on attachment 8719286 [details] [diff] [review]
0001-Bug-1247860-Enable-ChaCha20-Poly1305-cipher-suites.patch, v2

David is pretty busy, perhaps we can get a sanity check first and allow David to just approve/sanity check.
Attachment #8719286 - Flags: review?(VYV03354)
Comment on attachment 8719286 [details] [diff] [review]
0001-Bug-1247860-Enable-ChaCha20-Poly1305-cipher-suites.patch, v2

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

LGTM, but David should sign-off.

::: security/manager/ssl/nsNSSCallbacks.cpp
@@ +1088,5 @@
>      case TLS_DHE_DSS_WITH_CAMELLIA_128_CBC_SHA: value = 27; break;
>      case TLS_DHE_DSS_WITH_AES_256_CBC_SHA: value = 28; break;
>      case TLS_DHE_DSS_WITH_CAMELLIA_256_CBC_SHA: value = 29; break;
>      case TLS_DHE_DSS_WITH_3DES_EDE_CBC_SHA: value = 30; break;
> +    case TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256: value = 31; break;

Is this needed?
Attachment #8719286 - Flags: review?(VYV03354) → review+
(In reply to Masatoshi Kimura [:emk] from comment #16)
> ::: security/manager/ssl/nsNSSCallbacks.cpp
> @@ +1088,5 @@
> >      case TLS_DHE_DSS_WITH_CAMELLIA_128_CBC_SHA: value = 27; break;
> >      case TLS_DHE_DSS_WITH_AES_256_CBC_SHA: value = 28; break;
> >      case TLS_DHE_DSS_WITH_CAMELLIA_256_CBC_SHA: value = 29; break;
> >      case TLS_DHE_DSS_WITH_3DES_EDE_CBC_SHA: value = 30; break;
> > +    case TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256: value = 31; break;
> 
> Is this needed?

We don't need it, but I thought this was supposed to reflect all available ciphers even if we don't use them. See for example TLS_DHE_DSS_WITH_3DES_EDE_CBC_SHA in the list, we don't use it but it has an assigned value, maybe in case we ever decide to use it.
Depends on: 1126830
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Comment on attachment 8719286 [details] [diff] [review]
0001-Bug-1247860-Enable-ChaCha20-Poly1305-cipher-suites.patch, v2

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

LGTM, but I'd like :mcmanus to confirm that we want to enable these cipher suites for false start.

::: security/manager/ssl/nsNSSCallbacks.cpp
@@ +1088,5 @@
>      case TLS_DHE_DSS_WITH_CAMELLIA_128_CBC_SHA: value = 27; break;
>      case TLS_DHE_DSS_WITH_AES_256_CBC_SHA: value = 28; break;
>      case TLS_DHE_DSS_WITH_CAMELLIA_256_CBC_SHA: value = 29; break;
>      case TLS_DHE_DSS_WITH_3DES_EDE_CBC_SHA: value = 30; break;
> +    case TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256: value = 31; break;

Yeah, let's only add what is possible to be enabled by prefs. (The leftover entries are from when we removed support for various cipher suites in Firefox.)

::: toolkit/components/telemetry/Histograms.json
@@ -7901,5 @@
>    "SSL_CIPHER_SUITE_FULL": {
>      "expires_in_version": "never",
>      "kind": "enumerated",
>      "n_values": 128,
> -    "description": "Negotiated cipher suite in full handshake (see key in HandshakeCallback in nsNSSCallbacks.cpp)"

I'm not sure it's a good idea to change the description of these preexisting histograms.
Attachment #8719286 - Flags: review?(dkeeler)
Attachment #8719286 - Flags: review+
Attachment #8719286 - Flags: feedback?(mcmanus)
>   if (cipherInfo.macAlgorithm != ssl_mac_aead) {

By the way, I took this idea from Http2Session::ConfirmTLSProfile:
https://dxr.mozilla.org/mozilla-central/rev/d848a5628d801a460a7244cbcdea22d328d8b310/netwerk/protocol/http/Http2Session.cpp#3516
These are OK for both false start and h2.  If they weren't we wouldn't be adding them.
(In reply to David Keeler [:keeler] (use needinfo?) from comment #18)
> ::: security/manager/ssl/nsNSSCallbacks.cpp
> @@ +1088,5 @@
> >      case TLS_DHE_DSS_WITH_CAMELLIA_128_CBC_SHA: value = 27; break;
> >      case TLS_DHE_DSS_WITH_AES_256_CBC_SHA: value = 28; break;
> >      case TLS_DHE_DSS_WITH_CAMELLIA_256_CBC_SHA: value = 29; break;
> >      case TLS_DHE_DSS_WITH_3DES_EDE_CBC_SHA: value = 30; break;
> > +    case TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256: value = 31; break;
> 
> Yeah, let's only add what is possible to be enabled by prefs. (The leftover
> entries are from when we removed support for various cipher suites in
> Firefox.)

Ok, removed.

> ::: toolkit/components/telemetry/Histograms.json
> @@ -7901,5 @@
> >    "SSL_CIPHER_SUITE_FULL": {
> >      "expires_in_version": "never",
> >      "kind": "enumerated",
> >      "n_values": 128,
> > -    "description": "Negotiated cipher suite in full handshake (see key in HandshakeCallback in nsNSSCallbacks.cpp)"
> 
> I'm not sure it's a good idea to change the description of these preexisting
> histograms.

Reverted.
Comment on attachment 8719286 [details] [diff] [review]
0001-Bug-1247860-Enable-ChaCha20-Poly1305-cipher-suites.patch, v2

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

::: security/manager/ssl/nsNSSCallbacks.cpp
@@ +987,5 @@
>  
>    // Prevent downgrade attacks on the symmetric cipher. We do not allow CBC
>    // mode due to BEAST, POODLE, and other attacks on the MAC-then-Encrypt
>    // design. See bug 1109766 for more details.
> +  if (cipherInfo.macAlgorithm != ssl_mac_aead) {

adding cha cha to the false start list is fine (and desirable).

My only question is if this diff does anything other than adding cha cha? That might not be ok.
Attachment #8719286 - Flags: feedback?(mcmanus)
(In reply to Patrick McManus [:mcmanus] from comment #22)
> > +  if (cipherInfo.macAlgorithm != ssl_mac_aead) {
> 
> adding cha cha to the false start list is fine (and desirable).
> 
> My only question is if this diff does anything other than adding cha cha?

No, AES-GCM and ChaCha20/Poly1305 are the only two supported AEAD ciphers:

https://mxr.mozilla.org/nss/ident?i=M_AEAD_128
And even if NSS supports other cipher suites, Firefox does not enable them anyway.
What is left before landing?
Keywords: checkin-needed
Let's do a try push first. I'll land it later today when that looks good:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=724d7f5be309
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9604b2f00497
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Blocks: 1178092
Release Note Request (optional, but appreciated)
[Why is this notable]: We added two new cipher suites using a new authenticated cipher.
[Suggested wording]: Added support for ChaCha20/Poly1305 cipher suites
[Links (documentation, blog post, etc)]: (nothing yet, might update later)
relnote-firefox: --- → ?
You need to log in before you can comment on or make changes to this bug.