Prefer GCM ciphers over CBC #1660

mikemaccana · 2015-05-08T17:19:56Z

AES-GCM or CHACHA20_POLY1305 ciphers must be used in current version of Chrome to avoid an 'obsolete cryptography' warning. See http://www.chromium.org/Home/chromium-security/education/tls#TOC-Deprecation-of-TLS-Features-Algorithms-in-Chrome

This list comes from the Mozilla Server Side TLS project https://github.com/mozilla/server-side-tls

silverwind · 2015-05-08T17:41:29Z

@mikemaccana I'd be interested in the output of ~~StartSSL's~~ SSLLabs handshake simulation of this.

cc: @indutny @jorangreef

mikemaccana · 2015-05-08T17:45:42Z

@silverwind I couldn't find startssl's handshake simulator - do you mean SSL Labs? If so, here they are:

https://www.ssllabs.com/ssltest/analyze.html?d=ssltest.certsimple.com&hideResults=on

Note in particular:

Chrome 42 / OS X R TLS 1.2 TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (0xc02f) FS

silverwind · 2015-05-08T17:47:25Z

@mikemaccana Err, yes of course, SSLLabs :)

silverwind · 2015-05-08T17:54:11Z

The suite looks to break several clients like Android 2.3, IE8 and OpenSSL 0.9.8, which is too much breakage for us. Maybe a few more compatibilty ciphers could be included to fix those?

Here are results from the current defaults for comparison: https://gist.github.com/silverwind/577571de9e2d375b7155

indutny · 2015-05-08T18:04:10Z

@silverwind oh right! https://gist.github.com/indutny/344b7ebd245068ffd1d6 this is what I use :) Looks like I have overlooked this thing.

mikemaccana · 2015-05-08T18:05:56Z

@silverwind Yep, that's the 'high' setting. Moz have an intermediate with greater compatibility, I'll deploy & show you results.

silverwind · 2015-05-08T18:08:46Z

@mikemaccana best to save the results to gist or similar, like I did. These ssllabs links aren't static after all.

mikemaccana · 2015-05-08T18:11:38Z

Results with Moz intermedia settings (still prefers GCM, so fixes obsolete cryptography warning): https://gist.github.com/mikemaccana/85e17ded64b65eae3b55

silverwind · 2015-05-08T18:16:54Z

@mikemaccana the intermediate still fails on the same clients. I think we need a custom suite that both includes GCM, while maintaining backward compat. Here's what the problematic clients get through the current configuration (from HIGH):

Android 2.3.7                 TLS 1.0     TLS_DHE_RSA_WITH_AES_128_CBC_SHA (0x33)
IE 8 / XP                     TLS 1.0     TLS_RSA_WITH_3DES_EDE_CBC_SHA (0xa)
Java 6u45                     TLS 1.0     TLS_DHE_RSA_WITH_AES_128_CBC_SHA (0x33)
OpenSSL 0.9.8y                TLS 1.0     TLS_DHE_RSA_WITH_AES_256_CBC_SHA (0x39)

I'd suggest starting at the modern mozilla configuration and add ciphers until all except IE 6 can connect.

mikemaccana · 2015-05-08T18:20:03Z

Just rebased to make the compare easier.

mikemaccana · 2015-05-08T18:31:30Z

@silverwind Odd, Moz thinks intermediate should work on IE7 upwards: https://wiki.mozilla.org/Security/Server_Side_TLS#Intermediate_compatibility_.28default.29

But I've re-rested (just in case it was user error) and same is occurring: https://gist.github.com/mikemaccana/5e12d4d2b8dcfc87b197

IE7 Vista fine, IE8 XP fails. Wonder what XP they use? Pre SP3 there's no SHA256, so any current cert (that doesn't trigger huge warnings in Chrome) won't work - it could be that. I'll investigate further and report back.

mikemaccana · 2015-05-08T18:48:48Z

OK simply adding GCM to top of current ciphers, no errors in Chrome, and everything except IE6:

https://gist.github.com/mikemaccana/67b94d06bbdf01c94fa4

silverwind · 2015-05-08T18:50:19Z

@mikemaccana these results look good. Lets update the PR and give other collaborators some time for feedback.

mikemaccana · 2015-05-08T18:57:12Z

@silverwind Done. Also improved the docs.

silverwind · 2015-05-08T19:04:01Z

doc/api/tls.markdown

+    offering *some* backward compatibiltity.
+
+    Old clients that rely on insecure and deprecated RC4 or DES-based ciphers
+    (like Internet Explorer 6) aren't able to complete the handshake with the default


