crypto: migrate timingSafeEqual to internal/errors

PR-URL: #16448 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
nodejs · Oct 26, 2017 · eeada6c · eeada6c
1 parent 6645126
commit eeada6c
Show file tree

Hide file tree

Showing 6 changed files with 60 additions and 21 deletions.
diff --git a/doc/api/errors.md b/doc/api/errors.md
@@ -648,6 +648,12 @@ Used when an invalid crypto engine identifier is passed to
 
 Used when an invalid [crypto digest algorithm][] is specified.
 
+<a id="ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH"></a>
+### ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH
+
+Used when calling [`crypto.timingSafeEqual()`][] with `Buffer`, `TypedArray`,
+or `DataView` arguments of different lengths.
+
 <a id="ERR_DNS_SET_SERVERS_FAILED"></a>
 ### ERR_DNS_SET_SERVERS_FAILED
 
@@ -1348,6 +1354,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
@@ -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
@@ -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';
 
@@ -60,12 +64,28 @@ function setEngine(id, flags) {
     throw new errors.Error('ERR_CRYPTO_ENGINE_UNKNOWN', id);
 }
 
+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,
   getDefaultEncoding,
   getHashes,
   setDefaultEncoding,
   setEngine,
+  timingSafeEqual,
   toBuf
 };
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
@@ -160,6 +160,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
@@ -5848,15 +5848,11 @@ void ExportChallenge(const FunctionCallbackInfo<Value>& args) {
 }
 
 void TimingSafeEqual(const FunctionCallbackInfo<Value>& 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
@@ -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'
+  }
+);