From f63569bd59ff1f26171d686bb38284dde8458159 Mon Sep 17 00:00:00 2001 From: ZaneHannanAU Date: Tue, 9 Oct 2018 09:36:46 +1100 Subject: [PATCH 1/4] crypto: use byteLength in timingSafeEqual --- lib/internal/crypto/util.js | 2 +- lib/internal/errors.js | 2 +- .../test-crypto-timing-safe-equal.js | 104 +++++++++++++++++- 3 files changed, 105 insertions(+), 3 deletions(-) diff --git a/lib/internal/crypto/util.js b/lib/internal/crypto/util.js index ddef1a163ceec2..544d44669ae466 100644 --- a/lib/internal/crypto/util.js +++ b/lib/internal/crypto/util.js @@ -78,7 +78,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); diff --git a/lib/internal/errors.js b/lib/internal/errors.js index eba69899163dc7..f374301f56026d 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -755,7 +755,7 @@ E('ERR_CRYPTO_SCRYPT_NOT_SUPPORTED', 'Scrypt algorithm not supported', Error); // Switch to TypeError. The current implementation does not seem right. E('ERR_CRYPTO_SIGN_KEY_REQUIRED', 'No key provided to sign', Error); E('ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH', - 'Input buffers must have the same length', RangeError); + 'Input buffers must have the same byteLength', RangeError); E('ERR_DNS_SET_SERVERS_FAILED', 'c-ares failed to set servers: "%s" [%s]', Error); E('ERR_DOMAIN_CALLBACK_NOT_AVAILABLE', diff --git a/test/sequential/test-crypto-timing-safe-equal.js b/test/sequential/test-crypto-timing-safe-equal.js index dcebef29d782b0..064180ad205e6e 100644 --- a/test/sequential/test-crypto-timing-safe-equal.js +++ b/test/sequential/test-crypto-timing-safe-equal.js @@ -18,12 +18,114 @@ assert.strictEqual( false ); +{ + const ab32 = new ArrayBuffer(32); + const dv32 = new DataView(ab32); + dv32.setUint32(0, 1); + dv32.setUint32(4, 1, true); + dv32.setBigUint64(8, 1n); + dv32.setBigUint64(16, 1n, true); + dv32.setUint16(24, 1); + dv32.setUint16(26, 1, true); + dv32.setUint8(28, 1); + dv32.setUint8(29, 1); + + // 'should consider equal buffers to be equal' + assert.strictEqual( + crypto.timingSafeEqual(Buffer.from(ab32), dv32), + true + ); + assert.strictEqual( + crypto.timingSafeEqual(new Uint32Array(ab32), dv32), + true + ); + assert.strictEqual( + crypto.timingSafeEqual( + new Uint8Array(ab32, 0, 16), + Buffer.from(ab32, 0, 16) + ), + true + ); + assert.strictEqual( + crypto.timingSafeEqual( + new Uint32Array(ab32, 0, 8), + Buffer.of(0, 0, 0, 1, // 4 + 1, 0, 0, 0, // 8 + 0, 0, 0, 0, 0, 0, 0, 1, // 16 + 1, 0, 0, 0, 0, 0, 0, 0, // 24 + 0, 1, // 26 + 1, 0, // 28 + 1, 1, // 30 + 0, 0) // 32 + ), + true + ); + + // 'should consider unequal buffer views to be unequal' + assert.strictEqual( + crypto.timingSafeEqual( + new Uint32Array(ab32, 16, 4), + Buffer.from(ab32, 0, 16) + ), + false + ); + assert.strictEqual( + crypto.timingSafeEqual( + new Uint8Array(ab32, 0, 16), + Buffer.from(ab32, 16, 16) + ), + false + ); + assert.strictEqual( + crypto.timingSafeEqual( + new Uint32Array(ab32, 0, 8), + Buffer.of(0, 0, 0, 1, // 4 + 1, 0, 0, 0, // 8 + 0, 0, 0, 0, 0, 0, 0, 1, // 16 + 1, 0, 0, 0, 0, 0, 0, 0, // 24 + 0, 1, // 26 + 1, 0, // 28 + 1, 1, // 30 + 0, 1) // 32 + ), + false + ); + // 'buffers with differing byteLength must throw an equal length error' + common.expectsError( + () => crypto.timingSafeEqual(Buffer.from(ab32, 0, 8), + Buffer.from(ab32, 0, 9)), + { + code: 'ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH', + type: RangeError, + message: 'Input buffers must have the same byteLength' + } + ); + common.expectsError( + () => crypto.timingSafeEqual( + new Uint32Array(ab32, 0, 8), // 32 + Buffer.of(0, 0, 0, 1, // 4 + 1, 0, 0, 0, // 8 + 0, 0, 0, 0, 0, 0, 0, 1, // 16 + 1, 0, 0, 0, 0, 0, 0, 0, // 24 + 0, 1, // 26 + 1, 0, // 28 + 1, 1, // 30 + 0) // 31 + ), + { + code: 'ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH', + type: RangeError, + message: 'Input buffers must have the same byteLength' + } + ); +} + common.expectsError( () => crypto.timingSafeEqual(Buffer.from([1, 2, 3]), Buffer.from([1, 2])), { code: 'ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH', type: RangeError, - message: 'Input buffers must have the same length' + message: 'Input buffers must have the same byteLength' } ); From 8f42de8f154b6e64fc57ab6b7af4e7a271d2b06e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Mon, 23 Sep 2019 00:05:08 +0200 Subject: [PATCH 2/4] errors: improve byteLength error message --- lib/internal/errors.js | 2 +- test/sequential/test-crypto-timing-safe-equal.js | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index f374301f56026d..7bba850c5ebf80 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -755,7 +755,7 @@ E('ERR_CRYPTO_SCRYPT_NOT_SUPPORTED', 'Scrypt algorithm not supported', Error); // Switch to TypeError. The current implementation does not seem right. E('ERR_CRYPTO_SIGN_KEY_REQUIRED', 'No key provided to sign', Error); E('ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH', - 'Input buffers must have the same byteLength', RangeError); + 'Input buffers must have the same number of bytes', RangeError); E('ERR_DNS_SET_SERVERS_FAILED', 'c-ares failed to set servers: "%s" [%s]', Error); E('ERR_DOMAIN_CALLBACK_NOT_AVAILABLE', diff --git a/test/sequential/test-crypto-timing-safe-equal.js b/test/sequential/test-crypto-timing-safe-equal.js index 064180ad205e6e..684e658485b469 100644 --- a/test/sequential/test-crypto-timing-safe-equal.js +++ b/test/sequential/test-crypto-timing-safe-equal.js @@ -97,7 +97,7 @@ assert.strictEqual( { code: 'ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH', type: RangeError, - message: 'Input buffers must have the same byteLength' + message: 'Input buffers must have the same number of bytes' } ); common.expectsError( @@ -115,7 +115,7 @@ assert.strictEqual( { code: 'ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH', type: RangeError, - message: 'Input buffers must have the same byteLength' + message: 'Input buffers must have the same number of bytes' } ); } @@ -125,7 +125,7 @@ common.expectsError( { code: 'ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH', type: RangeError, - message: 'Input buffers must have the same byteLength' + message: 'Input buffers must have the same number of bytes' } ); From e1ada5bf7c596f3a71e872d8e52cfce5eb7a66a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Mon, 23 Sep 2019 02:43:30 +0200 Subject: [PATCH 3/4] test: simplify timingSafeEqual test case --- .../test-crypto-timing-safe-equal.js | 106 ++---------------- 1 file changed, 9 insertions(+), 97 deletions(-) diff --git a/test/sequential/test-crypto-timing-safe-equal.js b/test/sequential/test-crypto-timing-safe-equal.js index 684e658485b469..1a944352f0a45a 100644 --- a/test/sequential/test-crypto-timing-safe-equal.js +++ b/test/sequential/test-crypto-timing-safe-equal.js @@ -19,105 +19,17 @@ assert.strictEqual( ); { - const ab32 = new ArrayBuffer(32); - const dv32 = new DataView(ab32); - dv32.setUint32(0, 1); - dv32.setUint32(4, 1, true); - dv32.setBigUint64(8, 1n); - dv32.setBigUint64(16, 1n, true); - dv32.setUint16(24, 1); - dv32.setUint16(26, 1, true); - dv32.setUint8(28, 1); - dv32.setUint8(29, 1); + // Test TypedArrays with different lengths but equal byteLengths. + const buf = crypto.randomBytes(16).buffer; + const a1 = new Uint8Array(buf); + const a2 = new Uint16Array(buf); + const a3 = new Uint32Array(buf); - // 'should consider equal buffers to be equal' - assert.strictEqual( - crypto.timingSafeEqual(Buffer.from(ab32), dv32), - true - ); - assert.strictEqual( - crypto.timingSafeEqual(new Uint32Array(ab32), dv32), - true - ); - assert.strictEqual( - crypto.timingSafeEqual( - new Uint8Array(ab32, 0, 16), - Buffer.from(ab32, 0, 16) - ), - true - ); - assert.strictEqual( - crypto.timingSafeEqual( - new Uint32Array(ab32, 0, 8), - Buffer.of(0, 0, 0, 1, // 4 - 1, 0, 0, 0, // 8 - 0, 0, 0, 0, 0, 0, 0, 1, // 16 - 1, 0, 0, 0, 0, 0, 0, 0, // 24 - 0, 1, // 26 - 1, 0, // 28 - 1, 1, // 30 - 0, 0) // 32 - ), - true - ); - - // 'should consider unequal buffer views to be unequal' - assert.strictEqual( - crypto.timingSafeEqual( - new Uint32Array(ab32, 16, 4), - Buffer.from(ab32, 0, 16) - ), - false - ); - assert.strictEqual( - crypto.timingSafeEqual( - new Uint8Array(ab32, 0, 16), - Buffer.from(ab32, 16, 16) - ), - false - ); - assert.strictEqual( - crypto.timingSafeEqual( - new Uint32Array(ab32, 0, 8), - Buffer.of(0, 0, 0, 1, // 4 - 1, 0, 0, 0, // 8 - 0, 0, 0, 0, 0, 0, 0, 1, // 16 - 1, 0, 0, 0, 0, 0, 0, 0, // 24 - 0, 1, // 26 - 1, 0, // 28 - 1, 1, // 30 - 0, 1) // 32 - ), - false - ); - // 'buffers with differing byteLength must throw an equal length error' - common.expectsError( - () => crypto.timingSafeEqual(Buffer.from(ab32, 0, 8), - Buffer.from(ab32, 0, 9)), - { - code: 'ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH', - type: RangeError, - message: 'Input buffers must have the same number of bytes' - } - ); - common.expectsError( - () => crypto.timingSafeEqual( - new Uint32Array(ab32, 0, 8), // 32 - Buffer.of(0, 0, 0, 1, // 4 - 1, 0, 0, 0, // 8 - 0, 0, 0, 0, 0, 0, 0, 1, // 16 - 1, 0, 0, 0, 0, 0, 0, 0, // 24 - 0, 1, // 26 - 1, 0, // 28 - 1, 1, // 30 - 0) // 31 - ), - { - code: 'ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH', - type: RangeError, - message: 'Input buffers must have the same number of bytes' + for (const left of [a1, a2, a3]) { + for (const right of [a1, a2, a3]) { + assert.strictEqual(crypto.timingSafeEqual(left, right), true); } - ); + } } common.expectsError( From 8f12d81ee8cc571d913b6503d952cc4cdc39ab53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Mon, 23 Sep 2019 17:15:08 -0300 Subject: [PATCH 4/4] Update lib/internal/errors.js Co-Authored-By: Rich Trott --- lib/internal/errors.js | 2 +- test/sequential/test-crypto-timing-safe-equal.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 7bba850c5ebf80..53f448cf2f993c 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -755,7 +755,7 @@ E('ERR_CRYPTO_SCRYPT_NOT_SUPPORTED', 'Scrypt algorithm not supported', Error); // Switch to TypeError. The current implementation does not seem right. E('ERR_CRYPTO_SIGN_KEY_REQUIRED', 'No key provided to sign', Error); E('ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH', - 'Input buffers must have the same number of bytes', RangeError); + 'Input buffers must have the same byte length', RangeError); E('ERR_DNS_SET_SERVERS_FAILED', 'c-ares failed to set servers: "%s" [%s]', Error); E('ERR_DOMAIN_CALLBACK_NOT_AVAILABLE', diff --git a/test/sequential/test-crypto-timing-safe-equal.js b/test/sequential/test-crypto-timing-safe-equal.js index 1a944352f0a45a..75385e5f88ad50 100644 --- a/test/sequential/test-crypto-timing-safe-equal.js +++ b/test/sequential/test-crypto-timing-safe-equal.js @@ -37,7 +37,7 @@ common.expectsError( { code: 'ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH', type: RangeError, - message: 'Input buffers must have the same number of bytes' + message: 'Input buffers must have the same byte length' } );