Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tls: more secure defaults #826

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
7 participants
@silverwind
Copy link
Contributor

commented Feb 12, 2015

This changes the TLS defaults to be more secure. It includes these changes:

  1. Remove the AES128-GCM-SHA256 cipherset. It seems Chrome likes to choose it over an more secure ECDHE variant with honorCipherOrder enabled.
  2. Disable the insecure RC4 algorithm completely. This doesn't seem to cause any new incompatibilties with clients, as IE6 can't handshake with the current defaults right now.
  3. Enable honorCipherOrder by default. This probably should've been the default for a while now.

Combined with DH parameters > 1024 bit and HSTS, this gives an A+ rating on the OpenSSL test (was an B rating before). All clients except IE on Windows XP achieve Forward Secrecy.

Motivating issue: #818
CC: @indutny

Ciphers before this change:

Android 2.3.7                TLS 1.0    TLS_RSA_WITH_RC4_128_SHA (0x5)
Android 4.0.4                TLS 1.0    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (0xc014)   
Android 4.1.1                TLS 1.0    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (0xc014)   
Android 4.2.2                TLS 1.0    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (0xc014)   
Android 4.3                  TLS 1.0    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (0xc014)   
Android 4.4.2                TLS 1.2    TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (0xc030)
BingBot Dec 2013             TLS 1.0    TLS_RSA_WITH_AES_128_CBC_SHA (0x2f) 
BingPreview Jun 2014         TLS 1.0    TLS_DHE_RSA_WITH_AES_256_CBC_SHA (0x39)
Chrome 39 / OS X             TLS 1.2    TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (0xc02f)
Firefox 31.3.0 ESR / Win 7   TLS 1.2    TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (0xc02f)
Firefox 34 / OS X            TLS 1.2    TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (0xc02f)
Googlebot Jun 2014           TLS 1.0    TLS_ECDHE_RSA_WITH_RC4_128_SHA (0xc011)
IE 6 / XP                    Protocol or cipher suite mismatch
IE 7 / Vista                 TLS 1.0    TLS_RSA_WITH_AES_128_CBC_SHA (0x2f)
IE 8 / XP                    TLS 1.0    TLS_RSA_WITH_RC4_128_SHA (0x5)
IE 8-10 / Win 7              TLS 1.0    TLS_RSA_WITH_AES_128_CBC_SHA (0x2f)
IE 11 / Win 7                TLS 1.2    TLS_RSA_WITH_AES_128_CBC_SHA256 (0x3c)
IE 11 / Win 10 Preview       TLS 1.2    TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (0xc030)
IE 11 / Win 8.1              TLS 1.2    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384 (0xc028)
IE Mobile 10 / Win Phone 8.0 TLS 1.0    TLS_RSA_WITH_AES_128_CBC_SHA (0x2f) 
IE Mobile 11 / Win Phone 8.1 TLS 1.2    TLS_RSA_WITH_AES_128_CBC_SHA256 (0x3c)
Java 6u45                    TLS 1.0    TLS_RSA_WITH_RC4_128_SHA (0x5)
Java 7u25                    TLS 1.0    TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (0xc013)   
Java 8b132                   TLS 1.2    TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)
OpenSSL 0.9.8y               TLS 1.0    TLS_DHE_RSA_WITH_AES_256_CBC_SHA (0x39)
OpenSSL 1.0.1h               TLS 1.2    TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (0xc030)
Safari 5.1.9 / OS X 10.6.8   TLS 1.0    TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (0xc013)   
Safari 6 / iOS 6.0.1         TLS 1.2    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384 (0xc028)
Safari 7 / iOS 7.1           TLS 1.2    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384 (0xc028)
Safari 8 / iOS 8.0 Beta      TLS 1.2    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384 (0xc028)
Safari 6.0.4 / OS X 10.8.4   TLS 1.0    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (0xc014)   
Safari 7 / OS X 10.9         TLS 1.2    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384 (0xc028)
Yahoo Slurp Jun 2014         TLS 1.2    TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (0xc030)
YandexBot Sep 2014           TLS 1.2    TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (0xc030)

