Remove (or discourage) small DH groups in crypto.getDiffieHellman

[thinred]

Hi,
the set of predefined RFC DH groups offered by getDiffieHellman should be reconsidered. Due to the new Logjam revelations (https://weakdh.org/sysadmin.html, search for OpenSSH), groups of preset prime and of size smaller than 2048 bits should be considered easily breakable (well, it's not Logjam that showed it, but only put the problem into the public's attention).

According to https://tools.ietf.org/html/rfc4253#section-8.1 Oakley 2 Group is a MUST in SSH protocol implementations (there is at least one that exists: https://github.com/mscdex/ssh2-streams), so we cannot simply drop it. However we could do either:
1) drop modp1 group (which has 768 bits) and, possibly, modp5 as well (1536 bits)
2) deprecate all groups < 2048 bits in the docs
I can prepare a patch for any of these options, but please comment.

Tomasz

[thinred]

Update on the issue.
I've just read the paper https://weakdh.org/imperfect-forward-secrecy.pdf (a really nice read!). There are a few direct lessons and recommendations:
1) 768-bit fixed-prime DH groups can be efficiently cracked by running a precomputation process, which is in the range of academic computing power
2) 1024-bit fixed-prime DH groups can be already broken by some agencies (you know who I'm talking about here...); this group is used in SSH, however
3) the authors recommend to use fixed-prime groups of at least 2048 bit

Therefore I propose to: (a) deprecate everything below 2048 bits (i.e., modp1, modp2, modp5) and remove it in a future according to deprecation policy of nodejs (which I don't know); since abb0702 these groups can be constructed manually; (b) mention this plainly in the docs

I'm preparing a patch as we speak.

[dnakamura]

I might even go further and discourage use of getDiffieHellman in favor of createDiffieHellman

[mhdawson]

Setting P1 as we should consider for the next release @jasnell and @misterdjules I know you had already started to think about what we should do for logjam.

[thinred]

@dnakamura I don't have opinion on that. Both approaches will work and there are not that many users of getDiffieHellman anyway. However, getDiffieHellman gives people an easy-to-use blackbox to do DH.

[dnakamura]

@thinred I'm not suggesting completely deprecating the function, but maybe just adding a line to the docs, something along the lines of
"Using precomputed Diffie-Hellman potentially decreases security, consider using createDiffieHellman instead"
Just to discourage new code from using it.

[mhdawson]

@thinred see latest discussion in #25509. Do you still have time to put together a patch along those lines ?

[thinred]

Yes, assuming the patch is for crypto.getDiffieHellman (and not to everything mentioned there). It would be good if somebody would make a common interface to this command line/env. variable interface. I don't claim to know very well the source code of node...

[mhdawson]

Yes I just had in mind the parts related to crypto.getDiffieHellman and the command line/env process can be separate you'll just need to use the result to control whether modp1 is enabled or not

[thinred]

Seems easy, with pleasure. :)

[misterdjules]

@joyent/node-collaborators Moved to 0.12.6 milestone according to #25509 (comment), but please feel free to suggest otherwise if you do not agree with that comment.

[thinred]

No problem with moving it. I shall work on the patch tonight, anyway, so may be it will manage to hit 0.12.5 in the end :).

[thinred]

I rebased my preliminary fix in #25372.
It requires the code that handles --enable-small-dh-groups, but that is trivial to merge.
However, I'm not sure how I'm supposed to test the code - are tests executed with or without ENABLE_SMALL_DH_GROUPS?

[mhdawson]

We should run tests both with/without the env variable/command line set. 67d9a56 and related patches are an example of doing that. It basically launches a new Node process setting the command line or environment variable to run the required variants

[thinred]

A new set of patches in #25372. This only needs the ENABLE_SMALL_DH_GROUPS patch (and rebase/merge of my topmost patch).
(btw. a function like node_output in my patch could be put in some shared place, it doesn't seem right to reimplement it every time)

[thinred]

@mhdawson I pushed a new squashed fix in #25372 which is on top of the current master. Please review.