Weak Diffie-Hellman groups provided by crypto module #44539

davidben · 2022-09-06T15:48:18Z

Node exposes various IKE MODP groups. It appears the list was chosen by exporting every group provided by OpenSSL:
https://github.com/nodejs/node/blob/main/src/crypto/crypto_dh.cc#L222-L229
https://nodejs.org/api/crypto.html#class-diffiehellmangroup

However, some of these groups are too small to be used. See RFC 8247, section 2.4:

Group 5 or the 1536-bit MODP Group has been downgraded from MAY in
RFC 4307 to SHOULD NOT. It was specified earlier, but is now
considered to be vulnerable to being broken within the next few years
by a nation-state-level attack, so its security margin is considered
too narrow.

Group 2 or the 1024-bit MODP Group has been downgraded from MUST- in
RFC 4307 to SHOULD NOT. It is known to be weak against sufficiently
funded attackers using commercially available mass-computing
resources, so its security margin is considered too narrow. It is
expected in the near future to be downgraded to MUST NOT.

Group 1 or the 768-bit MODP Group was not mentioned in RFC 4307 and
so its status was MAY. It can be broken within hours using cheap
off-the-shelf hardware. It provides no security whatsoever. It has,
therefore, been downgraded to MUST NOT.

These are all exposed by Node as "modp1", "modp2", and "modp5". The documentation should reflect their status and they should be deprecated and removed, especially modp1.

mscdex · 2022-09-06T15:57:58Z

-1 to removing modp2 yet, it's still used by older SSH (2.0) implementations and in some places it's the only exchange algorithm offered.

davidben · 2022-09-06T16:58:15Z

It seems OpenSSH itself hasn't supported that since 2016:
https://www.openssh.com/txt/release-7.2

ssh(1), sshd(8): increase the minimum modulus size supported for
diffie-hellman-group-exchange to 2048 bits.

Node just calls every function they can find. I've added the other ones from RFC 3526 (although some of these are *quite* large) but, for now, skipped the 768-bit and 1024-bit ones. Those are too small. See nodejs/node#44539. I've also reordered so DH_get_rfc7919_2048 is first. In so far as we want to recommend DH at all, that's probably the one to list first. Change-Id: If101b32114cc631f80ac6696733c440e222d769a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54305 Reviewed-by: Bob Beck <bbe@google.com> Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: Bob Beck <bbe@google.com>

mscdex · 2022-09-07T00:08:04Z

It seems OpenSSH itself hasn't supported that since 2016: https://www.openssh.com/txt/release-7.2

ssh(1), sshd(8): increase the minimum modulus size supported for
diffie-hellman-group-exchange to 2048 bits.

That's for the group exchange, which is separate from the modp-based algorithms. Specifically, the modp2-based algorithm is called diffie-hellman-group1-sha1, which is still supported by OpenSSH.

bnoordhuis · 2022-09-08T09:02:46Z

I remember being mildly apprehensive when they were added back in 2012, modp1 in particular. I'm feeling vindicated now.

What is an acceptable way forward? Remove modp1 and doc-deprecate (or runtime deprecate?) the other two?

I'm sympathetic to @mscdex's concern w.r.t. ssh but if working on open source has taught me one thing, it's that users never read the documentation.

As a first, small step toward deprecating modp1, stop using it in an example that users might copy. Refs: nodejs#44539

As a first, small step toward deprecating modp1, stop using it in an example that users might copy. Refs: #44539 PR-URL: #44585 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: theanarkh <theratliter@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

These MODP groups should not be used by new applications, and existing applications should attempt to migrate to stronger groups (or different key exchange mechanisms). Some applications still rely on these particular groups, so Node.js will likely maintain support, directly or indirectly, for the foreseeable future. Refs: nodejs#44539

tniessen · 2022-09-10T21:53:59Z

It appears the list was chosen by exporting every group provided by OpenSSL:
https://github.com/nodejs/node/blob/main/src/crypto/crypto_dh.cc#L222-L229

FWIW, that's not entirely accurate. I only recently wrote that part of code to replace a large header file that previously explicitly specified all of these groups. In other words, until recently, the MODP implementation in node did not use any constants provided by OpenSSL :)

What is an acceptable way forward? Remove modp1 and doc-deprecate (or runtime deprecate?) the other two?

Let's add a documentation-only deprecation for all three groups as a first step, which is not a semver-major change and can thus land and be released quickly: #44588

bnoordhuis · 2022-09-11T06:30:43Z

Warning about or outright removing modp1 is not semver-major under the security exception. I don't expect huge ecosystem fallout, there's probably very little software that would be affected.

These MODP groups should not be used by new applications, and existing applications should attempt to migrate to stronger groups (or different key exchange mechanisms). Some applications still rely on these particular groups, so Node.js will likely maintain support, directly or indirectly, for the foreseeable future. Refs: nodejs#44539

These MODP groups should not be used by new applications, and existing applications should attempt to migrate to stronger groups (or different key exchange mechanisms). Some applications still rely on these particular groups, so Node.js will likely maintain support, directly or indirectly, for the foreseeable future. Refs: #44539 PR-URL: #44588 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

As a first, small step toward deprecating modp1, stop using it in an example that users might copy. Refs: nodejs#44539 PR-URL: nodejs#44585 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: theanarkh <theratliter@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

These MODP groups should not be used by new applications, and existing applications should attempt to migrate to stronger groups (or different key exchange mechanisms). Some applications still rely on these particular groups, so Node.js will likely maintain support, directly or indirectly, for the foreseeable future. Refs: nodejs#44539 PR-URL: nodejs#44588 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>