Ciphers after this change:

Android 2.3.7                TLS 1.0    TLS_DHE_RSA_WITH_AES_128_CBC_SHA (0x33)  
Android 4.0.4                TLS 1.0    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (0xc014)  
Android 4.1.1                TLS 1.0    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (0xc014)  
Android 4.2.2                TLS 1.0    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (0xc014)  
Android 4.3                  TLS 1.0    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (0xc014)  
Android 4.4.2                TLS 1.2    TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)  
BingBot Dec 2013             TLS 1.0    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (0xc014)  
BingPreview Jun 2014         TLS 1.0    TLS_DHE_RSA_WITH_AES_256_CBC_SHA (0x39)  
Chrome 39 / OS X             TLS 1.2    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (0xc014)  
Firefox 31.3.0 ESR / Win 7   TLS 1.2    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (0xc014)  
Firefox 34 / OS X            TLS 1.2    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (0xc014)  
Googlebot Jun 2014           TLS 1.0    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (0xc014)  
IE 6 / XP                    Protocol or cipher suite mismatch
IE 7 / Vista                 TLS 1.0    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (0xc014)  
IE 8 / XP                    TLS 1.0    TLS_RSA_WITH_3DES_EDE_CBC_SHA (0xa)
IE 8-10 / Win 7              TLS 1.0    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (0xc014)  
IE 11 / Win 7                TLS 1.2    TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)  
IE 11 / Win 10 Preview       TLS 1.2    TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)  
IE 11 / Win 8.1              TLS 1.2    TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)  
IE Mobile 10 / Win Phone 8.0 TLS 1.0    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (0xc014)  
IE Mobile 11 / Win Phone 8.1 TLS 1.2    TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)  
Java 6u45                    TLS 1.0    TLS_DHE_RSA_WITH_AES_128_CBC_SHA (0x33)  
Java 7u25                    TLS 1.0    TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (0xc013)  
Java 8b132                   TLS 1.2    TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)  
OpenSSL 0.9.8y               TLS 1.0    TLS_DHE_RSA_WITH_AES_256_CBC_SHA (0x39)  
OpenSSL 1.0.1h               TLS 1.2    TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)  
Safari 5.1.9 / OS X 10.6.8   TLS 1.0    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (0xc014)  
Safari 6 / iOS 6.0.1         TLS 1.2    TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)  
Safari 7 / iOS 7.1           TLS 1.2    TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)  
Safari 8 / iOS 8.0 Beta      TLS 1.2    TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)  
Safari 6.0.4 / OS X 10.8.4   TLS 1.0    TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (0xc014)  
Safari 7 / OS X 10.9         TLS 1.2    TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)  
Yahoo Slurp Jun 2014         TLS 1.2    TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)  
YandexBot Sep 2014           TLS 1.2    TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)  

Haven't touched the docs yet (and I probably need some assistance on these), as I'd like to get some feedback on this first.

@indutny

View changes

lib/tls.js Outdated
@@ -15,9 +15,9 @@ exports.SLAB_BUFFER_SIZE = 10 * 1024 * 1024;

exports.DEFAULT_CIPHERS =
// TLS 1.2
'ECDHE-RSA-AES128-SHA256:DHE-RSA-AES128-SHA256:AES128-GCM-SHA256:' +
'ECDHE-RSA-AES128-SHA256:DHE-RSA-AES128-SHA256:' +

This comment has been minimized.

Copy link
@indutny

This comment has been minimized.

Copy link
@indutny

indutny Feb 12, 2015

Member

Though, maybe without AES128-GCM-SHA256 if it breaks stuff. cc @bnoordhuis

This comment has been minimized.

Copy link
@silverwind

silverwind Feb 12, 2015

Author Contributor

Is that list one of those well-tested-and-battle-proven ones? I have a similar long one ready here, but I think it breaks handshakes on some clients: https://gist.github.com/silverwind/8331aae19f57878fabd1

This comment has been minimized.

Copy link
@silverwind

silverwind Feb 12, 2015

Author Contributor