Wrap those long lines at 80 chars, even if it means moving that link to a new line :)

mikemaccana · 2015-05-08T19:10:35Z

@silverwind done.

silverwind · 2015-05-08T19:19:33Z

doc/api/tls.markdown

-        HIGH:!aNULL:!eNULL:!EXPORT:!DES:!RC4:!MD5:!PSK:!SRP:!CAMELLIA
+        DHE-RSA-AES256-SHA256:ECDHE-RSA-AES128-SHA256:
+        DHE-RSA-AES128-SHA256:HIGH:!aNULL:!eNULL:!EXPORT:!DES:!RC4:!MD5:!PSK:
+        !SRP:!CAMELLIA'


Maybe put DHE-RSA-AES128-SHA256 on the line above. Docs don't stricly follow 80 chars, and it would more pleasant visually.

mikemaccana · 2015-05-08T19:24:45Z

Done.

Yep, split links work, I was worried about the same thing but I tested it and it worked fine after rendering.

silverwind · 2015-05-08T19:26:56Z

doc/api/tls.markdown

        ECDHE-RSA-AES256-SHA384:DHE-RSA-AES256-SHA384:ECDHE-RSA-AES256-SHA256:
        DHE-RSA-AES256-SHA256:ECDHE-RSA-AES128-SHA256:DHE-RSA-AES128-SHA256:
-        HIGH:!aNULL:!eNULL:!EXPORT:!DES:!RC4:!MD5:!PSK:!SRP:!CAMELLIA
+        HIGH:!aNULL:!eNULL:!EXPORT:!DES:!RC4:!MD5:!PSK:!SRP:!CAMELLIA'


Another nitpick, but I'd not quote these :)

mikemaccana · 2015-05-08T19:31:28Z

It's a string. Shouldn't it look like a string?

silverwind · 2015-05-08T19:34:32Z

Strictly speaking, it's not a true string (not counting template string) because of the newlines, so I'd not add the quotes.

indutny · 2015-05-08T19:36:44Z

lib/tls.js

+  'ECDHE-ECDSA-AES256-GCM-SHA384',
+  'DHE-RSA-AES128-GCM-SHA256',
+  'DHE-DSS-AES128-GCM-SHA256',
+  'kEDH+AESGCM',


Mmm... is this one necessary? I'm really afraid of this things... And in fact it only does:

DHE-RSA-AES256-GCM-SHA384 DHE-RSA-AES128-GCM-SHA256

May I ask you to replace it with these ones?

~~Hrm, I thought Chrome only did GCM with 128 bit ciphers (still considered more secure than 256 bit AES with CBC). Let me find a reference...~~

OK, that's not what I was thinking of: a 128 bit GCM is preferred over 256 bit with CBC, but that's all: https://wiki.mozilla.org/Security/Server_Side_TLS#Prioritization_logic

I'll try 256 bit with GCM and re-test...

Re: 'is this one necessary?'

Do you mean the whole six lines with the GCM ciphers? Yes, that's necessary. Most things which don't explicitly have GCM in the name are CBC. You can prove this and convert the custom names OpenSSL uses to the ones Chrome and SSL Labs use (which mention CBC explicitly) here: https://www.openssl.org/docs/apps/ciphers.html

When you say 'And in fact it only does:'

DHE-RSA-AES256-GCM-SHA384
DHE-RSA-AES128-GCM-SHA256

That's not true - see https://gist.github.com/mikemaccana/67b94d06bbdf01c94fa4, most of the time the ephemeral (ECDHE) version is used.

PS. Sorry if I've misunderstood something.

Isn't @indutny talking about kEDH+AESGCM?

I'm not sure. @indutny?

kEDH+AESGCM doesn't even seem implemented on default build of OpenSSL 1.0.1e-fips @indutny if you meant we should get rid of kEDH+AESGCM I'd agree.

Yes, I was talking about kEDH+AESGCM and it is implemented in ciphers that I initially listed ;)

mikemaccana · 2015-05-08T19:38:30Z

Ack @silverwind, I normally avoid fixed-width and just use a string, but if we're wrapping at 80 then I guess it's not. Amended.

silverwind · 2015-05-08T19:42:55Z

Alright, docs are good from my point of view. Leaving the added ciphers up for discussion.

Also cc: @shigeki

indutny · 2015-05-08T19:44:30Z

Left some comments.

mikemaccana · 2015-05-08T20:11:32Z

