crypto: key size must be int32 in DiffieHellman()

The JS code accepted any value where `typeof sizeOrKey === 'number'` was true but the C++ code checked that `args[0]->IsInt32()` and subsequently aborted. Fixes: #32738 PR-URL: #32739 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
nodejs · May 7, 2020 · adfa469 · adfa469
1 parent eba1772
commit adfa469
Show file tree

Hide file tree

Showing 2 changed files with 29 additions and 1 deletion.
diff --git a/lib/internal/crypto/diffiehellman.js b/lib/internal/crypto/diffiehellman.js
@@ -14,7 +14,10 @@ const {
   ERR_INVALID_ARG_TYPE,
   ERR_INVALID_OPT_VALUE
 } = require('internal/errors').codes;
-const { validateString } = require('internal/validators');
+const {
+  validateString,
+  validateInt32,
+} = require('internal/validators');
 const { isArrayBufferView } = require('internal/util/types');
 const { KeyObject } = require('internal/crypto/keys');
 const {
@@ -51,6 +54,13 @@ function DiffieHellman(sizeOrKey, keyEncoding, generator, genEncoding) {
     );
   }
 
+  // Sizes < 0 don't make sense but they _are_ accepted (and subsequently
+  // rejected with ERR_OSSL_BN_BITS_TOO_SMALL) by OpenSSL. The glue code
+  // in node_crypto.cc accepts values that are IsInt32() for that reason
+  // and that's why we do that here too.
+  if (typeof sizeOrKey === 'number')
+    validateInt32(sizeOrKey, 'sizeOrKey');
+
   if (keyEncoding && !Buffer.isEncoding(keyEncoding) &&
       keyEncoding !== 'buffer') {
     genEncoding = generator;

diff --git a/test/parallel/test-crypto-dh.js b/test/parallel/test-crypto-dh.js
@@ -20,6 +20,24 @@ assert.strictEqual(secret2.toString('base64'), secret1);
 assert.strictEqual(dh1.verifyError, 0);
 assert.strictEqual(dh2.verifyError, 0);
 
+// https://github.com/nodejs/node/issues/32738
+// XXX(bnoordhuis) validateInt32() throwing ERR_OUT_OF_RANGE and RangeError
+// instead of ERR_INVALID_ARG_TYPE and TypeError is questionable, IMO.
+assert.throws(() => crypto.createDiffieHellman(13.37), {
+  code: 'ERR_OUT_OF_RANGE',
+  name: 'RangeError',
+  message: 'The value of "sizeOrKey" is out of range. ' +
+           'It must be an integer. Received 13.37',
+});
+
+for (const bits of [-1, 0, 1]) {
+  assert.throws(() => crypto.createDiffieHellman(bits), {
+    code: 'ERR_OSSL_BN_BITS_TOO_SMALL',
+    name: 'Error',
+    message: /bits too small/,
+  });
+}
+
 {
   const DiffieHellman = crypto.DiffieHellman;
   const dh = DiffieHellman(p1, 'buffer');