Anyways, I'll run a few more test tomorrow if we can get even better results!

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Feb 12, 2015

Member

Though, maybe without AES128-GCM-SHA256 if it breaks stuff.

Maybe not outright break as in handshake failures but I imagine it's possible that without AES128-GCM-SHA256, the connection gets downgraded to a TLS 1.0 cipher when the other end doesn't speak ECDH(E) or DHE.

This comment has been minimized.

Copy link
@indutny

indutny Feb 12, 2015

Member

Let's use my list @bnoordhuis ;)

This comment has been minimized.

Copy link
@shigeki

shigeki Feb 13, 2015

Contributor

The bug of load balancers due to the size of CLIENT_HELLO can be resolved by using SSL_OP_TLSEXT_PADDING options. We are no longer worrying about the size of cipher list.
In order to get A+ score from SSL Labs Server Test, the cipher list needs https://gist.github.com/shigeki/986c53242f5bd3d78609#file-server-js-L19

This comment has been minimized.

Copy link
@silverwind

silverwind Feb 13, 2015

Author Contributor

@shigeki that list you linked seems pretty old and includes EECDH chiphers. I'm not familiar with those, and @indutny's list doesn't include them either. Is there any benefit to them over ECDHE?

This comment has been minimized.

Copy link
@shigeki

shigeki Feb 13, 2015

Contributor

@silentrob Oh, it's a synonym. https://github.com/openssl/openssl/blob/master/ssl/ssl_locl.h#L307-L310 . ECDHE is popular and better now.

This comment has been minimized.

Copy link
@silverwind

silverwind Feb 13, 2015

Author Contributor

@shigeki The discussion got split a bit here. see #818 (comment) for my latest test with Fedor's list.

@bnoordhuis

View changes

lib/tls.js Outdated
// TLS 1.0
'RC4:HIGH:!MD5:!aNULL';
':HIGH:!RC4:!MD5:!aNULL';

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Feb 12, 2015

Member

!eNULL would be good to have as well. That should already effectively be the case but it's better to be explicit about it.

This comment has been minimized.

Copy link
@silverwind

silverwind Feb 12, 2015

Author Contributor

I have !EXPORT on my list of exclusions here too, good fit?

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Feb 12, 2015

Member

It doesn't hurt but it's implied by HIGH.

This comment has been minimized.

Copy link
@indutny

indutny Feb 12, 2015

Member

@bnoordhuis could somebody please tell me what's the point of using such generic string? There are like 30-40 ciphers, and it is much easier to select from them, then to write such query.

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Feb 12, 2015

Member

Easy, it's so you don't have to update the list when ciphers are added or dropped.

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Feb 12, 2015

Member

You're forgetting about distro packagers that link against shared openssl versions. :-)

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Feb 12, 2015

Member

But to be clear, I don't have a strong opinion on the subject. I'm just pointing out possible issues.

This comment has been minimized.

Copy link
@indutny

indutny Feb 12, 2015

Member

My cipher list includes ciphers that was supported since 0.9.8 , but do we support shared 0.9.8? :)

This comment has been minimized.

Copy link
@silverwind

silverwind Feb 12, 2015

Author Contributor

btw: I'm fine with either way, but if we go explicit @indutny should update the list when needed ;)

This comment has been minimized.

Copy link
@indutny

indutny Feb 12, 2015

Member

I'll do it.

@silverwind

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2015

Here's the list from the previous issue. Default config, but honorCipherOrder enabled. Notice Chrome choosing TLS_RSA_WITH_AES_128_GCM_SHA256:

