crypto: unify validation of checkPrime checks

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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
nodejs · Apr 7, 2023 · 254a03b · 254a03b
1 parent e33dfce
commit 254a03b
Show file tree

Hide file tree

Showing 3 changed files with 19 additions and 14 deletions.
diff --git a/lib/internal/crypto/random.js b/lib/internal/crypto/random.js
@@ -52,7 +52,6 @@ const {
   validateFunction,
   validateInt32,
   validateObject,
-  validateUint32,
 } = require('internal/validators');
 
 const {
@@ -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;
@@ -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();

diff --git a/src/crypto/crypto_random.cc b/src/crypto/crypto_random.cc
@@ -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;
@@ -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 =
@@ -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);
 }

diff --git a/test/parallel/test-crypto-prime.js b/test/parallel/test-crypto-prime.js
@@ -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])));