[thinred]

In particular:

• DH groups of size < 1024 are disabled by default
  (there is only one such group: modp1)
• a new cmdline switch --enable-small-dh-groups and
  SMALL_DH_GROUPS_ENABLE env. variable are introduced;
  they override the default setting and therefore enable
  modp1 group
• the docs & tests are updated

[mhdawson]

This looks good to me but would like @shigeki to take a look as well. If shigeki thinks it looks ok I can create a PR that pulls over nodejs/node#1739 and updates to include a check against SMALL_DH_GROUPS_ENABLE.

shigeki I'd also like your opinion on whether we should add a similar check in crypto.createDiffieHellman(prime_length) as well. ie reject a prime_length smaller than 768 and use the same command line/env variable to allow fallback.

[shigeki]

I think that this is somehow confusing that only modp1 leads throwing an error and modp2 and modp5 can be used without no message of deprecation whereas the size of 2048 bits is considered deprecated.
Is it better to say that the group size of less than 1024 bits was deprecated and that of less than 2048 bits is not recommended?

[thinred]

Ok, let's do it that way.

[shigeki]

Commit title need to be changed to follow https://github.com/joyent/node/blob/master/CONTRIBUTING.md#commit.

[shigeki]

I put some comments.
Actually I do not have a confidence to make a deprecation of low security key sizes in crypto.createDiffieHellman because RSA below 512 bits also needs it. Another security risk exists in md5 and sha1 but we still continue to using them.
I made the size check in DHE because TLS has a security risk of MiTM but I wonder if we need to extend it for the use not in the wire.

[thinred]

I improved the PR.

[mhdawson]

@shigeki does your last comment apply only to createDiffieHellman or are you wondering if we should be deprecating for getDiffieHellman() ?

[shigeki]

My comment above is more general for security hardening on crypto module not tls.
After deprecating modp1,2 and 5 in getDiffieHellman() here, we still allow createDiffieHellman() with a low key size. In v0.12, we still allow createECDH() with the curve names that have a small size. We can also use 512 bits RSA in publicEncrypt().
I'm wondering if this fix leads inconsistent security level between crypto functionalities.

Edit: correction that only modp1 is to be deprecated.

[thinred]

To be clear, only modp1 is deprecated and disabled. modp2 & modp5 work just fine, but are not recommended.
The main security issue here is that calculations necessary to crack the modp1 group (and bigger groups but to a smaller extent) can be put into a precomputation step and then reused very fast. If you generate a random DH group with createDiffieHellman and size of 768 bits, you may be relatively safe actually, especially if it is a short-term group.
I originally proposed to disable or discourage use of small groups. I'm still ok for the latter - maybe an update to the docs is enough?

[shigeki]

Yes, only modp1 is to be deprecated. I updated and corrected the above comment.
I agree that modp1 is the most vulnerable for it can solved within a minute during TLS and VPN handshake. But this crypto module is not only used on wire but also in offline where they do not have a handshake timeout. Attacker can crack these encrypted data for several days, weeks and months in this case. In this point of this view, I think there is no reason to have a special treatment only on modp1.

But to be sure, I'm not against deprecation. If we do this, I'm wondering we are going to the way to have -enable-small-ecdh-curve, -enable-small-dh-key, -enable-small-rsa-key and whatever on Node in future. This does not seem to be smart. Updating docs is one of our choices to put user's responsibility for using low ciphers.

[mhdawson]

How about if we rename -enable-small-dh-key to -enable-small-keys. This could then be used for this case and others in the future as well as potentially the other cases you mentioned.

As was the case for the option proposed in case of the RC4 removal, we could make it take a parameter which is the version of node so that we can fall back to the same limits as any previous version. So for example:

-enable-small-keys=0.10.39

would fall back to the limits that were in place for 0.10.39

This would mean we could add further limits without affecting the API and also allow easy fallback to a set that was being used by an earlier version

[shigeki]

@mhdawson Let me confirm that the option is for tls, crypto or both module? This PR is for crypto functionality and nodejs/node#1739 is for tls. As I wrote, they have different security requirements.

[mhdawson]

