Closed
Bug 934663
Opened 11 years ago
Closed 11 years ago
Change set of cipher suites enabled by default in Gecko to match cipher suite proposal
Categories
(Core :: Security: PSM, enhancement)
Core
Security: PSM
Tracking
()
VERIFIED
FIXED
mozilla28
Tracking | Status | |
---|---|---|
firefox26 | --- | unaffected |
firefox27 | + | verified |
People
(Reporter: briansmith, Assigned: briansmith)
References
Details
Attachments
(1 file)
(deleted),
patch
|
cviecco
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
See https://briansmith.org/browser-ciphersuites-01.html.
For now, there will be three differences:
1. TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 and TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384 are not implemented in NSS, we we can't enable them yet. I will find/file a separate bug for enabling these in PSM when they are enabled in NSS.
2. I kept TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA and TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA
enabled because we should make sure we have the ephemeral key exchange version of every RSA key exchange cipher suite enabled if possible. (Note that there aren't any TLS_DHE_RSA_WITH_RC4_* cipher suites.) I will update the proposal to add these two cipher suites.
3. The ordering of cipher suites doesn't match the ordering in the proposal, because the ordering is controlled by NSS. I will file a separate bug about fixing the ordering.
Assignee | ||
Comment 1•11 years ago
|
||
To verify this fix, try one or more of the following:
1. Visit https://www.mikestoolbox.net/ (Yes, it really does require a certificate error exception) and compare the reported cipher suites to the set recommended at https://briansmith.org/browser-ciphersuites-01.html, taking into consideration the expected differences in comment 0.
2. Make a connection to Google and verify that the TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 cipher suite is being used.
3. Visit any HTTPS site with Wireshark active and verify that the ClientHello we send contains all the above cipher suites.
Assignee | ||
Comment 2•11 years ago
|
||
One more thing: I removed the entries for the cipher suites preferences from netwerk/base/public/security-prefs.js. The effect that this has is to hide these prefs from the about:config dialog box. This is a good idea because we don't really want people tweaking these prefs, IMO. Also, it makes it easier for us to do the right thing when modifying the list since we won't have two lists to keep in sync.
Comment 3•11 years ago
|
||
So the patch has the following suites enabled:
TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA
TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA
TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA
TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA
TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA
TLS_DHE_RSA_WITH_AES_128_CBC_SHA
TLS_DHE_RSA_WITH_AES_256_CBC_SHA
SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA
TLS_DHE_DSS_WITH_AES_128_CBC_SHA
TLS_ECDHE_RSA_WITH_RC4_128_SHA
TLS_ECDHE_ECDSA_WITH_RC4_128_SHA
TLS_RSA_WITH_AES_128_CBC_SHA
TLS_RSA_WITH_AES_256_CBC_SHA
SSL_RSA_WITH_3DES_EDE_CBC_SHA
SSL_RSA_WITH_RC4_128_SHA
SSL_RSA_WITH_RC4_128_MD5
(I think you want to get them in that order, but I understand that it requires changes in other places?)
What I'm missing is:
TLS_DHE_RSA_WITH_AES_128_GCM_SHA256 (I suggest it as first TLS_DHE_*)
TLS_RSA_WITH_AES_128_GCM_SHA256 (I suggest it was first TKS_RSA_*)
Those (and aes256 version) are supported by apache 2.2 while you need 2.4 for the ECDHE versions.
PS: Shouldn't those that are disabled by default actually get a "false"?
Assignee | ||
Comment 4•11 years ago
|
||
(In reply to Kurt Roeckx from comment #3)
> (I think you want to get them in that order, but I understand that it
> requires changes in other places?)
Right. I will file the NSS bug for that soon.
> What I'm missing is:
> TLS_DHE_RSA_WITH_AES_128_GCM_SHA256 (I suggest it as first TLS_DHE_*)
> TLS_RSA_WITH_AES_128_GCM_SHA256 (I suggest it was first TKS_RSA_*)
Let's discuss whether we should add these on the mailing list thread. It's out of scope for this bug. I will post a message to the mailing list thread soon explaining again why I don't want to add these (yet).
> PS: Shouldn't those that are disabled by default actually get a "false"?
Static variables are zero-initialized so it isn't necessary. I avoided adding the explicit 'false' to minimize the changes in the diff, to make it easier to review. I can add a follow-up patch to make the 'false' explicit if desired.
Comment 5•11 years ago
|
||
Comment on attachment 826989 [details] [diff] [review]
Change set of enabled cipher suites
Review of attachment 826989 [details] [diff] [review]:
-----------------------------------------------------------------
::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +881,1 @@
> {"security.ssl3.dhe_rsa_camellia_256_sha", TLS_DHE_RSA_WITH_CAMELLIA_256_CBC_SHA}, // 256-bit Camellia encryption with RSA, DHE, and a SHA1 MAC
From here on the last parameter is not explicit. I would add a note saying somethink like: since this is a static const the missing parameters are set to zero.
Attachment #826989 -
Flags: review?(cviecco) → review+
Comment 6•11 years ago
|
||
I disagree about preferring 128 bit variants over 256 bit variants, and also about removing cipher preferences from about:config, user choice is important.
Comment 7•11 years ago
|
||
(In reply to BoerenkoolMetWorst from comment #6)
> I disagree about preferring 128 bit variants over 256 bit variants, and also
> about removing cipher preferences from about:config, user choice is
> important.
This patch does not change the preference from 256 to 128 bits. For more information please see the mozilla-dev-crypto maillist, where this was discussed in depth.
Also, you can enable the disabled prefs by adding a new preference value (yes this disables easy modification). User choice, while user-unfriendly, remains there.
Are bug 926773 and bug 663320 duplicates of this one?
Comment 9•11 years ago
|
||
(In reply to Coyote from comment #8)
> Are bug 926773 and bug 663320 duplicates of this one?
bug 926773 is superseeded by this one.
bug 663320 is not nor a dup. (AES_256_CGM)
Comment 10•11 years ago
|
||
(In reply to Camilo Viecco (:cviecco) from comment #7)
> (In reply to BoerenkoolMetWorst from comment #6)
> > I disagree about preferring 128 bit variants over 256 bit variants, and also
> > about removing cipher preferences from about:config, user choice is
> > important.
>
> This patch does not change the preference from 256 to 128 bits. For more
> information please see the mozilla-dev-crypto maillist, where this was
> discussed in depth.
> Also, you can enable the disabled prefs by adding a new preference value
> (yes this disables easy modification). User choice, while user-unfriendly,
> remains there.
Thank you for the clarification.
Assignee | ||
Comment 11•11 years ago
|
||
I landed a slightly different version of the patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/23e213d57704
The difference is that I did not disable the Camellia cipher suites. During IETF 88, I learned some things that might change our decision about whether to disable them or not, so I'm taking the conservative approach and leaving them enabled. We can disable them later if we decided not to amend the "spec" for this work.
This conservative approach will also make it more reasonable to uplift this to mozilla-aurora.
Assignee | ||
Comment 12•11 years ago
|
||
I will ask for uplift to mozilla-aurora next week.
Flags: needinfo?(brian)
Target Milestone: --- → mozilla28
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(brian)
Comment 13•11 years ago
|
||
(In reply to Brian Smith from comment #4)
> (In reply to Kurt Roeckx from comment #3)
> > What I'm missing is:
> > TLS_DHE_RSA_WITH_AES_128_GCM_SHA256 (I suggest it as first TLS_DHE_*)
> > TLS_RSA_WITH_AES_128_GCM_SHA256 (I suggest it was first TKS_RSA_*)
>
> Let's discuss whether we should add these on the mailing list thread. It's
> out of scope for this bug. I will post a message to the mailing list thread
> soon explaining again why I don't want to add these (yet).
I've asked about those on the list before and nobody replied to that. I'm also still waiting for that message to the list.
Comment 14•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 15•11 years ago
|
||
Fixed unused variable warning:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e24e7d00b56d
Flags: needinfo?(brian)
Comment 16•11 years ago
|
||
Comment 17•11 years ago
|
||
With today's aurora update I ended up with those ciphers enabled by default:
ECDHE-ECDSA-AES128-SHA
ECDHE-RSA-AES128-SHA
ECDHE-ECDSA-AES256-SHA
ECDHE-RSA-AES256-SHA
ECDHE-ECDSA-3DES-EDE-SHA
ECDHE-RSA-3DES-EDE-SHA
ECDHE-ECDSA-RC4128-SHA
ECDHE-RSA-RC4128-SHA
DHE-RSA-AES128-SHA
DHE-DSS-AES128-SHA
DHE-RSA-CAMELLIA128-SHA
DHE-DSS-CAMELLIA128-SHA
DHE-RSA-AES256-SHA
DHE-DSS-AES256-SHA
DHE-RSA-CAMELLIA256-SHA
DHE-DSS-CAMELLIA256-SHA
DHE-RSA-3DES-EDE-SHA
DHE-DSS-3DES-EDE-SHA
ECDH-ECDSA-AES128-SHA
ECDH-RSA-AES128-SHA
ECDH-ECDSA-AES256-SHA
ECDH-RSA-AES256-SHA
ECDH-ECDSA-3DES-EDE-SHA
ECDH-RSA-3DES-EDE-SHA
ECDH-ECDSA-RC4128-SHA
ECDH-RSA-RC4128-SHA
RSA-AES128-SHA
RSA-CAMELLIA128-SHA
RSA-AES256-SHA
RSA-CAMELLIA256-SHA
RSA-SEED-SHA
RSA-FIPS-3DES-EDE-SHA
RSA-3DES-EDE-SHA
RSA-RC4128-SHA
RSA-RC4128-MD5
This isn't what I expected. I for instance didn't expect any ECDH-* ciphers to be enabled.
The following 2 seem to be disabled by default in the prefs, but when enabled end up at the top of the list.
ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
ECDHE_RSA_WITH_AES_128_GCM_SHA256
Comment 18•11 years ago
|
||
(In reply to Kurt Roeckx from comment #17)
> With today's aurora update I ended up with those ciphers enabled by default:
(snip)
> This isn't what I expected. I for instance didn't expect any ECDH-* ciphers
> to be enabled.
In 27 aurora and earlier, cipher suites has not changed at all.
Only 28 nightly has been affected by this bug.
Ciphers enabled by default in 28 nightly (no ECDH-* cuites is enabled)
ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
ECDHE_RSA_WITH_AES_128_GCM_SHA256
ECDHE_ECDSA_WITH_AES_128_CBC_SHA
ECDHE_RSA_WITH_AES_128_CBC_SHA
ECDHE_ECDSA_WITH_AES_256_CBC_SHA
ECDHE_RSA_WITH_AES_256_CBC_SHA
ECDHE_RSA_WITH_3DES_EDE_CBC_SHA
ECDHE_ECDSA_WITH_RC4_128_SHA
ECDHE_RSA_WITH_RC4_128_SHA
DHE_RSA_WITH_AES_128_CBC_SHA
DHE_DSS_WITH_AES_128_CBC_SHA
DHE_RSA_WITH_CAMELLIA_128_CBC_SHA
DHE_RSA_WITH_AES_256_CBC_SHA
DHE_DSS_WITH_AES_256_CBC_SHA
DHE_RSA_WITH_CAMELLIA_256_CBC_SHA
DHE_RSA_WITH_3DES_EDE_CBC_SHA
RSA_WITH_AES_128_CBC_SHA
RSA_WITH_CAMELLIA_128_CBC_SHA
RSA_WITH_AES_256_CBC_SHA
RSA_WITH_CAMELLIA_256_CBC_SHA
RSA_WITH_3DES_EDE_CBC_SHA
RSA_WITH_RC4_128_SHA
RSA_WITH_RC4_128_MD5
> The following 2 seem to be disabled by default in the prefs, but when
> enabled end up at the top of the list.
> ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
> ECDHE_RSA_WITH_AES_128_GCM_SHA256
These are already enabled in 28 nightly.
Comment 19•11 years ago
|
||
(In reply to Kosuke Kaizuka from comment #18)
> (In reply to Kurt Roeckx from comment #17)
> > With today's aurora update I ended up with those ciphers enabled by default:
> In 27 aurora and earlier, cipher suites has not changed at all.
> Only 28 nightly has been affected by this bug.
I'm telling you that in aurora this morning the list didn't change yet, but this afternoon (CET) it changed to the list I showed.
Comment 20•11 years ago
|
||
This might be the(In reply to Kurt Roeckx from comment #19)
> I'm telling you that in aurora this morning the list didn't change yet, but
> this afternoon (CET) it changed to the list I showed.
That might be the result of an update to NSS and not firefox itself.
Assignee | ||
Comment 21•11 years ago
|
||
Comment on attachment 826989 [details] [diff] [review]
Change set of enabled cipher suites
[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 926116
User impact if declined: AES-GCM doesn't work, which is the main motivation for using/testing TLS 1.2.
Testing completed (on m-c, etc.): This has been on mozilla-central since revision 23e213d57704 was checked in on 2013-11-11 in comment 14. Additionally, Google Chrome shipped a release with the AES-GCM code.
Risk to taking this patch (and alternatives if risky): This is very low risk. We disabled a few never-used cipher suites with this patch. There is some small risk that doing so might have some compatibility impact, but we won't fully know for sure until we get user feedback from testing. Note that the telemetry in bug 707275 will help with this, especially if bug 707275 gets uplifted to mozilla-beta.
String or IDL/UUID changes made by this patch: none.
Attachment #826989 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 22•11 years ago
|
||
(In reply to Kurt Roeckx from comment #20)
> This might be the(In reply to Kurt Roeckx from comment #19)
> > I'm telling you that in aurora this morning the list didn't change yet, but
> > this afternoon (CET) it changed to the list I showed.
>
> That might be the result of an update to NSS and not firefox itself.
The order is controlled by NSS and was changed in NSS bug 936828. The updated NSS (NSS 3.15.4 beta 3) landed in mozilla-central yesterday when I landed the latest patch in bug 898431.
I just asked for this patch to be uplifted to mozilla-aurora above, so that AES-GCM would be enabled in Aurora if the uplift request is approved.
Assignee | ||
Comment 23•11 years ago
|
||
(In reply to Brian Smith (:briansmith, was :bsmith; Please NEEDINFO? me if you want a response) from comment #22)
> The order is controlled by NSS and was changed in NSS bug 936828. The
> updated NSS (NSS 3.15.4 beta 3) landed in mozilla-central yesterday when I
> landed the latest patch in bug 898431.
s/mozilla-central/mozilla-aurora/. It landed in mozilla-central on 2013-11-11, as I mentioned in comment 21.
Comment 25•11 years ago
|
||
Maybe not the right place here, but there's something weird to me with this site:
https://sparen.nibcdirect.nl/
Nightly negotiates: SSL_RSA_WITH_RC4_128_SHA
However, this site should be able to negotiate TLS 1.2 according to ssllabs; these ciphers are available:
TLS_RSA_WITH_RC4_128_SHA
TLS_RSA_WITH_AES_256_CBC_SHA
TLS_RSA_WITH_AES_128_CBC_SHA
For instance, Chrome seems to do TLS 1.2 according to the SSLlabs simulation report.
Assignee | ||
Comment 26•11 years ago
|
||
(In reply to Coyote from comment #25)
> Maybe not the right place here, but there's something weird to me with this
> site:
> https://sparen.nibcdirect.nl/
>
> Nightly negotiates: SSL_RSA_WITH_RC4_128_SHA
Thanks for the feedback. This is also what Chrome Release and Chrome Canary negotiate. Note that Firefox doesn't show the TLS version in the UI like Chrome does, but that doesn't mean that Firefox isn't using TLS 1.2.
Comment 27•11 years ago
|
||
Ah ok, I thought that Firefox did show if SSL was negotiated or TLS 1.x and that the part not shown what TLS 1.x version.
So this feature should be on a wishlist ;-)
Thanks for the explanation.
Updated•11 years ago
|
status-firefox26:
--- → unaffected
status-firefox27:
--- → affected
tracking-firefox27:
--- → +
Keywords: qawanted
QA Contact: mwobensmith
Comment 28•11 years ago
|
||
So as far as I know this isn't in aurora yet. Will this get uplifted?
Updated•11 years ago
|
Attachment #826989 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 29•11 years ago
|
||
(In reply to Kurt Roeckx from comment #28)
> So as far as I know this isn't in aurora yet. Will this get uplifted?
yep, I meant to approve it last time when setting the flags/ QA contact, it slipped. I've a+ed it, should land today/tomorrow.
Assignee | ||
Comment 30•11 years ago
|
||
I will land this today along with the NSS bugfix that it requires.
Assignee | ||
Comment 31•11 years ago
|
||
Updated•11 years ago
|
Comment 32•11 years ago
|
||
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (X11; Linux i686; rv:27.0) Gecko/20100101 Firefox/27.0
Mozilla/5.0 (X11; Linux i686; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:27.0) Gecko/20100101 Firefox/27.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:28.0) Gecko/20100101 Firefox/28.0
Reproduced with Firefox 26 beta 1 (Build ID: 20131028225529): TLS_ECDHE_RSA_WITH_RC4_128_SHA cipher suite is used when making a connection to Google.
Verified as fixed on Firefox 27 beta 4 (build ID: 20140106141415) and latest Aurora 28.0a2 (Build ID: 20140107004003) with STR from comment 1:
1. The following ciphers are enabled by default (including the differences from comment 0):
ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
ECDHE_RSA_WITH_AES_128_GCM_SHA256
ECDHE_ECDSA_WITH_AES_128_CBC_SHA
ECDHE_RSA_WITH_AES_128_CBC_SHA
ECDHE_ECDSA_WITH_AES_256_CBC_SHA
ECDHE_RSA_WITH_AES_256_CBC_SHA
ECDHE_RSA_WITH_3DES_EDE_CBC_SHA
ECDHE_ECDSA_WITH_RC4_128_SHA
ECDHE_RSA_WITH_RC4_128_SHA
DHE_RSA_WITH_AES_128_CBC_SHA
DHE_DSS_WITH_AES_128_CBC_SHA
DHE_RSA_WITH_CAMELLIA_128_CBC_SHA
DHE_RSA_WITH_AES_256_CBC_SHA
DHE_DSS_WITH_AES_256_CBC_SHA
DHE_RSA_WITH_CAMELLIA_256_CBC_SHA
DHE_RSA_WITH_3DES_EDE_CBC_SHA
RSA_WITH_AES_128_CBC_SHA
RSA_WITH_CAMELLIA_128_CBC_SHA
RSA_WITH_AES_256_CBC_SHA
RSA_WITH_CAMELLIA_256_CBC_SHA
RSA_WITH_3DES_EDE_CBC_SHA
RSA_WITH_RC4_128_SHA
RSA_WITH_RC4_128_MD5
2. TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 cipher suite is used when making a connection to Google.
3. When navigating to https://www.mozilla.org with Wireshark active, the Client Hello contains all the above cipher suites.
Comment 33•11 years ago
|
||
Why no TLS_DHE_RSA_WITH_AES_128_GCM_SHA256 and TLS_RSA_WITH_AES_128_GCM_SHA256?
Wikipedia for example only has support for the RSA version:
https://www.ssllabs.com/ssltest/analyze.html?d=wikipedia.org
Comment 34•11 years ago
|
||
Hi,
I see only:
TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 (0xc02b)
TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (0xc02f)
Where are others? For example:
TLS_RSA_WITH_AES_256_CBC_SHA256 (0x3d)
Many web-sites have only TLS_RSA_WITH_AES_256_CBC_SHA256 as kind of strong(even without PFS) and weak RC4 and 3DES. If I have not TLS_RSA_WITH_AES_256_CBC_SHA256 server will choose RC4 or 3DES!
Why Mozilla doesn't add others "SHA256" cipher suits? What is the problem to add them, is it so hard? :)
Assignee | ||
Comment 35•11 years ago
|
||
(In reply to BoerenkoolMetWorst from comment #33)
> Why no TLS_DHE_RSA_WITH_AES_128_GCM_SHA256 and
> TLS_RSA_WITH_AES_128_GCM_SHA256?
> Wikipedia for example only has support for the RSA version:
> https://www.ssllabs.com/ssltest/analyze.html?d=wikipedia.org
> Why Mozilla doesn't add others "SHA256" cipher suits? What is the problem to
> add them, is it so hard? :)
I will replyto the thread that Rasj started on the dev-tech-crypto mailing list:
https://groups.google.com/d/msg/mozilla.dev.tech.crypto/xNcq_2g30BU/kOyr-wkfZb0J
You can subscribe to dev-tech-crypto here: https://lists.mozilla.org/listinfo/dev-tech-crypto
Please don't continue the discussion of the cipher suite decisions in this bug.
Assignee | ||
Updated•10 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•