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: nodejs#28836
tniessen · Jul 24, 2019 · b287ab9 · b287ab9
1 parent 1dc458c
commit b287ab9
Show file tree

Hide file tree

Showing 2 changed files with 41 additions and 6 deletions.
diff --git a/lib/internal/crypto/scrypt.js b/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;

diff --git a/test/parallel/test-crypto-scrypt.js b/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));
+}