916226 - Add support for TLS_ECDHE_RSA_AES_GCM_SHA256 and TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 to Gecko

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[Camilo Viecco (:cviecco)]

Attachment: add-gcm-ciphers

NSS now has support of both rsa and ecdsa ecdhe aes128 gcm ciphersuites. We should allow users so enable them even tough we still have some tls 1.2 compatibility issues

Comment 1

[Camilo Viecco (:cviecco)]

Attachment: add-gcm-ciphers

Comment 2

[Camilo Viecco (:cviecco)]

Attachment: add-gcm-ciphers (v1.2)

Comment 3

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

Comment on attachment 804591 [details] [diff] [review]
add-gcm-ciphers (v1.2)

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

Looks fine, but I think you should find out what happens when you enable one of these but don't enable TLS 1.2.

::: netwerk/base/public/security-prefs.js
@@ +51,5 @@
>  pref("security.ssl3.rsa_aes_128_sha", true);
>  pref("security.ssl3.dhe_rsa_des_ede3_sha", true);
>  pref("security.ssl3.dhe_dss_des_ede3_sha", true);
>  pref("security.ssl3.rsa_seed_sha", true);
> +pref("security.ssl3.ecdhe_ecdsa_aes_128_gcm_sha256", false);

In the code that does the enabling of ciphers, do we need to add any kind of check that TLS 1.2 is actually enabled if these are ever set to true?

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +866,5 @@
>   {"security.ssl3.rsa_aes_128_sha", TLS_RSA_WITH_AES_128_CBC_SHA}, // 128-bit AES encryption with RSA and a SHA1 MAC
>   {"security.ssl3.dhe_rsa_des_ede3_sha", SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA}, // 168-bit Triple DES with RSA, DHE, and a SHA1 MAC
>   {"security.ssl3.dhe_dss_des_ede3_sha", SSL_DHE_DSS_WITH_3DES_EDE_CBC_SHA}, // 168-bit Triple DES with DSA, DHE, and a SHA1 MAC
>   {"security.ssl3.rsa_seed_sha", TLS_RSA_WITH_SEED_CBC_SHA}, // SEED encryption with RSA and a SHA1 MAC
> + // TLS 1.2 aes 128 gcm cipher suites

Looks like the other comments use /* */
Also, maybe just say "TLS 1.2 cipher suites" for if/when we add more? (Are there any more?)

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 804591 [details] [diff] [review]
add-gcm-ciphers (v1.2)

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

::: netwerk/base/public/security-prefs.js
@@ +52,5 @@
>  pref("security.ssl3.dhe_rsa_des_ede3_sha", true);
>  pref("security.ssl3.dhe_dss_des_ede3_sha", true);
>  pref("security.ssl3.rsa_seed_sha", true);
> +pref("security.ssl3.ecdhe_ecdsa_aes_128_gcm_sha256", false);
> +pref("security.ssl3.ecdhe_rsa_aes_128_gcm_sha256", false);

Enable these by default. NSS's libssl will only enable these cipher suites on TLS 1.2 connections and there's no reason to have them disabled by default now, AFAICT.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +866,5 @@
>   {"security.ssl3.rsa_aes_128_sha", TLS_RSA_WITH_AES_128_CBC_SHA}, // 128-bit AES encryption with RSA and a SHA1 MAC
>   {"security.ssl3.dhe_rsa_des_ede3_sha", SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA}, // 168-bit Triple DES with RSA, DHE, and a SHA1 MAC
>   {"security.ssl3.dhe_dss_des_ede3_sha", SSL_DHE_DSS_WITH_3DES_EDE_CBC_SHA}, // 168-bit Triple DES with DSA, DHE, and a SHA1 MAC
>   {"security.ssl3.rsa_seed_sha", TLS_RSA_WITH_SEED_CBC_SHA}, // SEED encryption with RSA and a SHA1 MAC
> + // TLS 1.2 aes 128 gcm cipher suites

I do not think any comment is necessary, really. The names are self-describing.

Comment 5

[Camilo Viecco (:cviecco)]

Attachment: add-gcm-ciphers 1.3

