Skip to content

Commit

Permalink
crypto: add crypto.timingSafeEqual()
Browse files Browse the repository at this point in the history
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
  • Loading branch information
not-an-aardvark authored and Fishrock123 committed Sep 14, 2016
1 parent cffe7b7 commit afb9917
Show file tree
Hide file tree
Showing 4 changed files with 199 additions and 0 deletions.
13 changes: 13 additions & 0 deletions doc/api/crypto.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down
3 changes: 3 additions & 0 deletions lib/crypto.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -649,6 +650,8 @@ Object.defineProperty(exports, 'fips', {
set: setFipsCrypto
});

exports.timingSafeEqual = timingSafeEqual;

// Legacy API
Object.defineProperty(exports, 'createCredentials', {
configurable: true,
Expand Down
17 changes: 17 additions & 0 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
166 changes: 166 additions & 0 deletions test/sequential/test-crypto-timing-safe-equal.js
Original file line number Diff line number Diff line change
@@ -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.