Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit bf4e778

Browse filesBrowse files
addaleaxaddaleax
committed
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 <riclau@uk.ibm.com> Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com>
1 parent bb54217 commit bf4e778
Copy full SHA for bf4e778

File tree

Expand file treeCollapse file tree

6 files changed

+33
-27
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

6 files changed

+33
-27
lines changed
Open diff view settings
Collapse file

‎lib/crypto.js‎

Copy file name to clipboardExpand all lines: lib/crypto.js
+5-2Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,11 @@ const { getOptionValue } = require('internal/options');
4444
const pendingDeprecation = getOptionValue('--pending-deprecation');
4545
const { fipsMode } = internalBinding('config');
4646
const fipsForced = getOptionValue('--force-fips');
47-
const { getFipsCrypto, setFipsCrypto } = internalBinding('crypto');
47+
const {
48+
getFipsCrypto,
49+
setFipsCrypto,
50+
timingSafeEqual,
51+
} = internalBinding('crypto');
4852
const {
4953
randomBytes,
5054
randomFill,
@@ -101,7 +105,6 @@ const {
101105
getHashes,
102106
setDefaultEncoding,
103107
setEngine,
104-
timingSafeEqual
105108
} = require('internal/crypto/util');
106109
const Certificate = require('internal/crypto/certificate');
107110

Collapse file

‎lib/internal/crypto/util.js‎

Copy file name to clipboardExpand all lines: lib/internal/crypto/util.js
-18Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ const {
99
getCurves: _getCurves,
1010
getHashes: _getHashes,
1111
setEngine: _setEngine,
12-
timingSafeEqual: _timingSafeEqual
1312
} = internalBinding('crypto');
1413

1514
const {
@@ -20,7 +19,6 @@ const {
2019
hideStackFrames,
2120
codes: {
2221
ERR_CRYPTO_ENGINE_UNKNOWN,
23-
ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH,
2422
ERR_INVALID_ARG_TYPE,
2523
}
2624
} = require('internal/errors');
@@ -76,21 +74,6 @@ function setEngine(id, flags) {
7674
throw new ERR_CRYPTO_ENGINE_UNKNOWN(id);
7775
}
7876

79-
function timingSafeEqual(buf1, buf2) {
80-
if (!isArrayBufferView(buf1)) {
81-
throw new ERR_INVALID_ARG_TYPE('buf1',
82-
['Buffer', 'TypedArray', 'DataView'], buf1);
83-
}
84-
if (!isArrayBufferView(buf2)) {
85-
throw new ERR_INVALID_ARG_TYPE('buf2',
86-
['Buffer', 'TypedArray', 'DataView'], buf2);
87-
}
88-
if (buf1.byteLength !== buf2.byteLength) {
89-
throw new ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH();
90-
}
91-
return _timingSafeEqual(buf1, buf2);
92-
}
93-
9477
const getArrayBufferView = hideStackFrames((buffer, name, encoding) => {
9578
if (typeof buffer === 'string') {
9679
if (encoding === 'buffer')
@@ -116,6 +99,5 @@ module.exports = {
11699
kHandle,
117100
setDefaultEncoding,
118101
setEngine,
119-
timingSafeEqual,
120102
toBuf
121103
};
Collapse file

‎lib/internal/errors.js‎

Copy file name to clipboardExpand all lines: lib/internal/errors.js
-2Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -800,8 +800,6 @@ E('ERR_CRYPTO_SCRYPT_INVALID_PARAMETER', 'Invalid scrypt parameter', Error);
800800
E('ERR_CRYPTO_SCRYPT_NOT_SUPPORTED', 'Scrypt algorithm not supported', Error);
801801
// Switch to TypeError. The current implementation does not seem right.
802802
E('ERR_CRYPTO_SIGN_KEY_REQUIRED', 'No key provided to sign', Error);
803-
E('ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH',
804-
'Input buffers must have the same byte length', RangeError);
805803
E('ERR_DIR_CLOSED', 'Directory handle was closed', Error);
806804
E('ERR_DIR_CONCURRENT_OPERATION',
807805
'Cannot do synchronous work on directory handle with concurrent ' +
Collapse file

‎src/node_crypto.cc‎

Copy file name to clipboardExpand all lines: src/node_crypto.cc
+23-3Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6766,10 +6766,30 @@ void StatelessDiffieHellman(const FunctionCallbackInfo<Value>& args) {
67666766

67676767

67686768
void TimingSafeEqual(const FunctionCallbackInfo<Value>& args) {
6769-
ArrayBufferViewContents<char> buf1(args[0]);
6770-
ArrayBufferViewContents<char> buf2(args[1]);
6769+
// Moving the type checking into JS leads to test failures, most likely due
6770+
// to V8 inlining certain parts of the wrapper. Therefore, keep them in C++.
6771+
// Refs: https://github.com/nodejs/node/issues/34073.
6772+
Environment* env = Environment::GetCurrent(args);
6773+
if (!args[0]->IsArrayBufferView()) {
6774+
THROW_ERR_INVALID_ARG_TYPE(
6775+
env, "The \"buf1\" argument must be an instance of "
6776+
"Buffer, TypedArray, or DataView.");
6777+
return;
6778+
}
6779+
if (!args[1]->IsArrayBufferView()) {
6780+
THROW_ERR_INVALID_ARG_TYPE(
6781+
env, "The \"buf2\" argument must be an instance of "
6782+
"Buffer, TypedArray, or DataView.");
6783+
return;
6784+
}
6785+
6786+
ArrayBufferViewContents<char> buf1(args[0].As<ArrayBufferView>());
6787+
ArrayBufferViewContents<char> buf2(args[1].As<ArrayBufferView>());
67716788

6772-
CHECK_EQ(buf1.length(), buf2.length());
6789+
if (buf1.length() != buf2.length()) {
6790+
THROW_ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH(env);
6791+
return;
6792+
}
67736793

67746794
return args.GetReturnValue().Set(
67756795
CRYPTO_memcmp(buf1.data(), buf2.data(), buf1.length()) == 0);
Collapse file

‎src/node_errors.h‎

Copy file name to clipboardExpand all lines: src/node_errors.h
+3Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ void OnFatalError(const char* location, const char* message);
3333
V(ERR_BUFFER_TOO_LARGE, Error) \
3434
V(ERR_CONSTRUCT_CALL_REQUIRED, TypeError) \
3535
V(ERR_CONSTRUCT_CALL_INVALID, TypeError) \
36+
V(ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH, RangeError) \
3637
V(ERR_CRYPTO_UNKNOWN_CIPHER, Error) \
3738
V(ERR_CRYPTO_UNKNOWN_DH_GROUP, Error) \
3839
V(ERR_EXECUTION_ENVIRONMENT_NOT_AVAILABLE, Error) \
@@ -86,6 +87,8 @@ void OnFatalError(const char* location, const char* message);
8687
"Buffer is not available for the current Context") \
8788
V(ERR_CONSTRUCT_CALL_INVALID, "Constructor cannot be called") \
8889
V(ERR_CONSTRUCT_CALL_REQUIRED, "Cannot call constructor without `new`") \
90+
V(ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH, \
91+
"Input buffers must have the same byte length") \
8992
V(ERR_CRYPTO_UNKNOWN_CIPHER, "Unknown cipher") \
9093
V(ERR_CRYPTO_UNKNOWN_DH_GROUP, "Unknown DH group") \
9194
V(ERR_EXECUTION_ENVIRONMENT_NOT_AVAILABLE, \
Collapse file

‎test/sequential/test-crypto-timing-safe-equal.js‎

Copy file name to clipboardExpand all lines: test/sequential/test-crypto-timing-safe-equal.js
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ assert.throws(
4848
name: 'TypeError',
4949
message:
5050
'The "buf1" argument must be an instance of Buffer, TypedArray, or ' +
51-
"DataView. Received type string ('not a buffer')"
51+
'DataView.'
5252
}
5353
);
5454

@@ -59,6 +59,6 @@ assert.throws(
5959
name: 'TypeError',
6060
message:
6161
'The "buf2" argument must be an instance of Buffer, TypedArray, or ' +
62-
"DataView. Received type string ('not a buffer')"
62+
'DataView.'
6363
}
6464
);

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.