doc,test: clarify behavior of DH generateKeys

The DiffieHellman class is an old and thin wrapper around certain
OpenSSL functions, many of which are deprecated in OpenSSL 3.0. Because
the Node.js API mirrors the OpenSSL API, it adopts some of its
peculiarities, but the Node.js documentation does not properly reflect
these. Most importantly, despite the documentation saying otherwise,
diffieHellman.generateKeys() does not generate a new private key when
one has already been set or generated. Based on the documentation alone,
users may be led to misuse the API in a way that results in key reuse,
which can have drastic negative consequences for subsequent operations
that consume the shared secret.

These design issues in this old API have been around for many years, and
we are not currently aware of any misuse in the ecosystem that falls
into the above scenario. Changing the behavior of the API would be a
significant breaking change and is thus not appropriate for a security
release (nor is it a goal.) The reported issue is treated as CWE-1068
(after a vast amount of uncertainty whether to treat it as a
vulnerability at all), therefore, this change only updates the
documentation to match the actual behavior. Tests are also added that
demonstrate this particular oddity.

Newer APIs exist that can be used for some, but not all, Diffie-Hellman
operations (e.g., crypto.diffieHellman() that was added in 2020). We
should keep modernizing crypto APIs, but that is a non-goal for this
security release.

The ECDH class mirrors the DiffieHellman class in many ways, but it does
not appear to be affected by this particular peculiarity. In particular,
ecdh.generateKeys() does appear to always generate a new private key.

PR-URL: nodejs-private/node-private#426
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
CVE-ID: CVE-2023-30590

Author: tniessen

doc/api/crypto.md

@@ -1060,12 +1060,17 @@ added: v0.5.0
 * `encoding` {string} The [encoding][] of the return value.
 * Returns: {Buffer | string}
 
-Generates private and public Diffie-Hellman key values, and returns
+Generates private and public Diffie-Hellman key values unless they have been
+generated or computed already, and returns
 the public key in the specified `encoding`. This key should be
 transferred to the other party.
 If `encoding` is provided a string is returned; otherwise a
 [`Buffer`][] is returned.
 
+This function is a thin wrapper around [`DH_generate_key()`][]. In particular,
+once a private key has been generated or set, calling this function only updates
+the public key but does not generate a new private key.
+
 ### `diffieHellman.getGenerator([encoding])`
 
 <!-- YAML
@@ -1132,6 +1137,10 @@ Sets the Diffie-Hellman private key. If the `encoding` argument is provided,
 to be a string. If no `encoding` is provided, `privateKey` is expected
 to be a [`Buffer`][], `TypedArray`, or `DataView`.
 
+This function does not automatically compute the associated public key. Either
+[`diffieHellman.setPublicKey()`][] or [`diffieHellman.generateKeys()`][] can be
+used to manually provide the public key or to automatically derive it.
+
 ### `diffieHellman.setPublicKey(publicKey[, encoding])`
 
 <!-- YAML
@@ -6056,6 +6065,7 @@ See the [list of SSL OP Flags][] for details.
 [Web Crypto API documentation]: webcrypto.md
 [`BN_is_prime_ex`]: https://www.openssl.org/docs/man1.1.1/man3/BN_is_prime_ex.html
 [`Buffer`]: buffer.md
+[`DH_generate_key()`]: https://www.openssl.org/docs/man3.0/man3/DH_generate_key.html
 [`DiffieHellmanGroup`]: #class-diffiehellmangroup
 [`EVP_BytesToKey`]: https://www.openssl.org/docs/man1.1.0/crypto/EVP_BytesToKey.html
 [`KeyObject`]: #class-keyobject
@@ -6092,6 +6102,7 @@ See the [list of SSL OP Flags][] for details.
 [`crypto.webcrypto.subtle`]: webcrypto.md#class-subtlecrypto
 [`decipher.final()`]: #decipherfinaloutputencoding
 [`decipher.update()`]: #decipherupdatedata-inputencoding-outputencoding
+[`diffieHellman.generateKeys()`]: #diffiehellmangeneratekeysencoding
 [`diffieHellman.setPublicKey()`]: #diffiehellmansetpublickeypublickey-encoding
 [`ecdh.generateKeys()`]: #ecdhgeneratekeysencoding-format
 [`ecdh.setPrivateKey()`]: #ecdhsetprivatekeyprivatekey-encoding

test/parallel/test-crypto-dh.js

@@ -202,3 +202,56 @@ assert.throws(
   () => crypto.createDiffieHellman('', 'base64', generator),
   { code: 'ERR_INVALID_ARG_TYPE' }
 ));
+
+{
+  function unlessInvalidState(f) {
+    try {
+      return f();
+    } catch (err) {
+      if (err.code !== 'ERR_CRYPTO_INVALID_STATE') {
+        throw err;
+      }
+    }
+  }
+
+  function testGenerateKeysChangesKeys(setup, expected) {
+    const dh = crypto.createDiffieHellman(size);
+    setup(dh);
+    const firstPublicKey = unlessInvalidState(() => dh.getPublicKey());
+    const firstPrivateKey = unlessInvalidState(() => dh.getPrivateKey());
+    dh.generateKeys();
+    const secondPublicKey = dh.getPublicKey();
+    const secondPrivateKey = dh.getPrivateKey();
+    function changed(shouldChange, first, second) {
+      if (shouldChange) {
+        assert.notDeepStrictEqual(first, second);
+      } else {
+        assert.deepStrictEqual(first, second);
+      }
+    }
+    changed(expected.includes('public'), firstPublicKey, secondPublicKey);
+    changed(expected.includes('private'), firstPrivateKey, secondPrivateKey);
+  }
+
+  // Both the private and the public key are missing: generateKeys() generates both.
+  testGenerateKeysChangesKeys(() => {
+    // No setup.
+  }, ['public', 'private']);
+
+  // Neither key is missing: generateKeys() does nothing.
+  testGenerateKeysChangesKeys((dh) => {
+    dh.generateKeys();
+  }, []);
+
+  // Only the public key is missing: generateKeys() generates only the public key.
+  testGenerateKeysChangesKeys((dh) => {
+    dh.setPrivateKey(Buffer.from('01020304', 'hex'));
+  }, ['public']);
+
+  // The public key is outdated: generateKeys() generates only the public key.
+  testGenerateKeysChangesKeys((dh) => {
+    const oldPublicKey = dh.generateKeys();
+    dh.setPrivateKey(Buffer.from('01020304', 'hex'));
+    assert.deepStrictEqual(dh.getPublicKey(), oldPublicKey);
+  }, ['public']);
+}