tls: make server not use DHE in less than 1024bits for fix agaist Logjam Attack #1739

shigeki · 2015-05-20T06:13:06Z

DHE key lengths less than 1024bits is already weaken as pointed out in https://weakdh.org/ . 1024bits will not be safe in near future. We will extend this up to 2048bits somedays later.

What do about for clients? We can obtain DHE keylength via SSL_get_server_tmp_key() but I think we'd better to have an new options to limit the DHE key size with 1024bits default. Thoughts?

CI results are https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/697/. It looks fine except jenkins, child_process and sync-io-option error.

R= @bnoordhuis @indutny

brendanashworth · 2015-05-20T06:23:49Z

As this is a security patch (?), does this cheat the semver major cycle?

shigeki · 2015-05-20T06:29:26Z

Yes, this is a security patch but it is a similar kind of the fix for limiting cipher suites we did before. We did not change semver in changing the default cipher suite so I think this follows it.

shigeki · 2015-05-20T06:47:45Z

Update the doc description to recommend to use 2048bits or more.

ChALkeR · 2015-05-20T09:56:44Z

A quick question: when 1024 bits will be not safe, will you make a similar patch to make it throw an error then? Maybe it would be reasonable to print a warning to stderr in advance (maybe even now) for 1024 bits so less people will get surprised when that will be turned into an error?

bnoordhuis · 2015-05-20T10:09:14Z

doc/api/tls.markdown

+    Its key length should be greater than or equal to 1024bits, otherwise
+    it throws an error. It is recommended to use 2048bits or more for
+    enough strong security. If omitted or invalid, it is silently
+    discarded and DHE ciphers won't be available.


s/1024bits/1024 bits/, ditto for 2048bits.

I would s/recommended/strongly recommended/ and maybe add a warning that someday we're going to enforce it.

s/enough strong security/stronger security/

DHE key lengths less than 1024bits is already weaken as pointed out in https://weakdh.org/ . 1024bits will not be safe in near future. We will extend this up to 2048bits somedays later.

shigeki · 2015-05-20T11:54:12Z

@ChALkeR Printing WARNING is a good idea to me. Updated it in this PR.

@bnoordhuis Updated according your comments. My editor was wired when I wrote assert.throws. I also found a bug of my tests that the 512 bits test does not invoke the next test. PTAL.

bnoordhuis · 2015-05-20T14:36:49Z

src/node_crypto.cc

