From 7c75e02b76c9aefa930ada5043254ace6c6ce885 Mon Sep 17 00:00:00 2001 From: Kess Plasmeier Date: Tue, 18 Aug 2026 14:42:22 -0700 Subject: [PATCH] fix(kms-keyring-node): copy branch key material on read getBranchKeyMaterials returned the cached NodeBranchKeyMaterial by reference. The cryptographic materials cache zeroes a material's buffer in place when the entry is evicted, so under concurrency an eviction (overwrite, TTL expiry, or tail eviction) could zero the branch key another in-flight operation was about to derive its wrapping key from. Triggering this requires two branch key acquisitions to resolve within the same event-loop tick. This does not arise during normal operation with the DynamoDB/KMS-backed keystore; fetches complete on separate I/O callbacks. We are nevertheless patching it. Return an independent deep copy from getBranchKeyMaterials so callers never share a buffer the cache can zero. Add concurrency regression tests covering both the encrypt and decrypt paths. Fixes #1691 --- .../src/kms_hkeyring_node_helpers.ts | 18 +- .../kms_hkeyring_node.concurrency.test.ts | 160 ++++++++++++++++++ 2 files changed, 177 insertions(+), 1 deletion(-) create mode 100644 modules/kms-keyring-node/test/kms_hkeyring_node.concurrency.test.ts diff --git a/modules/kms-keyring-node/src/kms_hkeyring_node_helpers.ts b/modules/kms-keyring-node/src/kms_hkeyring_node_helpers.ts index d142c9341..f005f4402 100644 --- a/modules/kms-keyring-node/src/kms_hkeyring_node_helpers.ts +++ b/modules/kms-keyring-node/src/kms_hkeyring_node_helpers.ts @@ -252,7 +252,23 @@ export async function getBranchKeyMaterials( branchKeyMaterials = cacheEntry.response } - return branchKeyMaterials + /* Hand back a copy the cache can never touch. The CMC zeroes a material's + * buffer in place on eviction; callers read the branch key AFTER this await, + * so a concurrent eviction (overwrite, TTL, or tail) could otherwise zero the + * buffer mid-derivation. On encrypt that is silent (data key wrapped under a + * zeroed key); on decrypt it surfaces as a GCM auth failure. See #1691. */ + return deepCopyBranchKeyMaterial(branchKeyMaterials) +} + +function deepCopyBranchKeyMaterial( + material: NodeBranchKeyMaterial +): NodeBranchKeyMaterial { + return new NodeBranchKeyMaterial( + Buffer.from(material.branchKey()), + material.branchKeyIdentifier, + material.branchKeyVersion.toString('utf-8'), + { ...material.encryptionContext } + ) } //= aws-encryption-sdk-specification/framework/aws-kms/aws-kms-hierarchical-keyring.md#onencrypt diff --git a/modules/kms-keyring-node/test/kms_hkeyring_node.concurrency.test.ts b/modules/kms-keyring-node/test/kms_hkeyring_node.concurrency.test.ts new file mode 100644 index 000000000..087db3e7f --- /dev/null +++ b/modules/kms-keyring-node/test/kms_hkeyring_node.concurrency.test.ts @@ -0,0 +1,160 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { + EncryptedDataKey, + NodeBranchKeyMaterial, + NodeDecryptionMaterial, + NodeEncryptionMaterial, + unwrapDataKey, +} from '@aws-crypto/material-management' +import { + BRANCH_KEY_ID_A, + BRANCH_KEY_ID_B, + EC_A, + EC_B, + KEYSTORE, + TEST_ESDK_ALG_SUITE, + TTL, +} from './fixtures' +import { KmsHierarchicalKeyRingNode } from '../src/kms_hkeyring_node' +import { + BRANCH_KEY_ID_SUPPLIER, + deepCopyBranchKeyMaterial, +} from './kms_hkeyring_node.test' +import { expect } from 'chai' +import Sinon from 'sinon' +import { + BranchKeyStoreNode, + KeyStoreInfoOutput, +} from '@aws-crypto/branch-keystore-node' + +// Regression tests for #1691. Before the fix, getBranchKeyMaterials returned the +// cached NodeBranchKeyMaterial by reference. The CMC zeroes a material's buffer +// in place on eviction, so a concurrent operation that evicted an entry could +// zero the branch key another in-flight operation was about to derive from. +// +// The trigger is co-resolution: two branch-key fetches completing in the same +// event-loop tick, so one operation's cache write (and its eviction) lands +// between another operation's fetch and its synchronous wrap/unwrap. A stubbed +// keystore resolves on the microtask queue, which reproduces that deterministically +// (a real DDB+KMS fetch resolves on a later macrotask, so the window is far +// harder to hit in practice -- but the fix must hold regardless). maxCacheSize=1 +// with two branch keys forces an eviction on every alternating operation. + +const CONCURRENCY = 25 + +let activeMaterialA: NodeBranchKeyMaterial +let activeMaterialB: NodeBranchKeyMaterial +before(async function () { + activeMaterialA = await KEYSTORE.getActiveBranchKey(BRANCH_KEY_ID_A) + activeMaterialB = await KEYSTORE.getActiveBranchKey(BRANCH_KEY_ID_B) +}) + +// A keystore stub that returns fresh material (its own buffer, as a real +// keystore would) and resolves on the microtask queue. +function stubKeyStore(): BranchKeyStoreNode { + const keyStore = Sinon.createStubInstance(BranchKeyStoreNode) + const forId = async (branchKeyId: string) => + deepCopyBranchKeyMaterial( + branchKeyId === BRANCH_KEY_ID_A ? activeMaterialA : activeMaterialB + ) + keyStore.getActiveBranchKey.callsFake(forId) + // The two active versions are the only versions used here, so map by id. + keyStore.getBranchKeyVersion.callsFake(forId) + keyStore.getKeyStoreInfo.callsFake( + (): KeyStoreInfoOutput => ({ + keystoreId: 'keyStoreId', + keystoreTableName: 'keystoreTableName', + logicalKeyStoreName: 'logicalKeyStoreName', + grantTokens: [], + kmsConfiguration: null as any, + }) + ) + return keyStore +} + +function newKeyring(maxCacheSize?: number): KmsHierarchicalKeyRingNode { + return new KmsHierarchicalKeyRingNode({ + branchKeyIdSupplier: BRANCH_KEY_ID_SUPPLIER, + keyStore: stubKeyStore(), + cacheLimitTtl: TTL, + maxCacheSize, + }) +} + +describe('KmsHierarchicalKeyRingNode: concurrent cold-cache operations (#1691)', () => { + it('concurrent onEncrypt does not wrap data keys under an evicted (zeroed) branch key', async () => { + // maxCacheSize=1 + two branch keys => every alternating encrypt evicts the + // other entry, zeroing its buffer. + const hkr = newKeyring(1) + const materials = Array.from( + { length: CONCURRENCY }, + (_, i) => + new NodeEncryptionMaterial( + TEST_ESDK_ALG_SUITE, + i % 2 === 0 ? EC_A : EC_B + ) + ) + + await Promise.all( + materials.map(async (m) => { + await hkr.onEncrypt(m) + }) + ) + + // Every EDK must round-trip. A buffer zeroed mid-wrap produces an EDK bound + // to an all-zero key, which fails to decrypt under the real branch key. + const verifier = newKeyring() + for (const m of materials) { + const expectedPdk = unwrapDataKey(m.getUnencryptedDataKey()) + const decryptionMaterial = new NodeDecryptionMaterial( + TEST_ESDK_ALG_SUITE, + m.encryptionContext + ) + await verifier.onDecrypt(decryptionMaterial, m.encryptedDataKeys) + expect( + unwrapDataKey(decryptionMaterial.getUnencryptedDataKey()) + ).to.deep.equal(expectedPdk) + } + }) + + it('concurrent onDecrypt does not derive from an evicted (zeroed) branch key', async () => { + // Build valid ciphertexts for both branch keys (sequential = uncorrupted). + const setup = newKeyring() + const encA = new NodeEncryptionMaterial(TEST_ESDK_ALG_SUITE, EC_A) + const encB = new NodeEncryptionMaterial(TEST_ESDK_ALG_SUITE, EC_B) + await setup.onEncrypt(encA) + await setup.onEncrypt(encB) + + const cases = [ + { + edks: encA.encryptedDataKeys, + ec: EC_A, + pdk: unwrapDataKey(encA.getUnencryptedDataKey()), + }, + { + edks: encB.encryptedDataKeys, + ec: EC_B, + pdk: unwrapDataKey(encB.getUnencryptedDataKey()), + }, + ] + + const hkr = newKeyring(1) + const recovered = await Promise.all( + Array.from({ length: CONCURRENCY }, async (_, i) => { + const { edks, ec } = cases[i % 2] + const decryptionMaterial = new NodeDecryptionMaterial( + TEST_ESDK_ALG_SUITE, + ec + ) + await hkr.onDecrypt(decryptionMaterial, edks as EncryptedDataKey[]) + return unwrapDataKey(decryptionMaterial.getUnencryptedDataKey()) + }) + ) + + for (let i = 0; i < recovered.length; i++) { + expect(recovered[i]).to.deep.equal(cases[i % 2].pdk) + } + }) +})