From c090e6d929cec9bd00d09bb8d8d040bed539e272 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 10 Apr 2020 12:42:22 +0200 Subject: [PATCH 1/3] 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: https://github.com/nodejs/node/issues/32738 --- lib/internal/crypto/diffiehellman.js | 12 +++++++++++- test/parallel/test-crypto-dh.js | 18 ++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/lib/internal/crypto/diffiehellman.js b/lib/internal/crypto/diffiehellman.js index ae6b68b73b5767..70e4100d500ce9 100644 --- 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 index baa7402a88fdb6..f5eddb8c88244b 100644 --- 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'); From a3d413a4c170799d45ca51c0267dd003d9e0469d Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 10 Apr 2020 12:42:22 +0200 Subject: [PATCH 2/3] crypto: generator must be int32 in DiffieHellman() Validate the generator argument in `crypto.createDiffieHellman(key, g)`. When it's a number, it should be an int32. Fixes: https://github.com/nodejs/node/issues/32748 --- lib/internal/crypto/diffiehellman.js | 4 +++- test/parallel/test-crypto-dh.js | 7 +++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/internal/crypto/diffiehellman.js b/lib/internal/crypto/diffiehellman.js index 70e4100d500ce9..8f86911757fe1f 100644 --- a/lib/internal/crypto/diffiehellman.js +++ b/lib/internal/crypto/diffiehellman.js @@ -77,7 +77,9 @@ function DiffieHellman(sizeOrKey, keyEncoding, generator, genEncoding) { if (!generator) generator = DH_GENERATOR; - else if (typeof generator !== 'number') + else if (typeof generator === 'number') + validateInt32(generator, 'generator'); + else generator = toBuf(generator, genEncoding); this[kHandle] = new _DiffieHellman(sizeOrKey, generator); diff --git a/test/parallel/test-crypto-dh.js b/test/parallel/test-crypto-dh.js index f5eddb8c88244b..e1c5db968329f3 100644 --- a/test/parallel/test-crypto-dh.js +++ b/test/parallel/test-crypto-dh.js @@ -30,6 +30,13 @@ assert.throws(() => crypto.createDiffieHellman(13.37), { 'It must be an integer. Received 13.37', }); +assert.throws(() => crypto.createDiffieHellman('abcdef', 13.37), { + code: 'ERR_OUT_OF_RANGE', + name: 'RangeError', + message: 'The value of "generator" 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', From 80ee6c090120cc97dbaa5124f09142f92fb0388a Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 10 Apr 2020 12:42:22 +0200 Subject: [PATCH 3/3] crypto: check DiffieHellman p and g params It's possible to pass in the prime and generator params as buffers but that mode of input wasn't as rigorously checked as numeric input. --- src/node_crypto.cc | 25 +++++++++++++++++++++++-- test/parallel/test-crypto-dh.js | 31 +++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 348d407f0eb13a..34b19407c93150 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -5136,6 +5136,14 @@ bool DiffieHellman::Init(int primeLength, int g) { bool DiffieHellman::Init(const char* p, int p_len, int g) { dh_.reset(DH_new()); + if (p_len <= 0) { + BNerr(BN_F_BN_GENERATE_PRIME_EX, BN_R_BITS_TOO_SMALL); + return false; + } + if (g <= 1) { + DHerr(DH_F_DH_BUILTIN_GENPARAMS, DH_R_BAD_GENERATOR); + return false; + } BIGNUM* bn_p = BN_bin2bn(reinterpret_cast(p), p_len, nullptr); BIGNUM* bn_g = BN_new(); @@ -5151,10 +5159,23 @@ bool DiffieHellman::Init(const char* p, int p_len, int g) { bool DiffieHellman::Init(const char* p, int p_len, const char* g, int g_len) { dh_.reset(DH_new()); - BIGNUM* bn_p = - BN_bin2bn(reinterpret_cast(p), p_len, nullptr); + if (p_len <= 0) { + BNerr(BN_F_BN_GENERATE_PRIME_EX, BN_R_BITS_TOO_SMALL); + return false; + } + if (g_len <= 0) { + DHerr(DH_F_DH_BUILTIN_GENPARAMS, DH_R_BAD_GENERATOR); + return false; + } BIGNUM* bn_g = BN_bin2bn(reinterpret_cast(g), g_len, nullptr); + if (BN_is_zero(bn_g) || BN_is_one(bn_g)) { + BN_free(bn_g); + DHerr(DH_F_DH_BUILTIN_GENPARAMS, DH_R_BAD_GENERATOR); + return false; + } + BIGNUM* bn_p = + BN_bin2bn(reinterpret_cast(p), p_len, nullptr); if (!DH_set0_pqg(dh_.get(), bn_p, nullptr, bn_g)) { BN_free(bn_p); BN_free(bn_g); diff --git a/test/parallel/test-crypto-dh.js b/test/parallel/test-crypto-dh.js index e1c5db968329f3..b23a51c491a9b5 100644 --- a/test/parallel/test-crypto-dh.js +++ b/test/parallel/test-crypto-dh.js @@ -45,6 +45,37 @@ for (const bits of [-1, 0, 1]) { }); } +// Through a fluke of history, g=0 defaults to DH_GENERATOR (2). +{ + const g = 0; + crypto.createDiffieHellman('abcdef', g); + crypto.createDiffieHellman('abcdef', 'hex', g); +} + +for (const g of [-1, 1]) { + const ex = { + code: 'ERR_OSSL_DH_BAD_GENERATOR', + name: 'Error', + message: /bad generator/, + }; + assert.throws(() => crypto.createDiffieHellman('abcdef', g), ex); + assert.throws(() => crypto.createDiffieHellman('abcdef', 'hex', g), ex); +} + +crypto.createDiffieHellman('abcdef', Buffer.from([2])); // OK + +for (const g of [Buffer.from([]), + Buffer.from([0]), + Buffer.from([1])]) { + const ex = { + code: 'ERR_OSSL_DH_BAD_GENERATOR', + name: 'Error', + message: /bad generator/, + }; + assert.throws(() => crypto.createDiffieHellman('abcdef', g), ex); + assert.throws(() => crypto.createDiffieHellman('abcdef', 'hex', g), ex); +} + { const DiffieHellman = crypto.DiffieHellman; const dh = DiffieHellman(p1, 'buffer');