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: prevent server from using dhe keys < 768 #3890

Closed
wants to merge 1 commit into from

Conversation

mhdawson
Copy link
Member

As part of the fix for logjam, node was upgraded to a
level of openssl which rejects connections to servers that
are using keys smaller than 768 bits. It is still possible,
however, to create a server that uses a smaller key size
and and older client may be able to connect to it.

This PR moves us to a secure by default stance on the
server side as well, preventing the creation of a server
using a dhe key size less than 768. This can be overridden
with the command line option which is also added.

It is derived from

9b35be5

which was landed in later io.js/node versions but makes
the limit 1024. This PR uses the smaller limit in order
to meet the recomendations for logjam while matching was
was done on the client side in openssl to minimize the
potential impacton users.

@mhdawson
Copy link
Member Author

@shigeki @jasnell can you two take a look at this PR

@mscdex mscdex added the tls Issues and PRs related to the tls subsystem. label Nov 17, 2015
@MylesBorins
Copy link
Contributor

LGTM

Ran test suite locally. 1 failing (but unrelated) test in make test. make test-addons ran cleanly

@ChALkeR
Copy link
Member

ChALkeR commented Nov 18, 2015

This is a patch that prevents a security misconfiguration, IMO that makes it not a semver-major.

@jasnell
Copy link
Member

jasnell commented Nov 18, 2015

Technically it is semver-major but it is one that should have minimal impact. The introduction of the new command line switch to revert to the prior behavior is also significant. That said, this would land in v0.12.x which is in a weird place in semver-land anyway.

LGTM

@@ -203,7 +203,8 @@ automatically set as a listener for the [secureConnection][] event. The

- `dhparam`: DH parameter file to use for DHE key agreement. Use
`openssl dhparam` command to create it. If the file is invalid to
load, it is silently discarded.
load, it is silently discarded. Its key length should be greater
than or equal to 768 bits, otherwise it throws an error.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"its key length" -> "it throws an error", with "it" referring to two different things here. Maybe just "otherwise an error will be thrown" as the easy way out.

@rvagg
Copy link
Member

rvagg commented Nov 18, 2015

I'm not really happy about the --revert-dhe-server-limit commandline argument here and I think I'd prefer to make it something you can't change than introduce another API we have to care about into the future (commandline argument as an API). If we must have the argument then perhaps something more specific that doesn't use the term "revert", --allow-insecure-server-dhparam.

@rvagg
Copy link
Member

rvagg commented Nov 18, 2015

Also, I'd vote for semver-major, we can argue this is a bug or misconfiguration but in the spirit of the conservative approach to semver we've been taking we should hold off till the next major.

And, /cc @nodejs/crypto

@ChALkeR
Copy link
Member

ChALkeR commented Nov 18, 2015

@rvagg The only setups this is going to break is those that have been dangerously misconfigured. And they still have a way to get back the old behaviour. It looks like a sane thing to do.

An alternative non-breaking approach would be to reduce this to a big warning instead of an Error. But given that this part of code is generally one of the first things that the server does — anyone should notice the error immediately after the version update on the test setup.

@jasnell
Copy link
Member

jasnell commented Nov 18, 2015

@rvagg .. note that this does not print the --revert-dhe-server-limit in the node --help output or document it in any way. The intent would be for it to be supported strictly in the LTS branches and not something that would be supported going forward. The only place it would be documented is in the release notes as an indication that if this change breaks you, here's the workaround we're providing... which by the way is only supported in these specific LTS lines.

I'm good with --allow-insecure-server-dhparam as a name, tho.

@rvagg
Copy link
Member

rvagg commented Nov 18, 2015

oh, I see this is for 0.12, we are doing the right thing in v4+ I take it then?

@jasnell
Copy link
Member

jasnell commented Nov 18, 2015

@mhdawson , correct me if I'm wrong, but this fix would end up needing to go out in v4.x also (unfortunately). In v4.x it would be a semver-minor bump

@shigeki
Copy link
Contributor

shigeki commented Nov 18, 2015

I leave the decision of a new option name to native English speakers. I have no preference.

@mhdawson I have two comments here.

@jasnell It's already in v4.x with 1024 bits limit at https://github.com/nodejs/node/blob/v4.x/src/node_crypto.cc#L800-L806 . Do not be worry :-)

@Fishrock123 Fishrock123 added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 18, 2015
@Fishrock123
Copy link
Contributor

Note: this is at least semver-minor, because as @bnoordhuis mentioned recently, downgrading while using this option will cause an error.

@jasnell
Copy link
Member

jasnell commented Nov 18, 2015

@shigeki ... ah, that's right.. so hard to keep track sometimes :-/

@mhdawson
Copy link
Member Author

@shigeki I added the requested change to the Makefile for the test fixtures.

