From cfd4c8012d04d68212d95e760efd67a0a0a2b997 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 30 Jun 2020 22:36:10 +0200 Subject: [PATCH] crypto: move typechecking for timingSafeEqual into C++ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This makes the function more robust against V8 inlining. Fixes: https://github.com/nodejs/node/issues/34073 PR-URL: https://github.com/nodejs/node/pull/34141 Reviewed-By: Richard Lau Reviewed-By: Ujjwal Sharma Reviewed-By: Ben Noordhuis Reviewed-By: Tobias Nießen Reviewed-By: Rich Trott Reviewed-By: Zeyu Yang --- lib/crypto.js | 7 +++-- lib/internal/crypto/util.js | 18 ------------- lib/internal/errors.js | 2 -- src/node_crypto.cc | 26 ++++++++++++++++--- src/node_errors.h | 3 +++ .../test-crypto-timing-safe-equal.js | 4 +-- 6 files changed, 33 insertions(+), 27 deletions(-) diff --git a/lib/crypto.js b/lib/crypto.js index 5e45a1cd987260..e4b9d479a50671 100644 --- a/lib/crypto.js +++ b/lib/crypto.js @@ -44,7 +44,11 @@ const { getOptionValue } = require('internal/options'); const pendingDeprecation = getOptionValue('--pending-deprecation'); const { fipsMode } = internalBinding('config'); const fipsForced = getOptionValue('--force-fips'); -const { getFipsCrypto, setFipsCrypto } = internalBinding('crypto'); +const { + getFipsCrypto, + setFipsCrypto, + timingSafeEqual, +} = internalBinding('crypto'); const { randomBytes, randomFill, @@ -101,7 +105,6 @@ const { getHashes, setDefaultEncoding, setEngine, - timingSafeEqual } = 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 286efc39a61e4f..8d43514906fde8 100644 --- a/lib/internal/crypto/util.js +++ b/lib/internal/crypto/util.js @@ -9,7 +9,6 @@ const { getCurves: _getCurves, getHashes: _getHashes, setEngine: _setEngine, - timingSafeEqual: _timingSafeEqual } = internalBinding('crypto'); const { @@ -20,7 +19,6 @@ const { hideStackFrames, codes: { ERR_CRYPTO_ENGINE_UNKNOWN, - ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH, ERR_INVALID_ARG_TYPE, } } = require('internal/errors'); @@ -76,21 +74,6 @@ function setEngine(id, flags) { throw new ERR_CRYPTO_ENGINE_UNKNOWN(id); } -function timingSafeEqual(buf1, buf2) { - if (!isArrayBufferView(buf1)) { - throw new ERR_INVALID_ARG_TYPE('buf1', - ['Buffer', 'TypedArray', 'DataView'], buf1); - } - if (!isArrayBufferView(buf2)) { - throw new ERR_INVALID_ARG_TYPE('buf2', - ['Buffer', 'TypedArray', 'DataView'], buf2); - } - if (buf1.byteLength !== buf2.byteLength) { - throw new ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH(); - } - return _timingSafeEqual(buf1, buf2); -} - const getArrayBufferView = hideStackFrames((buffer, name, encoding) => { if (typeof buffer === 'string') { if (encoding === 'buffer') @@ -116,6 +99,5 @@ module.exports = { kHandle, setDefaultEncoding, setEngine, - timingSafeEqual, toBuf }; diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 2e877edc6a78e3..e43c0006363d13 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -804,8 +804,6 @@ E('ERR_CRYPTO_SCRYPT_INVALID_PARAMETER', 'Invalid scrypt parameter', Error); E('ERR_CRYPTO_SCRYPT_NOT_SUPPORTED', 'Scrypt algorithm not supported', Error); // Switch to TypeError. The current implementation does not seem right. E('ERR_CRYPTO_SIGN_KEY_REQUIRED', 'No key provided to sign', Error); -E('ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH', - 'Input buffers must have the same byte length', RangeError); E('ERR_DIR_CLOSED', 'Directory handle was closed', Error); E('ERR_DIR_CONCURRENT_OPERATION', 'Cannot do synchronous work on directory handle with concurrent ' + diff --git a/src/node_crypto.cc b/src/node_crypto.cc index e34b27b166c5d3..aace8e2c05e915 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -6831,10 +6831,30 @@ void StatelessDiffieHellman(const FunctionCallbackInfo& args) { void TimingSafeEqual(const FunctionCallbackInfo& args) { - ArrayBufferViewContents buf1(args[0]); - ArrayBufferViewContents buf2(args[1]); + // Moving the type checking into JS leads to test failures, most likely due + // to V8 inlining certain parts of the wrapper. Therefore, keep them in C++. + // Refs: https://github.com/nodejs/node/issues/34073. + Environment* env = Environment::GetCurrent(args); + if (!args[0]->IsArrayBufferView()) { + THROW_ERR_INVALID_ARG_TYPE( + env, "The \"buf1\" argument must be an instance of " + "Buffer, TypedArray, or DataView."); + return; + } + if (!args[1]->IsArrayBufferView()) { + THROW_ERR_INVALID_ARG_TYPE( + env, "The \"buf2\" argument must be an instance of " + "Buffer, TypedArray, or DataView."); + return; + } + + ArrayBufferViewContents buf1(args[0].As()); + ArrayBufferViewContents buf2(args[1].As()); - CHECK_EQ(buf1.length(), buf2.length()); + if (buf1.length() != buf2.length()) { + THROW_ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH(env); + return; + } return args.GetReturnValue().Set( CRYPTO_memcmp(buf1.data(), buf2.data(), buf1.length()) == 0); diff --git a/src/node_errors.h b/src/node_errors.h index b9f001d777bb07..45498f3e00463e 100644 --- a/src/node_errors.h +++ b/src/node_errors.h @@ -33,6 +33,7 @@ void OnFatalError(const char* location, const char* message); V(ERR_BUFFER_TOO_LARGE, Error) \ V(ERR_CONSTRUCT_CALL_REQUIRED, TypeError) \ V(ERR_CONSTRUCT_CALL_INVALID, TypeError) \ + V(ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH, RangeError) \ V(ERR_CRYPTO_UNKNOWN_CIPHER, Error) \ V(ERR_CRYPTO_UNKNOWN_DH_GROUP, Error) \ V(ERR_EXECUTION_ENVIRONMENT_NOT_AVAILABLE, Error) \ @@ -86,6 +87,8 @@ void OnFatalError(const char* location, const char* message); "Buffer is not available for the current Context") \ V(ERR_CONSTRUCT_CALL_INVALID, "Constructor cannot be called") \ V(ERR_CONSTRUCT_CALL_REQUIRED, "Cannot call constructor without `new`") \ + V(ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH, \ + "Input buffers must have the same byte length") \ V(ERR_CRYPTO_UNKNOWN_CIPHER, "Unknown cipher") \ V(ERR_CRYPTO_UNKNOWN_DH_GROUP, "Unknown DH group") \ V(ERR_EXECUTION_ENVIRONMENT_NOT_AVAILABLE, \ diff --git a/test/sequential/test-crypto-timing-safe-equal.js b/test/sequential/test-crypto-timing-safe-equal.js index f9709ac966d6a1..769b5f7d8810c3 100644 --- a/test/sequential/test-crypto-timing-safe-equal.js +++ b/test/sequential/test-crypto-timing-safe-equal.js @@ -48,7 +48,7 @@ assert.throws( name: 'TypeError', message: 'The "buf1" argument must be an instance of Buffer, TypedArray, or ' + - "DataView. Received type string ('not a buffer')" + 'DataView.' } ); @@ -59,6 +59,6 @@ assert.throws( name: 'TypeError', message: 'The "buf2" argument must be an instance of Buffer, TypedArray, or ' + - "DataView. Received type string ('not a buffer')" + 'DataView.' } );