From 30157e872a72f78f695037be5a5ea23462d3e751 Mon Sep 17 00:00:00 2001 From: not-an-aardvark Date: Thu, 8 Sep 2016 19:00:58 -0400 Subject: [PATCH 1/4] test: make crypto.timingSafeEqual test less flaky --- ...crypto-timing-safe-equal-benchmark-func.js | 17 +++ ...est-crypto-timing-safe-equal-benchmarks.js | 118 ++++++++++++++++ .../test-crypto-timing-safe-equal.js | 132 ------------------ 3 files changed, 135 insertions(+), 132 deletions(-) create mode 100644 test/fixtures/crypto-timing-safe-equal-benchmark-func.js create mode 100644 test/sequential/test-crypto-timing-safe-equal-benchmarks.js diff --git a/test/fixtures/crypto-timing-safe-equal-benchmark-func.js b/test/fixtures/crypto-timing-safe-equal-benchmark-func.js new file mode 100644 index 00000000000000..96470e3e4434a8 --- /dev/null +++ b/test/fixtures/crypto-timing-safe-equal-benchmark-func.js @@ -0,0 +1,17 @@ +'use strict'; + +const assert = require('assert'); +module.exports = (compareFunc, firstBufFill, secondBufFill, bufSize) => { + const firstBuffer = Buffer.alloc(bufSize, firstBufFill); + const secondBuffer = Buffer.alloc(bufSize, secondBufFill); + + const startTime = process.hrtime(); + const result = compareFunc(firstBuffer, secondBuffer); + 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, firstBufFill === secondBufFill); + + return endTime[0] * 1e9 + endTime[1]; +}; diff --git a/test/sequential/test-crypto-timing-safe-equal-benchmarks.js b/test/sequential/test-crypto-timing-safe-equal-benchmarks.js new file mode 100644 index 00000000000000..a2e1b437649a44 --- /dev/null +++ b/test/sequential/test-crypto-timing-safe-equal-benchmarks.js @@ -0,0 +1,118 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); + +if (!common.hasCrypto) { + common.skip('missing crypto'); + return; +} + +const crypto = require('crypto'); + +const BENCHMARK_FUNC_PATH = + '../fixtures/crypto-timing-safe-equal-benchmark-func'; +function runOneBenchmark(...args) { + const benchmarkFunc = require(BENCHMARK_FUNC_PATH); + const result = benchmarkFunc(...args); + + // Don't let the comparison function get cached. This avoid a timing + // inconsistency due to V8 optimization where the function would take + // less time when called with a specific set of parameters. + delete require.cache[require.resolve(BENCHMARK_FUNC_PATH)]; + return result; +} + +function getTValue(compareFunc) { + const numTrials = 10000; + const bufSize = 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++) { + if (Math.random() < 0.5) { + // First benchmark: comparing two equal buffers + rawEqualBenches[i] = runOneBenchmark(compareFunc, 'A', 'A', bufSize); + // Second benchmark: comparing two unequal buffers + rawUnequalBenches[i] = runOneBenchmark(compareFunc, 'B', 'C', bufSize); + } else { + // Flip the order of the benchmarks half of the time. + rawUnequalBenches[i] = runOneBenchmark(compareFunc, 'B', 'C', bufSize); + rawEqualBenches[i] = runOneBenchmark(compareFunc, 'A', 'A', bufSize); + } + } + + 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})` +); diff --git a/test/sequential/test-crypto-timing-safe-equal.js b/test/sequential/test-crypto-timing-safe-equal.js index 7b04f15339d21d..6e15a577188cc3 100644 --- a/test/sequential/test-crypto-timing-safe-equal.js +++ b/test/sequential/test-crypto-timing-safe-equal.js @@ -32,135 +32,3 @@ assert.throws(function() { 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})` -); From c6a2af236ed2a04d4887ddc656767f763260a746 Mon Sep 17 00:00:00 2001 From: not-an-aardvark Date: Fri, 9 Sep 2016 01:30:55 -0400 Subject: [PATCH 2/4] squash: use common.fixturesDir --- test/sequential/test-crypto-timing-safe-equal-benchmarks.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/sequential/test-crypto-timing-safe-equal-benchmarks.js b/test/sequential/test-crypto-timing-safe-equal-benchmarks.js index a2e1b437649a44..778bdfaeeef126 100644 --- a/test/sequential/test-crypto-timing-safe-equal-benchmarks.js +++ b/test/sequential/test-crypto-timing-safe-equal-benchmarks.js @@ -10,7 +10,7 @@ if (!common.hasCrypto) { const crypto = require('crypto'); const BENCHMARK_FUNC_PATH = - '../fixtures/crypto-timing-safe-equal-benchmark-func'; + `${common.fixturesDir}/crypto-timing-safe-equal-benchmark-func`; function runOneBenchmark(...args) { const benchmarkFunc = require(BENCHMARK_FUNC_PATH); const result = benchmarkFunc(...args); From 5abe8b912302c82ef273b2b060bb5dbba73eaaaf Mon Sep 17 00:00:00 2001 From: not-an-aardvark Date: Fri, 9 Sep 2016 01:31:34 -0400 Subject: [PATCH 3/4] squash: skip tests on Raspberry Pi --- test/sequential/test-crypto-timing-safe-equal-benchmarks.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/sequential/test-crypto-timing-safe-equal-benchmarks.js b/test/sequential/test-crypto-timing-safe-equal-benchmarks.js index 778bdfaeeef126..69ae7e3bd44fbb 100644 --- a/test/sequential/test-crypto-timing-safe-equal-benchmarks.js +++ b/test/sequential/test-crypto-timing-safe-equal-benchmarks.js @@ -7,6 +7,11 @@ if (!common.hasCrypto) { return; } +if (!common.enoughTestMem) { + common.skip('skipping memory-intensive test'); + return; +} + const crypto = require('crypto'); const BENCHMARK_FUNC_PATH = From 0e2e8e949c91bb67a30db8538673ef3e9e1d7f2b Mon Sep 17 00:00:00 2001 From: not-an-aardvark Date: Fri, 9 Sep 2016 01:47:48 -0400 Subject: [PATCH 4/4] squash: skip `skip()`'s 'skipping' --- test/sequential/test-crypto-timing-safe-equal-benchmarks.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/sequential/test-crypto-timing-safe-equal-benchmarks.js b/test/sequential/test-crypto-timing-safe-equal-benchmarks.js index 69ae7e3bd44fbb..de46a899f263f5 100644 --- a/test/sequential/test-crypto-timing-safe-equal-benchmarks.js +++ b/test/sequential/test-crypto-timing-safe-equal-benchmarks.js @@ -8,7 +8,7 @@ if (!common.hasCrypto) { } if (!common.enoughTestMem) { - common.skip('skipping memory-intensive test'); + common.skip('memory-intensive test'); return; }