In terms of documenting this the discussion I had with James was that we'd prefer not to add to the regular documentation. We don't want anybody to use it and it will not be in any later release. Instead we planned to just document it in the migration notes so that people are aware of it if they need to use it when upgrading but otherwise are less likely to use it. I think Jame's comment above echo's the same approach.

Does that make sense to you or do you still think we need to add it to printHelp() and tls.markdown ?

@mhdawson
Copy link
Member Author

@rvagg updated to address wording comments and change name of the option

@shigeki
Copy link
Contributor

shigeki commented Nov 19, 2015

@mhdawson I'm okay for no doc. Someone in the future would have the same question as mine so please write its description in the commit log. Otherwise, LGTM after CI is fine.

As part of the fix for logjam, node was upgraded to a
level of openssl which rejects connections to servers that
are using keys smaller than 768 bits. It is still possible,
however, to create a server that uses a smaller key size
and and older client may be able to connect to it.

This PR moves us to a secure by default stance on the
server side as well, preventing the creation of a server
using a dhe key size less than 768. This can be overridden
with the command line option which is also added.

It is derived from

nodejs@9b35be5

which was landed in later io.js/node versions but makes
the limit 1024.  This PR uses the smaller limit in order
to meet the recomendations for logjam while matching was
was done on the client side in openssl to minimize the
potential impacton users.

The command line option will only be documented in the
release notes and will not be added to the tls
documentation.  The goal is that people who are
upgrading are aware and can use the option if they
run into issues, but otherwise the option is not
visible/used.

Address review comments 1

Address second set of review comments
@mhdawson
Copy link
Member Author

Updated based on shigeki's comment to add to the commit comment and squashed.

// USE OR OTHER DEALINGS IN THE SOFTWARE.
//
// Flags: --allow-insecure-server-dhparam

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The copyright/license header should be removed. nm... oy, forgot these are v0.12

@jasnell
Copy link
Member

jasnell commented Nov 19, 2015

Changes LGTM. Disregard my previous two comments, keep forgetting this is v0.12.

@mhdawson
Copy link
Member Author

@mhdawson
Copy link
Member Author

CI run is green on all but windows. Looking at earlier failures most of them are pre-existing. The only one that worries me a bit is "not ok 721 - test-tls-ticket-cluster.js" which failed 2/3 times and which I don't see in the previous run, going to start another CI run to see how many times it recreates. Don't think its related as I'd expect any failure to be 100% but more data can't hurt https://ci.nodejs.org/job/node-test-pull-request/782/

@jasnell
Copy link
Member

jasnell commented Nov 19, 2015

If that continues to fail, may want to pass it through the new stress test job in CI to see if it's flaky

@mhdawson
Copy link
Member Author

From test https://github.com/nodejs/node/blob/v0.12/test/simple/test-tls-ticket-cluster.js
var keyFile = join(common.fixturesDir, 'agent.key');
var certFile = join(common.fixturesDir, 'agent.crt');
From https://github.com/nodejs/node/blob/v0.12/test/fixtures/agent.key

-----BEGIN RSA PRIVATE KEY-----
MIIEowIBAAKCAQEAz+LXZOjcQCJq3+ZKUFabj71oo/ex/XsBcFqtBThjjTw9CVEV

So looks to be an RSA key, don't see how that could be related to limiting DHE key size

@mhdawson
Copy link
Member Author

No recreates in second run, only failures commonly seen on windows so seems unrelated. I think we are good to go CI run wise

@jasnell
Copy link
Member

jasnell commented Nov 19, 2015

@rvagg ... can we get one more review from you on this before landing?

@rvagg
Copy link
Member

rvagg commented Nov 19, 2015

I'm not entirely comfortable with squeezing this in to v0.12 cause of the breaking nature of it but I'll throw in an lgtm for the sake of secure-by-default and the low likelihood of anyone being impacted.

@shigeki
Copy link
Contributor

shigeki commented Nov 20, 2015

@mhdawson test-tls-ticket-cluster.js has nothing to do with DHE key exchange. Probably it was a timing issue in process fork and die.

FYI. Google is planning to deprecate DHE in Chrome because they consider that 1024bits DHE has not enough security and moving to ECDHE is better.
"Intent to deprecate: DHE-based cipher suites" https://groups.google.com/a/chromium.org/d/msg/security-dev/dYyhKHPnrI0/pNxx8vTKBAAJ

@jasnell
Copy link
Member

jasnell commented Nov 20, 2015

@rvagg ... noted, I have the same concerns but overall, LGTM. @mhdawson , can you go ahead and get this landed in v0.12-staging?

@silverwind
Copy link
Contributor

I'm not happy with adding another seldom used command line switch, but if you go ahead, please at add it to node.1 and the --help print, so it's properly documented.

