From 72f21560839988a23dcd7e940b009b0e7a9bf16a Mon Sep 17 00:00:00 2001 From: ZaneHannanAU Date: Tue, 19 Jun 2018 15:54:17 +1000 Subject: [PATCH 1/4] Change `timingSafeEqual` to use `byteLength`. Former implementation of `timingSafeEqual` would allow different length; allowing a core dump. ```shell [zeen3@zeen3 ~]$ node Running node v10.4.1 (npm v6.1.0) > crypto.timingSafeEqual(new BigUint64Array(32), new Uint32Array(32)) node[16304]: ../src/node_crypto.cc:5158:void node::crypto::TimingSafeEqual(const v8::FunctionCallbackInfo&): Assertion `(buf_length) == (Buffer::Length(args[1]))' failed. 1: node::Abort() [node] 2: 0x8938b5 [node] 3: 0x97212a [node] 4: 0xb00989 [node] 5: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) [node] 6: 0x2dbaff1841bd Aborted (core dumped) [zeen3@zeen3 ~]$ node Running node v10.4.1 (npm v6.1.0) > crypto.timingSafeEqual(new BigUint64Array(32), new Uint32Array(64)) RangeError [ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH]: Input buffers must have the same length at Object.timingSafeEqual (internal/crypto/util.js:81:11) > crypto.timingSafeEqual(new BigUint64Array(32), new Uint32Array(32)) node[16856]: ../src/node_crypto.cc:5158:void node::crypto::TimingSafeEqual(const v8::FunctionCallbackInfo&): Assertion `(buf_length) == (Buffer::Length(args[1]))' failed. 1: node::Abort() [node] 2: 0x8938b5 [node] 3: 0x97212a [node] 4: 0xb00989 [node] 5: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) [node] 6: 0x128c0a5041bd Aborted (core dumped) [zeen3@zeen3 ~]$ ``` Though unlikely to occur it's possible. --- lib/internal/crypto/util.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/crypto/util.js b/lib/internal/crypto/util.js index 32c9eb7f6a12b6..bd56edc89134a8 100644 --- a/lib/internal/crypto/util.js +++ b/lib/internal/crypto/util.js @@ -77,7 +77,7 @@ function timingSafeEqual(buf1, buf2) { throw new ERR_INVALID_ARG_TYPE('buf2', ['Buffer', 'TypedArray', 'DataView'], buf2); } - if (buf1.length !== buf2.length) { + if (buf1.byteLength !== buf2.byteLength) { throw new ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH(); } return _timingSafeEqual(buf1, buf2); From 90ff08f621a8c88d727a9ef4ad10d5e7cb5d91b3 Mon Sep 17 00:00:00 2001 From: ZaneHannanAU Date: Tue, 19 Jun 2018 21:17:45 +1000 Subject: [PATCH 2/4] Update test-crypto-timing-safe-equal.js Add various modern ArrayBuffer based timingSafeEqual variants. --- .../test-crypto-timing-safe-equal.js | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/test/sequential/test-crypto-timing-safe-equal.js b/test/sequential/test-crypto-timing-safe-equal.js index 6aaf7de2284a67..c6077ef8bfa887 100644 --- a/test/sequential/test-crypto-timing-safe-equal.js +++ b/test/sequential/test-crypto-timing-safe-equal.js @@ -12,6 +12,36 @@ assert.strictEqual( 'should consider equal strings to be equal' ); +assert.strictEqual( + crypto.timingSafeEqual(Uint8Array.of(1, 0), Uint16Array.of(1)), + true, + 'should consider equal binary integers to be equal' +); + +assert.strictEqual( + crypto.timingSafeEqual(Uint8Array.of(0, 1), Uint16Array.of(1)), + false, + 'should consider unequal binary integers to be unequal' +); + +assert.strictEqual( + crypto.timingSafeEqual(Uint8Array.of(1, 0, 0, 0, 0, 0, 0), BigUint64Array.of(1n)), + true, + 'should consider equal binary integers to be equal' +); + +assert.strictEqual( + crypto.timingSafeEqual(Buffer.allocUnsafe(8).fill(255), BigInt64Array.of(-1n)), + true, + 'should consider equal binary integers to be equal' +); + +assert.strictEqual( + crypto.timingSafeEqual(new DataView(new ArrayBuffer(8)), BigInt64Array.of(0n)), + true, + 'should consider equal views to be equal' +); + assert.strictEqual( crypto.timingSafeEqual(Buffer.from('foo'), Buffer.from('bar')), false, @@ -27,6 +57,15 @@ common.expectsError( } ); +common.expectsError( + () => crypto.timingSafeEqual(Uint8Array.of(1, 2, 3), Uint16Array.of(1, 2, 3)), + { + code: 'ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH', + type: RangeError, + message: 'Input buffers must have the same length' + } +); + common.expectsError( () => crypto.timingSafeEqual('not a buffer', Buffer.from([1, 2])), { From 6eba5d01aa9191856823d83b5c8ff99f4bd9aa8e Mon Sep 17 00:00:00 2001 From: ZaneHannanAU Date: Tue, 19 Jun 2018 22:23:48 +1000 Subject: [PATCH 3/4] Update test-crypto-timing-safe-equal.js Linter length < 80 Fix one issue where I wrote the wrong amount of tests. --- .../test-crypto-timing-safe-equal.js | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/test/sequential/test-crypto-timing-safe-equal.js b/test/sequential/test-crypto-timing-safe-equal.js index c6077ef8bfa887..47e390a4c3ff99 100644 --- a/test/sequential/test-crypto-timing-safe-equal.js +++ b/test/sequential/test-crypto-timing-safe-equal.js @@ -25,19 +25,28 @@ assert.strictEqual( ); assert.strictEqual( - crypto.timingSafeEqual(Uint8Array.of(1, 0, 0, 0, 0, 0, 0), BigUint64Array.of(1n)), + crypto.timingSafeEqual( + Uint8Array.of(1, 0, 0, 0, 0, 0, 0, 0), + BigUint64Array.of(1n) + ), true, 'should consider equal binary integers to be equal' ); assert.strictEqual( - crypto.timingSafeEqual(Buffer.allocUnsafe(8).fill(255), BigInt64Array.of(-1n)), + crypto.timingSafeEqual( + Buffer.allocUnsafe(8).fill(255), + BigInt64Array.of(-1n) + ), true, 'should consider equal binary integers to be equal' ); assert.strictEqual( - crypto.timingSafeEqual(new DataView(new ArrayBuffer(8)), BigInt64Array.of(0n)), + crypto.timingSafeEqual( + new DataView(new ArrayBuffer(8)), + BigInt64Array.of(0n) + ), true, 'should consider equal views to be equal' ); @@ -58,7 +67,10 @@ common.expectsError( ); common.expectsError( - () => crypto.timingSafeEqual(Uint8Array.of(1, 2, 3), Uint16Array.of(1, 2, 3)), + () => crypto.timingSafeEqual( + Uint8Array.of(1, 2, 3), + Uint16Array.of(1, 2, 3) + ), { code: 'ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH', type: RangeError, From 06a4f4f3a3d5cdc8331732b5bd44dc8c2780ab45 Mon Sep 17 00:00:00 2001 From: ZaneHannanAU Date: Tue, 26 Jun 2018 15:09:29 +1000 Subject: [PATCH 4/4] More descriptive errors if it does error this time. ... please don't error this time. You have no reason to. --- .../sequential/test-crypto-timing-safe-equal.js | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/test/sequential/test-crypto-timing-safe-equal.js b/test/sequential/test-crypto-timing-safe-equal.js index 47e390a4c3ff99..12c1785d168b6f 100644 --- a/test/sequential/test-crypto-timing-safe-equal.js +++ b/test/sequential/test-crypto-timing-safe-equal.js @@ -15,13 +15,15 @@ assert.strictEqual( assert.strictEqual( crypto.timingSafeEqual(Uint8Array.of(1, 0), Uint16Array.of(1)), true, - 'should consider equal binary integers to be equal' + 'should consider equal binary views to be equal' + + ' (Uint8Array 1 0, Uint16Array 1)' ); assert.strictEqual( crypto.timingSafeEqual(Uint8Array.of(0, 1), Uint16Array.of(1)), false, - 'should consider unequal binary integers to be unequal' + 'should consider unequal binary views to be unequal' + + ' (Uint8Array 0 1, Uint16Array 1)' ); assert.strictEqual( @@ -30,16 +32,18 @@ assert.strictEqual( BigUint64Array.of(1n) ), true, - 'should consider equal binary integers to be equal' + 'should consider equal binary views to be equal' + + ' (Uint8Array 1 ...0, BigUint64Array 1n)' ); assert.strictEqual( crypto.timingSafeEqual( - Buffer.allocUnsafe(8).fill(255), + Buffer.alloc(8, 255), BigInt64Array.of(-1n) ), true, - 'should consider equal binary integers to be equal' + 'should consider equal binary views to be equal' + + ' (Buffer 0xFF, BigInt64Array -1)' ); assert.strictEqual( @@ -48,7 +52,8 @@ assert.strictEqual( BigInt64Array.of(0n) ), true, - 'should consider equal views to be equal' + 'should consider equal binary views to be equal' + + ' (DataView, BigInt64Array)' ); assert.strictEqual(