Skip to content

Commit 079accc

Browse files
not-an-aardvarkTrott
authored andcommitted
crypto: add crypto.timingSafeEqual()
Reinstate crypto.timingSafeEqual() which was reverted due to test issues. The flaky test issues are resolved in this new changeset. PR-URL: nodejs#8304 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
1 parent c436437 commit 079accc

File tree

4 files changed

+199
-0
lines changed

4 files changed

+199
-0
lines changed

doc/api/crypto.md

+13
Original file line numberDiff line numberDiff line change
@@ -1217,6 +1217,19 @@ keys:
12171217

12181218
All paddings are defined in `crypto.constants`.
12191219

1220+
### crypto.timingSafeEqual(a, b)
1221+
1222+
Returns true if `a` is equal to `b`, without leaking timing information that
1223+
would allow an attacker to guess one of the values. This is suitable for
1224+
comparing HMAC digests or secret values like authentication cookies or
1225+
[capability urls](https://www.w3.org/TR/capability-urls/).
1226+
1227+
`a` and `b` must both be `Buffer`s, and they must have the same length.
1228+
1229+
**Note**: Use of `crypto.timingSafeEqual` does not guarantee that the
1230+
*surrounding* code is timing-safe. Care should be taken to ensure that the
1231+
surrounding code does not introduce timing vulnerabilities.
1232+
12201233
### crypto.privateEncrypt(private_key, buffer)
12211234

12221235
Encrypts `buffer` with `private_key`.

lib/crypto.js

+3
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ const getHashes = binding.getHashes;
1616
const getCurves = binding.getCurves;
1717
const getFipsCrypto = binding.getFipsCrypto;
1818
const setFipsCrypto = binding.setFipsCrypto;
19+
const timingSafeEqual = binding.timingSafeEqual;
1920

2021
const Buffer = require('buffer').Buffer;
2122
const stream = require('stream');
@@ -649,6 +650,8 @@ Object.defineProperty(exports, 'fips', {
649650
set: setFipsCrypto
650651
});
651652

653+
exports.timingSafeEqual = timingSafeEqual;
654+
652655
// Legacy API
653656
Object.defineProperty(exports, 'createCredentials', {
654657
configurable: true,

src/node_crypto.cc

+17
Original file line numberDiff line numberDiff line change
@@ -5771,6 +5771,22 @@ void ExportChallenge(const FunctionCallbackInfo<Value>& args) {
57715771
args.GetReturnValue().Set(outString);
57725772
}
57735773

5774+
void TimingSafeEqual(const FunctionCallbackInfo<Value>& args) {
5775+
Environment* env = Environment::GetCurrent(args);
5776+
5777+
THROW_AND_RETURN_IF_NOT_BUFFER(args[0], "First argument");
5778+
THROW_AND_RETURN_IF_NOT_BUFFER(args[1], "Second argument");
5779+
5780+
size_t buf_length = Buffer::Length(args[0]);
5781+
if (buf_length != Buffer::Length(args[1])) {
5782+
return env->ThrowTypeError("Input buffers must have the same length");
5783+
}
5784+
5785+
const char* buf1 = Buffer::Data(args[0]);
5786+
const char* buf2 = Buffer::Data(args[1]);
5787+
5788+
return args.GetReturnValue().Set(CRYPTO_memcmp(buf1, buf2, buf_length) == 0);
5789+
}
57745790

57755791
void InitCryptoOnce() {
57765792
OPENSSL_config(NULL);
@@ -5903,6 +5919,7 @@ void InitCrypto(Local<Object> target,
59035919
env->SetMethod(target, "setFipsCrypto", SetFipsCrypto);
59045920
env->SetMethod(target, "PBKDF2", PBKDF2);
59055921
env->SetMethod(target, "randomBytes", RandomBytes);
5922+
env->SetMethod(target, "timingSafeEqual", TimingSafeEqual);
59065923
env->SetMethod(target, "getSSLCiphers", GetSSLCiphers);
59075924
env->SetMethod(target, "getCiphers", GetCiphers);
59085925
env->SetMethod(target, "getHashes", GetHashes);
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,166 @@
1+
'use strict';
2+
const common = require('../common');
3+
const assert = require('assert');
4+
5+
if (!common.hasCrypto) {
6+
common.skip('missing crypto');
7+
return;
8+
}
9+
10+
const crypto = require('crypto');
11+
12+
assert.strictEqual(
13+
crypto.timingSafeEqual(Buffer.from('foo'), Buffer.from('foo')),
14+
true,
15+
'should consider equal strings to be equal'
16+
);
17+
18+
assert.strictEqual(
19+
crypto.timingSafeEqual(Buffer.from('foo'), Buffer.from('bar')),
20+
false,
21+
'should consider unequal strings to be unequal'
22+
);
23+
24+
assert.throws(function() {
25+
crypto.timingSafeEqual(Buffer.from([1, 2, 3]), Buffer.from([1, 2]));
26+
}, 'should throw when given buffers with different lengths');
27+
28+
assert.throws(function() {
29+
crypto.timingSafeEqual('not a buffer', Buffer.from([1, 2]));
30+
}, 'should throw if the first argument is not a buffer');
31+
32+
assert.throws(function() {
33+
crypto.timingSafeEqual(Buffer.from([1, 2]), 'not a buffer');
34+
}, 'should throw if the second argument is not a buffer');
35+
36+
function getTValue(compareFunc) {
37+
const numTrials = 10000;
38+
const testBufferSize = 10000;
39+
// Perform benchmarks to verify that timingSafeEqual is actually timing-safe.
40+
41+
const rawEqualBenches = Array(numTrials);
42+
const rawUnequalBenches = Array(numTrials);
43+
44+
for (let i = 0; i < numTrials; i++) {
45+
46+
// The `runEqualBenchmark` and `runUnequalBenchmark` functions are
47+
// intentionally redefined on every iteration of this loop, to avoid
48+
// timing inconsistency.
49+
function runEqualBenchmark(compareFunc, bufferA, bufferB) {
50+
const startTime = process.hrtime();
51+
const result = compareFunc(bufferA, bufferB);
52+
const endTime = process.hrtime(startTime);
53+
54+
// Ensure that the result of the function call gets used, so it doesn't
55+
// get discarded due to engine optimizations.
56+
assert.strictEqual(result, true);
57+
return endTime[0] * 1e9 + endTime[1];
58+
}
59+
60+
// This is almost the same as the runEqualBenchmark function, but it's
61+
// duplicated to avoid timing issues with V8 optimization/inlining.
62+
function runUnequalBenchmark(compareFunc, bufferA, bufferB) {
63+
const startTime = process.hrtime();
64+
const result = compareFunc(bufferA, bufferB);
65+
const endTime = process.hrtime(startTime);
66+
67+
assert.strictEqual(result, false);
68+
return endTime[0] * 1e9 + endTime[1];
69+
}
70+
71+
if (i % 2) {
72+
const bufferA1 = Buffer.alloc(testBufferSize, 'A');
73+
const bufferB = Buffer.alloc(testBufferSize, 'B');
74+
const bufferA2 = Buffer.alloc(testBufferSize, 'A');
75+
const bufferC = Buffer.alloc(testBufferSize, 'C');
76+
77+
// First benchmark: comparing two equal buffers
78+
rawEqualBenches[i] = runEqualBenchmark(compareFunc, bufferA1, bufferA2);
79+
80+
// Second benchmark: comparing two unequal buffers
81+
rawUnequalBenches[i] = runUnequalBenchmark(compareFunc, bufferB, bufferC);
82+
} else {
83+
// Swap the order of the benchmarks every second iteration, to avoid any
84+
// patterns caused by memory usage.
85+
const bufferB = Buffer.alloc(testBufferSize, 'B');
86+
const bufferA1 = Buffer.alloc(testBufferSize, 'A');
87+
const bufferC = Buffer.alloc(testBufferSize, 'C');
88+
const bufferA2 = Buffer.alloc(testBufferSize, 'A');
89+
rawUnequalBenches[i] = runUnequalBenchmark(compareFunc, bufferB, bufferC);
90+
rawEqualBenches[i] = runEqualBenchmark(compareFunc, bufferA1, bufferA2);
91+
}
92+
}
93+
94+
const equalBenches = filterOutliers(rawEqualBenches);
95+
const unequalBenches = filterOutliers(rawUnequalBenches);
96+
97+
// Use a two-sample t-test to determine whether the timing difference between
98+
// the benchmarks is statistically significant.
99+
// https://wikipedia.org/wiki/Student%27s_t-test#Independent_two-sample_t-test
100+
101+
const equalMean = mean(equalBenches);
102+
const unequalMean = mean(unequalBenches);
103+
104+
const equalLen = equalBenches.length;
105+
const unequalLen = unequalBenches.length;
106+
107+
const combinedStd = combinedStandardDeviation(equalBenches, unequalBenches);
108+
const standardErr = combinedStd * Math.sqrt(1 / equalLen + 1 / unequalLen);
109+
110+
return (equalMean - unequalMean) / standardErr;
111+
}
112+
113+
// Returns the mean of an array
114+
function mean(array) {
115+
return array.reduce((sum, val) => sum + val, 0) / array.length;
116+
}
117+
118+
// Returns the sample standard deviation of an array
119+
function standardDeviation(array) {
120+
const arrMean = mean(array);
121+
const total = array.reduce((sum, val) => sum + Math.pow(val - arrMean, 2), 0);
122+
return Math.sqrt(total / (array.length - 1));
123+
}
124+
125+
// Returns the common standard deviation of two arrays
126+
function combinedStandardDeviation(array1, array2) {
127+
const sum1 = Math.pow(standardDeviation(array1), 2) * (array1.length - 1);
128+
const sum2 = Math.pow(standardDeviation(array2), 2) * (array2.length - 1);
129+
return Math.sqrt((sum1 + sum2) / (array1.length + array2.length - 2));
130+
}
131+
132+
// Filter large outliers from an array. A 'large outlier' is a value that is at
133+
// least 50 times larger than the mean. This prevents the tests from failing
134+
// due to the standard deviation increase when a function unexpectedly takes
135+
// a very long time to execute.
136+
function filterOutliers(array) {
137+
const arrMean = mean(array);
138+
return array.filter((value) => value / arrMean < 50);
139+
}
140+
141+
// t_(0.99995, ∞)
142+
// i.e. If a given comparison function is indeed timing-safe, the t-test result
143+
// has a 99.99% chance to be below this threshold. Unfortunately, this means
144+
// that this test will be a bit flakey and will fail 0.01% of the time even if
145+
// crypto.timingSafeEqual is working properly.
146+
// t-table ref: http://www.sjsu.edu/faculty/gerstman/StatPrimer/t-table.pdf
147+
// Note that in reality there are roughly `2 * numTrials - 2` degrees of
148+
// freedom, not ∞. However, assuming `numTrials` is large, this doesn't
149+
// significantly affect the threshold.
150+
const T_THRESHOLD = 3.892;
151+
152+
const t = getTValue(crypto.timingSafeEqual);
153+
assert(
154+
Math.abs(t) < T_THRESHOLD,
155+
`timingSafeEqual should not leak information from its execution time (t=${t})`
156+
);
157+
158+
// As a sanity check to make sure the statistical tests are working, run the
159+
// same benchmarks again, this time with an unsafe comparison function. In this
160+
// case the t-value should be above the threshold.
161+
const unsafeCompare = (bufA, bufB) => bufA.equals(bufB);
162+
const t2 = getTValue(unsafeCompare);
163+
assert(
164+
Math.abs(t2) > T_THRESHOLD,
165+
`Buffer#equals should leak information from its execution time (t=${t2})`
166+
);

0 commit comments

Comments
 (0)