crypto: fix handling of malicious getters (scrypt)

It is possible to bypass parameter validation in crypto.scrypt and
crypto.scryptSync by crafting option objects with malicious getters as
demonstrated in the regression test. After bypassing validation, any
value can be passed to the C++ layer, causing an assertion to crash
the process.

Fixes: #28836

PR-URL: #28838
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>

Author: tniessen

lib/internal/crypto/scrypt.js

@@ -80,31 +80,31 @@ function check(password, salt, keylen, options) {
   if (options && options !== defaults) {
     let has_N, has_r, has_p;
     if (has_N = (options.N !== undefined)) {
-      validateUint32(options.N, 'N');
       N = options.N;
+      validateUint32(N, 'N');
     }
     if (options.cost !== undefined) {
       if (has_N) throw new ERR_CRYPTO_SCRYPT_INVALID_PARAMETER();
-      validateUint32(options.cost, 'cost');
       N = options.cost;
+      validateUint32(N, 'cost');
     }
     if (has_r = (options.r !== undefined)) {
-      validateUint32(options.r, 'r');
       r = options.r;
+      validateUint32(r, 'r');
     }
     if (options.blockSize !== undefined) {
       if (has_r) throw new ERR_CRYPTO_SCRYPT_INVALID_PARAMETER();
-      validateUint32(options.blockSize, 'blockSize');
       r = options.blockSize;
+      validateUint32(r, 'blockSize');
     }
     if (has_p = (options.p !== undefined)) {
-      validateUint32(options.p, 'p');
       p = options.p;
+      validateUint32(p, 'p');
     }
     if (options.parallelization !== undefined) {
       if (has_p) throw new ERR_CRYPTO_SCRYPT_INVALID_PARAMETER();
-      validateUint32(options.parallelization, 'parallelization');
       p = options.parallelization;
+      validateUint32(p, 'parallelization');
     }
     if (options.maxmem !== undefined) {
       maxmem = options.maxmem;

test/parallel/test-crypto-scrypt.js

@@ -235,3 +235,38 @@ for (const { args, expected } of badargs) {
     code: 'ERR_OUT_OF_RANGE'
   });
 }
+
+{
+  // Regression test for https://github.com/nodejs/node/issues/28836.
+
+  function testParameter(name, value) {
+    let accessCount = 0;
+
+    // Find out how often the value is accessed.
+    crypto.scryptSync('', '', 1, {
+      get [name]() {
+        accessCount++;
+        return value;
+      }
+    });
+
+    // Try to crash the process on the last access.
+    common.expectsError(() => {
+      crypto.scryptSync('', '', 1, {
+        get [name]() {
+          if (--accessCount === 0)
+            return '';
+          return value;
+        }
+      });
+    }, {
+      code: 'ERR_INVALID_ARG_TYPE'
+    });
+  }
+
+  [
+    ['N', 16384], ['cost', 16384],
+    ['r', 8], ['blockSize', 8],
+    ['p', 1], ['parallelization', 1]
+  ].forEach((arg) => testParameter(...arg));
+}