From 333adf61eb3d8d973b28e6c86d8b93d39d95cfe6 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 19 Mar 2018 14:13:39 +0100 Subject: [PATCH] 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 Reviewed-By: Matteo Collina Reviewed-By: Joyee Cheung Reviewed-By: Anna Henningsen --- lib/internal/crypto/random.js | 55 ++--- test/parallel/test-crypto-random.js | 325 +++++++--------------------- 2 files changed, 111 insertions(+), 269 deletions(-) diff --git a/lib/internal/crypto/random.js b/lib/internal/crypto/random.js index 95e38dcc443131..9f444cdcb7aac8 100644 --- a/lib/internal/crypto/random.js +++ b/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) { - if (typeof offset !== 'number' || Number.isNaN(offset)) { - throw new ERR_INVALID_ARG_TYPE('offset', 'number'); +function assertOffset(offset, elementSize, length) { + if (typeof offset !== 'number') { + throw new ERR_INVALID_ARG_TYPE('offset', 'number', offset); } - if (offset > kMaxUint32 || offset < 0) { - throw new ERR_INVALID_ARG_TYPE('offset', 'uint32'); - } + offset *= elementSize; - if (offset > kMaxLength || offset > length) { - throw new ERR_OUT_OF_RANGE('offset'); + const maxLength = Math.min(length, kMaxPossibleLength); + if (Number.isNaN(offset) || offset > maxLength || offset < 0) { + throw new ERR_OUT_OF_RANGE('offset', `>= 0 && <= ${maxLength}`, offset); } + + return offset; } -function assertSize(size, offset = 0, length = Infinity) { - if (typeof size !== 'number' || Number.isNaN(size)) { - throw new ERR_INVALID_ARG_TYPE('size', 'number'); +function assertSize(size, elementSize, offset, length) { + if (typeof size !== 'number') { + throw new ERR_INVALID_ARG_TYPE('size', 'number', size); } - if (size > kMaxUint32 || size < 0) { - throw new ERR_INVALID_ARG_TYPE('size', 'uint32'); + size *= elementSize; + + if (Number.isNaN(size) || size > kMaxPossibleLength || size < 0) { + throw new ERR_OUT_OF_RANGE('size', + `>= 0 && <= ${kMaxPossibleLength}`, size); } - if (size + offset > length || size > kMaxLength) { - throw new ERR_OUT_OF_RANGE('size'); + if (size + offset > length) { + 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; - assertOffset(offset, buf.byteLength); + offset = assertOffset(offset, elementSize, 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); } diff --git a/test/parallel/test-crypto-random.js b/test/parallel/test-crypto-random.js index 886d560d2f33aa..77801f6d53eef2 100644 --- a/test/parallel/test-crypto-random.js +++ b/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) { - - common.expectsError( - () => f(value), - { +{ + [crypto.randomBytes, crypto.pseudoRandomBytes].forEach((f) => { + [undefined, null, false, true, {}, []].forEach((value) => { + const errObj = { code: 'ERR_INVALID_ARG_TYPE', - type: TypeError, - message: /^The "size" argument must be of type (number|uint32)$/ - } - ); + name: 'TypeError [ERR_INVALID_ARG_TYPE]', + message: 'The "size" argument must be of type number. ' + + `Received type ${typeof value}` + }; + assert.throws(() => f(value), errObj); + assert.throws(() => f(value, common.mustNotCall()), errObj); + }); - common.expectsError( - () => f(value, common.mustNotCall()), - { - code: 'ERR_INVALID_ARG_TYPE', - type: TypeError, - message: /^The "size" argument must be of type (number|uint32)$/ - } - ); - }); + [-1, NaN, 2 ** 32].forEach((value) => { + 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(function(len) { - f(len, common.mustCall(function(ex, buf) { - assert.strictEqual(ex, null); - assert.strictEqual(buf.length, Math.floor(len)); - assert.ok(Buffer.isBuffer(buf)); - })); + [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)) - ]; - - const max = require('buffer').kMaxLength + 1; - - for (const buf of bufs) { + ].forEach((buf) => { const len = Buffer.byteLength(buf); assert.strictEqual(len, 10, `Expected byteLength of 10, got ${len}`); - common.expectsError( - () => crypto.randomFillSync(buf, 'test'), - { - code: 'ERR_INVALID_ARG_TYPE', - type: TypeError, - message: 'The "offset" argument must be of type number' - } - ); + const typeErrObj = { + code: 'ERR_INVALID_ARG_TYPE', + name: 'TypeError [ERR_INVALID_ARG_TYPE]', + message: 'The "offset" argument must be of type number. ' + + 'Received type string' + }; - common.expectsError( - () => crypto.randomFillSync(buf, NaN), - { - code: 'ERR_INVALID_ARG_TYPE', - type: TypeError, - message: 'The "offset" argument must be of type number' - } - ); + assert.throws(() => crypto.randomFillSync(buf, 'test'), typeErrObj); - common.expectsError( + assert.throws( () => crypto.randomFill(buf, 'test', common.mustNotCall()), - { - code: 'ERR_INVALID_ARG_TYPE', - type: TypeError, - message: 'The "offset" argument must be of type number' - } - ); - - common.expectsError( - () => crypto.randomFill(buf, NaN, common.mustNotCall()), - { - code: 'ERR_INVALID_ARG_TYPE', - type: TypeError, - message: 'The "offset" argument must be of type number' - } - ); - - common.expectsError( - () => 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.' - } - ); + typeErrObj); - 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' - } - ); + typeErrObj.message = 'The "size" argument must be of type number. ' + + 'Received type string'; + assert.throws(() => crypto.randomFillSync(buf, 0, 'test'), typeErrObj); - common.expectsError( + assert.throws( () => crypto.randomFill(buf, 0, 'test', common.mustNotCall()), - { - code: 'ERR_INVALID_ARG_TYPE', - type: TypeError, - message: 'The "size" argument must be of type number' - } + typeErrObj ); - common.expectsError( - () => crypto.randomFill(buf, 0, NaN, common.mustNotCall()), - { - code: 'ERR_INVALID_ARG_TYPE', - type: TypeError, - message: 'The "size" argument must be of type number' - } - ); + [NaN, kMaxPossibleLength + 1, -10, (-1 >>> 0) + 1].forEach((offsetSize) => { + const errObj = { + code: 'ERR_OUT_OF_RANGE', + name: 'RangeError [ERR_OUT_OF_RANGE]', + message: 'The value of "offset" is out of range. ' + + `It must be >= 0 && <= 10. Received ${offsetSize}` + }; - { - 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' - } - ); + assert.throws(() => crypto.randomFillSync(buf, offsetSize), errObj); - common.expectsError( - () => crypto.randomFillSync(buf, 0, size), - { - code: 'ERR_INVALID_ARG_TYPE', - type: TypeError, - message: 'The "size" argument must be of type uint32' - } - ); + assert.throws( + () => crypto.randomFill(buf, offsetSize, common.mustNotCall()), + errObj); - 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' - } - ); + 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); - 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' - } + assert.throws( + () => crypto.randomFill(buf, 1, offsetSize, common.mustNotCall()), + errObj ); - } - - 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, - message: 'The value of "size" is out of range.' - } - ); + 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); - common.expectsError( + assert.throws( () => crypto.randomFill(buf, 1, 10, common.mustNotCall()), - { - code: 'ERR_OUT_OF_RANGE', - type: RangeError, - message: 'The value of "size" is out of range.' - } + rangeErrObj ); - - 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', - type: TypeError, - message: 'The "size" argument must be of type uint32' + 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 4294967296` } );