From 32c527d80f74bb269c48c4fe693f8368ebdeee4b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= <tniessen@tnie.de>
Date: Sun, 12 Mar 2023 19:35:55 +0100
Subject: [PATCH] tls: support automatic DHE

Node.js has so far only supported user-defined DHE parameters and even
recommended generating custom parameters. This change lets users set the
dhparam option to 'auto' instead, in which case DHE parameters of
sufficient strength are selected automatically (from a small set of
well-known parameters). This has been recommended by OpenSSL for quite a
while, and it makes it much easier for Node.js TLS servers to properly
support DHE-based perfect forward secrecy.

This also updates the documentation to prioritize ECDHE over DHE, mostly
because the former tends to be more efficient and is enabled by default.

PR-URL: https://github.com/nodejs/node/pull/46978
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
---
 doc/api/tls.md                                | 42 ++++++++--------
 lib/internal/tls/secure-context.js            |  2 +-
 src/crypto/crypto_context.cc                  |  9 ++++
 .../test-tls-client-getephemeralkeyinfo.js    | 17 ++++++-
 test/parallel/test-tls-dhe.js                 | 50 +++++++++++++++----
 5 files changed, 87 insertions(+), 33 deletions(-)

diff --git a/doc/api/tls.md b/doc/api/tls.md
index 34d3a9a96da672..a2a6279e0e374d 100644
--- a/doc/api/tls.md
+++ b/doc/api/tls.md
@@ -123,23 +123,17 @@ all sessions). Methods implementing this technique are called "ephemeral".
 Currently two methods are commonly used to achieve perfect forward secrecy (note
 the character "E" appended to the traditional abbreviations):
 
-* [DHE][]: An ephemeral version of the Diffie-Hellman key-agreement protocol.
 * [ECDHE][]: An ephemeral version of the Elliptic Curve Diffie-Hellman
   key-agreement protocol.
+* [DHE][]: An ephemeral version of the Diffie-Hellman key-agreement protocol.
 
-To use perfect forward secrecy using `DHE` with the `node:tls` module, it is
-required to generate Diffie-Hellman parameters and specify them with the
-`dhparam` option to [`tls.createSecureContext()`][]. The following illustrates
-the use of the OpenSSL command-line interface to generate such parameters:
-
-```bash
-openssl dhparam -outform PEM -out dhparam.pem 2048
-```
+Perfect forward secrecy using ECDHE is enabled by default. The `ecdhCurve`
+option can be used when creating a TLS server to customize the list of supported
+ECDH curves to use. See [`tls.createServer()`][] for more info.
 
-If using perfect forward secrecy using `ECDHE`, Diffie-Hellman parameters are
-not required and a default ECDHE curve will be used. The `ecdhCurve` property
-can be used when creating a TLS Server to specify the list of names of supported
-curves to use, see [`tls.createServer()`][] for more info.
+DHE is disabled by default but can be enabled alongside ECDHE by setting the
+`dhparam` option to `'auto'`. Custom DHE parameters are also supported but
+discouraged in favor of automatically selected, well-known parameters.
 
 Perfect forward secrecy was optional up to TLSv1.2. As of TLSv1.3, (EC)DHE is
 always used (with the exception of PSK-only connections).
@@ -1796,6 +1790,10 @@ argument.
 <!-- YAML
 added: v0.11.13
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/46978
+    description: The `dhparam` option can now be set to `'auto'` to
+                 enable DHE with appropriate well-known parameters.
   - version: v12.12.0
     pr-url: https://github.com/nodejs/node/pull/28973
     description: Added `privateKeyIdentifier` and `privateKeyEngine` options
@@ -1880,13 +1878,10 @@ changes:
     client certificate.
   * `crl` {string|string\[]|Buffer|Buffer\[]} PEM formatted CRLs (Certificate
     Revocation Lists).
-  * `dhparam` {string|Buffer} Diffie-Hellman parameters, required for non-ECDHE
-    [perfect forward secrecy][]. Use `openssl dhparam` to create the parameters.
-    The key length must be greater than or equal to 1024 bits or else an error
-    will be thrown. Although 1024 bits is permissible, use 2048 bits or larger
-    for stronger security. If omitted or invalid, the parameters are silently
-    discarded and DHE ciphers will not be available. [ECDHE][]-based [perfect
-    forward secrecy][] will still be available.
+  * `dhparam` {string|Buffer} `'auto'` or custom Diffie-Hellman parameters,
+    required for non-ECDHE [perfect forward secrecy][]. If omitted or invalid,
+    the parameters are silently discarded and DHE ciphers will not be available.
+    [ECDHE][]-based [perfect forward secrecy][] will still be available.
   * `ecdhCurve` {string} A string describing a named curve or a colon separated
     list of curve NIDs or names, for example `P-521:P-384:P-256`, to use for
     ECDH key agreement. Set to `auto` to select the