@@ -757,6 +757,12 @@ void SecureContext::SetDHParam(const FunctionCallbackInfo<Value>& args) {
  if (dh == nullptr)
    return;

+  int keylen = BN_num_bits(dh->p);


Suggestion: make keylen const.

bnoordhuis · 2015-05-20T14:37:56Z

LGTM

A suggestion for a follow-up PR: print the warning from JS land with console.trace() to make it easy to figure out where the call comes from.

indutny · 2015-05-20T14:50:43Z

LGTM

DHE key lengths less than 1024bits is already weaken as pointed out in https://weakdh.org/ . 1024bits will not be safe in near future. We will extend this up to 2048bits somedays later. PR-URL: #1739 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>

shigeki · 2015-05-20T15:06:45Z

@bnoordhuis @indutny Thanks for reviewing. Landed in 9b35be5 with Ben's suggestion to add const for kenlen. For console.trace, I will work it with a client fix later.

DHE key lengths less than 1024bits is already weaken as pointed out in https://weakdh.org/ . 1024bits will not be safe in near future. We will extend this up to 2048bits somedays later. PR-URL: nodejs#1739 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>

PR-URL: nodejs#1532 Notable Changes: * crypto: Diffie-Hellman key exchange (DHE) parameters ('dhparams') must now be 1024 bits or longer or an error will be thrown. A warning will also be printed to the console if you supply less than 2048 bits. See https://weakdh.org/ for further context on this security concern. (Shigeki Ohtsu) nodejs#1739. * node: A new --trace-sync-io command line flag will print a warning and a stack trace whenever a synchronous API is used. This can be used to track down synchronous calls that may be slowing down an application. (Trevor Norris) nodejs#1707. * node: To allow for chaining of methods, the setTimeout(), setKeepAlive(), setNoDelay(), ref() and unref() methods used in 'net', 'dgram', 'http', 'https' and 'tls' now return the current instance instead of undefined (Roman Reiss) nodejs#1779. * util: A significant speed-up (in the order of 35%) for the common-case of a single string argument to util.format(), used by console.log() (Сковорода Никита Андреевич) nodejs#1749.

PR-URL: nodejs#1532 Notable Changes: * crypto: Diffie-Hellman key exchange (DHE) parameters ('dhparams') must now be 1024 bits or longer or an error will be thrown. A warning will also be printed to the console if you supply less than 2048 bits. See https://weakdh.org/ for further context on this security concern. (Shigeki Ohtsu) nodejs#1739. * node: A new --trace-sync-io command line flag will print a warning and a stack trace whenever a synchronous API is used. This can be used to track down synchronous calls that may be slowing down an application. (Trevor Norris) nodejs#1707. * node: To allow for chaining of methods, the setTimeout(), setKeepAlive(), setNoDelay(), ref() and unref() methods used in 'net', 'dgram', 'http', 'https' and 'tls' now return the current instance instead of undefined (Roman Reiss & Evan Lucas) nodejs#1699 nodejs#1768 nodejs#1779. * util: A significant speed-up (in the order of 35%) for the common-case of a single string argument to util.format(), used by console.log() (Сковорода Никита Андреевич) nodejs#1749.

PR-URL: nodejs#1532 Notable Changes: * crypto: Diffie-Hellman key exchange (DHE) parameters ('dhparams') must now be 1024 bits or longer or an error will be thrown. A warning will also be printed to the console if you supply less than 2048 bits. See https://weakdh.org/ for further context on this security concern. (Shigeki Ohtsu) nodejs#1739. * node: A new --trace-sync-io command line flag will print a warning and a stack trace whenever a synchronous API is used. This can be used to track down synchronous calls that may be slowing down an application. (Trevor Norris) nodejs#1707. * node: To allow for chaining of methods, the setTimeout(), setKeepAlive(), setNoDelay(), ref() and unref() methods used in 'net', 'dgram', 'http', 'https' and 'tls' now return the current instance instead of undefined (Roman Reiss & Evan Lucas) nodejs#1699 nodejs#1768 nodejs#1779. * npm: Upgraded to v2.10.1, release notes can be found in https://github.com/npm/npm/releases/tag/v2.10.1 and https://github.com/npm/npm/releases/tag/v2.10.0. * util: A significant speed-up (in the order of 35%) for the common-case of a single string argument to util.format(), used by console.log() (Сковорода Никита Андреевич) nodejs#1749.

PR-URL: #1777 Notable Changes: * crypto: Diffie-Hellman key exchange (DHE) parameters ('dhparams') must now be 1024 bits or longer or an error will be thrown. A warning will also be printed to the console if you supply less than 2048 bits. See https://weakdh.org/ for further context on this security concern. (Shigeki Ohtsu) #1739. * node: A new --trace-sync-io command line flag will print a warning and a stack trace whenever a synchronous API is used. This can be used to track down synchronous calls that may be slowing down an application. (Trevor Norris) #1707. * node: To allow for chaining of methods, the setTimeout(), setKeepAlive(), setNoDelay(), ref() and unref() methods used in 'net', 'dgram', 'http', 'https' and 'tls' now return the current instance instead of undefined (Roman Reiss & Evan Lucas) #1699 #1768 #1779. * npm: Upgraded to v2.10.1, release notes can be found in https://github.com/npm/npm/releases/tag/v2.10.1 and https://github.com/npm/npm/releases/tag/v2.10.0. * util: A significant speed-up (in the order of 35%) for the common-case of a single string argument to util.format(), used by console.log() (Сковорода Никита Андреевич) #1749.

To make it easy to figure out where the warning comes from. Also fix style and variable name that was made in nodejs#1739.

DHE key lengths less than 1024bits is already weaken as pointed out in https://weakdh.org/ . 1024bits will not be safe in near future. We will extend this up to 2048bits somedays later. PR-URL: nodejs/node#1739 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>

DHE key lengths less than 1024bits is already weaken as pointed out in https://weakdh.org/ . 1024bits will not be safe in near future. We will extend this up to 2048bits somedays later. PR-URL: nodejs#1739 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>

To make it easy to figure out where the warning comes from. Also fix style and variable name that was made in nodejs#1739.

To make it easy to figure out where the warning comes from. Also fix style and variable name that was made in #1739. PR-URL: #1831 Reviewed-By: indutny - Fedor Indutny <[email protected]> Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>

shigeki added the tls Issues and PRs related to the tls subsystem. label May 20, 2015

shigeki force-pushed the fix_LogjamAttack branch from e9e7ccf to cdc7c17 Compare May 20, 2015 06:45

bnoordhuis reviewed May 20, 2015
View reviewed changes

tls: make server not use DHE in less than 1024bits

81c4808

DHE key lengths less than 1024bits is already weaken as pointed out in https://weakdh.org/ . 1024bits will not be safe in near future. We will extend this up to 2048bits somedays later.

shigeki force-pushed the fix_LogjamAttack branch from cdc7c17 to 81c4808 Compare May 20, 2015 11:49

bnoordhuis reviewed May 20, 2015
View reviewed changes

shigeki closed this May 20, 2015

rvagg mentioned this pull request May 23, 2015

Release proposal v2.1.0 #1777

Closed

yosuke-furukawa mentioned this pull request May 23, 2015

Evangelism: Weekly Update for May 22nd nodejs/evangelism#73

Closed

yosuke-furukawa mentioned this pull request May 25, 2015

Create weekly-update.2015-05-22.md nodejs/evangelism#83

Merged

shigeki pushed a commit to shigeki/node that referenced this pull request May 30, 2015

tls: output warning of setDHParam to console.trace

dd8a2a2

To make it easy to figure out where the warning comes from. Also fix style and variable name that was made in nodejs#1739.

shigeki pushed a commit to shigeki/node that referenced this pull request Jun 1, 2015

tls: output warning of setDHParam to console.trace

b993f36

To make it easy to figure out where the warning comes from. Also fix style and variable name that was made in nodejs#1739.

shigeki pushed a commit to shigeki/node that referenced this pull request Jun 1, 2015

tls: output warning of setDHParam to console.trace

9a5d761

To make it easy to figure out where the warning comes from. Also fix style and variable name that was made in nodejs#1739.

shigeki pushed a commit to shigeki/node that referenced this pull request Jun 1, 2015

tls: output warning of setDHParam to console.trace

1a999d7

To make it easy to figure out where the warning comes from. Also fix style and variable name that was made in nodejs#1739.

mhdawson mentioned this pull request Jun 11, 2015

Update openssl to 1.0.1o nodejs/node-v0.x-archive#25509

Closed

shigeki mentioned this pull request Jun 12, 2015

Upgrade to openssl-1.0.2b #1950

Closed

This was referenced Jun 23, 2015

Limit use of small DH keys in Node nodejs/node-v0.x-archive#25562

Closed

Fix for the issue 25366. nodejs/node-v0.x-archive#25564

Closed

shigeki pushed a commit to shigeki/node that referenced this pull request Aug 27, 2015

tls: output warning of setDHParam to console.trace

b867a31

To make it easy to figure out where the warning comes from. Also fix style and variable name that was made in nodejs#1739.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: make server not use DHE in less than 1024bits for fix agaist Logjam Attack #1739

tls: make server not use DHE in less than 1024bits for fix agaist Logjam Attack #1739

shigeki commented May 20, 2015

brendanashworth commented May 20, 2015

shigeki commented May 20, 2015

shigeki commented May 20, 2015

ChALkeR commented May 20, 2015

bnoordhuis May 20, 2015

shigeki commented May 20, 2015

bnoordhuis May 20, 2015

bnoordhuis commented May 20, 2015

indutny commented May 20, 2015

shigeki commented May 20, 2015

tls: make server not use DHE in less than 1024bits for fix agaist Logjam Attack #1739

tls: make server not use DHE in less than 1024bits for fix agaist Logjam Attack #1739

Conversation

shigeki commented May 20, 2015

brendanashworth commented May 20, 2015

shigeki commented May 20, 2015

shigeki commented May 20, 2015

ChALkeR commented May 20, 2015

bnoordhuis May 20, 2015

Choose a reason for hiding this comment

shigeki commented May 20, 2015

bnoordhuis May 20, 2015

Choose a reason for hiding this comment

bnoordhuis commented May 20, 2015

indutny commented May 20, 2015

shigeki commented May 20, 2015