fix(key-wallet): zeroize ExtendedBLSPrivKey and ExtendedEd25519PrivKey on drop#859
Conversation
…y on drop The BLS and Ed25519 extended private key types held raw secret material but had no Zeroize/Drop implementations, so their secret bytes were left in freed heap memory when dropped. Their secp256k1 siblings (ExtendedPrivKey, RootExtendedPrivKey) already wipe themselves on drop. Mirror the secp256k1 pattern for both types: a hand-written Zeroize that wipes the secret key, chain code, and derivation metadata, plus a Drop impl that calls it. Add a Zeroize impl for Fingerprint so modules outside bip32 can wipe it too. Also replace the derived PartialEq on ExtendedEd25519PrivKey, which short-circuited over the raw 32-byte secret, with a constant-time comparison of the secret bytes. Fixes #858 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #859 +/- ##
==========================================
+ Coverage 73.62% 73.66% +0.03%
==========================================
Files 324 324
Lines 73195 73268 +73
==========================================
+ Hits 53888 53970 +82
+ Misses 19307 19298 -9
|
Fixes #858
Problem
ExtendedBLSPrivKey(key-wallet/src/derivation_bls_bip32.rs) andExtendedEd25519PrivKey(key-wallet/src/derivation_slip10.rs) hold raw secret key material but had noZeroize/Dropimplementations, so their secret bytes were left behind in freed heap memory when dropped. Their secp256k1 siblings (ExtendedPrivKey,RootExtendedPrivKey) already zeroize on drop, so intermediate BLS operator / Ed25519 platform-node keys created during path derivation were the odd ones out.Changes
ExtendedBLSPrivKey: hand-writtenZeroize(wipes the inner scalar via its public field —blstrs_plus::ScalarimplementsDefaultIsZeroes, and blsful'sSecretKeyitself has noZeroizeimpl — plus the chain code and derivation metadata) and aDropimpl calling it, mirroringExtendedPrivKeyinbip32.rs.ExtendedEd25519PrivKey: same pattern —Zeroizewiping the[u8; 32]secret, chain code, and metadata, plusDrop.Fingerprint: added aZeroizeimpl inbip32.rs(its inner field is private outside that module) so the new impls can wipe derivation metadata the same way the secp256k1 types do.EqforExtendedEd25519PrivKey(secondary issue from the report): the derivedPartialEqshort-circuited byte-by-byte over the raw secret; replaced with a constant-time fold over the secret bytes (metadata still compared normally).No caller changes needed: all existing field accesses are borrows or
Copy-field reads, so addingDropbreaks no moves.Testing
test_zeroize_clears_key_materialin both modules, andtest_partial_eq_matches_field_equalityfor the hand-writtenEq.cargo test -p key-wallet --all-features: 524 passed; the only failures are pre-existing wall-clock performance tests that also fail (worse) on unmodifieddevon this machine.cargo clippy -p key-wallet --all-features --all-targetsandcargo fmt --check: clean.cargo check -p key-wallet-ffi -p dash-spv --all-features: clean.🤖 Generated with Claude Code