From 627cbc0ee24a720c6e7e269d194505509d8b9330 Mon Sep 17 00:00:00 2001 From: not-an-aardvark Date: Tue, 23 Aug 2016 22:49:22 -0400 Subject: [PATCH 01/16] Revert "Revert "crypto: add crypto.timingSafeEqual"" This reverts commit 0764bc4711e24de21c0b0066f2cd4bddca62adee. --- 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 insertions(+) create mode 100644 test/sequential/test-crypto-timing-safe-equal.js diff --git a/doc/api/crypto.md b/doc/api/crypto.md index 7a85f20a81bd62..dfef0b2dc098a7 100644 --- a/doc/api/crypto.md +++ b/doc/api/crypto.md @@ -1217,6 +1217,15 @@ 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 9ffff06f7f18ed..1f6e0c46b29d05 100644 --- a/lib/crypto.js +++ b/lib/crypto.js @@ -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, diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 9cf216f2d60440..53c50a6bba4113 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -5771,6 +5771,22 @@ 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); @@ -5903,6 +5919,7 @@ 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 d228c93b4534e9..54f8c4d4561494 100644 --- a/test/sequential/sequential.status +++ b/test/sequential/sequential.status @@ -6,6 +6,11 @@ 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 new file mode 100644 index 00000000000000..a4312f86ada38f --- /dev/null +++ b/test/sequential/test-crypto-timing-safe-equal.js @@ -0,0 +1,144 @@ +// 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})` +); From 7e9869aa74587211355b73efd34663069fa04af1 Mon Sep 17 00:00:00 2001 From: not-an-aardvark Date: Sat, 27 Aug 2016 15:24:40 -0400 Subject: [PATCH 02/16] test: fix timing flake for crypto.timingSafeEqual --- test/sequential/sequential.status | 5 ----- test/sequential/test-crypto-timing-safe-equal.js | 6 ------ 2 files changed, 11 deletions(-) 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 index a4312f86ada38f..90db8453e9d056 100644 --- a/test/sequential/test-crypto-timing-safe-equal.js +++ b/test/sequential/test-crypto-timing-safe-equal.js @@ -1,4 +1,3 @@ -// Flags: --allow_natives_syntax 'use strict'; const common = require('../common'); const assert = require('assert'); @@ -111,11 +110,6 @@ function filterOutliers(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 From ca0e08ab43003a6649b3cda044f02af0adffd6f8 Mon Sep 17 00:00:00 2001 From: not-an-aardvark Date: Sat, 27 Aug 2016 20:51:32 -0400 Subject: [PATCH 03/16] squash: temporarily log detailed results if test fails --- .../test-crypto-timing-safe-equal.js | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/test/sequential/test-crypto-timing-safe-equal.js b/test/sequential/test-crypto-timing-safe-equal.js index 90db8453e9d056..474096fd63051c 100644 --- a/test/sequential/test-crypto-timing-safe-equal.js +++ b/test/sequential/test-crypto-timing-safe-equal.js @@ -79,6 +79,25 @@ function getTValue(compareFunc) { const combinedStd = combinedStandardDeviation(equalBenches, unequalBenches); const standardErr = combinedStd * Math.sqrt(1 / equalLen + 1 / unequalLen); + // FIXME: This if-block is only here to debug tests. + // It should be removed before merging. + if (Math.abs(equalMean - unequalMean) / standardErr > T_THRESHOLD && + compareFunc === crypto.timingSafeEqual) { + console.log({ + equalMean, + unequalMean, + equalLen, + unequalLen, + equalStd: standardDeviation(equalBenches), + unequalStd: standardDeviation(unequalBenches), + combinedStd, + standardErr, + t: (equalMean - unequalMean) / standardErr, + rawEqualBenches: JSON.stringify(rawEqualBenches), + rawUnequalBenches: JSON.stringify(rawUnequalBenches) + }); + } + return (equalMean - unequalMean) / standardErr; } From 03ff774c29f1a13cc935c28c4cc3ca2896e56969 Mon Sep 17 00:00:00 2001 From: not-an-aardvark Date: Sun, 28 Aug 2016 02:56:13 -0400 Subject: [PATCH 04/16] squash: flip the order of the benchmarks --- test/sequential/test-crypto-timing-safe-equal.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/sequential/test-crypto-timing-safe-equal.js b/test/sequential/test-crypto-timing-safe-equal.js index 474096fd63051c..5b46632431d1e4 100644 --- a/test/sequential/test-crypto-timing-safe-equal.js +++ b/test/sequential/test-crypto-timing-safe-equal.js @@ -57,10 +57,13 @@ function getTValue(compareFunc) { const rawUnequalBenches = Array(numTrials); for (let i = 0; i < numTrials; i++) { - // First benchmark: comparing two equal buffers - rawEqualBenches[i] = runBenchmark(compareFunc, bufferA1, bufferA2, true); + // FIXME: The order of these benchmarks is swapped. + // Flip them back before merging. + // Second benchmark: comparing two unequal buffers rawUnequalBenches[i] = runBenchmark(compareFunc, bufferB, bufferC, false); + // First benchmark: comparing two equal buffers + rawEqualBenches[i] = runBenchmark(compareFunc, bufferA1, bufferA2, true); } const equalBenches = filterOutliers(rawEqualBenches); From 0c2077c1496cfb41a328137d072944aab8e623d4 Mon Sep 17 00:00:00 2001 From: not-an-aardvark Date: Sun, 28 Aug 2016 03:24:22 -0400 Subject: [PATCH 05/16] squash: change the order of the benchmarks every iteration --- test/sequential/test-crypto-timing-safe-equal.js | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/test/sequential/test-crypto-timing-safe-equal.js b/test/sequential/test-crypto-timing-safe-equal.js index 5b46632431d1e4..da94a7988de355 100644 --- a/test/sequential/test-crypto-timing-safe-equal.js +++ b/test/sequential/test-crypto-timing-safe-equal.js @@ -57,13 +57,17 @@ function getTValue(compareFunc) { const rawUnequalBenches = Array(numTrials); for (let i = 0; i < numTrials; i++) { - // FIXME: The order of these benchmarks is swapped. - // Flip them back before merging. - // Second benchmark: comparing two unequal buffers - rawUnequalBenches[i] = runBenchmark(compareFunc, bufferB, bufferC, false); - // First benchmark: comparing two equal buffers - rawEqualBenches[i] = runBenchmark(compareFunc, bufferA1, bufferA2, true); + if (i % 2) { + // 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); + } else { + // Flip the order of the benchmarks every other iteration + rawUnequalBenches[i] = runBenchmark(compareFunc, bufferB, bufferC, false); + rawEqualBenches[i] = runBenchmark(compareFunc, bufferA1, bufferA2, true); + } } const equalBenches = filterOutliers(rawEqualBenches); From 872e56341170fafc4f8a9708ad66dc29c074f25c Mon Sep 17 00:00:00 2001 From: not-an-aardvark Date: Sun, 28 Aug 2016 03:56:31 -0400 Subject: [PATCH 06/16] squash: split into two benchmark functions --- .../test-crypto-timing-safe-equal.js | 34 ++++++++++--------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/test/sequential/test-crypto-timing-safe-equal.js b/test/sequential/test-crypto-timing-safe-equal.js index da94a7988de355..3878520720c337 100644 --- a/test/sequential/test-crypto-timing-safe-equal.js +++ b/test/sequential/test-crypto-timing-safe-equal.js @@ -33,14 +33,25 @@ 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) { +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 that it doesn't // get discarded due to engine optimizations. - assert.strictEqual(result, expectedResult); + 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]; } @@ -57,17 +68,10 @@ function getTValue(compareFunc) { const rawUnequalBenches = Array(numTrials); for (let i = 0; i < numTrials; i++) { - - if (i % 2) { - // 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); - } else { - // Flip the order of the benchmarks every other iteration - rawUnequalBenches[i] = runBenchmark(compareFunc, bufferB, bufferC, false); - rawEqualBenches[i] = runBenchmark(compareFunc, bufferA1, bufferA2, true); - } + // First benchmark: comparing two equal buffers + rawEqualBenches[i] = runEqualBenchmark(compareFunc, bufferA1, bufferA2); + // Second benchmark: comparing two unequal buffers + rawUnequalBenches[i] = runUnequalBenchmark(compareFunc, bufferB, bufferC); } const equalBenches = filterOutliers(rawEqualBenches); @@ -99,9 +103,7 @@ function getTValue(compareFunc) { unequalStd: standardDeviation(unequalBenches), combinedStd, standardErr, - t: (equalMean - unequalMean) / standardErr, - rawEqualBenches: JSON.stringify(rawEqualBenches), - rawUnequalBenches: JSON.stringify(rawUnequalBenches) + t: (equalMean - unequalMean) / standardErr }); } From e484b5f4c989ab95350b23610ae0610384640d02 Mon Sep 17 00:00:00 2001 From: not-an-aardvark Date: Sun, 28 Aug 2016 04:13:29 -0400 Subject: [PATCH 07/16] squash: remove temporary logging --- .../sequential/test-crypto-timing-safe-equal.js | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/test/sequential/test-crypto-timing-safe-equal.js b/test/sequential/test-crypto-timing-safe-equal.js index 3878520720c337..c317b08e2b7cac 100644 --- a/test/sequential/test-crypto-timing-safe-equal.js +++ b/test/sequential/test-crypto-timing-safe-equal.js @@ -90,23 +90,6 @@ function getTValue(compareFunc) { const combinedStd = combinedStandardDeviation(equalBenches, unequalBenches); const standardErr = combinedStd * Math.sqrt(1 / equalLen + 1 / unequalLen); - // FIXME: This if-block is only here to debug tests. - // It should be removed before merging. - if (Math.abs(equalMean - unequalMean) / standardErr > T_THRESHOLD && - compareFunc === crypto.timingSafeEqual) { - console.log({ - equalMean, - unequalMean, - equalLen, - unequalLen, - equalStd: standardDeviation(equalBenches), - unequalStd: standardDeviation(unequalBenches), - combinedStd, - standardErr, - t: (equalMean - unequalMean) / standardErr - }); - } - return (equalMean - unequalMean) / standardErr; } From 7d60f7581e8ef5eaab3fadf87df1513e77706a71 Mon Sep 17 00:00:00 2001 From: not-an-aardvark Date: Wed, 31 Aug 2016 21:41:43 -0400 Subject: [PATCH 08/16] squash: add some eval() --- test/sequential/test-crypto-timing-safe-equal.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/sequential/test-crypto-timing-safe-equal.js b/test/sequential/test-crypto-timing-safe-equal.js index c317b08e2b7cac..6109c7e46b1731 100644 --- a/test/sequential/test-crypto-timing-safe-equal.js +++ b/test/sequential/test-crypto-timing-safe-equal.js @@ -41,6 +41,9 @@ function runEqualBenchmark(compareFunc, bufferA, bufferB) { // Ensure that the result of the function call gets used, so that it doesn't // get discarded due to engine optimizations. assert.strictEqual(result, true); + + // Prevent V8 from optimizing this function + eval(''); return endTime[0] * 1e9 + endTime[1]; } @@ -52,6 +55,7 @@ function runUnequalBenchmark(compareFunc, bufferA, bufferB) { const endTime = process.hrtime(startTime); assert.strictEqual(result, false); + eval(''); return endTime[0] * 1e9 + endTime[1]; } @@ -90,6 +94,9 @@ function getTValue(compareFunc) { const combinedStd = combinedStandardDeviation(equalBenches, unequalBenches); const standardErr = combinedStd * Math.sqrt(1 / equalLen + 1 / unequalLen); + // Prevent V8 from optimizing this function + eval(''); + return (equalMean - unequalMean) / standardErr; } From f901e3223cd918f031fa3aae13ba67f61c17d156 Mon Sep 17 00:00:00 2001 From: not-an-aardvark Date: Thu, 1 Sep 2016 01:29:13 -0400 Subject: [PATCH 09/16] squash: never mind about eval() --- test/sequential/test-crypto-timing-safe-equal.js | 7 ------- 1 file changed, 7 deletions(-) diff --git a/test/sequential/test-crypto-timing-safe-equal.js b/test/sequential/test-crypto-timing-safe-equal.js index 6109c7e46b1731..c317b08e2b7cac 100644 --- a/test/sequential/test-crypto-timing-safe-equal.js +++ b/test/sequential/test-crypto-timing-safe-equal.js @@ -41,9 +41,6 @@ function runEqualBenchmark(compareFunc, bufferA, bufferB) { // Ensure that the result of the function call gets used, so that it doesn't // get discarded due to engine optimizations. assert.strictEqual(result, true); - - // Prevent V8 from optimizing this function - eval(''); return endTime[0] * 1e9 + endTime[1]; } @@ -55,7 +52,6 @@ function runUnequalBenchmark(compareFunc, bufferA, bufferB) { const endTime = process.hrtime(startTime); assert.strictEqual(result, false); - eval(''); return endTime[0] * 1e9 + endTime[1]; } @@ -94,9 +90,6 @@ function getTValue(compareFunc) { const combinedStd = combinedStandardDeviation(equalBenches, unequalBenches); const standardErr = combinedStd * Math.sqrt(1 / equalLen + 1 / unequalLen); - // Prevent V8 from optimizing this function - eval(''); - return (equalMean - unequalMean) / standardErr; } From 5a36833fa6fc3f8c1a5d00a6e93010dca37ea327 Mon Sep 17 00:00:00 2001 From: not-an-aardvark Date: Thu, 1 Sep 2016 02:52:04 -0400 Subject: [PATCH 10/16] squash: use memory very inefficiently --- .../test-crypto-timing-safe-equal.js | 56 ++++++++++--------- 1 file changed, 30 insertions(+), 26 deletions(-) diff --git a/test/sequential/test-crypto-timing-safe-equal.js b/test/sequential/test-crypto-timing-safe-equal.js index c317b08e2b7cac..5197400e534a6a 100644 --- a/test/sequential/test-crypto-timing-safe-equal.js +++ b/test/sequential/test-crypto-timing-safe-equal.js @@ -33,44 +33,48 @@ assert.throws(function() { crypto.timingSafeEqual(Buffer.from([1, 2]), 'not a buffer'); }, 'should throw if the second argument is not a buffer'); -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 that 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]; -} - 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++) { + + // 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]; + } // First benchmark: comparing two equal buffers + const bufferA1 = Buffer.alloc(testBufferSize, 'A'); + const bufferA2 = Buffer.alloc(testBufferSize, 'A'); rawEqualBenches[i] = runEqualBenchmark(compareFunc, bufferA1, bufferA2); + // Second benchmark: comparing two unequal buffers + const bufferB = Buffer.alloc(testBufferSize, 'B'); + const bufferC = Buffer.alloc(testBufferSize, 'C'); rawUnequalBenches[i] = runUnequalBenchmark(compareFunc, bufferB, bufferC); } From 9a0bba2bb8a26633895917c473d342b5cac742e7 Mon Sep 17 00:00:00 2001 From: not-an-aardvark Date: Fri, 2 Sep 2016 01:06:01 -0400 Subject: [PATCH 11/16] squash: update t-threshold for 1/10000 failure rate --- test/sequential/test-crypto-timing-safe-equal.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/sequential/test-crypto-timing-safe-equal.js b/test/sequential/test-crypto-timing-safe-equal.js index 5197400e534a6a..230c6793f01e19 100644 --- a/test/sequential/test-crypto-timing-safe-equal.js +++ b/test/sequential/test-crypto-timing-safe-equal.js @@ -125,16 +125,16 @@ function filterOutliers(array) { return array.filter((value) => value / arrMean < 50); } -// t_(0.9995, ∞) +// t_(0.99995, ∞) // 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 +// 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.291; +const T_THRESHOLD = 3.892; const t = getTValue(crypto.timingSafeEqual); assert( From bcee4a85c8853d5fa3705a1f7aa588fa62206821 Mon Sep 17 00:00:00 2001 From: not-an-aardvark Date: Fri, 2 Sep 2016 17:24:21 -0400 Subject: [PATCH 12/16] squash: allocate everything before benchmarking --- test/sequential/test-crypto-timing-safe-equal.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/sequential/test-crypto-timing-safe-equal.js b/test/sequential/test-crypto-timing-safe-equal.js index 230c6793f01e19..6e1c04fcdf0996 100644 --- a/test/sequential/test-crypto-timing-safe-equal.js +++ b/test/sequential/test-crypto-timing-safe-equal.js @@ -67,14 +67,15 @@ function getTValue(compareFunc) { assert.strictEqual(result, false); return endTime[0] * 1e9 + endTime[1]; } - // First benchmark: comparing two equal buffers 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 - const bufferB = Buffer.alloc(testBufferSize, 'B'); - const bufferC = Buffer.alloc(testBufferSize, 'C'); rawUnequalBenches[i] = runUnequalBenchmark(compareFunc, bufferB, bufferC); } From 240e8ac2be0185ee1d80160492d69b5615b76ad5 Mon Sep 17 00:00:00 2001 From: not-an-aardvark Date: Fri, 2 Sep 2016 18:00:12 -0400 Subject: [PATCH 13/16] squash: change the order of the benchmarks every iteration (take 2) --- .../sequential/test-crypto-timing-safe-equal.js | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/test/sequential/test-crypto-timing-safe-equal.js b/test/sequential/test-crypto-timing-safe-equal.js index 6e1c04fcdf0996..5b7bef615afbfd 100644 --- a/test/sequential/test-crypto-timing-safe-equal.js +++ b/test/sequential/test-crypto-timing-safe-equal.js @@ -72,11 +72,18 @@ function getTValue(compareFunc) { 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); + if (i % 2) { + // 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. + rawUnequalBenches[i] = runUnequalBenchmark(compareFunc, bufferB, bufferC); + rawEqualBenches[i] = runEqualBenchmark(compareFunc, bufferA1, bufferA2); + } } const equalBenches = filterOutliers(rawEqualBenches); From d810e06faaebd0df6f174d065afb1928c1132e95 Mon Sep 17 00:00:00 2001 From: not-an-aardvark Date: Fri, 2 Sep 2016 18:41:47 -0400 Subject: [PATCH 14/16] squash: alternate the allocation order too --- test/sequential/test-crypto-timing-safe-equal.js | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/test/sequential/test-crypto-timing-safe-equal.js b/test/sequential/test-crypto-timing-safe-equal.js index 5b7bef615afbfd..7b04f15339d21d 100644 --- a/test/sequential/test-crypto-timing-safe-equal.js +++ b/test/sequential/test-crypto-timing-safe-equal.js @@ -67,12 +67,13 @@ function getTValue(compareFunc) { assert.strictEqual(result, false); return endTime[0] * 1e9 + endTime[1]; } - const bufferA1 = Buffer.alloc(testBufferSize, 'A'); - const bufferB = Buffer.alloc(testBufferSize, 'B'); - const bufferA2 = Buffer.alloc(testBufferSize, 'A'); - const bufferC = Buffer.alloc(testBufferSize, 'C'); 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); @@ -81,6 +82,10 @@ function getTValue(compareFunc) { } 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); } From 5a6f19e66190cdfc43f328548371c1ef9deb25dc Mon Sep 17 00:00:00 2001 From: not-an-aardvark Date: Fri, 2 Sep 2016 21:48:31 -0400 Subject: [PATCH 15/16] squash: add note to docs about ensuring timing-safety --- doc/api/crypto.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/doc/api/crypto.md b/doc/api/crypto.md index dfef0b2dc098a7..0a95b8c6b8708f 100644 --- a/doc/api/crypto.md +++ b/doc/api/crypto.md @@ -1226,6 +1226,8 @@ comparing HMAC digests or secret values like authentication cookies or `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`. From 3c5ba3c1a3558b8d46c0fb6000b1104d68c44a3d Mon Sep 17 00:00:00 2001 From: not-an-aardvark Date: Sat, 3 Sep 2016 14:45:09 -0400 Subject: [PATCH 16/16] squash: add linebreaks in docs --- doc/api/crypto.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/api/crypto.md b/doc/api/crypto.md index 0a95b8c6b8708f..07f9a005c3a43a 100644 --- a/doc/api/crypto.md +++ b/doc/api/crypto.md @@ -1226,7 +1226,9 @@ comparing HMAC digests or secret values like authentication cookies or `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. +**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)