@shigeki, I believe we want an option to fall back to previous behaviour for #1739 since it is a breaking change. I was thinking the option would be for both as I think we want to limit the number of different command line options if at all possible. If the issue is that we don't believe that we only need to limit the key sizes for tls in order to address logjam then maybe we should just limit for tls and cover the other cases through documentation.

[mhdawson]

@shigeki would lit make sense to try and find a time that we could talk over hangouts to come to agreement on what we think we should do ?

[shigeki]

@mhdawson Sorry, for my late reply. I agree that #25514 needs an option and that's no problem. I am going to update my PR to have the option.
For crypto modules, I think we had better to write down some caveats in the doc at first We do not recommend to use low security dh group and ecdh curve, RSA/DH with small key size and MD5/SHA-1. But there may be use cases for users who have to use them such as to handle old encrypted backup data in offline.

[mhdawson]

@shigeki. Sounds reasonable to me to do what you suggest in your last comment which I understand to be:

1. update 25514 to support the option to revert ( you are going to update PR to include command line option/env variable)
2. Document recommendations. Does it make sense for @thinred to update this PR to simply update the documentation with the the recommendations that they not use 'modp1', 'modp2' and `'modp5' ?

[thinred]

Any update on this?

[shigeki]

@thinred Very sorry, I have little time to work on this before July 23 and will be back to do it after that.

[shigeki]

Thank you for your patience. shigeki/node@a1e289d is my proposal to update the doc and please take a look at it. If it is acceptable I will submit a new PR.

[thinred]

I added two more patches on top of that: https://github.com/thinred/node/tree/c_c
Feel free to squash them, reorder or whatever.

The important change is that it should say "at least" instead of "more than".
Then, nobody will read Caveats at the end of the page, unless we link to it. This is what I do in the second commit (well, I also update the example which used modp5).

[shigeki]

@thinred Thanks for revising my English. Your change and additions are fine with me but I also would like to add one more commit of shigeki/node@fbd32d6 to remove md5 and sha1 in the example.

@mhdawson We'd better to move to the iojs issue not to miss this after convergence. How do you think about it?

[thinred]

@shigeki You forgot about this one: thinred@8d8d084 :)
... and to be honest, I mistyped my commit message. Make sure it is squashed properly.

[mhdawson]

We'll want this in 0.10.X and 0.12.X, and everything from 0.12.X should be merged into nodejs/node as part of the convergance work right ? Is what you are suggesting that once its in 0.10.X and 0.12.X that we should open another issue to get it into io.js master ?

[mhdawson]

@shigeki I assume you are planning a separate PR for pulling in nodejs/node#1739 with a command line option to revert

[shigeki]

@mhdawson I'm planning to separating against #25514 where a flag should be included. This PR becomes a doc fix and it should be applied to both iojs and nodejs. After final review in iojs and getting merged, then we can backport it to joyent/node v0.10/v0.12. I fear the description gets inconsistent.

[jasnell]

@shigeki ... what is the status on this one?

[thinred]

FYI The current solution is in the branch https://github.com/thinred/node/commits/c_c.

[MichalStaruch]

@shigeki In my opinion much better solution than --enable-small-dh-key would be adding something like *.disabledAlgorithms properties used in Java (defined in jre/lib/security/java.security file). It's a set of predefined reasonably safe settings, but it allows to either weaken or strengthen security constraints (depending on your need).

Sample entries from java.security file:
jdk.certpath.disabledAlgorithms=MD2, MD5, RSA keySize < 1024
jdk.tls.disabledAlgorithms=SSLv3, RC4, MD5withRSA, DH keySize < 768

[shigeki]

@MichalStaruch Thanks for your comment. Node do not have such a kind of external property files. IMHO, it is not a good idea to have a new parser for it. Anyway, this fix was already made in another PR of nodejs/node#3890. So I'm going to close this.

[MichalStaruch]

@shigeki It was just an example of a more generic solution, where cryptography constraints are defined by user, not hard-coded - I never said it has to be defined in file :). Similar mechanism (defaults we can override) is already present in node.js, see --tls-cipher-list.