From 499533f72a2dce111d6fde9c21b90b51fff35ab6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= <tniessen@tnie.de>
Date: Tue, 23 Jul 2019 15:12:32 +0200
Subject: [PATCH] crypto: fix handling of malicious getters (scrypt)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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: https://github.com/nodejs/node/issues/28836

PR-URL: https://github.com/nodejs/node/pull/28838
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
---
 lib/internal/crypto/scrypt.js       | 12 +++++-----
 test/parallel/test-crypto-scrypt.js | 35 +++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/lib/internal/crypto/scrypt.js b/lib/internal/crypto/scrypt.js
index 2705611832f03a..e2751f8fa58261 100644
--- 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
index 63ddcdfb42af28..b5d6cbb4a8d4a6 100644
--- 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));
+}