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

remove smaller DH modp's #3406

Closed
calvinmetcalf opened this issue Oct 16, 2015 · 10 comments
Closed

remove smaller DH modp's #3406

calvinmetcalf opened this issue Oct 16, 2015 · 10 comments
Labels
crypto Issues and PRs related to the crypto subsystem. security Issues and PRs related to security. tls Issues and PRs related to the tls subsystem.

Comments

@calvinmetcalf
Copy link
Contributor

based on the recent paper on new attacks against diffie-hellman it might be a good idea to remove 2 of the well known primes that node comes with from the defaults, namely modp1 and modp2 aka Oakley Group 1 and 2 which are mentioned specifically in the linked paper as being vulnerable to pre computation attacks with academic and state level resources respectively.

@mscdex
Copy link
Contributor

mscdex commented Oct 16, 2015

Couple of questions:

  1. Is this suggestion just for TLS or also for the crypto module?
  2. Just recently the default DH prime size for node TLS clients was set to 1024 (with the node TLS server rejecting anything less than 2048 by default), which is the same size as modp2. I guess this means we'd have to then bump this to 2048 or something? How would that affect existing TLS clients?

@mscdex mscdex added tls Issues and PRs related to the tls subsystem. crypto Issues and PRs related to the crypto subsystem. labels Oct 16, 2015
@rvagg
Copy link
Member

rvagg commented Oct 16, 2015

Yeah, 1024 and 1536 are mentioned in the paper, so even though we've moved up to 1024 as a minimum we're still not at an ideal level, but thus is the compromise involved in choosing default parameters!

I think it might be Java 7 based clients that are the biggest hurdle to moving beyond 1024 as a minimum and Java 7 is still in very wide use and not likely to disappear any time soon.

This whole area is making me wonder if we shouldn't try a new approach to our crypto defaults. We are basically forced to select our settings and cipher suite based on lowest-common-denominator client capabilities while considering the whole Node.js ecosystem. However, many Node.js users are deploying to environments where their target clients are well above the minimum that we're considering. Therefore, perhaps we need to have at least two levels of crypto settings, a default and an ideal, maybe even an uber secure for those that can afford it (e.g. situations where they have full control over their clients and may even be doing Node.js <-> Node.js comms). I see two obvious ways of doing this:

  1. Documentation - create a new section in our documentation with details about what you can do to move beyond the default level of crypto by providing a custom cipher suite and other settings that harden your clients & servers.
  2. Options - create a simple option where you select the level of security you are opting for and that maps to our internal coded version of what we consider the best settings for that level of security are. Being able do something like tls.createServer({ key, cert, ca, securityLevel: 'ideal' }) would be fantastic. Beyond usability it has a benefit above documentation of letting the user trust us to update these settings as the landscape changes, so they don't need to keep on coming back to check what our latest recommendations are.

/cc @nodejs/crypto

@calvinmetcalf
Copy link
Contributor Author

So this is more specifically about well known primes then small primes but
the gist is more to remove the presets so those 2 specific ones can't be
used by name.

On Fri, Oct 16, 2015, 4:27 PM Brian White [email protected] wrote:

Couple of questions:

Is this suggestion just for TLS or also for the crypto module?
2.

Just recently
f72e178
the default DH prime size for node TLS clients was set to 1024 (with the
node TLS server rejecting anything less than 2048 by default), which is the
same size as modp2. I guess this means we'd have to then bump this to 2048
or something? How would that affect existing TLS clients?


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

@alexlamsl
Copy link

Java 7 supports ECDHE, so I don't think it's fatal that it cannot handle DH keys > 1024 bits. Java 6 OTOH would be in trouble.

I have tested that using Qualys SSL Server Test.

@bnoordhuis
Copy link
Member

Do we know what other languages / frameworks do or are doing in the short term? My (admittedly limited) googling didn't really turn up anything.

Removing the ability to use modp1 and modp2 by name seems reasonable at first glance. Alternatively, we can prefix them with insecure_ to get the message across but still leave people with an escape hatch?

/cc @nodejs/crypto - you should chime in here.

@shigeki
Copy link
Contributor

shigeki commented Oct 22, 2015

Sorry to be late. I missed this.
tls module does not use standard groups for DH key exchange and 5.0 is already limits its key size above 1024bits. The fix in LTS is in discussion of nodejs/Release#49. So we need not to care this in tls here.

crypto module has a API of crypto.getDiffieHellman which uses these groups, however, it would be used not only on the wire but also in offline or internal network and my opinion is as the same of @rvagg 's comment of 1 to add documentation to notify users of caveats to use weak algorithms and key.
This was discussed in nodejs/node-v0.x-archive#25564 and I will move the PR to here and resubmit it.

@indutny
Copy link
Member

indutny commented Oct 22, 2015

I'm +1 for prefix, or stderr warning on use.

@mscdex
Copy link
Contributor

mscdex commented Oct 22, 2015

FWIW modp1 is one of the groups required to be supported by ssh2 implementations, but so is modp14. I agree with @shigeki though, I think I'd prefer just a documentation change for now.

@ChALkeR ChALkeR added the security Issues and PRs related to security. label Feb 29, 2016
@jasnell
Copy link
Member

jasnell commented Apr 4, 2016

Is there reason to keep this open still?

@mscdex
Copy link
Contributor

mscdex commented Apr 4, 2016

@jasnell IMHO I don't think so, a documentation update was committed awhile back.

@jasnell jasnell closed this as completed Apr 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. security Issues and PRs related to security. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

No branches or pull requests

9 participants