From 7826a14cb2ee8b12ade5ea2d64259d8558377ace Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Fri, 10 Jul 2026 17:21:19 +0700 Subject: [PATCH] fix(key-wallet): zeroize ExtendedBLSPrivKey and ExtendedEd25519PrivKey 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 --- key-wallet/src/bip32.rs | 9 +++ key-wallet/src/derivation_bls_bip32.rs | 44 ++++++++++++++ key-wallet/src/derivation_slip10.rs | 81 +++++++++++++++++++++++++- 3 files changed, 133 insertions(+), 1 deletion(-) diff --git a/key-wallet/src/bip32.rs b/key-wallet/src/bip32.rs index 041794a1d..f48bcaee7 100644 --- a/key-wallet/src/bip32.rs +++ b/key-wallet/src/bip32.rs @@ -120,6 +120,15 @@ impl zeroize::Zeroize for ChainCode { } } +// Manual implementation of Zeroize for Fingerprint, so extended-key types in +// other modules (BLS, Ed25519) can wipe their derivation metadata too. +// Note: Fingerprint is Copy, so this won't prevent copies in registers/stack +impl zeroize::Zeroize for Fingerprint { + fn zeroize(&mut self) { + zeroize::Zeroize::zeroize(&mut self.0); + } +} + impl fmt::LowerHex for ChainCode { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { for &byte in &self.0 { diff --git a/key-wallet/src/derivation_bls_bip32.rs b/key-wallet/src/derivation_bls_bip32.rs index 7c76caca1..ec85f0333 100644 --- a/key-wallet/src/derivation_bls_bip32.rs +++ b/key-wallet/src/derivation_bls_bip32.rs @@ -78,6 +78,33 @@ pub struct ExtendedBLSPrivKey { pub chain_code: ChainCode, } +// Hand-written (not `#[derive(Zeroize)]`): `BlsSecretKey` has no `Zeroize` +// impl of its own, but its inner scalar (public field `0`) does, so we wipe +// the value field by field. `Drop` (below) calls this, so the key is wiped +// automatically on scope exit with no caller action required. +// Cf. `ExtendedPrivKey` in `bip32`. +impl zeroize::Zeroize for ExtendedBLSPrivKey { + fn zeroize(&mut self) { + // Secret key material. + self.private_key.0.zeroize(); + self.chain_code.zeroize(); + // Derivation metadata — cleared too so the whole value is wiped. + self.depth.zeroize(); + self.parent_fingerprint.zeroize(); + self.child_number = ChildNumber::Normal { + index: 0, + }; + self.network = Network::Mainnet; // repr(u8)=0 discriminant, the "zero" value + } +} + +impl Drop for ExtendedBLSPrivKey { + fn drop(&mut self) { + use zeroize::Zeroize; + self.zeroize(); + } +} + impl ExtendedBLSPrivKey { /// Create a new master key from a seed pub fn new_master(network: Network, seed: &[u8]) -> Result { @@ -1213,4 +1240,21 @@ mod tests { assert_eq!(child_hardened.parent_fingerprint, master.fingerprint()); assert_eq!(child_unhardened.parent_fingerprint, master.fingerprint()); } + + #[test] + fn test_zeroize_clears_key_material() { + use zeroize::Zeroize; + + let seed = [42u8; 32]; + let mut key = ExtendedBLSPrivKey::new_master(Network::Testnet, &seed).unwrap(); + assert_ne!(key.private_key.to_be_bytes(), [0u8; 32]); + assert_ne!(key.chain_code.as_ref(), &[0u8; 32]); + + key.zeroize(); + + assert_eq!(key.private_key.to_be_bytes(), [0u8; 32]); + assert_eq!(key.chain_code.as_ref(), &[0u8; 32]); + assert_eq!(key.depth, 0); + assert_eq!(key.parent_fingerprint, Fingerprint::default()); + } } diff --git a/key-wallet/src/derivation_slip10.rs b/key-wallet/src/derivation_slip10.rs index b24f1c91f..804154003 100644 --- a/key-wallet/src/derivation_slip10.rs +++ b/key-wallet/src/derivation_slip10.rs @@ -26,7 +26,7 @@ pub use dashcore::ed25519_dalek::VerifyingKey as Ed25519PublicKey; pub use crate::bip32::DerivationPath; /// Extended Ed25519 private key for SLIP-0010 -#[derive(Clone, PartialEq, Eq)] +#[derive(Clone)] pub struct ExtendedEd25519PrivKey { /// Network this key is for pub network: Network, @@ -42,6 +42,52 @@ pub struct ExtendedEd25519PrivKey { pub chain_code: ChainCode, } +// Hand-written instead of derived so the raw secret bytes are compared in +// constant time (a derived `==` on `[u8; 32]` short-circuits on the first +// differing byte, leaking how much of the secret matches). +impl PartialEq for ExtendedEd25519PrivKey { + fn eq(&self, other: &Self) -> bool { + let mut diff = 0u8; + for (a, b) in self.private_key.iter().zip(other.private_key.iter()) { + diff |= a ^ b; + } + // black_box keeps the compiler from turning the fold back into an + // early-exit comparison. + core::hint::black_box(diff) == 0 + && self.network == other.network + && self.depth == other.depth + && self.parent_fingerprint == other.parent_fingerprint + && self.child_number == other.child_number + && self.chain_code == other.chain_code + } +} + +impl Eq for ExtendedEd25519PrivKey {} + +// `Drop` (below) calls this, so the key is wiped automatically on scope exit +// with no caller action required. Cf. `ExtendedPrivKey` in `bip32`. +impl zeroize::Zeroize for ExtendedEd25519PrivKey { + fn zeroize(&mut self) { + // Secret key material. + self.private_key.zeroize(); + self.chain_code.zeroize(); + // Derivation metadata — cleared too so the whole value is wiped. + self.depth.zeroize(); + self.parent_fingerprint.zeroize(); + self.child_number = ChildNumber::Normal { + index: 0, + }; + self.network = Network::Mainnet; // repr(u8)=0 discriminant, the "zero" value + } +} + +impl Drop for ExtendedEd25519PrivKey { + fn drop(&mut self) { + use zeroize::Zeroize; + self.zeroize(); + } +} + impl ExtendedEd25519PrivKey { /// Create a new master key from seed pub fn new_master(network: Network, seed: &[u8]) -> Result { @@ -741,4 +787,37 @@ mod test { hex::encode(private_key) } + + #[test] + fn test_zeroize_clears_key_material() { + use zeroize::Zeroize; + + let seed = hex::decode(CASE_1_SEED).unwrap(); + let mut key = ExtendedEd25519PrivKey::new_master(Network::Mainnet, &seed).unwrap(); + assert_ne!(key.private_key, [0u8; 32]); + assert_ne!(key.chain_code.as_ref(), &[0u8; 32]); + + key.zeroize(); + + assert_eq!(key.private_key, [0u8; 32]); + assert_eq!(key.chain_code.as_ref(), &[0u8; 32]); + assert_eq!(key.depth, 0); + assert_eq!(key.parent_fingerprint, Fingerprint::default()); + } + + #[test] + fn test_partial_eq_matches_field_equality() { + let seed = hex::decode(CASE_1_SEED).unwrap(); + let master = ExtendedEd25519PrivKey::new_master(Network::Mainnet, &seed).unwrap(); + + assert_eq!(master, master.clone()); + + let child = master.ckd_priv(ChildNumber::from_hardened_idx(0).unwrap()).unwrap(); + assert_ne!(master, child); + + // Same secret but different metadata must not compare equal + let mut other_network = master.clone(); + other_network.network = Network::Testnet; + assert_ne!(master, other_network); + } }