Skip to content

Commit

Permalink
crypto: unify validation of checkPrime checks
Browse files Browse the repository at this point in the history
Previously, the JS layer would validate that the value of the 'checks'
option was an unsigned 32-bit integer, otherwise throwing an appropriate
error but with a slightly misleading error message. Then the C++ layer
would validate that the value was an unsigned 31-bit integer, otherwise
throwing an appropriate error, but with a different (and even less
helpful) error message.

Instead, make the JS layer aware of the 31-bit restriction so that no
validation in C++ is necessary and so that the error message always
matches the exact requirement.

PR-URL: #47165
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
  • Loading branch information
tniessen authored and RafaelGSS committed Apr 7, 2023
1 parent e33dfce commit 254a03b
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 14 deletions.
7 changes: 4 additions & 3 deletions lib/internal/crypto/random.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ const {
validateFunction,
validateInt32,
validateObject,
validateUint32,
} = require('internal/validators');

const {
Expand Down Expand Up @@ -563,7 +562,8 @@ function checkPrime(candidate, options = kEmptyObject, callback) {
checks = 0,
} = options;

validateUint32(checks, 'options.checks');
// The checks option is unsigned but must fit into a signed C int for OpenSSL.
validateInt32(checks, 'options.checks', 0);

const job = new CheckPrimeJob(kCryptoJobAsync, candidate, checks);
job.ondone = callback;
Expand Down Expand Up @@ -591,7 +591,8 @@ function checkPrimeSync(candidate, options = kEmptyObject) {
checks = 0,
} = options;

validateUint32(checks, 'options.checks');
// The checks option is unsigned but must fit into a signed C int for OpenSSL.
validateInt32(checks, 'options.checks', 0);

const job = new CheckPrimeJob(kCryptoJobSync, candidate, checks);
const { 0: err, 1: result } = job.run();
Expand Down
15 changes: 4 additions & 11 deletions src/crypto/crypto_random.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ using v8::ArrayBuffer;
using v8::BackingStore;
using v8::False;
using v8::FunctionCallbackInfo;
using v8::Int32;
using v8::Just;
using v8::Local;
using v8::Maybe;
Expand Down Expand Up @@ -185,8 +186,6 @@ Maybe<bool> CheckPrimeTraits::AdditionalConfig(
const FunctionCallbackInfo<Value>& args,
unsigned int offset,
CheckPrimeConfig* params) {
Environment* env = Environment::GetCurrent(args);

ArrayBufferOrViewContents<unsigned char> candidate(args[offset]);

params->candidate =
Expand All @@ -195,15 +194,9 @@ Maybe<bool> CheckPrimeTraits::AdditionalConfig(
candidate.size(),
nullptr));

CHECK(args[offset + 1]->IsUint32()); // Checks

const int checks = static_cast<int>(args[offset + 1].As<Uint32>()->Value());
if (checks < 0) {
THROW_ERR_OUT_OF_RANGE(env, "invalid options.checks");
return Nothing<bool>();
}

params->checks = checks;
CHECK(args[offset + 1]->IsInt32()); // Checks
params->checks = args[offset + 1].As<Int32>()->Value();
CHECK_GE(params->checks, 0);

return Just(true);
}
Expand Down
11 changes: 11 additions & 0 deletions test/parallel/test-crypto-prime.js
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,17 @@ for (const checks of ['hello', {}, []]) {
});
}

for (const checks of [-(2 ** 31), -1, 2 ** 31, 2 ** 32 - 1, 2 ** 32, 2 ** 50]) {
assert.throws(() => checkPrime(2n, { checks }, common.mustNotCall()), {
code: 'ERR_OUT_OF_RANGE',
message: /<= 2147483647/
});
assert.throws(() => checkPrimeSync(2n, { checks }), {
code: 'ERR_OUT_OF_RANGE',
message: /<= 2147483647/
});
}

assert(!checkPrimeSync(Buffer.from([0x1])));
assert(checkPrimeSync(Buffer.from([0x2])));
assert(checkPrimeSync(Buffer.from([0x3])));
Expand Down

0 comments on commit 254a03b

Please sign in to comment.