@jasnell
Copy link
Member

jasnell commented Nov 22, 2015

not documenting it is intentional. This switch would only exist in v0.12
and is only for people who absolutely need it. There is no intent to
support it in any other version. The documentation would be the release
notes. By not putting it into any other documentation, we are not
committing to support it beyond the v0.12 branch
On Nov 22, 2015 12:15 PM, "silverwind" [email protected] wrote:

I'm not happy with adding another seldom used command line switch, but if
you go ahead, please at add it to the man page and the --help print, so
it's properly documented.


Reply to this email directly or view it on GitHub
#3890 (comment).

@silverwind
Copy link
Contributor

@jasnell alright, carry on!

@mhdawson
Copy link
Member Author

I was out Friday afternoon so just see the comment from James to land, will do that now

mhdawson added a commit that referenced this pull request Nov 23, 2015
As part of the fix for logjam, node was upgraded to a
level of openssl which rejects connections to servers that
are using keys smaller than 768 bits. It is still possible,
however, to create a server that uses a smaller key size
and and older client may be able to connect to it.

This PR moves us to a secure by default stance on the
server side as well, preventing the creation of a server
using a dhe key size less than 768. This can be overridden
with the command line option which is also added.

It is derived from

9b35be5

which was landed in later io.js/node versions but makes
the limit 1024.  This PR uses the smaller limit in order
to meet the recomendations for logjam while matching was
was done on the client side in openssl to minimize the
potential impacton users.

The command line option will only be documented in the
release notes and will not be added to the tls
documentation.  The goal is that people who are
upgrading are aware and can use the option if they
run into issues, but otherwise the option is not
visible/used.

PR-URL: #3890
Fixes: nodejs/Release#49
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: James Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
@mhdawson
Copy link
Member Author

landed caa16b4

@mhdawson mhdawson closed this Nov 23, 2015
@mhdawson mhdawson self-assigned this Nov 23, 2015
rvagg pushed a commit that referenced this pull request Dec 8, 2015
PR #3890 [1] introduced the variable ALLOW_INSECURE_SERVER_DHPARAM defined
in src/node_crypto.cc. However, if nodejs is built without OpenSSL support,
the build fails:
 error: ‘ALLOW_INSECURE_SERVER_DHPARAM’ was not declared in this scope
       ALLOW_INSECURE_SERVER_DHPARAM = true;

Fix this by using the preprocessor macro HAVE_OPENSSL to opt-out the use of
ALLOW_INSECURE_SERVER_DHPARAM in non-OpenSSL builds.

[1] #3890

PR-URL: #4201
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
PR #3890 [1] introduced the variable ALLOW_INSECURE_SERVER_DHPARAM defined
in src/node_crypto.cc. However, if nodejs is built without OpenSSL support,
the build fails:
 error: ‘ALLOW_INSECURE_SERVER_DHPARAM’ was not declared in this scope
       ALLOW_INSECURE_SERVER_DHPARAM = true;

Fix this by using the preprocessor macro HAVE_OPENSSL to opt-out the use of
ALLOW_INSECURE_SERVER_DHPARAM in non-OpenSSL builds.

[1] #3890

PR-URL: #4201
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@mhdawson mhdawson deleted the logjam branch May 9, 2016 22:39
jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
As part of the fix for logjam, node was upgraded to a
level of openssl which rejects connections to servers that
are using keys smaller than 768 bits. It is still possible,
however, to create a server that uses a smaller key size
and and older client may be able to connect to it.

This PR moves us to a secure by default stance on the
server side as well, preventing the creation of a server
using a dhe key size less than 768. This can be overridden
with the command line option which is also added.

It is derived from

nodejs/node@9b35be5

which was landed in later io.js/node versions but makes
the limit 1024.  This PR uses the smaller limit in order
to meet the recomendations for logjam while matching was
was done on the client side in openssl to minimize the
potential impacton users.

The command line option will only be documented in the
release notes and will not be added to the tls
documentation.  The goal is that people who are
upgrading are aware and can use the option if they
run into issues, but otherwise the option is not
visible/used.

PR-URL: nodejs/node#3890
Fixes: nodejs/Release#49
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: James Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
PR nodejs#3890 [1] introduced the variable ALLOW_INSECURE_SERVER_DHPARAM defined
in src/node_crypto.cc. However, if nodejs is built without OpenSSL support,
the build fails:
 error: ‘ALLOW_INSECURE_SERVER_DHPARAM’ was not declared in this scope
       ALLOW_INSECURE_SERVER_DHPARAM = true;

Fix this by using the preprocessor macro HAVE_OPENSSL to opt-out the use of
ALLOW_INSECURE_SERVER_DHPARAM in non-OpenSSL builds.

[1] nodejs/node#3890

PR-URL: nodejs/node#4201
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants