Add support for TLS_ECDHE_RSA_AES_GCM_SHA256 and TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 to Gecko

VERIFIED FIXED in Firefox 27

Status

()

Core
Security: PSM
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: cviecco, Assigned: cviecco)

Tracking

({sec-want})

Trunk
mozilla27
sec-want
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox27+ verified)

Details

(Whiteboard: [adv-main27-])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 804588 [details]
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
(Assignee)

Comment 1

4 years ago
Created attachment 804589 [details] [diff] [review]
add-gcm-ciphers
Attachment #804588 - Attachment is obsolete: true
(Assignee)

Comment 2

4 years ago
Created attachment 804591 [details] [diff] [review]
add-gcm-ciphers (v1.2)
Attachment #804589 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #804591 - Flags: review?(dkeeler)
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?)
Attachment #804591 - Flags: review?(dkeeler) → review+
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.
Attachment #804591 - Flags: review+
(Assignee)

Comment 5

4 years ago
Created attachment 805638 [details] [diff] [review]
add-gcm-ciphers 1.3

Keeping r+ from bsmith and dkeeler. 
Try run: 
 https://tbpl.mozilla.org/?tree=Try&rev=d3313bb01573
Attachment #804591 - Attachment is obsolete: true
Attachment #805638 - Flags: review+
(Assignee)

Comment 6

4 years ago
rebase to recent to solve issues with b2g ics,

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

Updated

4 years ago
Assignee: nobody → cviecco
(Assignee)

Comment 7

4 years ago
push to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/52abbdc1271a
https://hg.mozilla.org/mozilla-central/rev/52abbdc1271a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Summary: Allow ecdhe AES128 GCM ciphersuites off (by default) → Add support for TLS_ECDHE_RSA_AES_GCM_SHA256 and TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 to Gecko
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.
status-firefox27: --- → fixed
tracking-firefox26: --- → ?
tracking-firefox27: --- → +
Depends on: 880543
Flags: needinfo?(cviecco)
Keywords: sec-want
(Assignee)

Comment 10

4 years ago
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.
Flags: needinfo?(cviecco)
Please nominate for uplift and provide a risk evaluation.
status-firefox26: --- → affected
tracking-firefox26: ? → +
(Assignee)

Comment 12

4 years ago
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
Attachment #805638 - Flags: approval-mozilla-aurora?
Attachment #805638 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/968f4dec5f9d
status-firefox26: affected → fixed
Depends on: 926116
According to bug 926116 this needs to be backed out until the patch in bug 919677 is landed.
Flags: needinfo?(ryanvm)
Depends on: 926773
Backed out.
https://hg.mozilla.org/releases/mozilla-aurora/rev/e465bd05d4ef
status-firefox26: fixed → affected
Flags: needinfo?(ryanvm)
Also note this needs a bump to configure.in's required system nss version, which the aurora landing didn't change.
In fact, the m-c landing didn't either.
(Assignee)

Comment 18

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

Updated

4 years ago
Keywords: verifyme
(Assignee)

Updated

4 years ago
status-firefox26: affected → ---
tracking-firefox26: + → ---
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

4 years ago
(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?
Flags: needinfo?(cviecco)
(Assignee)

Comment 21

4 years ago
This bug was superseeded by bug 934663. Which is now on m-c
Flags: needinfo?(cviecco)
Thanks Camilo. Would it be safe to call this bug a duplicate?
Flags: needinfo?(cviecco)
(Assignee)

Comment 23

4 years ago
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)
Flags: needinfo?(cviecco)
OK, sounds like it was fixed by bug 934663. Thanks again.
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?
Flags: needinfo?(cviecco)
(Assignee)

Comment 26

4 years ago
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)
Flags: needinfo?(cviecco)
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.
Status: RESOLVED → VERIFIED
status-firefox27: fixed → verified
Keywords: verifyme
Whiteboard: [adv-main27-]
You need to log in before you can comment on or make changes to this bug.