From 15e8901299a38439d67ac84f82682dc9fc4b14e7 Mon Sep 17 00:00:00 2001 From: Filip Skokan Date: Sun, 6 Oct 2024 21:06:45 +0200 Subject: [PATCH] crypto: allow non-multiple of 8 in SubtleCrypto.deriveBits PR-URL: https://github.com/nodejs/node/pull/55296 Reviewed-By: James M Snell --- doc/api/webcrypto.md | 3 --- lib/internal/crypto/diffiehellman.js | 26 +++++++++++++------ .../test-webcrypto-derivebits-cfrg.js | 8 +++--- .../test-webcrypto-derivebits-ecdh.js | 8 +++--- test/wpt/status/WebCryptoAPI.cjs | 10 ------- 5 files changed, 28 insertions(+), 27 deletions(-) diff --git a/doc/api/webcrypto.md b/doc/api/webcrypto.md index 371073397ad7d6..921f173573bfe7 100644 --- a/doc/api/webcrypto.md +++ b/doc/api/webcrypto.md @@ -601,9 +601,6 @@ Using the method and parameters specified in `algorithm` and the keying material provided by `baseKey`, `subtle.deriveBits()` attempts to generate `length` bits. -The Node.js implementation requires that `length`, when a number, is a multiple -of `8`. - When `length` is not provided or `null` the maximum number of bits for a given algorithm is generated. This is allowed for the `'ECDH'`, `'X25519'`, and `'X448'` algorithms, for other algorithms `length` is required to be a number. diff --git a/lib/internal/crypto/diffiehellman.js b/lib/internal/crypto/diffiehellman.js index 410759414f635d..9ebd9766f6bc8c 100644 --- a/lib/internal/crypto/diffiehellman.js +++ b/lib/internal/crypto/diffiehellman.js @@ -5,6 +5,7 @@ const { MathCeil, ObjectDefineProperty, SafeSet, + Uint8Array, } = primordials; const { Buffer } = require('buffer'); @@ -295,6 +296,8 @@ function diffieHellman(options) { return statelessDH(privateKey[kHandle], publicKey[kHandle]); } +let masks; + // The ecdhDeriveBits function is part of the Web Crypto API and serves both // deriveKeys and deriveBits functions. async function ecdhDeriveBits(algorithm, baseKey, length) { @@ -341,18 +344,25 @@ async function ecdhDeriveBits(algorithm, baseKey, length) { // If the length is not a multiple of 8 the nearest ceiled // multiple of 8 is sliced. - length = MathCeil(length / 8); - const { byteLength } = bits; + const sliceLength = MathCeil(length / 8); + const { byteLength } = bits; // If the length is larger than the derived secret, throw. - // Otherwise, we either return the secret or a truncated - // slice. - if (byteLength < length) + if (byteLength < sliceLength) throw lazyDOMException('derived bit length is too small', 'OperationError'); - return length === byteLength ? - bits : - ArrayBufferPrototypeSlice(bits, 0, length); + const slice = ArrayBufferPrototypeSlice(bits, 0, sliceLength); + + const mod = length % 8; + if (mod === 0) + return slice; + + // eslint-disable-next-line no-sparse-arrays + masks ||= [, 0b10000000, 0b11000000, 0b11100000, 0b11110000, 0b11111000, 0b11111100, 0b11111110]; + + const masked = new Uint8Array(slice); + masked[sliceLength - 1] = masked[sliceLength - 1] & masks[mod]; + return masked.buffer; } module.exports = { diff --git a/test/parallel/test-webcrypto-derivebits-cfrg.js b/test/parallel/test-webcrypto-derivebits-cfrg.js index 3e28774d05a9ce..f5c602b6deb630 100644 --- a/test/parallel/test-webcrypto-derivebits-cfrg.js +++ b/test/parallel/test-webcrypto-derivebits-cfrg.js @@ -140,9 +140,11 @@ async function prepareKeys() { public: publicKey }, privateKey, 8 * size - 11); - assert.strictEqual( - Buffer.from(bits).toString('hex'), - result.slice(0, -2)); + const expected = Buffer.from(result.slice(0, -2), 'hex'); + expected[size - 2] = expected[size - 2] & 0b11111000; + assert.deepStrictEqual( + Buffer.from(bits), + expected); } })); diff --git a/test/parallel/test-webcrypto-derivebits-ecdh.js b/test/parallel/test-webcrypto-derivebits-ecdh.js index 4dba34d84a7907..6c99946752b4b6 100644 --- a/test/parallel/test-webcrypto-derivebits-ecdh.js +++ b/test/parallel/test-webcrypto-derivebits-ecdh.js @@ -161,9 +161,11 @@ async function prepareKeys() { public: publicKey }, privateKey, 8 * size - 11); - assert.strictEqual( - Buffer.from(bits).toString('hex'), - result.slice(0, -2)); + const expected = Buffer.from(result.slice(0, -2), 'hex'); + expected[size - 2] = expected[size - 2] & 0b11111000; + assert.deepStrictEqual( + Buffer.from(bits), + expected); } })); diff --git a/test/wpt/status/WebCryptoAPI.cjs b/test/wpt/status/WebCryptoAPI.cjs index d443ae86caaeb7..0057d5f72cc937 100644 --- a/test/wpt/status/WebCryptoAPI.cjs +++ b/test/wpt/status/WebCryptoAPI.cjs @@ -8,16 +8,6 @@ module.exports = { 'algorithm-discards-context.https.window.js': { 'skip': 'Not relevant in Node.js context', }, - 'derive_bits_keys/derived_bits_length.https.any.js': { - 'fail': { - // See https://github.com/nodejs/node/pull/55296 - // The fix is pending a decision whether truncation in ECDH/X* will be removed from the spec entirely - 'expected': [ - "ECDH derivation with 230 as 'length' parameter", - "X25519 derivation with 230 as 'length' parameter", - ], - }, - }, 'historical.any.js': { 'skip': 'Not relevant in Node.js context', },