Android 2.3.7                TLS 1.0  TLS_RSA_WITH_RC4_128_SHA (0x5)
Android 4.0.4                TLS 1.0  TLS_ECDHE_RSA_WITH_RC4_128_SHA (0xc011)
Android 4.1.1                TLS 1.0  TLS_ECDHE_RSA_WITH_RC4_128_SHA (0xc011)
Android 4.2.2                TLS 1.0  TLS_ECDHE_RSA_WITH_RC4_128_SHA (0xc011)
Android 4.3                  TLS 1.0  TLS_ECDHE_RSA_WITH_RC4_128_SHA (0xc011)
Android 4.4.2                TLS 1.2  TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)
BingBot Dec 2013             TLS 1.0  TLS_RSA_WITH_RC4_128_SHA (0x5)
BingPreview Jun 2014         TLS 1.0  TLS_RSA_WITH_RC4_128_SHA (0x5)
Chrome 39 / OS X             TLS 1.2  TLS_RSA_WITH_AES_128_GCM_SHA256 (0x9c)
Firefox 31.3.0 ESR / Win 7   TLS 1.2  TLS_ECDHE_RSA_WITH_RC4_128_SHA (0xc011)
Firefox 34 / OS X            TLS 1.2  TLS_ECDHE_RSA_WITH_RC4_128_SHA (0xc011)
Googlebot Jun 2014           TLS 1.0  TLS_ECDHE_RSA_WITH_RC4_128_SHA (0xc011)
IE 6 / XP                    Protocol or cipher suite mismatch
IE 7 / Vista                 TLS 1.0  TLS_RSA_WITH_RC4_128_SHA (0x5)
IE 8 / XP                    TLS 1.0  TLS_RSA_WITH_RC4_128_SHA (0x5)
IE 8-10 / Win 7              TLS 1.0  TLS_RSA_WITH_RC4_128_SHA (0x5)
IE 11 / Win 7                TLS 1.2  TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)
IE 11 / Win 10 Preview       TLS 1.2  TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)
IE 11 / Win 8.1              TLS 1.2  TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)
IE Mobile 10 / Win Phone 8.0 TLS 1.0  TLS_RSA_WITH_RC4_128_SHA (0x5)
IE Mobile 11 / Win Phone 8.1 TLS 1.2  TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)
Java 6u45                    TLS 1.0  TLS_RSA_WITH_RC4_128_SHA (0x5)
Java 7u25                    TLS 1.0  TLS_ECDHE_RSA_WITH_RC4_128_SHA (0xc011)
Java 8b132                   TLS 1.2  TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)
OpenSSL 0.9.8y               TLS 1.0  TLS_RSA_WITH_RC4_128_SHA (0x5)
OpenSSL 1.0.1h               TLS 1.2  TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)
Safari 5.1.9 / OS X 10.6.8   TLS 1.0  TLS_ECDHE_RSA_WITH_RC4_128_SHA (0xc011)
Safari 6 / iOS 6.0.1         TLS 1.2  TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)
Safari 7 / iOS 7.1           TLS 1.2  TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)
Safari 8 / iOS 8.0 Beta      TLS 1.2  TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)
Safari 6.0.4 / OS X 10.8.4   TLS 1.0  TLS_ECDHE_RSA_WITH_RC4_128_SHA (0xc011)
Safari 7 / OS X 10.9         TLS 1.2  TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)
Yahoo Slurp Jun 2014         TLS 1.2  TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)
YandexBot Sep 2014           TLS 1.2  TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)

@silverwind silverwind changed the title tls - more secure defaults tls: more secure defaults Feb 13, 2015

@silverwind silverwind force-pushed the silverwind:better-tls-defaults branch Feb 13, 2015

@silverwind

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2015

Updated the PR with @indutny's list. Here are the results with it: https://gist.github.com/silverwind/db82299eff01b1c6a200

If we agree on using this, I'll start on the docs update.

@silverwind

View changes

lib/tls.js Outdated
'AES128-GCM-SHA256' +
'AES128-SHA256' +
'AES128-SHA' +
'DES-CBC3-SHA';

This comment has been minimized.

Copy link
@silverwind

silverwind Feb 13, 2015

Author Contributor

Could probably use a template string here. Edit, oh, and it's missing :, will fix.

@silverwind silverwind force-pushed the silverwind:better-tls-defaults branch Feb 13, 2015

@silverwind

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2015

Some more cipher discussion here: #818

@silverwind

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2015

Regarding docs, I think it's worthwile to mention that IE6 can't handshake with the defaults, maybe we should provide an alternate ultra-compatible cipher set in the docs for these cases.

