tls: make server not use DHE in less than 1024bits for fix agaist Logjam Attack by shigeki · Pull Request #1739 · nodejs/node · GitHub

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

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

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.

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

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?

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.

@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.

Suggestion: make keylen const.

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.

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 <info@bnoordhuis.nl> Reviewed-By: Fedor Indutny <fedor@indutny.com>

@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 <info@bnoordhuis.nl> Reviewed-By: Fedor Indutny <fedor@indutny.com>

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 <info@bnoordhuis.nl> Reviewed-By: Fedor Indutny <fedor@indutny.com>

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 <info@bnoordhuis.nl> Reviewed-By: Fedor Indutny <fedor@indutny.com>

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 <fedor.indutny@gmail.com> Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

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.

tls: make server not use DHE in less than 1024bits for fix agaist Logjam Attack by shigeki · Pull Request #1739 · nodejs/node · GitHub

Uh oh!

Uh oh!

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 wants to merge 1 commit into
nodejs:masterfrom
shigeki:fix_LogjamAttack

shigeki commented May 20, 2015

Uh oh!

brendanashworth commented May 20, 2015

Uh oh!

shigeki commented May 20, 2015

Uh oh!

shigeki commented May 20, 2015

Uh oh!

ChALkeR commented May 20, 2015

Uh oh!

bnoordhuis May 20, 2015

Uh oh!

shigeki commented May 20, 2015

Uh oh!

bnoordhuis May 20, 2015

Uh oh!

bnoordhuis commented May 20, 2015

Uh oh!

indutny commented May 20, 2015

Uh oh!

shigeki commented May 20, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tls: make server not use DHE in less than 1024bits for fix agaist Logjam Attack by shigeki · Pull Request #1739 · nodejs/node · GitHub

Uh oh!

Uh oh!

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#1739shigeki wants to merge 1 commit intonodejs:masternodejs/node:masterfrom shigeki:fix_LogjamAttackshigeki/node:fix_LogjamAttackCopy head branch name to clipboard

Conversation

shigeki commented May 20, 2015

Uh oh!

brendanashworth commented May 20, 2015

Uh oh!

shigeki commented May 20, 2015

Uh oh!

shigeki commented May 20, 2015

Uh oh!

ChALkeR commented May 20, 2015

Uh oh!

bnoordhuis May 20, 2015

Choose a reason for hiding this comment

Uh oh!

shigeki commented May 20, 2015

Uh oh!

bnoordhuis May 20, 2015

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented May 20, 2015

Uh oh!

indutny commented May 20, 2015

Uh oh!

shigeki commented May 20, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tls: make server not use DHE in less than 1024bits for fix agaist Logjam Attack#1739
shigeki wants to merge 1 commit into
nodejs:masterfrom
shigeki:fix_LogjamAttack