@@ -1973,6 +1968,13 @@ A key is _required_ for ciphers that use certificates. Either `key` or
 If the `ca` option is not given, then Node.js will default to using
 [Mozilla's publicly trusted list of CAs][].
 
+Custom DHE parameters are discouraged in favor of the new `dhparam: 'auto'`
+option. When set to `'auto'`, well-known DHE parameters of sufficient strength
+will be selected automatically. Otherwise, if necessary, `openssl dhparam` can
+be used to create custom parameters. The key length must be greater than or
+equal to 1024 bits or else an error will be thrown. Although 1024 bits is
+permissible, use 2048 bits or larger for stronger security.
+
 ## `tls.createSecurePair([context][, isServer][, requestCert][, rejectUnauthorized][, options])`
 
 <!-- YAML
diff --git a/lib/internal/tls/secure-context.js b/lib/internal/tls/secure-context.js
index d91df0e2a03846..9b3f8ccce0ce8b 100644
--- a/lib/internal/tls/secure-context.js
+++ b/lib/internal/tls/secure-context.js
@@ -240,7 +240,7 @@ function configSecureContext(context, options = kEmptyObject, name = 'options')
 
   if (dhparam !== undefined && dhparam !== null) {
     validateKeyOrCertOption(`${name}.dhparam`, dhparam);
-    const warning = context.setDHParam(dhparam);
+    const warning = context.setDHParam(dhparam === 'auto' || dhparam);
     if (warning)
       process.emitWarning(warning, 'SecurityWarning');
   }
diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc
index 50036c6466131b..b9f323035ff543 100644
--- a/src/crypto/crypto_context.cc
+++ b/src/crypto/crypto_context.cc
@@ -854,6 +854,14 @@ void SecureContext::SetDHParam(const FunctionCallbackInfo<Value>& args) {
 
   CHECK_GE(args.Length(), 1);  // DH argument is mandatory
 
+  // If the user specified "auto" for dhparams, the JavaScript layer will pass
+  // true to this function instead of the original string. Any other string
+  // value will be interpreted as custom DH parameters below.
+  if (args[0]->IsTrue()) {
+    CHECK(SSL_CTX_set_dh_auto(sc->ctx_.get(), true));
+    return;
+  }
+
   DHPointer dh;
   {
     BIOPointer bio(LoadBIO(env, args[0]));
@@ -864,6 +872,7 @@ void SecureContext::SetDHParam(const FunctionCallbackInfo<Value>& args) {
   }
 
   // Invalid dhparam is silently discarded and DHE is no longer used.
+  // TODO(tniessen): don't silently discard invalid dhparam.
   if (!dh)
     return;
 
diff --git a/test/parallel/test-tls-client-getephemeralkeyinfo.js b/test/parallel/test-tls-client-getephemeralkeyinfo.js
index 8abb319b828868..82ed1e27f49e6c 100644
--- a/test/parallel/test-tls-client-getephemeralkeyinfo.js
+++ b/test/parallel/test-tls-client-getephemeralkeyinfo.js
@@ -5,6 +5,7 @@ if (!common.hasCrypto)
 const fixtures = require('../common/fixtures');
 
 const assert = require('assert');
+const { X509Certificate } = require('crypto');
 const tls = require('tls');
 
 const key = fixtures.readKey('agent2-key.pem');
@@ -29,7 +30,20 @@ function test(size, type, name, cipher) {
 
   if (name) options.ecdhCurve = name;
 
-  if (type === 'DH') options.dhparam = loadDHParam(size);
+  if (type === 'DH') {
+    if (size === 'auto') {
+      options.dhparam = 'auto';
+      // The DHE parameters selected by OpenSSL depend on the strength of the
+      // certificate's key. For this test, we can assume that the modulus length
+      // of the certificate's key is equal to the size of the DHE parameter, but
+      // that is really only true for a few modulus lengths.
+      ({
+        publicKey: { asymmetricKeyDetails: { modulusLength: size } }
+      } = new X509Certificate(cert));
+    } else {
+      options.dhparam = loadDHParam(size);
+    }
+  }
 
   const server = tls.createServer(options, common.mustCall((conn) => {
     assert.strictEqual(conn.getEphemeralKeyInfo(), null);
@@ -54,6 +68,7 @@ function test(size, type, name, cipher) {
 }
 
 test(undefined, undefined, undefined, 'AES128-SHA256');
+test('auto', 'DH', undefined, 'DHE-RSA-AES128-GCM-SHA256');
 test(1024, 'DH', undefined, 'DHE-RSA-AES128-GCM-SHA256');
 test(2048, 'DH', undefined, 'DHE-RSA-AES128-GCM-SHA256');
 test(256, 'ECDH', 'prime256v1', 'ECDHE-RSA-AES128-GCM-SHA256');
diff --git a/test/parallel/test-tls-dhe.js b/test/parallel/test-tls-dhe.js
index d0dbfd77258e5f..a5cdc2d35ecbb7 100644
--- a/test/parallel/test-tls-dhe.js
+++ b/test/parallel/test-tls-dhe.js
@@ -29,6 +29,7 @@ if (!common.opensslCli)
   common.skip('missing openssl-cli');
 
 const assert = require('assert');
+const { X509Certificate } = require('crypto');
 const { once } = require('events');
 const tls = require('tls');
 const { execFile } = require('child_process');
@@ -36,7 +37,11 @@ const fixtures = require('../common/fixtures');
 
 const key = fixtures.readKey('agent2-key.pem');
 const cert = fixtures.readKey('agent2-cert.pem');
-const ciphers = 'DHE-RSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256';
+
+// Prefer DHE over ECDHE when possible.
+const dheCipher = 'DHE-RSA-AES128-SHA256';
+const ecdheCipher = 'ECDHE-RSA-AES128-SHA256';
+const ciphers = `${dheCipher}:${ecdheCipher}`;
 
 // Test will emit a warning because the DH parameter size is < 2048 bits
 common.expectWarning('SecurityWarning',
@@ -47,12 +52,12 @@ function loadDHParam(n) {
   return fixtures.readKey(keyname);
 }
 
-function test(keylen, expectedCipher) {
+function test(dhparam, keylen, expectedCipher) {
   const options = {
-    key: key,
-    cert: cert,
-    ciphers: ciphers,
-    dhparam: loadDHParam(keylen),
+    key,
+    cert,
+    ciphers,
+    dhparam,
     maxVersion: 'TLSv1.2',
   };
 
@@ -63,7 +68,7 @@ function test(keylen, expectedCipher) {
                   '-cipher', ciphers];
 
     execFile(common.opensslCli, args, common.mustSucceed((stdout) => {
-      assert(keylen === 'error' ||
+      assert(keylen === null ||
              stdout.includes(`Server Temp Key: DH, ${keylen} bits`));
       assert(stdout.includes(`Cipher    : ${expectedCipher}`));
       server.close();
@@ -73,12 +78,35 @@ function test(keylen, expectedCipher) {
   return once(server, 'close');
 }
 
+function testCustomParam(keylen, expectedCipher) {
+  const dhparam = loadDHParam(keylen);
+  if (keylen === 'error') keylen = null;
+  return test(dhparam, keylen, expectedCipher);
+}
+
 (async () => {
+  // By default, DHE is disabled while ECDHE is enabled.
+  for (const dhparam of [undefined, null]) {
+    await test(dhparam, null, ecdheCipher);
+  }
+
+  // The DHE parameters selected by OpenSSL depend on the strength of the
+  // certificate's key. For this test, we can assume that the modulus length
+  // of the certificate's key is equal to the size of the DHE parameter, but
+  // that is really only true for a few modulus lengths.
+  const {
+    publicKey: { asymmetricKeyDetails: { modulusLength } }
+  } = new X509Certificate(cert);
+  await test('auto', modulusLength, dheCipher);
+
   assert.throws(() => {
-    test(512, 'DHE-RSA-AES128-SHA256');
+    testCustomParam(512);
   }, /DH parameter is less than 1024 bits/);
 
-  await test(1024, 'DHE-RSA-AES128-SHA256');
-  await test(2048, 'DHE-RSA-AES128-SHA256');
-  await test('error', 'ECDHE-RSA-AES128-SHA256');
+  // Custom DHE parameters are supported (but discouraged).
+  await testCustomParam(1024, dheCipher);
+  await testCustomParam(2048, dheCipher);
+
+  // Invalid DHE parameters are discarded. ECDHE remains enabled.
+  await testCustomParam('error', ecdheCipher);
 })().then(common.mustCall());