aws / aws/aws-encryption-sdk-javascript
Hierarchical keyring: concurrent decrypts corrupt shared branch-key material
- Dominant language
- TypeScript
- Stars
- 260
- Forks
- 68
- Avg merge
- 22h 19m
- Merged PRs (30d)
- 2
Description
### Problem:
Seen on `@aws-crypto/client-node@5.0.2`.
When a branch-key cache entry is evicted, the cache wipes its key buffer by filling it with zeros. But `branchKey()` returns that buffer directly instead of a copy, so every caller shares one buffer. If a decrypt is still using it when another decrypt evicts the entry, its key gets zeroed mid-use, so it derives the wrong wrapping key and the unwrap fails gcm auth.
This only happens on a cold cache under concurrency. Retrying, or decrypting one at a time, works.
Three functions in the branch-key path line up to cause it:
- [`dispose`](https://github.com/aws/aws-encryption-sdk-javascript/blob/a05029577aa41713bab481ef5992bfbb4067b32c/modules/cache-material/src/get_local_cryptographic_materials_cache.ts#L38-L40) calls `zeroUnencryptedDataKey()` whenever an entry leaves the cache, including on an overwrite.
- [`zeroUnencryptedDataKey()`](https://github.com/aws/aws-encryption-sdk-javascript/blob/a05029577aa41713bab481ef5992bfbb4067b32c/modules/material-management/src/cryptographic_material.ts#L211-L213) runs `this._branchKey.fill(0)`, wiping the buffer in place.
- [`branchKey()`](https://github.com/aws/aws-encryption-sdk-javascript/blob/a05029577aa41713bab481ef5992bfbb4067b32c/modules/material-management/src/cryptographic_material.ts#L204-L206) returns `this._branchKey` directly, so a reader shares the buffer the cache will wipe.
Any eviction while a decrypt still holds that buffer corrupts it: an overwrite from a concurrent cold-miss, a ttl expiry, or a tail eviction.
```
Error: Unable to decrypt data key Error #1
Error: Unsupported state or unable to authenticate data
at Decipheriv.final (node:internal/crypto/cipher)
at unwrapEncryptedDataKey (...)
at KmsHierarchicalKeyRingNode._onDecrypt (...)
```
The overwrite is the usual trigger under concurrency. The cache is only written after the branch-key fetch (DynamoDB and KMS) finishes, and nothing dedupes in-flight fetches. So a burst of decrypts on a cold cache all miss, all fetch, and all write the same cache key. Each write after the first overwrites a live entry and zeros a buffer another decrypt is still using.
Impact: concurrent decrypts through one shared hierarchical keyring on a cold cache intermittently fail to unwrap valid ciphertext.
Reproduction:
```
npm i @aws-crypto/cache-material@5.0.2 @aws-crypto/material-management@5.0.2
node repro.js
```
```js
// repro.js
// The branch-key cache hands out its key buffer by reference and zeros it on
// eviction. Under concurrency one decrypt reads that buffer, then a second
// decrypt's cold-miss overwrites the same entry and zeros the buffer the first
// is about to derive from. Modeled deterministically against the cache alone.
const { getLocalCryptographicMaterialsCache } = require('@aws-crypto/cache-material')
const { NodeBranchKeyMaterial } = require('@aws-crypto/material-management')
const CACHE_KEY = 'branch-id:version'
const branchKeyMaterial = (fill) =>
new NodeBranchKeyMaterial(Buffer.alloc(32, fill), 'branch-id', '22222222-2222-4222-8222-222222222222', {})
const first4 = (buf) => Buffer.from(buf.slice(0, 4)).toString('hex')
const cache = getLocalCryptographicMaterialsCache(100)
// decrypt #1 cold-misses, populates the entry, and reads the branch key by
// reference to derive its wrapping key (what the unwrap path does).
cache.putBranchKeyMaterial(CACHE_KEY, branchKeyMaterial(0xaa))
const decrypt1BranchKey = cache.getBranchKeyMaterial(CACHE_KEY).response.branchKey()
// decrypt #2 also cold-missed; its fetch resolves and populates the SAME entry,
// evicting #1's material -> dispose() -> zeroUnencryptedDataKey().
cache.putBranchKeyMaterial(CACHE_KEY, branchKeyMaterial(0xbb))
// decrypt #1 has not finished; it now derives from the buffer it read earlier.
console.log('decrypt #1 branch key, first 4 bytes:')
console.log(' expected: aaaaaaaa (the key it read)')
console.log(` actual: ${first4(decrypt1BranchKey)} (zeroed by #2, so #1 derives the wrong wrapping key and gcm auth fails)`)
```
Actual output:
```
decrypt #1 branch key, first 4 bytes:
expected: aaaaaaaa (the key it read)
actual: 00000000 (zeroed by #2, so #1 derives the wrong wrapping key and gcm auth fails)
```
### Solution:
Stop a reader from ever sharing a Buffer the cache can zero. Either:
- Return a copy from `branchKey()` (or copy in the unwrap read path), so eviction can't mutate a reference a decrypt is still using. Same shape as the #970 copy fix, different path.
- And/or don't zero while readers are outstanding (refcount, or skip zeroing on overwrite).
### Workaround:
Decrypt one at a time when the calls share a keyring. The bug needs two decrypts running at once, so if only one runs at a time, nothing can wipe the buffer it is using. The tradeoff is you lose the speedup of decrypting in parallel, though you also avoid the duplicate keystore fetches.
### Out of scope:
- Not a duplicate of the cold-cache stampede (#1663), though they share the same trigger. The single-flight fix proposed there would stop the common case (many decrypts of the same key racing). It won't stop a different branch key from evicting and zeroing this one while a decrypt still holds it, which happens once the cache is at capacity, so this needs its own change.
Contributor guide
Assessment
This issue has not been assessed yet.