crypto: move typechecking for timingSafeEqual into C++

This makes the function more robust against V8 inlining. Fixes: #34073 PR-URL: #34141 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Zeyu Yang <[email protected]>
nodejs · Sep 22, 2020 · bf4e778 · bf4e778
1 parent bb54217
commit bf4e778
Show file tree

Hide file tree

Showing 6 changed files with 33 additions and 27 deletions.
diff --git 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
@@ -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
@@ -800,8 +800,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
@@ -6766,10 +6766,30 @@ void StatelessDiffieHellman(const FunctionCallbackInfo<Value>& args) {
 
 
 void TimingSafeEqual(const FunctionCallbackInfo<Value>& args) {
-  ArrayBufferViewContents<char> buf1(args[0]);
-  ArrayBufferViewContents<char> 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<char> buf1(args[0].As<ArrayBufferView>());
+  ArrayBufferViewContents<char> buf2(args[1].As<ArrayBufferView>());
 
-  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
@@ -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
@@ -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.'
   }
 );