1247860 - Enable ChaCha20/Poly1305 cipher suites for Firefox

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

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?

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1247860-Enable-ChaCha20-Poly1305-cipher-suites.patch

Here's a WIP patch if someone wants to play around with this. Needs NSS trunk revision from ~today.

Comment 3

[Masatoshi Kimura [:emk]]

(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?

Comment 4

[Masatoshi Kimura [:emk]]

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

Comment 5

[Martin Thomson [:mt:]]

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

Comment 6

[Masatoshi Kimura [:emk]]

(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 :)

Comment 7

[Martin Thomson [:mt:]]

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.

Comment 8

[Masatoshi Kimura [:emk]]

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.

Comment 9

[Masatoshi Kimura [:emk]]

You should also update security-prefs.js.

Comment 10

[Masatoshi Kimura [:emk]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e35ac1b596d3

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1247860-Enable-ChaCha20-Poly1305-cipher-suites.patch, v2

Comment 13

[Masatoshi Kimura [:emk]]

NSS_3_23_BETA5 will bring ChaCha20-Poly1305.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 15

[Martin Thomson [:mt:]]

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.

Comment 16

[Masatoshi Kimura [:emk]]

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?

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 18

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 19

[Masatoshi Kimura [:emk]]

>   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

Comment 20

[Martin Thomson [:mt:]]

These are OK for both false start and h2.  If they weren't we wouldn't be adding them.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

(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 22

[Patrick McManus [:mcmanus]]

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.

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

(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

Comment 24

[Masatoshi Kimura [:emk]]

And even if NSS supports other cipher suites, Firefox does not enable them anyway.

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1247860-Enable-ChaCha20-Poly1305-cipher-suites-r.patch, v3

r=emk,keeler

Comment 26

[Masatoshi Kimura [:emk]]

What is left before landing?

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9604b2f00497

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9604b2f00497

Comment 30

[Tim Taubert [:ttaubert] (inactive)]

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)

Comment 31

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx47 (beta) release notes.