From b7c67a9c7ca8f450269e3e096440b972ad3574ee Mon Sep 17 00:00:00 2001 From: James M Snell Date: Mon, 22 Aug 2016 11:15:00 -0700 Subject: [PATCH] Revert "crypto: add crypto.timingSafeEqual" This reverts commit 0fc5e0dcd9c628b62fc9908daf64bc5db8d503c0. Additional testing indicates that there may still be timing issues with this implementation. Revert in order to give more time for testing before this goes out into a release... Refs: https://github.com/nodejs/node/pull/8040 Refs: https://github.com/nodejs/node/pull/8203 --- doc/api/crypto.md | 9 -- lib/crypto.js | 3 - src/node_crypto.cc | 17 --- test/sequential/sequential.status | 5 - .../test-crypto-timing-safe-equal.js | 144 ------------------ 5 files changed, 178 deletions(-) delete mode 100644 test/sequential/test-crypto-timing-safe-equal.js diff --git a/doc/api/crypto.md b/doc/api/crypto.md index 8221aea54ca823..1d4b451be0d682 100644 --- a/doc/api/crypto.md +++ b/doc/api/crypto.md @@ -1217,15 +1217,6 @@ 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. - ### crypto.privateEncrypt(private_key, buffer) Encrypts `buffer` with `private_key`. diff --git a/lib/crypto.js b/lib/crypto.js index 1f6e0c46b29d05..9ffff06f7f18ed 100644 --- a/lib/crypto.js +++ b/lib/crypto.js @@ -16,7 +16,6 @@ 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'); @@ -650,8 +649,6 @@ Object.defineProperty(exports, 'fips', { set: setFipsCrypto }); -exports.timingSafeEqual = timingSafeEqual; - // Legacy API Object.defineProperty(exports, 'createCredentials', { configurable: true, diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 53c50a6bba4113..9cf216f2d60440 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -5771,22 +5771,6 @@ void ExportChallenge(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(outString); } -void TimingSafeEqual(const FunctionCallbackInfo& 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); @@ -5919,7 +5903,6 @@ void InitCrypto(Local 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); diff --git a/test/sequential/sequential.status b/test/sequential/sequential.status index 54f8c4d4561494..d228c93b4534e9 100644 --- a/test/sequential/sequential.status +++ b/test/sequential/sequential.status @@ -6,11 +6,6 @@ prefix sequential [true] # This section applies to all platforms -# crypto.timingSafeEqual contains a statistical timing test to verify that the -# function is timing-safe. As a result, the test sometimes fails due to random -# timing fluctuations. -test-crypto-timing-safe-equal : PASS,FLAKY - [$system==win32] [$system==linux] diff --git a/test/sequential/test-crypto-timing-safe-equal.js b/test/sequential/test-crypto-timing-safe-equal.js deleted file mode 100644 index a4312f86ada38f..00000000000000 --- a/test/sequential/test-crypto-timing-safe-equal.js +++ /dev/null @@ -1,144 +0,0 @@ -// Flags: --allow_natives_syntax -'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 runBenchmark(compareFunc, bufferA, bufferB, expectedResult) { - 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 that it doesn't - // get discarded due to engine optimizations. - assert.strictEqual(result, expectedResult); - return endTime[0] * 1e9 + endTime[1]; -} - -function getTValue(compareFunc) { - const numTrials = 10000; - const testBufferSize = 10000; - // Perform benchmarks to verify that timingSafeEqual is actually timing-safe. - const bufferA1 = Buffer.alloc(testBufferSize, 'A'); - const bufferA2 = Buffer.alloc(testBufferSize, 'A'); - const bufferB = Buffer.alloc(testBufferSize, 'B'); - const bufferC = Buffer.alloc(testBufferSize, 'C'); - - const rawEqualBenches = Array(numTrials); - const rawUnequalBenches = Array(numTrials); - - for (let i = 0; i < numTrials; i++) { - // First benchmark: comparing two equal buffers - rawEqualBenches[i] = runBenchmark(compareFunc, bufferA1, bufferA2, true); - // Second benchmark: comparing two unequal buffers - rawUnequalBenches[i] = runBenchmark(compareFunc, bufferB, bufferC, false); - } - - 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); -} - -// Force optimization before starting the benchmark -runBenchmark(crypto.timingSafeEqual, Buffer.from('A'), Buffer.from('A'), true); -eval('%OptimizeFunctionOnNextCall(runBenchmark)'); -runBenchmark(crypto.timingSafeEqual, Buffer.from('A'), Buffer.from('A'), true); - -// t_(0.9995, ∞) -// i.e. If a given comparison function is indeed timing-safe, the t-test result -// has a 99.9% chance to be below this threshold. Unfortunately, this means that -// this test will be a bit flakey and will fail 0.1% 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.291; - -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})` -);