Keeping r+ from bsmith and dkeeler. 
Try run: 
 https://tbpl.mozilla.org/?tree=Try&rev=d3313bb01573

Comment 6

[Camilo Viecco (:cviecco)]

rebase to recent to solve issues with b2g ics,

https://tbpl.mozilla.org/?tree=Try&rev=49621219fb60

Comment 7

[Camilo Viecco (:cviecco)]

push to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/52abbdc1271a

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/52abbdc1271a

Comment 9

[Daniel Veditz [:dveditz]]

Are there dependencies of this bug that aren't marked? The NSS support in 3.15.2 is available on Aurora so maybe we can uplift this one? If it's just a matter of adding this patch it'd be a shame to wait an extra 6 weeks to get this out there.

Comment 10

[Camilo Viecco (:cviecco)]

There are no dependencie, the are some server intolerance issues with tls 1.2 but since this is not enabled by default I see no problem here. I would uplift it.

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Please nominate for uplift and provide a risk evaluation.

Comment 12

[Camilo Viecco (:cviecco)]

Comment on attachment 805638 [details] [diff] [review]
add-gcm-ciphers 1.3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Not a regression but a feature that makes certain attacks impossible. Solves BEAST without using RC4 (which now seems unsage). 

User impact if declined: 
Lack of availability of extra protections if TLS 1.2 is enabled.

Testing completed (on m-c, etc.): 
Landed on m-c. Enabled personally and have being going to google properties with AES-GCM on nightly since landing.

Risk to taking this patch (and alternatives if risky): 
If sites have buggy ssl servers AND the user has enabled TLS 1.2 (disabled by default) then the user will not be able to connect to that particular server.

String or IDL/UUID changes made by this patch:
None

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/968f4dec5f9d

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

According to bug 926116 this needs to be backed out until the patch in bug 919677 is landed.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out.
https://hg.mozilla.org/releases/mozilla-aurora/rev/e465bd05d4ef

Comment 16

[Mike Hommey [:glandium]]

Also note this needs a bump to configure.in's required system nss version, which the aurora landing didn't change.

Comment 17

[Mike Hommey [:glandium]]

In fact, the m-c landing didn't either.

Comment 18

[Camilo Viecco (:cviecco)]

no need to track for ff 26 as it has been backed out of aurora.

Comment 19

[Matt Wobensmith [:mwobensmith][:matt:]]

Was this bug fixed, backed out, or changed by the state of the bug(s) mentioned in comment 14? 

Code checked in and then backed out usually does not indicate a status of resolved/fixed.

Comment 20

[Florian Bender]

(In reply to Matt Wobensmith from comment #19)
> Was this bug fixed, backed out, or changed by the state of the bug(s)
> mentioned in comment 14?

Comment 21

[Camilo Viecco (:cviecco)]

This bug was superseeded by bug 934663. Which is now on m-c

Comment 22

[Matt Wobensmith [:mwobensmith][:matt:]]

Thanks Camilo. Would it be safe to call this bug a duplicate?

Comment 23

[Camilo Viecco (:cviecco)]

I would not call it a dup as the superseeding bug makes a bunch o other changes. But I would mark the commit for the other bug (dont know if this makes sense)

Comment 24

[Matt Wobensmith [:mwobensmith][:matt:]]

OK, sounds like it was fixed by bug 934663. Thanks again.

Comment 25

[Bogdan Maris, Desktop QA]

Seems this is also verified by bug 934663, but I was unable to see cipher "TLS_ECDHE_RSA_AES_GCM_SHA256" in a generated dump. I am looking at the first Client hello entry from the secure website I visit. 
Is there a way to see that cipher in order to verify this?

Comment 26

[Camilo Viecco (:cviecco)]

Bogdan: I see it in wirehsark using nightly. The name would be: TLS_ECDHE_RSA_WITH_AES_128_CGM_SHA256 (cipher suite c02f) (second suite selected)

Comment 27

[Bogdan Maris, Desktop QA]

Yes, I see TLS_ECDHE_RSA_WITH_AES_128_CGM_SHA256 in the dump, saw it before using Firefox 27 beta 5 but was not sure that is the same with TLS_ECDHE_RSA_AES_GCM_SHA256.
Thanks for clarification Camilo.