From cda7700718c4b8e57bffc26012403620a32778d1 Mon Sep 17 00:00:00 2001 From: Filip Skokan Date: Sat, 25 Nov 2023 14:19:54 +0100 Subject: [PATCH 1/2] crypto: update CryptoKey symbol properties --- lib/internal/crypto/keys.js | 84 ++++++++++++++++++++++++++++--------- 1 file changed, 65 insertions(+), 19 deletions(-) diff --git a/lib/internal/crypto/keys.js b/lib/internal/crypto/keys.js index 1e49bd62473a1b..fc4240ffd28864 100644 --- a/lib/internal/crypto/keys.js +++ b/lib/internal/crypto/keys.js @@ -703,32 +703,81 @@ ObjectDefineProperties(CryptoKey.prototype, { }, }); +/** + * @param {InternalCryptoKey} key + * @param {KeyObject} keyObject + * @param {object} algorithm + * @param {boolean} extractable + * @param {Set} keyUsages + */ +function defineCryptoKeyProperties( + key, + keyObject, + algorithm, + extractable, + keyUsages, +) { + // Using symbol properties here currently instead of private + // properties because (for now) the performance penalty of + // private fields is still too high. + ObjectDefineProperties(key, { + [kKeyObject]: { + __proto__: null, + value: keyObject, + enumerable: false, + configurable: false, + writable: false, + }, + [kAlgorithm]: { + __proto__: null, + value: algorithm, + enumerable: false, + configurable: false, + writable: false, + }, + [kExtractable]: { + __proto__: null, + value: extractable, + enumerable: false, + configurable: false, + writable: false, + }, + [kKeyUsages]: { + __proto__: null, + value: keyUsages, + enumerable: false, + configurable: false, + writable: false, + }, + }); +} + // All internal code must use new InternalCryptoKey to create // CryptoKey instances. The CryptoKey class is exposed to end // user code but is not permitted to be constructed directly. // Using markTransferMode also allows the CryptoKey to be // cloned to Workers. class InternalCryptoKey { - constructor( - keyObject, - algorithm, - keyUsages, - extractable) { + constructor(keyObject, algorithm, keyUsages, extractable) { markTransferMode(this, true, false); - // Using symbol properties here currently instead of private - // properties because (for now) the performance penalty of - // private fields is still too high. - this[kKeyObject] = keyObject; - this[kAlgorithm] = algorithm; - this[kExtractable] = extractable; - this[kKeyUsages] = keyUsages; + // When constructed during transfer the properties get assigned + // in the kDeserialize call. + if (keyObject) { + defineCryptoKeyProperties( + this, + keyObject, + algorithm, + extractable, + keyUsages, + ); + } } [kClone]() { const keyObject = this[kKeyObject]; - const algorithm = this.algorithm; - const extractable = this.extractable; - const usages = this.usages; + const algorithm = this[kAlgorithm]; + const extractable = this[kExtractable]; + const usages = this[kKeyUsages]; return { data: { @@ -742,10 +791,7 @@ class InternalCryptoKey { } [kDeserialize]({ keyObject, algorithm, usages, extractable }) { - this[kKeyObject] = keyObject; - this[kAlgorithm] = algorithm; - this[kKeyUsages] = usages; - this[kExtractable] = extractable; + defineCryptoKeyProperties(this, keyObject, algorithm, extractable, usages); } } InternalCryptoKey.prototype.constructor = CryptoKey; From 09efacbc53fedebd905b39b35061bf61049edf43 Mon Sep 17 00:00:00 2001 From: Filip Skokan Date: Fri, 24 Nov 2023 17:33:24 +0100 Subject: [PATCH 2/2] assert,crypto: make KeyObject and CryptoKey testable for equality --- lib/internal/util/comparisons.js | 18 ++++++++++ test/parallel/test-assert-deep.js | 59 ++++++++++++++++++++++++++++++- 2 files changed, 76 insertions(+), 1 deletion(-) diff --git a/lib/internal/util/comparisons.js b/lib/internal/util/comparisons.js index d86e59adae9880..8a9b4128c2efdc 100644 --- a/lib/internal/util/comparisons.js +++ b/lib/internal/util/comparisons.js @@ -43,6 +43,8 @@ const { isSymbolObject, isFloat32Array, isFloat64Array, + isKeyObject, + isCryptoKey, } = types; const { constants: { @@ -60,6 +62,8 @@ const kIsArray = 1; const kIsSet = 2; const kIsMap = 3; +let kKeyObject; + // Check if they have the same source and flags function areSimilarRegExps(a, b) { return a.source === b.source && @@ -249,6 +253,20 @@ function innerDeepEqual(val1, val2, strict, memos) { isNativeError(val2) || val2 instanceof Error) { return false; + } else if (isKeyObject(val1)) { + if (!isKeyObject(val2) || !val1.equals(val2)) { + return false; + } + } else if (isCryptoKey(val1)) { + kKeyObject ??= require('internal/crypto/util').kKeyObject; + if (!isCryptoKey(val2) || + val1.extractable !== val2.extractable || + !innerDeepEqual(val1.algorithm, val2.algorithm, strict, memos) || + !innerDeepEqual(val1.usages, val2.usages, strict, memos) || + !innerDeepEqual(val1[kKeyObject], val2[kKeyObject], strict, memos) + ) { + return false; + } } return keyCheck(val1, val2, strict, memos, kNoIterator); } diff --git a/test/parallel/test-assert-deep.js b/test/parallel/test-assert-deep.js index f7489993aa60ba..f147be1b41f2a8 100644 --- a/test/parallel/test-assert-deep.js +++ b/test/parallel/test-assert-deep.js @@ -1,6 +1,6 @@ 'use strict'; -require('../common'); +const common = require('../common'); const assert = require('assert'); const util = require('util'); const { AssertionError } = assert; @@ -1220,3 +1220,60 @@ assert.throws( assertNotDeepOrStrict(a, b); } + +// eslint-disable-next-line node-core/crypto-check +if (common.hasCrypto) { + const crypto = require('crypto'); + const { subtle } = globalThis.crypto; + + { + const a = crypto.createSecretKey(Buffer.alloc(1, 0)); + const b = crypto.createSecretKey(Buffer.alloc(1, 1)); + + assertNotDeepOrStrict(a, b); + } + + { + const a = crypto.createSecretKey(Buffer.alloc(0)); + const b = crypto.createSecretKey(Buffer.alloc(0)); + + assertDeepAndStrictEqual(a, b); + } + + (async () => { + { + const a = await subtle.importKey('raw', Buffer.alloc(1, 0), { name: 'HMAC', hash: 'SHA-256' }, true, ['sign']); + const b = await subtle.importKey('raw', Buffer.alloc(1, 1), { name: 'HMAC', hash: 'SHA-256' }, true, ['sign']); + + assertNotDeepOrStrict(a, b); + } + + { + const a = await subtle.importKey('raw', Buffer.alloc(1), { name: 'HMAC', hash: 'SHA-256' }, true, ['sign']); + const b = await subtle.importKey('raw', Buffer.alloc(1), { name: 'HMAC', hash: 'SHA-256' }, false, ['sign']); + + assertNotDeepOrStrict(a, b); + } + + { + const a = await subtle.importKey('raw', Buffer.alloc(1), { name: 'HMAC', hash: 'SHA-256' }, true, ['sign']); + const b = await subtle.importKey('raw', Buffer.alloc(1), { name: 'HMAC', hash: 'SHA-384' }, true, ['sign']); + + assertNotDeepOrStrict(a, b); + } + + { + const a = await subtle.importKey('raw', Buffer.alloc(1), { name: 'HMAC', hash: 'SHA-256' }, true, ['sign']); + const b = await subtle.importKey('raw', Buffer.alloc(1), { name: 'HMAC', hash: 'SHA-256' }, true, ['verify']); + + assertNotDeepOrStrict(a, b); + } + + { + const a = await subtle.importKey('raw', Buffer.alloc(1), { name: 'HMAC', hash: 'SHA-256' }, true, ['sign']); + const b = await subtle.importKey('raw', Buffer.alloc(1), { name: 'HMAC', hash: 'SHA-256' }, true, ['sign']); + + assertDeepAndStrictEqual(a, b); + } + })().then(common.mustCall()); +}