@silverwind

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2015

Docs done. Removed lot of cruft and a ugly TODO.

Please review, @indutny @bnoordhuis

@indutny

This comment has been minimized.

Copy link
Member

commented Feb 13, 2015

LGTM

@silverwind silverwind force-pushed the silverwind:better-tls-defaults branch Feb 13, 2015

@silverwind

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2015

squashed

@silverwind

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2015

Hold on a sec, I just tested again with fresh build, and I get an A- score now, with a few browsers getting no FS, I suspect honorCipherOrder might not apply.

@silverwind

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2015

Apparently its because IE 8 / XP gets no FS and we don't have any CBC ciphers enabled, which it would need for FS (but just with DSA key). It's pretty questionable why they include it in their score, when the browser is effectively uncapable of FS.

Anyways, I think the list is fine as is, and the score problem will eventually vanish by itself once IE8 loses reference browser status. I also verified that honorCipherOrder was intact.

@jorangreef

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2015

It's also "IE Mobile 11 / Win Phone 8.1" and "IE 11 / Win 7" which are not getting FS with those latest ciphers in the pull request.

Also latest versions of Safari are only getting FS on DHE whereas they should be getting FS on ECDHE:

Safari 8 / iOS 8.0 Beta R TLS 1.2 TLS_DHE_RSA_WITH_AES_256_CBC_SHA256
Safari 7 / OS X 10.9 R TLS 1.2 TLS_DHE_RSA_WITH_AES_256_CBC_SHA256 (0x6b)

The default ciphers should definitely be using ECDHE on all the reference browsers.

@silverwind

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2015

@jorangreef your results differ from mine once again. I get the same as in the gist linked earlier (https://gist.github.com/silverwind/db82299eff01b1c6a200), except the score being lowered to A-. You sure that you have the ciphers, honorCipherOrder, and dhparam?

@silverwind

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2015

@jorangreef hmm, you may actually be right, it seems to have to do with cipher ordering. A simple reordering might suffice I think. Something like this?

ECDHE-ECDSA-AES256-GCM-SHA384
ECDHE-RSA-AES256-GCM-SHA384
ECDHE-ECDSA-AES256-GCM-SHA256
ECDHE-RSA-AES256-GCM-SHA256
ECDHE-ECDSA-AES256-SHA256
ECDHE-RSA-AES256-SHA256
ECDHE-ECDSA-AES128-GCM-SHA256
ECDHE-RSA-AES128-GCM-SHA256
ECDHE-ECDSA-AES128-SHA256
ECDHE-RSA-AES128-SHA256
ECDHE-ECDSA-AES128-SHA
ECDHE-RSA-AES128-SHA
DHE-RSA-AES256-GCM-SHA384
DHE-RSA-AES256-GCM-SHA256
DHE-RSA-AES256-SHA256
DHE-RSA-AES128-GCM-SHA256
DHE-RSA-AES128-SHA256
DHE-RSA-AES128-SHA
AES256-GCM-SHA384
AES256-SHA256
AES128-GCM-SHA256
AES128-SHA256
AES128-SHA
DES-CBC3-SHA
@indutny

This comment has been minimized.

Copy link
Member

commented Feb 14, 2015

@silverwind there is no point in preferring ECDHE over DHE. I'd better be using AES256 with DHE, than AES128 with ECDHE.

@silverwind

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2015

@indutny what about these:

AES256-GCM-SHA384
AES256-SHA256

your list has them ranked pretty high, and it seems some browsers get them over a FS cipher.

@silverwind

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2015

@jorangreef

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2015

@silverwind, I used https://gist.github.com/jorangreef/1aa7ec6ccd82585090bc which has the latest ciphers in your pull request and honorCipherOrder enabled. It doesn't get FS in the reference browsers. Also it only gets DHE for Safari where it should definitely be getting ECDHE.

@indutny

This comment has been minimized.

Copy link
Member

commented Feb 14, 2015

@silverwind these are the best ciphers without FS, are they selected when the browser did support FS?

@indutny

This comment has been minimized.

Copy link
Member

commented Feb 14, 2015

@jorangreef are you sure you are not using node.js 0.10? It is the only version that didn't have ECDHE support.

@jorangreef

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2015

@indutny quite sure ;)

