Repository navigation
fix(raw-aes-keyring-node): copy unencryptedMasterKey instead of consuming it - #1718
Open
musatoktas wants to merge 1 commit into
Open
musatoktas wants to merge 1 commit into
musatoktas wants to merge 1 commit into
Conversation
…ming it The keyring passed the caller's unencryptedMasterKey straight to NodeRawAesMaterial.setUnencryptedDataKey, which moves the secret into a KeyObject and then zeroes the buffer it was given. The caller's buffer was silently wiped, so a second keyring built from the same key, or any other use of that key, got all zero bytes. Pass a copy to the material instead. The isolated buffer precondition is still checked against the caller's buffer. The caller remains responsible for zeroing its own copy of the key. Fixes aws#970
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #, if available: #970
Description of changes:
RawAesKeyringNodepassed the caller'sunencryptedMasterKeydirectly toNodeRawAesMaterial.setUnencryptedDataKey. When KeyObject is supported,wrapWithKeyObjectIfSupportedmoves the secret into a KeyObject and then callsdataKey.fill(0)on the buffer it was given. The result is that the buffer the caller passed to the keyring constructor is zeroed.Observable effects on the current master:
unencryptedMasterKeyis all zero bytes after the constructor returns;unencryptedMasterKeysilently gets an all zero wrapping key, so its output cannot be decrypted by the first keyring.This change gives the material a copy of the key (
new Uint8Array(unencryptedMasterKey)), as proposed in the issue. The existing "isolated buffer" check (byteOffset === 0) is still made against the caller's buffer, so inputs that were rejected before are still rejected. The caller keeps ownership of its own buffer and is responsible for zeroing it once it no longer needs it.Tests added in
raw_aes_keyring_node.test.ts:Verification (GitHub Actions, Node 22, upstream commit 0ceb1f9):
raw-aes-keyring-nodemocha suite before the change plus the new tests: 17 passing, 2 failing (the first two new tests above).material-management-nodemocha suite with the change: 40 passing.npm run buildof the package: exit 0.Limits:
raw-aes-keyring-nodeis changed.raw-aes-keyring-browsertakes a CryptoKey, so it does not have this behavior.npm test, the karma browser suite, the test vectors and compliance checks.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Check any applicable: