Permalink
Browse files

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: #8304
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
  • Loading branch information...
not-an-aardvark authored and Fishrock123 committed Aug 24, 2016
1 parent cffe7b7 commit afb9917f16377312a5a22ef05886cda8323d9363
Showing with 199 additions and 0 deletions.
  1. +13 −0 doc/api/crypto.md
  2. +3 −0 lib/crypto.js
  3. +17 −0 src/node_crypto.cc
  4. +166 −0 test/sequential/test-crypto-timing-safe-equal.js
View
@@ -1217,6 +1217,19 @@ keys:
All paddings are defined in `crypto.constants`.
+### crypto.timingSafeEqual(a, b)
+
+Returns true if `a` is equal to `b`, without leaking timing information that
+would allow an attacker to guess one of the values. This is suitable for
+comparing HMAC digests or secret values like authentication cookies or
+[capability urls](https://www.w3.org/TR/capability-urls/).
+
+`a` and `b` must both be `Buffer`s, and they must have the same length.
+
+**Note**: Use of `crypto.timingSafeEqual` does not guarantee that the
+*surrounding* code is timing-safe. Care should be taken to ensure that the
+surrounding code does not introduce timing vulnerabilities.
+
### crypto.privateEncrypt(private_key, buffer)
Encrypts `buffer` with `private_key`.
View
@@ -16,6 +16,7 @@ const getHashes = binding.getHashes;
const getCurves = binding.getCurves;
const getFipsCrypto = binding.getFipsCrypto;
const setFipsCrypto = binding.setFipsCrypto;
+const timingSafeEqual = binding.timingSafeEqual;
const Buffer = require('buffer').Buffer;
const stream = require('stream');
@@ -649,6 +650,8 @@ Object.defineProperty(exports, 'fips', {
set: setFipsCrypto
});
+exports.timingSafeEqual = timingSafeEqual;
+
// Legacy API
Object.defineProperty(exports, 'createCredentials', {
configurable: true,
View
@@ -5771,6 +5771,22 @@ void ExportChallenge(const FunctionCallbackInfo<Value>& args) {
args.GetReturnValue().Set(outString);
}
+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");
+
+ 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");
+ }
+
+ const char* buf1 = Buffer::Data(args[0]);
+ const char* buf2 = Buffer::Data(args[1]);
+
+ return args.GetReturnValue().Set(CRYPTO_memcmp(buf1, buf2, buf_length) == 0);
+}
void InitCryptoOnce() {
OPENSSL_config(NULL);
@@ -5903,6 +5919,7 @@ void InitCrypto(Local<Object> target,
env->SetMethod(target, "setFipsCrypto", SetFipsCrypto);
env->SetMethod(target, "PBKDF2", PBKDF2);
env->SetMethod(target, "randomBytes", RandomBytes);
+ env->SetMethod(target, "timingSafeEqual", TimingSafeEqual);
env->SetMethod(target, "getSSLCiphers", GetSSLCiphers);
env->SetMethod(target, "getCiphers", GetCiphers);
env->SetMethod(target, "getHashes", GetHashes);
@@ -0,0 +1,166 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+
+if (!common.hasCrypto) {
+ common.skip('missing crypto');
+ return;
+}
+
+const crypto = require('crypto');
+
+assert.strictEqual(
+ crypto.timingSafeEqual(Buffer.from('foo'), Buffer.from('foo')),
+ true,
+ 'should consider equal strings to be equal'
+);
+
+assert.strictEqual(
+ crypto.timingSafeEqual(Buffer.from('foo'), Buffer.from('bar')),
+ false,
+ 'should consider unequal strings to be unequal'
+);
+
+assert.throws(function() {
+ crypto.timingSafeEqual(Buffer.from([1, 2, 3]), Buffer.from([1, 2]));
+}, 'should throw when given buffers with different lengths');
+
+assert.throws(function() {
+ crypto.timingSafeEqual('not a buffer', Buffer.from([1, 2]));
+}, 'should throw if the first argument is not a buffer');
+
+assert.throws(function() {
+ crypto.timingSafeEqual(Buffer.from([1, 2]), 'not a buffer');
+}, 'should throw if the second argument is not a buffer');
+
+function getTValue(compareFunc) {
+ const numTrials = 10000;
+ const testBufferSize = 10000;
+ // Perform benchmarks to verify that timingSafeEqual is actually timing-safe.
+
+ const rawEqualBenches = Array(numTrials);
+ const rawUnequalBenches = Array(numTrials);
+
+ for (let i = 0; i < numTrials; i++) {
+
+ // The `runEqualBenchmark` and `runUnequalBenchmark` functions are
+ // intentionally redefined on every iteration of this loop, to avoid
+ // timing inconsistency.
+ function runEqualBenchmark(compareFunc, bufferA, bufferB) {
+ const startTime = process.hrtime();
+ const result = compareFunc(bufferA, bufferB);
+ const endTime = process.hrtime(startTime);
+
+ // Ensure that the result of the function call gets used, so it doesn't
+ // get discarded due to engine optimizations.
+ assert.strictEqual(result, true);
+ return endTime[0] * 1e9 + endTime[1];
+ }
+
+ // This is almost the same as the runEqualBenchmark function, but it's
+ // duplicated to avoid timing issues with V8 optimization/inlining.
+ function runUnequalBenchmark(compareFunc, bufferA, bufferB) {
+ const startTime = process.hrtime();
+ const result = compareFunc(bufferA, bufferB);
+ const endTime = process.hrtime(startTime);
+
+ assert.strictEqual(result, false);
+ return endTime[0] * 1e9 + endTime[1];
+ }
+
+ if (i % 2) {
+ const bufferA1 = Buffer.alloc(testBufferSize, 'A');
+ const bufferB = Buffer.alloc(testBufferSize, 'B');
+ const bufferA2 = Buffer.alloc(testBufferSize, 'A');
+ const bufferC = Buffer.alloc(testBufferSize, 'C');
+
+ // First benchmark: comparing two equal buffers
+ rawEqualBenches[i] = runEqualBenchmark(compareFunc, bufferA1, bufferA2);
+
+ // Second benchmark: comparing two unequal buffers
+ rawUnequalBenches[i] = runUnequalBenchmark(compareFunc, bufferB, bufferC);
+ } else {
+ // Swap the order of the benchmarks every second iteration, to avoid any
+ // patterns caused by memory usage.
+ const bufferB = Buffer.alloc(testBufferSize, 'B');
+ const bufferA1 = Buffer.alloc(testBufferSize, 'A');
+ const bufferC = Buffer.alloc(testBufferSize, 'C');
+ const bufferA2 = Buffer.alloc(testBufferSize, 'A');
+ rawUnequalBenches[i] = runUnequalBenchmark(compareFunc, bufferB, bufferC);
+ rawEqualBenches[i] = runEqualBenchmark(compareFunc, bufferA1, bufferA2);
+ }
+ }
+
+ const equalBenches = filterOutliers(rawEqualBenches);
+ const unequalBenches = filterOutliers(rawUnequalBenches);
+
+ // Use a two-sample t-test to determine whether the timing difference between
+ // the benchmarks is statistically significant.
+ // https://wikipedia.org/wiki/Student%27s_t-test#Independent_two-sample_t-test
+
+ const equalMean = mean(equalBenches);
+ const unequalMean = mean(unequalBenches);
+
+ const equalLen = equalBenches.length;
+ const unequalLen = unequalBenches.length;
+
+ const combinedStd = combinedStandardDeviation(equalBenches, unequalBenches);
+ const standardErr = combinedStd * Math.sqrt(1 / equalLen + 1 / unequalLen);
+
+ return (equalMean - unequalMean) / standardErr;
+}
+
+// Returns the mean of an array
+function mean(array) {
+ return array.reduce((sum, val) => sum + val, 0) / array.length;
+}
+
+// Returns the sample standard deviation of an array
+function standardDeviation(array) {
+ const arrMean = mean(array);
+ const total = array.reduce((sum, val) => sum + Math.pow(val - arrMean, 2), 0);
+ return Math.sqrt(total / (array.length - 1));
+}
+
+// Returns the common standard deviation of two arrays
+function combinedStandardDeviation(array1, array2) {
+ const sum1 = Math.pow(standardDeviation(array1), 2) * (array1.length - 1);
+ const sum2 = Math.pow(standardDeviation(array2), 2) * (array2.length - 1);
+ return Math.sqrt((sum1 + sum2) / (array1.length + array2.length - 2));
+}
+
+// Filter large outliers from an array. A 'large outlier' is a value that is at
+// least 50 times larger than the mean. This prevents the tests from failing
+// due to the standard deviation increase when a function unexpectedly takes
+// a very long time to execute.
+function filterOutliers(array) {
+ const arrMean = mean(array);
+ return array.filter((value) => value / arrMean < 50);
+}
+
+// t_(0.99995, ∞)
+// i.e. If a given comparison function is indeed timing-safe, the t-test result
+// has a 99.99% chance to be below this threshold. Unfortunately, this means
+// that this test will be a bit flakey and will fail 0.01% of the time even if
+// crypto.timingSafeEqual is working properly.
+// t-table ref: http://www.sjsu.edu/faculty/gerstman/StatPrimer/t-table.pdf
+// Note that in reality there are roughly `2 * numTrials - 2` degrees of
+// freedom, not ∞. However, assuming `numTrials` is large, this doesn't
+// significantly affect the threshold.
+const T_THRESHOLD = 3.892;
+
+const t = getTValue(crypto.timingSafeEqual);
+assert(
+ Math.abs(t) < T_THRESHOLD,
+ `timingSafeEqual should not leak information from its execution time (t=${t})`
+);
+
+// As a sanity check to make sure the statistical tests are working, run the
+// same benchmarks again, this time with an unsafe comparison function. In this
+// case the t-value should be above the threshold.
+const unsafeCompare = (bufA, bufB) => bufA.equals(bufB);
+const t2 = getTValue(unsafeCompare);
+assert(
+ Math.abs(t2) > T_THRESHOLD,
+ `Buffer#equals should leak information from its execution time (t=${t2})`
+);

0 comments on commit afb9917

Please sign in to comment.