Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto: reject non-int32 values in DiffieHellman() #32739

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
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
  • Loading branch information
bnoordhuis committed Apr 10, 2020
commit c090e6d929cec9bd00d09bb8d8d040bed539e272
12 changes: 11 additions & 1 deletion lib/internal/crypto/diffiehellman.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add 0 as a third parameter here if you want to enforce the minimum in JS.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm aware but that changes the error message on inputs < 0. I added a regression test instead.


if (keyEncoding && !Buffer.isEncoding(keyEncoding) &&
keyEncoding !== 'buffer') {
genEncoding = generator;
Expand Down
18 changes: 18 additions & 0 deletions test/parallel/test-crypto-dh.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: it does throw ERR_INVALID_ARG_TYPE in case it's not a number.

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');
Expand Down