Would you like me to rebase so it looks like a single commit? I'm happy either way.

indutny · 2015-05-08T20:12:34Z

Please keep it as it is for now :)

indutny · 2015-05-08T20:13:05Z

LGTM!

indutny · 2015-05-08T20:13:13Z

Thank you :)

mikemaccana · 2015-05-08T20:17:40Z

Deployed and re-ran test with new ciphers:
https://gist.github.com/mikemaccana/38ecc4a4f9374144106d

Same as before obviously (the ciphers we removed weren't in use), but always good to check.

silverwind · 2015-05-08T20:17:50Z

lib/tls.js

+  'ECDHE-ECDSA-AES128-GCM-SHA256',
+  'ECDHE-RSA-AES256-GCM-SHA384',
+  'ECDHE-ECDSA-AES256-GCM-SHA384',
+  'DHE-RSA-AES128-GCM-SHA256',


The ordering doesn't look right to me. It should be strongest to weakest. I think something like this could do:

'ECDHE-ECDSA-AES256-GCM-SHA384', 'ECDHE-RSA-AES256-GCM-SHA384', 'ECDHE-ECDSA-AES128-GCM-SHA256', 'ECDHE-RSA-AES128-GCM-SHA256', 'DHE-RSA-AES128-GCM-SHA256',

Yeah that's another bit of Mozilla logic. I'll quote them, tell me your thoughts...

AES 128 is preferred to AES 256. There has been [discussions] on whether AES256 extra security was worth the cost, and the result is far from obvious. At the moment, AES128 is preferred, because it provides good security, is really fast, and seems to be more resistant to timing attacks.

IIRC there are actually some attacks that affect 256 bit AES but not 128 bit. Let me find a reference.

Hmm, I'd probably trust Mozilla engineers to make the right choice here, as I never perfed ciphers.

Discussion here: http://crypto.stackexchange.com/questions/5118/is-aes-256-weaker-than-192-and-128-bit-versions

For what it's worth Schneier: 'And for new applications I suggest that people don't use AES-256. AES-128 provides more than enough security margin for the forseeable future. But if you're already using AES-256, there's no reason to change. '

https://www.schneier.com/blog/archives/2009/07/another_new_aes.html

My own thoughts: either way we should be consistent. The subsequent ciphers are bit length first. If we do re-order to avoid the AES256 related key attacks we should do that consistently, and re-order the old ones as well as the new ones. Maybe that's for another PR?

mikemaccana · 2015-05-08T20:17:51Z

No probs!

silverwind · 2015-05-08T20:29:55Z

Alright, I'm convinced it's better to leave the ordering as-is.

LGTM

mikemaccana · 2015-05-08T20:37:50Z

OK, but we still need to be consistent: if we're avoiding the attacks against apply to 192 and 256 bit AES, we should do it everywhere, including in existing ciphers. I've updated the branch to reflect that and also add the rationale to the docs.

silverwind · 2015-05-08T20:42:06Z

doc/api/tls.markdown

+        !MD5:
+        !PSK:
+        !SRP:
+        !CAMELLIA:


Hmm, not sure how I feel about this. It's nice to have these laid out, but takes up a lot of vertical space. Most people don't care that much about ciphers, I think. Care to restore the old format?

Also: Trailing colon.

I see what you're saying, but since it's in order, being able to parse top-to-bottom really helps people interested in node/io's default ciphers. If they don't care they can just scroll past. What do you think?

Will fix trailing colon.

It helps interested people to understand, so that's a plus. The extra vertical space is a bit disturbing, but that's just bikeshedding now on my side.

silverwind · 2015-05-08T21:24:57Z

So, overall still LGTM. @indutny you fine with the latest AES128 reordering?

silverwind · 2015-05-08T21:26:12Z

@mikemaccana please show one more result table with the current list.

indutny · 2015-05-08T21:32:27Z

LGTM

shigeki · 2015-05-09T09:39:53Z

LGTM and let AES128 be first for several years from now. I always refer https://tools.ietf.org/html/rfc7525 for TLS parameters.

silverwind · 2015-05-09T11:09:20Z

Tested the ciphers myself, looking good.

@mikemaccana please squash the commits, add a description and make sure the commit message follows the guidline here.

mikemaccana · 2015-05-09T11:59:55Z

@silverwind ack will squash shortly. Will also paste handshake results - was past midnight here when I made last post.

AES-GCM or CHACHA20_POLY1305 ciphers must be used in current version of Chrome to avoid an 'obsolete cryptography' warning. Prefer 128 bit AES over 192 and 256 bit AES considering attacks that specifically affect the larger key sizes but do not affect AES 128.

mikemaccana · 2015-05-11T11:14:12Z

Rebase and modified commit message per guidelines - hope it's OK.
Updated handshake results are here: https://gist.github.com/mikemaccana/24b41f4ff1d4ee89eefd

AES-GCM or CHACHA20_POLY1305 ciphers must be used in current version of Chrome to avoid an 'obsolete cryptography' warning. Prefer 128 bit AES over 192 and 256 bit AES considering attacks that specifically affect the larger key sizes but do not affect AES 128. PR-URL: #1660 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

silverwind · 2015-05-11T11:44:07Z

Thanks Mike. Landed in 5755fc0. Your commit title was exceeding 50 chars, so I reworded it.

mikemaccana · 2015-05-11T11:54:31Z

😀🙏👍

PR-URL: nodejs#1679 Notable Changes: * win,node-gyp: the delay-load hook for windows addons has now been correctly enabled by default, it had wrongly defaulted to off in the release version of 2.0.0 (Bert Belder) nodejs#1433 * os: tmpdir()'s trailing slash stripping has been refined to fix an issue when the temp directory is at '/'. Also considers which slash is used by the operating system. (cjihrig) nodejs#1673 * tls: default ciphers have been updated to use gcm and aes128 (Mike MacCana) nodejs#1660 * build: v8 snapshots have been re-enabled by default as suggested by the v8 team, since prior security issues have been resolved. This should give some perf improvements to both startup and vm context creation. (Trevor Norris) nodejs#1663 * src: fixed preload modules not working when other flags were used before --require (Yosuke Furukawa) nodejs#1694 * dgram: fixed send()'s callback not being asynchronous (Yosuke Furukawa) nodejs#1313 * readline: emitKeys now keeps buffering data until it has enough to parse. This fixes an issue with parsing split escapes. (Alex Kocharin) * cluster: works now properly emit 'disconnect' to cluser.worker (Oleg Elifantiev) nodejs#1386 events: uncaught errors now provide some context (Evan Lucas) nodejs#1654

AES-GCM or CHACHA20_POLY1305 ciphers must be used in current version of Chrome to avoid an 'obsolete cryptography' warning. Prefer 128 bit AES over 192 and 256 bit AES considering attacks that specifically affect the larger key sizes but do not affect AES 128. PR-URL: nodejs#1660 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

Fishrock123 added the tls Issues and PRs related to the tls subsystem. label May 8, 2015

silverwind reviewed May 8, 2015
View reviewed changes

indutny reviewed May 8, 2015
View reviewed changes

silverwind reviewed May 8, 2015
View reviewed changes

silverwind closed this May 11, 2015

Fishrock123 mentioned this pull request May 12, 2015

Release proposal: v2.0.2 #1679

Merged

Prefer GCM ciphers over CBC #1660

Prefer GCM ciphers over CBC #1660

Conversation

mikemaccana commented May 8, 2015

silverwind commented May 8, 2015

mikemaccana commented May 8, 2015

silverwind commented May 8, 2015

silverwind commented May 8, 2015

indutny commented May 8, 2015

mikemaccana commented May 8, 2015

silverwind commented May 8, 2015

mikemaccana commented May 8, 2015

silverwind commented May 8, 2015

mikemaccana commented May 8, 2015

mikemaccana commented May 8, 2015

mikemaccana commented May 8, 2015

silverwind commented May 8, 2015

mikemaccana commented May 8, 2015

Choose a reason for hiding this comment

mikemaccana commented May 8, 2015

Choose a reason for hiding this comment

mikemaccana commented May 8, 2015

Choose a reason for hiding this comment

mikemaccana commented May 8, 2015

silverwind commented May 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikemaccana commented May 8, 2015

silverwind commented May 8, 2015

indutny commented May 8, 2015

mikemaccana commented May 8, 2015

indutny commented May 8, 2015

indutny commented May 8, 2015

indutny commented May 8, 2015

mikemaccana commented May 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikemaccana commented May 8, 2015

silverwind commented May 8, 2015

mikemaccana commented May 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silverwind commented May 8, 2015

silverwind commented May 8, 2015

indutny commented May 8, 2015

shigeki commented May 9, 2015

silverwind commented May 9, 2015

mikemaccana commented May 9, 2015

mikemaccana commented May 11, 2015

silverwind commented May 11, 2015

mikemaccana commented May 11, 2015