From 380c2985a82ef15ac7bd3fbc7d507853f6d73c35 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Tue, 24 Oct 2017 10:04:25 -0700 Subject: [PATCH] crypto: migrate timingSafeEqual to internal/errors --- doc/api/errors.md | 7 ++++ lib/crypto.js | 2 +- lib/internal/crypto/util.js | 22 ++++++++++- lib/internal/errors.js | 2 + src/node_crypto.cc | 8 ++-- .../test-crypto-timing-safe-equal.js | 38 +++++++++++++------ 6 files changed, 60 insertions(+), 19 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index b78c6b36d634db..ac8aec129b37d8 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -642,6 +642,12 @@ Used when an invalid value for the `format` argument has been passed to the Used when an invalid [crypto digest algorithm][] is specified. + +### ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH + +Used when calling [`crypto.timingSafeEqual()`][] with `Buffer`, `TypedArray`, +or `DataView` arguments of different lengths. + ### ERR_DNS_SET_SERVERS_FAILED @@ -1342,6 +1348,7 @@ Used when a given value is out of the accepted range. Used when an attempt is made to use a `zlib` object after it has already been closed. +[`crypto.timingSafeEqual()`]: crypto.html#crypto_crypto_timingsafeequal_a_b [`ERR_INVALID_ARG_TYPE`]: #ERR_INVALID_ARG_TYPE [`subprocess.kill()`]: child_process.html#child_process_subprocess_kill_signal [`subprocess.send()`]: child_process.html#child_process_subprocess_send_message_sendhandle_options_callback diff --git a/lib/crypto.js b/lib/crypto.js index 1337cccf2c3ce1..0082172c5c4a09 100644 --- a/lib/crypto.js +++ b/lib/crypto.js @@ -34,7 +34,6 @@ const constants = process.binding('constants').crypto; const { getFipsCrypto, setFipsCrypto, - timingSafeEqual } = process.binding('crypto'); const { randomBytes, @@ -75,6 +74,7 @@ const { getHashes, setDefaultEncoding, setEngine, + timingSafeEqual, toBuf } = require('internal/crypto/util'); const Certificate = require('internal/crypto/certificate'); diff --git a/lib/internal/crypto/util.js b/lib/internal/crypto/util.js index 9e242dc917045d..b2a4d8da942db1 100644 --- a/lib/internal/crypto/util.js +++ b/lib/internal/crypto/util.js @@ -4,7 +4,8 @@ const { getCiphers: _getCiphers, getCurves: _getCurves, getHashes: _getHashes, - setEngine: _setEngine + setEngine: _setEngine, + timingSafeEqual: _timingSafeEqual } = process.binding('crypto'); const { @@ -17,6 +18,9 @@ const { cachedResult, filterDuplicateStrings } = require('internal/util'); +const { + isArrayBufferView +} = require('internal/util/types'); var defaultEncoding = 'buffer'; @@ -59,6 +63,21 @@ function setEngine(id, flags) { return _setEngine(id, flags); } +function timingSafeEqual(a, b) { + if (!isArrayBufferView(a)) { + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'a', + ['Buffer', 'TypedArray', 'DataView']); + } + if (!isArrayBufferView(b)) { + throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'b', + ['Buffer', 'TypedArray', 'DataView']); + } + if (a.length !== b.length) { + throw new errors.RangeError('ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH'); + } + return _timingSafeEqual(a, b); +} + module.exports = { getCiphers, getCurves, @@ -66,5 +85,6 @@ module.exports = { getHashes, setDefaultEncoding, setEngine, + timingSafeEqual, toBuf }; diff --git a/lib/internal/errors.js b/lib/internal/errors.js index a719570841e090..86763fc0194194 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -159,6 +159,8 @@ E('ERR_CRYPTO_HASH_FINALIZED', 'Digest already called'); E('ERR_CRYPTO_HASH_UPDATE_FAILED', 'Hash update failed'); E('ERR_CRYPTO_INVALID_DIGEST', 'Invalid digest: %s'); E('ERR_CRYPTO_SIGN_KEY_REQUIRED', 'No key provided to sign'); +E('ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH', + 'Input buffers must have the same length'); E('ERR_DNS_SET_SERVERS_FAILED', (err, servers) => `c-ares failed to set servers: "${err}" [${servers}]`); E('ERR_ENCODING_INVALID_ENCODED_DATA', diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 7decd7dd1821e5..b627647d4d28f5 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -5850,13 +5850,11 @@ void ExportChallenge(const FunctionCallbackInfo& args) { void TimingSafeEqual(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - THROW_AND_RETURN_IF_NOT_BUFFER(args[0], "First argument"); - THROW_AND_RETURN_IF_NOT_BUFFER(args[1], "Second argument"); + CHECK(Buffer::HasInstance(args[0])); + CHECK(Buffer::HasInstance(args[1])); size_t buf_length = Buffer::Length(args[0]); - if (buf_length != Buffer::Length(args[1])) { - return env->ThrowTypeError("Input buffers must have the same length"); - } + CHECK_EQ(buf_length, Buffer::Length(args[1])); const char* buf1 = Buffer::Data(args[0]); const char* buf2 = Buffer::Data(args[1]); diff --git a/test/sequential/test-crypto-timing-safe-equal.js b/test/sequential/test-crypto-timing-safe-equal.js index 2847af9ef8bdb5..08cfc04755d638 100644 --- a/test/sequential/test-crypto-timing-safe-equal.js +++ b/test/sequential/test-crypto-timing-safe-equal.js @@ -18,17 +18,31 @@ assert.strictEqual( 'should consider unequal strings to be unequal' ); -assert.throws(function() { - crypto.timingSafeEqual(Buffer.from([1, 2, 3]), Buffer.from([1, 2])); -}, /^TypeError: Input buffers must have the same length$/, - 'should throw when given buffers with different lengths'); +common.expectsError( + () => crypto.timingSafeEqual(Buffer.from([1, 2, 3]), Buffer.from([1, 2])), + { + code: 'ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH', + type: RangeError, + message: 'Input buffers must have the same length' + } +); -assert.throws(function() { - crypto.timingSafeEqual('not a buffer', Buffer.from([1, 2])); -}, /^TypeError: First argument must be a buffer$/, - 'should throw if the first argument is not a buffer'); +common.expectsError( + () => crypto.timingSafeEqual('not a buffer', Buffer.from([1, 2])), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: + 'The "a" argument must be one of type Buffer, TypedArray, or DataView' + } +); -assert.throws(function() { - crypto.timingSafeEqual(Buffer.from([1, 2]), 'not a buffer'); -}, /^TypeError: Second argument must be a buffer$/, - 'should throw if the second argument is not a buffer'); +common.expectsError( + () => crypto.timingSafeEqual(Buffer.from([1, 2]), 'not a buffer'), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: + 'The "b" argument must be one of type Buffer, TypedArray, or DataView' + } +);