Skip to content

fix: borrow plaintext in Keystore::seal to avoid un-zeroized copy#438

Open
NnnOooPppEee wants to merge 1 commit into
mainfrom
vincentj/sec-2260-h1-worldcoinwalletkit-master-encryption-key-k_intermediate
Open

fix: borrow plaintext in Keystore::seal to avoid un-zeroized copy#438
NnnOooPppEee wants to merge 1 commit into
mainfrom
vincentj/sec-2260-h1-worldcoinwalletkit-master-encryption-key-k_intermediate

Conversation

@NnnOooPppEee

@NnnOooPppEee NnnOooPppEee commented Jul 9, 2026

Copy link
Copy Markdown

Summary

walletkit_db::Keystore::seal took its plaintext as an owned Vec<u8>. On first-time key generation, init_or_open_envelope_key called k_intermediate.to_vec() to satisfy that signature, creating a copy of the 32-byte master key (K_intermediate) that was never zeroized on drop and could linger in freed heap memory.

seal now borrows &[u8] for both aad and plaintext, so k_intermediate (already held in a Zeroizing buffer) is passed by reference and never copied into an un-zeroized owned buffer at this layer.

The Ks bridge in walletkit-core (which adapts Keystore onto the uniffi-exported DeviceKeystore trait) still makes one owned copy to cross into DeviceKeystore::seal. That is unavoidable: uniffi callback interfaces only support pass-by-value parameters (no &[u8]), confirmed against the current uniffi docs. This matches the known limitation already discussed on the report (further copies inside the foreign Swift/Kotlin/etc. implementation are outside Rust's control regardless of this change).

Scope note

This only changes Keystore::seal's parameter types (not open_sealed, whose aad/ciphertext aren't sensitive) and does not touch DeviceKeystore (walletkit-core's uniffi trait), since uniffi does not support borrowed parameters for foreign trait implementations.

walletkit-db is published (publish = true); this is a breaking change to Keystore::seal's signature for any external implementer, worth a minor version bump at the next release.

Linear

SEC-2260

Test plan

  • cargo test -p walletkit-db -p walletkit-core --lib (140 tests passed)
  • cargo clippy -p walletkit-db -p walletkit-core --lib --tests (clean)
  • rustfmt --check on changed files

Note

Medium Risk
Breaking API change to a published crate's crypto trait, but behavior is tighter around secret handling; FFI path still copies key material once by design.

Overview
walletkit_db::Keystore::seal now takes aad and plaintext as &[u8] instead of owned Vec<u8>, so callers can pass secrets from Zeroizing buffers without handing this crate an extra heap copy that may not be zeroized on drop.

On first envelope creation, init_or_open_envelope_key no longer calls k_intermediate.to_vec() before sealing; it passes k_intermediate.as_slice() directly. open_sealed is unchanged (still owned Vec for non-sensitive ciphertext/AAD).

The Ks adapter in walletkit-core still **to_vec()**s at the uniffi DeviceKeystore boundary (pass-by-value only); comments document that as an accepted limitation. Test XorKeystore and trait docs are updated for the new signature.

Breaking change for external Keystore implementers of the published walletkit-db crate.

Reviewed by Cursor Bugbot for commit 0f8a5aa. Bugbot is set up for automated code reviews on this repo. Configure here.

K_intermediate was copied into an owned, un-zeroized Vec<u8> before
being sealed on first-time key generation, so the plaintext master key
could linger in freed heap memory. Keystore::seal now borrows &[u8]
so the caller's Zeroizing buffer is never handed away as an owned
copy. The one remaining copy needed to cross into the uniffi
DeviceKeystore callback interface (which only supports pass-by-value
parameters) is an accepted, documented limitation.

Ref SEC-2260 / HackerOne #3598947
@NnnOooPppEee NnnOooPppEee requested a review from danielle-tfh July 9, 2026 19:25
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