Skip to content

fix(raw-aes-keyring-node): copy unencryptedMasterKey instead of consuming it - #1718

Open
musatoktas wants to merge 1 commit into
aws:masterfrom
musatoktas:fix/970-raw-aes-keyring-master-key-copy
Open

musatoktas wants to merge 1 commit into
aws:masterfrom
musatoktas:fix/970-raw-aes-keyring-master-key-copy

Conversation

@musatoktas

Copy link
Copy Markdown

Issue #, if available: #970

Description of changes:

RawAesKeyringNode passed the caller's unencryptedMasterKey directly to NodeRawAesMaterial.setUnencryptedDataKey. When KeyObject is supported, wrapWithKeyObjectIfSupported moves the secret into a KeyObject and then calls dataKey.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:

  • the caller's unencryptedMasterKey is all zero bytes after the constructor returns;
  • a second keyring built from the same unencryptedMasterKey silently 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:

  • the caller's key is unchanged after construction;
  • the keyring still encrypts and decrypts after the caller zeroes its own key;
  • two keyrings can be built from one key and interoperate;
  • a key that is a view into a larger ArrayBuffer is still rejected.

Verification (GitHub Actions, Node 22, upstream commit 0ceb1f9):

  • raw-aes-keyring-node mocha suite before the change plus the new tests: 17 passing, 2 failing (the first two new tests above).
  • Same suite with the change: 19 passing, 0 failing.
  • material-management-node mocha suite with the change: 40 passing.
  • eslint and prettier on the two changed files: clean. npm run build of the package: exit 0.

Limits:

  • Only raw-aes-keyring-node is changed. raw-aes-keyring-browser takes a CryptoKey, so it does not have this behavior.
  • Not run: the full repository npm test, the karma browser suite, the test vectors and compliance checks.
  • Behavior change: code that relied on the keyring zeroing the buffer it was handed will now keep a live copy of the key until it zeros the buffer itself. The public docs do not currently describe either behavior; I did not change the README.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Check any applicable:

  • Were any files moved? Moving files changes their URL, which breaks all hyperlinks to the files.

…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
@musatoktas
musatoktas requested a review from a team as a code owner October 15, 2026 14:53

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant