crypto: fix error handling

This fixes multiple cases where the wrong error was returned in
case of e.g. a overflow / wrong type.

PR-URL: https://github.com/nodejs/node/pull/19445
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

lib/internal/crypto/random.js

@@ -13,37 +13,44 @@ const {
 
 const { kMaxLength } = require('buffer');
 const kMaxUint32 = Math.pow(2, 32) - 1;
+const kMaxPossibleLength = Math.min(kMaxLength, kMaxUint32);
 
-function assertOffset(offset, length) {
+function assertOffset(offset, elementSize, length) {
-  if (typeof offset !== 'number' || Number.isNaN(offset)) {
+  if (typeof offset !== 'number') {
-    throw new ERR_INVALID_ARG_TYPE('offset', 'number');
+    throw new ERR_INVALID_ARG_TYPE('offset', 'number', offset);
   }
 
-  if (offset > kMaxUint32 || offset < 0) {
+  offset *= elementSize;
-    throw new ERR_INVALID_ARG_TYPE('offset', 'uint32');
+
+  const maxLength = Math.min(length, kMaxPossibleLength);
+  if (Number.isNaN(offset) || offset > maxLength || offset < 0) {
+    throw new ERR_OUT_OF_RANGE('offset', `>= 0 && <= ${maxLength}`, offset);
   }
 
-  if (offset > kMaxLength || offset > length) {
+  return offset;
-    throw new ERR_OUT_OF_RANGE('offset');
-  }
 }
 
-function assertSize(size, offset = 0, length = Infinity) {
+function assertSize(size, elementSize, offset, length) {
-  if (typeof size !== 'number' || Number.isNaN(size)) {
+  if (typeof size !== 'number') {
-    throw new ERR_INVALID_ARG_TYPE('size', 'number');
+    throw new ERR_INVALID_ARG_TYPE('size', 'number', size);
   }
 
-  if (size > kMaxUint32 || size < 0) {
+  size *= elementSize;
-    throw new ERR_INVALID_ARG_TYPE('size', 'uint32');
+
+  if (Number.isNaN(size) || size > kMaxPossibleLength || size < 0) {
+    throw new ERR_OUT_OF_RANGE('size',
+                               `>= 0 && <= ${kMaxPossibleLength}`, size);
   }
 
-  if (size + offset > length || size > kMaxLength) {
+  if (size + offset > length) {
-    throw new ERR_OUT_OF_RANGE('size');
+    throw new ERR_OUT_OF_RANGE('size + offset', `<= ${length}`, size + offset);
   }
+
+  return size;
 }
 
 function randomBytes(size, cb) {
-  assertSize(size);
+  assertSize(size, 1, 0, Infinity);
   if (cb !== undefined && typeof cb !== 'function')
     throw new ERR_INVALID_CALLBACK();
   return _randomBytes(size, cb);
@@ -56,17 +63,14 @@ function randomFillSync(buf, offset = 0, size) {
 
   const elementSize = buf.BYTES_PER_ELEMENT || 1;
 
-  offset *= elementSize;
+  offset = assertOffset(offset, elementSize, buf.byteLength);
-  assertOffset(offset, buf.byteLength);
 
   if (size === undefined) {
     size = buf.byteLength - offset;
   } else {
-    size *= elementSize;
+    size = assertSize(size, elementSize, offset, buf.byteLength);
   }
 
-  assertSize(size, offset, buf.byteLength);
-
   return _randomFill(buf, offset, size);
 }
 
@@ -83,20 +87,19 @@ function randomFill(buf, offset, size, cb) {
     size = buf.bytesLength;
   } else if (typeof size === 'function') {
     cb = size;
-    offset *= elementSize;
     size = buf.byteLength - offset;
   } else if (typeof cb !== 'function') {
     throw new ERR_INVALID_CALLBACK();
   }
+
+  offset = assertOffset(offset, elementSize, buf.byteLength);
+
   if (size === undefined) {
     size = buf.byteLength - offset;
   } else {
-    size *= elementSize;
+    size = assertSize(size, elementSize, offset, buf.byteLength);
   }
 
-  assertOffset(offset, buf.byteLength);
-  assertSize(size, offset, buf.byteLength);
-
   return _randomFill(buf, offset, size, cb);
 }
 

test/parallel/test-crypto-random.js

@@ -27,42 +27,49 @@ if (!common.hasCrypto)
 
 const assert = require('assert');
 const crypto = require('crypto');
+const { kMaxLength } = require('buffer');
+
+const kMaxUint32 = Math.pow(2, 32) - 1;
+const kMaxPossibleLength = Math.min(kMaxLength, kMaxUint32);
 
 crypto.DEFAULT_ENCODING = 'buffer';
 
 // bump, we register a lot of exit listeners
 process.setMaxListeners(256);
 
-[crypto.randomBytes, crypto.pseudoRandomBytes].forEach(function(f) {
+{
-  [-1, undefined, null, false, true, {}, []].forEach(function(value) {
+  [crypto.randomBytes, crypto.pseudoRandomBytes].forEach((f) => {
-
+    [undefined, null, false, true, {}, []].forEach((value) => {
-    common.expectsError(
+      const errObj = {
-      () => f(value),
-      {
         code: 'ERR_INVALID_ARG_TYPE',
-        type: TypeError,
+        name: 'TypeError [ERR_INVALID_ARG_TYPE]',
-        message: /^The "size" argument must be of type (number|uint32)$/
+        message: 'The "size" argument must be of type number. ' +
-      }
+                `Received type ${typeof value}`
-    );
+      };
-
+      assert.throws(() => f(value), errObj);
-    common.expectsError(
+      assert.throws(() => f(value, common.mustNotCall()), errObj);
-      () => f(value, common.mustNotCall()),
-      {
-        code: 'ERR_INVALID_ARG_TYPE',
-        type: TypeError,
-        message: /^The "size" argument must be of type (number|uint32)$/
-      }
-    );
     });
 
-  [0, 1, 2, 4, 16, 256, 1024, 101.2].forEach(function(len) {
+    [-1, NaN, 2 ** 32].forEach((value) => {
-    f(len, common.mustCall(function(ex, buf) {
+      const errObj = {
+        code: 'ERR_OUT_OF_RANGE',
+        name: 'RangeError [ERR_OUT_OF_RANGE]',
+        message: 'The value of "size" is out of range. It must be >= 0 && <= ' +
+                 `${kMaxPossibleLength}. Received ${value}`
+      };
+      assert.throws(() => f(value), errObj);
+      assert.throws(() => f(value, common.mustNotCall()), errObj);
+    });
+
+    [0, 1, 2, 4, 16, 256, 1024, 101.2].forEach((len) => {
+      f(len, common.mustCall((ex, buf) => {
         assert.strictEqual(ex, null);
         assert.strictEqual(buf.length, Math.floor(len));
         assert.ok(Buffer.isBuffer(buf));
       }));
     });
-});
+  });
+}
 
 {
   const buf = Buffer.alloc(10);
@@ -181,252 +188,84 @@ process.setMaxListeners(256);
 }
 
 {
-  const bufs = [
+  [
     Buffer.alloc(10),
     new Uint8Array(new Array(10).fill(0))
-  ];
+  ].forEach((buf) => {
-
-  const max = require('buffer').kMaxLength + 1;
-
-  for (const buf of bufs) {
     const len = Buffer.byteLength(buf);
     assert.strictEqual(len, 10, `Expected byteLength of 10, got ${len}`);
 
-    common.expectsError(
+    const typeErrObj = {
-      () => crypto.randomFillSync(buf, 'test'),
-      {
       code: 'ERR_INVALID_ARG_TYPE',
-        type: TypeError,
+      name: 'TypeError [ERR_INVALID_ARG_TYPE]',
-        message: 'The "offset" argument must be of type number'
+      message: 'The "offset" argument must be of type number. ' +
-      }
+               'Received type string'
-    );
+    };
 
-    common.expectsError(
+    assert.throws(() => crypto.randomFillSync(buf, 'test'), typeErrObj);
-      () => crypto.randomFillSync(buf, NaN),
-      {
-        code: 'ERR_INVALID_ARG_TYPE',
-        type: TypeError,
-        message: 'The "offset" argument must be of type number'
-      }
-    );
 
-    common.expectsError(
+    assert.throws(
       () => crypto.randomFill(buf, 'test', common.mustNotCall()),
-      {
+      typeErrObj);
-        code: 'ERR_INVALID_ARG_TYPE',
-        type: TypeError,
-        message: 'The "offset" argument must be of type number'
-      }
-    );
 
-    common.expectsError(
+    typeErrObj.message = 'The "size" argument must be of type number. ' +
-      () => crypto.randomFill(buf, NaN, common.mustNotCall()),
+                     'Received type string';
-      {
+    assert.throws(() => crypto.randomFillSync(buf, 0, 'test'), typeErrObj);
-        code: 'ERR_INVALID_ARG_TYPE',
-        type: TypeError,
-        message: 'The "offset" argument must be of type number'
-      }
-    );
 
-    common.expectsError(
+    assert.throws(
-      () => crypto.randomFillSync(buf, 11),
-      {
-        code: 'ERR_OUT_OF_RANGE',
-        type: RangeError,
-        message: 'The value of "offset" is out of range.'
-      }
-    );
-
-    common.expectsError(
-      () => crypto.randomFillSync(buf, max),
-      {
-        code: 'ERR_OUT_OF_RANGE',
-        type: RangeError,
-        message: 'The value of "offset" is out of range.'
-      }
-    );
-
-    common.expectsError(
-      () => crypto.randomFill(buf, 11, common.mustNotCall()),
-      {
-        code: 'ERR_OUT_OF_RANGE',
-        type: RangeError,
-        message: 'The value of "offset" is out of range.'
-      }
-    );
-
-    common.expectsError(
-      () => crypto.randomFill(buf, max, common.mustNotCall()),
-      {
-        code: 'ERR_OUT_OF_RANGE',
-        type: RangeError,
-        message: 'The value of "offset" is out of range.'
-      }
-    );
-
-    common.expectsError(
-      () => crypto.randomFillSync(buf, 0, 'test'),
-      {
-        code: 'ERR_INVALID_ARG_TYPE',
-        type: TypeError,
-        message: 'The "size" argument must be of type number'
-      }
-    );
-
-    common.expectsError(
-      () => crypto.randomFillSync(buf, 0, NaN),
-      {
-        code: 'ERR_INVALID_ARG_TYPE',
-        type: TypeError,
-        message: 'The "size" argument must be of type number'
-      }
-    );
-
-    common.expectsError(
       () => crypto.randomFill(buf, 0, 'test', common.mustNotCall()),
-      {
+      typeErrObj
-        code: 'ERR_INVALID_ARG_TYPE',
-        type: TypeError,
-        message: 'The "size" argument must be of type number'
-      }
     );
 
-    common.expectsError(
+    [NaN, kMaxPossibleLength + 1, -10, (-1 >>> 0) + 1].forEach((offsetSize) => {
-      () => crypto.randomFill(buf, 0, NaN, common.mustNotCall()),
+      const errObj = {
-      {
-        code: 'ERR_INVALID_ARG_TYPE',
-        type: TypeError,
-        message: 'The "size" argument must be of type number'
-      }
-    );
-
-    {
-      const size = (-1 >>> 0) + 1;
-
-      common.expectsError(
-        () => crypto.randomFillSync(buf, 0, -10),
-        {
-          code: 'ERR_INVALID_ARG_TYPE',
-          type: TypeError,
-          message: 'The "size" argument must be of type uint32'
-        }
-      );
-
-      common.expectsError(
-        () => crypto.randomFillSync(buf, 0, size),
-        {
-          code: 'ERR_INVALID_ARG_TYPE',
-          type: TypeError,
-          message: 'The "size" argument must be of type uint32'
-        }
-      );
-
-      common.expectsError(
-        () => crypto.randomFill(buf, 0, -10, common.mustNotCall()),
-        {
-          code: 'ERR_INVALID_ARG_TYPE',
-          type: TypeError,
-          message: 'The "size" argument must be of type uint32'
-        }
-      );
-
-      common.expectsError(
-        () => crypto.randomFill(buf, 0, size, common.mustNotCall()),
-        {
-          code: 'ERR_INVALID_ARG_TYPE',
-          type: TypeError,
-          message: 'The "size" argument must be of type uint32'
-        }
-      );
-    }
-
-    common.expectsError(
-      () => crypto.randomFillSync(buf, -10),
-      {
-        code: 'ERR_INVALID_ARG_TYPE',
-        type: TypeError,
-        message: 'The "offset" argument must be of type uint32'
-      }
-    );
-
-    common.expectsError(
-      () => crypto.randomFill(buf, -10, common.mustNotCall()),
-      {
-        code: 'ERR_INVALID_ARG_TYPE',
-        type: TypeError,
-        message: 'The "offset" argument must be of type uint32'
-      }
-    );
-
-    common.expectsError(
-      () => crypto.randomFillSync(buf, 1, 10),
-      {
         code: 'ERR_OUT_OF_RANGE',
-        type: RangeError,
+        name: 'RangeError [ERR_OUT_OF_RANGE]',
-        message: 'The value of "size" is out of range.'
+        message: 'The value of "offset" is out of range. ' +
-      }
+                 `It must be >= 0 && <= 10. Received ${offsetSize}`
-    );
+      };
 
-    common.expectsError(
+      assert.throws(() => crypto.randomFillSync(buf, offsetSize), errObj);
+
+      assert.throws(
+        () => crypto.randomFill(buf, offsetSize, common.mustNotCall()),
+        errObj);
+
+      errObj.message = 'The value of "size" is out of range. It must be >= ' +
+                       `0 && <= ${kMaxPossibleLength}. Received ${offsetSize}`;
+      assert.throws(() => crypto.randomFillSync(buf, 1, offsetSize), errObj);
+
+      assert.throws(
+        () => crypto.randomFill(buf, 1, offsetSize, common.mustNotCall()),
+        errObj
+      );
+    });
+
+    const rangeErrObj = {
+      code: 'ERR_OUT_OF_RANGE',
+      name: 'RangeError [ERR_OUT_OF_RANGE]',
+      message: 'The value of "size + offset" is out of range. ' +
+               'It must be <= 10. Received 11'
+    };
+    assert.throws(() => crypto.randomFillSync(buf, 1, 10), rangeErrObj);
+
+    assert.throws(
       () => crypto.randomFill(buf, 1, 10, common.mustNotCall()),
-      {
+      rangeErrObj
-        code: 'ERR_OUT_OF_RANGE',
-        type: RangeError,
-        message: 'The value of "size" is out of range.'
-      }
     );
-
+  });
-    common.expectsError(
-      () => crypto.randomFillSync(buf, 0, 12),
-      {
-        code: 'ERR_OUT_OF_RANGE',
-        type: RangeError,
-        message: 'The value of "size" is out of range.'
-      }
-    );
-
-    common.expectsError(
-      () => crypto.randomFill(buf, 0, 12, common.mustNotCall()),
-      {
-        code: 'ERR_OUT_OF_RANGE',
-        type: RangeError,
-        message: 'The value of "size" is out of range.'
-      }
-    );
-
-    {
-      // Offset is too big
-      const offset = (-1 >>> 0) + 1;
-      common.expectsError(
-        () => crypto.randomFillSync(buf, offset, 10),
-        {
-          code: 'ERR_INVALID_ARG_TYPE',
-          type: TypeError,
-          message: 'The "offset" argument must be of type uint32'
-        }
-      );
-
-      common.expectsError(
-        () => crypto.randomFill(buf, offset, 10, common.mustNotCall()),
-        {
-          code: 'ERR_INVALID_ARG_TYPE',
-          type: TypeError,
-          message: 'The "offset" argument must be of type uint32'
-        }
-      );
-    }
-  }
 }
 
 // https://github.com/nodejs/node-v0.x-archive/issues/5126,
 // "FATAL ERROR: v8::Object::SetIndexedPropertiesToExternalArrayData() length
 // exceeds max acceptable value"
-common.expectsError(
+assert.throws(
   () => crypto.randomBytes((-1 >>> 0) + 1),
   {
-    code: 'ERR_INVALID_ARG_TYPE',
+    code: 'ERR_OUT_OF_RANGE',
-    type: TypeError,
+    name: 'RangeError [ERR_OUT_OF_RANGE]',
-    message: 'The "size" argument must be of type uint32'
+    message: 'The value of "size" is out of range. ' +
+             `It must be >= 0 && <= ${kMaxPossibleLength}. Received 4294967296`
   }
 );