iojs --version gives v1.2.1

Also if I was using Node 0.10 I wouldn't be getting ECDHE for most of the reference browsers (only DHE for Safari).

@silverwind

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2015

are they selected when the browser did support FS?

Yes, it was IE11 and some Safari version that got them over a DHE/ECDHE as in @jorangreef's comment above.

@jorangreef

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2015

I think a better testing methodology would be:

  1. Start with Fedor's explicit ciphers list but with no DHE for starters (we add these back later once we get ECDHE on all the reference browsers).
  2. Keep adding minimum ciphers necessary for reference browsers which don't get FS at first (using the ssllabs support pages to see the preference order for each browser and what ciphers we are missing... probably an explicit CBC).
  3. When we get an A+ on ssllabs, add back the DHE ciphers in the right preference order and test again making sure we're still using ECDHE for the reference browsers.

All of this needs to have honorCipherOrder enabled obviously.

@indutny

This comment has been minimized.

Copy link
Member

commented Feb 14, 2015

@jorangreef I still don't quite get why I have FS with all reference browsers here: https://www.ssllabs.com/ssltest/analyze.html?d=blog.indutny.com

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Feb 15, 2015

LGTM with a question.

@silverwind silverwind force-pushed the silverwind:better-tls-defaults branch Feb 15, 2015

@silverwind

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2015

Updated, test change split out into #853

tls: more secure defaults
This updates the default cipher suite to an more secure list, which
prefers strong ciphers with Forward Secrecy. Additionally, it enables
`honorCipherOrder` by default.

Noteable effect of this change is that the insecure RC4 ciphers are
disabled and that Chrome negotiates a more secure ECDHE cipher.

@silverwind silverwind force-pushed the silverwind:better-tls-defaults branch to cbb369c Feb 15, 2015

@silverwind

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2015

Fixed two more spellings in the docs ('ciphersuite' -> 'cipher suite')

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Feb 15, 2015

Still LGTM. I'll leave it to @indutny to sign off on it and land it.

indutny added a commit that referenced this pull request Feb 16, 2015

tls: more secure defaults
This updates the default cipher suite to an more secure list, which
prefers strong ciphers with Forward Secrecy. Additionally, it enables
`honorCipherOrder` by default.

Noteable effect of this change is that the insecure RC4 ciphers are
disabled and that Chrome negotiates a more secure ECDHE cipher.

Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
PR-URL: #826
@indutny

This comment has been minimized.

Copy link
Member

commented Feb 16, 2015

@silverwind Landed in 77f3586, thank you! Please run make test next time, there was some minor failures and jslint errors.

@indutny indutny closed this Feb 16, 2015

@rvagg rvagg referenced this pull request Feb 18, 2015

Closed

Release proposal: v1.3.0 #872

rvagg added a commit that referenced this pull request Feb 20, 2015

2015-02-20 io.js v1.3.0 Release
Notable changes:

* url: `url.resolve('/path/to/file', '.')` now returns `/path/to/`
  with the trailing slash, `url.resolve('/', '.')` returns `/`.
  #278 (Amir Saboury)
* tls: tls (and in turn https) now rely on a stronger default
  cipher suite which excludes the RC4 cipher. If you still want to
  use RC4, you have to specify your own ciphers suite.
  #826 (Roman Reiss)

jasnell added a commit to jasnell/node-joyent that referenced this pull request Mar 31, 2015

tls: more secure defaults
Port of io.js commit: nodejs/node@77f3586

Original commit message:

This updates the default cipher suite to an more secure list, which
prefers strong ciphers with Forward Secrecy. Additionally, it enables
`honorCipherOrder` by default.

Noteable effect of this change is that the insecure RC4 ciphers are
disabled and that Chrome negotiates a more secure ECDHE cipher.

Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
PR-URL: nodejs/node#826
@Restuta

