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]>

Author: addaleax

lib/crypto.js

@@ -44,7 +44,11 @@ const{getOptionValue } = require('internal/options');
 constpendingDeprecation=getOptionValue('--pending-deprecation');
 const{ fipsMode }=internalBinding('config');
 constfipsForced=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');
 constCertificate=require('internal/crypto/certificate');
 

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){
 thrownewERR_CRYPTO_ENGINE_UNKNOWN(id);
 }
 
-functiontimingSafeEqual(buf1,buf2){
-if(!isArrayBufferView(buf1)){
-thrownewERR_INVALID_ARG_TYPE('buf1',
-['Buffer','TypedArray','DataView'],buf1);
-}
-if(!isArrayBufferView(buf2)){
-thrownewERR_INVALID_ARG_TYPE('buf2',
-['Buffer','TypedArray','DataView'],buf2);
-}
-if(buf1.byteLength!==buf2.byteLength){
-thrownewERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH();
-}
-return_timingSafeEqual(buf1,buf2);
-}
-
 constgetArrayBufferView=hideStackFrames((buffer,name,encoding)=>{
 if(typeofbuffer==='string'){
 if(encoding==='buffer')
@@ -116,6 +99,5 @@ module.exports ={
  kHandle,
  setDefaultEncoding,
  setEngine,
- timingSafeEqual,
  toBuf
 };

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 '+

src/node_crypto.cc

@@ -6766,10 +6766,30 @@ void StatelessDiffieHellman(const FunctionCallbackInfo<Value>& args){
 
 
 voidTimingSafeEqual(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);

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, \

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.'
 }
 );