This comment has been minimized.

Copy link

commented May 8, 2015

I am writing some web-scrapers that have to support not-so cool and secure web-sites that still use legacy ciphers. What is the right way to override defaults and say "yes, use RC4!"?

@silverwind

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2015

@Restuta Set the ciphers option to a string like this. It's mentioned in the docs.

ciphers: "ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES256-GCM-SHA384:DHE-RSA-AES128-GCM-SHA256:DHE-DSS-AES128-GCM-SHA256:kEDH+AESGCM:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES256-SHA384:ECDHE-ECDSA-AES256-SHA384:ECDHE-RSA-AES256-SHA:ECDHE-ECDSA-AES256-SHA:DHE-RSA-AES128-SHA256:DHE-RSA-AES128-SHA:DHE-DSS-AES128-SHA256:DHE-RSA-AES256-SHA256:DHE-DSS-AES256-SHA:DHE-RSA-AES256-SHA:ECDHE-RSA-DES-CBC3-SHA:ECDHE-ECDSA-DES-CBC3-SHA:AES128-GCM-SHA256:AES256-GCM-SHA384:AES128-SHA256:AES256-SHA256:AES128-SHA:AES256-SHA:AES:DES-CBC3-SHA:HIGH:!aNULL:!eNULL:!EXPORT:!DES:!RC4:!MD5:!PSK:!aECDH:!EDH-DSS-DES-CBC3-SHA:!EDH-RSA-DES-CBC3-SHA:!KRB5-DES-CBC3-SHA"
@Restuta

This comment has been minimized.

Copy link

commented May 8, 2015

@silverwind yeah, I read this, but I am struggling to make request work with this. It internally uses https module.

@Restuta

This comment has been minimized.

Copy link

commented May 8, 2015

it would be nice to have like a node flag --use-legacy-ciphers that globally overrides defaults, so we don't have to figure it out for every lib that uses https internally

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented May 8, 2015

You could do that through a pre-load (-r) module. I'm personally -1 on a flag. If people want insecure defaults, it's okay to make them jump through a few hoops; maybe it makes them reconsider.

@silverwind

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2015

@Restuta It's quite possible that request specifies a custom cipher suite, which would override such a flag. Also, i'd say, this is more of a feature request to request, because ultimately, they are in control of the ciphers, io.js just provides extendable defaults.

@silverwind

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2015

@Restuta For request, you should be able to specify ciphers in agentOptions.

@Restuta

This comment has been minimized.

Copy link

commented May 8, 2015

@bnoordhuis web-scraping is a good example, what could you possible reconsider there? Point is that some use-cases are valid and make that subset of people suffer so superset of people don't have an easy option to use insecure defaults and therefore less likely will use it is hard to justify. To me it's not very obvious decision.

@silverwind looks like. Mb web-scraping is very specific case and there are not that many libs like request, so it's okay. Thanks for the quick response.

Just found a way to do it with request:

var options = {
    url: 'https://www.usacycling.org,
    agentOptions: {
        ciphers: '<here you go>',
    }
};
//and then 
//request(options, ....

or set it globally:

request = require('request').defaults({
  agentOptions: {
    ciphers: '<here you go>',
  }
})
@bnoordhuis

This comment has been minimized.

Copy link
Member

commented May 8, 2015

web-scraping is a good example, what could you possible reconsider there? Point is that some use-cases are valid and make that subset of people suffer so superset of people don't have an easy option to use insecure defaults and therefore less likely will use it is hard to justify.

That's my point and I think it's very easy to justify. The people motivated enough will jump through the required hoops, whereas the quick hack crowd, people who would add the flag and move on without giving it a second thought, will now hopefully stop to consider the ramifications.

@Restuta

This comment has been minimized.

Copy link

commented May 8, 2015

@bnoordhuis to me something in the middle is more appealing. Like a big warning after you specified a flag saying that you just killed a baby unicorn (with an optional picture and your github avatar updated). So if you are desperate enough to read the docs, set this flag up, ignore the warning – you deserve all the consequences =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.