fix(crypto): harden secret key material against Debug leakage and zeroize gaps

crates/crypto/Cargo.toml

@@ -13,6 +13,7 @@ pluto-eth2api.workspace = true
 rand.workspace = true
 rand_core.workspace = true
 thiserror.workspace = true
+zeroize.workspace = true
 
 [lints.rust]
 # Allow unsafe code for blst C bindings (overrides workspace forbid)

crates/crypto/src/blst_impl.rs

@@ -12,6 +12,7 @@ use blst::{
     min_pk::{PublicKey as BlstPublicKey, SecretKey as BlstSecretKey, Signature as BlstSignature},
 };
 use rand_core::{CryptoRng, RngCore};
+use zeroize::Zeroizing;
 
 use crate::{
     tbls::Tbls,
@@ -30,10 +31,12 @@ pub struct BlstImpl;
 
 impl Tbls for BlstImpl {
     fn generate_secret_key(&self, mut rng: impl RngCore + CryptoRng) -> Result<PrivateKey, Error> {
-        let mut ikm = [0u8; 32];
-        rng.fill_bytes(&mut ikm);
+        // `ikm` is secret input key material; wipe it on drop. (`BlstSecretKey`
+        // itself is `#[zeroize(drop)]`, so the derived `sk` is wiped too.)
+        let mut ikm = Zeroizing::new([0u8; 32]);
+        rng.fill_bytes(ikm.as_mut());
 
-        let sk = BlstSecretKey::key_gen(&ikm, &[])
+        let sk = BlstSecretKey::key_gen(ikm.as_ref(), &[])
             .map_err(|_| Error::InvalidSecretKey(BlsError::KeyGeneration))?;
 
         Ok(sk.to_bytes())
@@ -44,11 +47,13 @@ impl Tbls for BlstImpl {
         mut rng: impl RngCore + CryptoRng,
     ) -> Result<PrivateKey, Error> {
         for _ in 0..100 {
-            let mut bytes = [0u8; 32];
-            rng.fill_bytes(&mut bytes);
+            // Wipe the candidate buffer on every iteration; on success its value
+            // is copied into the returned key first.
+            let mut bytes = Zeroizing::new([0u8; 32]);
+            rng.fill_bytes(bytes.as_mut());
 
-            if BlstSecretKey::from_bytes(&bytes).is_ok() {
-                return Ok(bytes);
+            if BlstSecretKey::from_bytes(bytes.as_ref()).is_ok() {
+                return Ok(*bytes);
             }
         }
         Err(Error::InvalidSecretKey(BlsError::KeyGeneration))
@@ -75,14 +80,16 @@ impl Tbls for BlstImpl {
 
         let sk = BlstSecretKey::from_bytes(secret_key)?;
 
-        // Create polynomial coefficients: a_0 = secret, a_1..a_{t-1} = random
+        // Create polynomial coefficients: a_0 = secret, a_1..a_{t-1} = random.
+        // `poly` holds `BlstSecretKey`s, each `#[zeroize(drop)]`, so the secret
+        // coefficients are wiped when `poly` is dropped.
         let mut poly = Vec::with_capacity(threashlod);
         poly.push(sk);
 
         for _ in 1..threshold {
-            let mut ikm = [0u8; 32];
-            rng.fill_bytes(&mut ikm);
-            let coeff = BlstSecretKey::key_gen(&ikm, &[])
+            let mut ikm = Zeroizing::new([0u8; 32]);
+            rng.fill_bytes(ikm.as_mut());
+            let coeff = BlstSecretKey::key_gen(ikm.as_ref(), &[])
                 .map_err(|_| Error::InvalidSecretKey(BlsError::KeyGeneration))?;
             poly.push(coeff);
         }
@@ -116,6 +123,8 @@ impl Tbls for BlstImpl {
         // points)
         let share_points: Vec<Index> = shares.keys().copied().collect();
 
+        // The reconstructed master secret and the parsed share scalars are all
+        // `BlstSecretKey`s (`#[zeroize(drop)]`), so they are wiped on drop.
         let share_secrets: Vec<BlstSecretKey> = shares
             .values()
             .map(|bytes| {

crates/frost/src/curve.rs

@@ -16,6 +16,27 @@ use subtle::ConstantTimeEq;
 use zeroize::Zeroize;
 
 /// BLS12-381 scalar field element. Wrapper around `blst_fr` in Montgomery form.
+///
+/// # Secret material & zeroization
+///
+/// `Scalar` is used to hold secret key material (DKG polynomial coefficients,
+/// signing shares). It implements [`Zeroize`] but intentionally **retains
+/// `Copy`** rather than deriving `Drop`/`ZeroizeOnDrop`:
+///
+/// - `Drop`/`ZeroizeOnDrop` are mutually exclusive with `Copy`, and `Scalar` is
+///   consumed by value throughout this crate (the arithmetic operators take
+///   `self`, `to_scalar()` returns by value). Removing `Copy` would ripple
+///   across every call site for no real guarantee, because the inner `blst_fr`
+///   is itself a `Copy` C struct — moves bit-copy it regardless.
+/// - Secret-holding wrapper types ([`crate::SigningShare`],
+///   [`crate::KeyPackage`], [`crate::kryptology::ShamirShare`]) derive
+///   `ZeroizeOnDrop`, which wipes their inner `Scalar`/bytes via this `Zeroize`
+///   impl on drop.
+/// - Bare secret `Scalar` locals (the DKG nonce and reconstructed key in
+///   `kryptology::round1`/`round2`) are wiped explicitly with
+///   [`Zeroize::zeroize`].
+///
+/// Zeroization here is best-effort defense-in-depth, not an absolute guarantee.
 #[derive(Copy, Clone, Default, PartialEq, Eq)]
 pub struct Scalar(pub(crate) blst_fr);
 

crates/frost/src/frost_core.rs

@@ -7,6 +7,7 @@
 use std::{
     cmp::Ordering,
     collections::{BTreeMap, BTreeSet},
+    fmt,
 };
 
 use super::*;
@@ -138,9 +139,17 @@ impl VerifiableSecretSharingCommitment {
 /// A secret scalar value representing a signer's share of the group secret.
 ///
 /// See: https://github.com/ZcashFoundation/frost/blob/3ffc19d8f473d5bc4e07ed41bc884bdb42d6c29f/frost-core/src/keys.rs#L82-L87
-#[derive(Clone, Debug, ZeroizeOnDrop)]
+#[derive(Clone, ZeroizeOnDrop)]
 pub struct SigningShare(Scalar);
 
+// Manual `Debug` so the secret scalar is never rendered. Mirrors the redacting
+// pattern used for `BlsSignature`/`BlsPartialSignature` in `kryptology.rs`.
+impl fmt::Debug for SigningShare {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_tuple("SigningShare").field(&"<redacted>").finish()
+    }
+}
+
 impl SigningShare {
     /// Create a signing share from a scalar.
     pub fn new(scalar: Scalar) -> Self {
@@ -263,7 +272,7 @@ impl SecretShare {
 /// A key package containing all key material for a participant.
 ///
 /// See: https://github.com/ZcashFoundation/frost/blob/3ffc19d8f473d5bc4e07ed41bc884bdb42d6c29f/frost-core/src/keys.rs#L617-L643
-#[derive(Debug, ZeroizeOnDrop)]
+#[derive(ZeroizeOnDrop)]
 pub struct KeyPackage {
     #[zeroize(skip)]
     identifier: Identifier,
@@ -276,6 +285,20 @@ pub struct KeyPackage {
     min_signers: u16,
 }
 
+// Manual `Debug` that exposes the public fields but redacts the secret
+// `signing_share` so it cannot leak via logs/panics.
+impl fmt::Debug for KeyPackage {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_struct("KeyPackage")
+            .field("identifier", &self.identifier)
+            .field("signing_share", &"<redacted>")
+            .field("verifying_share", &self.verifying_share)
+            .field("verifying_key", &self.verifying_key)
+            .field("min_signers", &self.min_signers)
+            .finish()
+    }
+}
+
 impl KeyPackage {
     /// Create a new key package.
     pub fn new(
@@ -526,6 +549,36 @@ mod tests {
         ));
     }
 
+    #[test]
+    fn signing_share_debug_redacts_secret_scalar() {
+        let share = SigningShare::new(Scalar::from(0x4142_4344_4546_4748));
+
+        let rendered = format!("{share:?}");
+
+        assert!(rendered.contains("<redacted>"));
+        // The unredacted form would render the inner `Scalar(...)`; it must not.
+        assert!(!rendered.contains("Scalar"));
+    }
+
+    #[test]
+    fn key_package_debug_redacts_signing_share() {
+        let id = Identifier::from_u32(1).unwrap();
+        let key_package = KeyPackage::new(
+            id,
+            SigningShare::new(Scalar::from(0x5152_5354_5556_5758)),
+            VerifyingShare::new(G1Projective::generator()),
+            VerifyingKey::new(G1Projective::generator()),
+            2,
+        );
+
+        let rendered = format!("{key_package:?}");
+
+        // The secret share field is redacted; public fields remain visible.
+        assert!(rendered.contains("signing_share: \"<redacted>\""));
+        assert!(rendered.contains("identifier"));
+        assert!(rendered.contains("verifying_key"));
+    }
+
     #[test]
     fn public_key_package_from_dkg_commitments_rejects_empty_commitments() {
         let empty_commitments = BTreeMap::new();

crates/frost/src/kryptology.rs

@@ -14,7 +14,7 @@ use std::{collections::BTreeMap, fmt};
 use blst::*;
 use rand_core::{CryptoRng, RngCore};
 use sha2::{Digest, Sha256};
-use zeroize::ZeroizeOnDrop;
+use zeroize::{Zeroize, ZeroizeOnDrop};
 
 use super::*;
 
@@ -91,7 +91,7 @@ pub struct Round2Bcast {
 /// A Shamir secret share matching Go's `sharing.ShamirShare`.
 ///
 /// The `value` field is in **big-endian** byte order.
-#[derive(Clone, Debug, PartialEq, Eq, ZeroizeOnDrop)]
+#[derive(Clone, PartialEq, Eq, ZeroizeOnDrop)]
 pub struct ShamirShare {
     /// The share identifier (1-indexed participant ID).
     #[zeroize(skip)]
@@ -100,6 +100,17 @@ pub struct ShamirShare {
     pub value: [u8; 32],
 }
 
+// Manual `Debug` so the secret share `value` is never rendered; only the
+// (non-secret) `id` is shown.
+impl fmt::Debug for ShamirShare {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_struct("ShamirShare")
+            .field("id", &self.id)
+            .field("value", &"<redacted>")
+            .finish()
+    }
+}
+
 /// Secret state held by a participant between round 1 and round 2.
 ///
 /// # Security
@@ -327,7 +338,7 @@ pub fn round1<R: RngCore + CryptoRng>(
     };
 
     // Schnorr proof of knowledge: sample nonce k, compute R = k*G
-    let k = loop {
+    let mut k = loop {
         let s = Scalar::random(&mut *rng);
         if s != Scalar::ZERO {
             break s;
@@ -336,7 +347,10 @@ pub fn round1<R: RngCore + CryptoRng>(
     let r_point = G1Projective::generator() * k;
     let id_u8 = u8::try_from(id).expect("id <= max_signers <= u8::MAX validated above");
     let ci = kryptology_challenge(id_u8, ctx, &commitment_points[0], &r_point);
-    let wi = k + coefficients[0] * ci;
+    let mut wi = k + coefficients[0] * ci;
+    // The nonce `k` is secret (it would reveal `coefficients[0]` together with
+    // the broadcast `wi`); wipe it now that `wi` is computed.
+    k.zeroize();
 
     // Pre-compute Shamir shares for every other participant
     let mut shares = BTreeMap::new();
@@ -345,14 +359,17 @@ pub fn round1<R: RngCore + CryptoRng>(
             continue;
         }
         let j_id = Identifier::from_u32(j)?;
-        let share_scalar = SigningShare::from_coefficients(&coefficients, j_id).to_scalar();
+        let mut share_scalar = SigningShare::from_coefficients(&coefficients, j_id).to_scalar();
         shares.insert(
             j,
             ShamirShare {
                 id: j,
                 value: scalar_to_be(&share_scalar),
             },
         );
+        // The per-peer secret share has been copied into `ShamirShare.value`
+        // (itself zeroized on drop); wipe the bare scalar copy.
+        share_scalar.zeroize();
     }
 
     let bcast = Round1Bcast {
@@ -363,6 +380,8 @@ pub fn round1<R: RngCore + CryptoRng>(
         wi: scalar_to_be(&wi),
         ci: scalar_to_be(&ci),
     };
+    // `wi` is broadcast, but wipe the local copy as defense-in-depth.
+    wi.zeroize();
 
     let secret = Round1Secret {
         id,
@@ -405,7 +424,7 @@ pub fn round2(
     }
 
     let own_identifier = Identifier::from_u32(secret.id)?;
-    let own_share_scalar =
+    let mut own_share_scalar =
         SigningShare::from_coefficients(&secret.coefficients, own_identifier).to_scalar();
 
     let mut peer_commitments: BTreeMap<Identifier, VerifiableSecretSharingCommitment> =
@@ -444,7 +463,7 @@ pub fn round2(
         if share.id != secret.id {
             return Err(KryptologyError::InvalidShare { culprit: sender_id });
         }
-        let share_scalar = scalar_from_be(&share.value)?;
+        let mut share_scalar = scalar_from_be(&share.value)?;
 
         let signing_share = SigningShare::new(share_scalar);
         let secret_share =
@@ -454,16 +473,25 @@ pub fn round2(
             .map_err(|_| KryptologyError::InvalidShare { culprit: sender_id })?;
 
         share_sum = share_sum + share_scalar;
+        // The received secret share has been folded into `share_sum`; wipe the
+        // bare copy (the `SigningShare` above wipes itself on drop).
+        share_scalar.zeroize();
 
         let sender_identifier = Identifier::from_u32(sender_id)?;
         peer_commitments.insert(sender_identifier, sender_commitment);
     }
 
-    let total_scalar = own_share_scalar + share_sum;
+    let mut total_scalar = own_share_scalar + share_sum;
+    // The summands are no longer needed once the signing key is reconstructed.
+    own_share_scalar.zeroize();
+    share_sum.zeroize();
 
     let signing_share = SigningShare::new(total_scalar);
     let verifying_share_element = G1Projective::generator() * total_scalar;
     let verifying_share = VerifyingShare::new(verifying_share_element);
+    // `total_scalar` is the reconstructed signing key; it has been copied into
+    // `signing_share` (zeroized on drop). Wipe this bare copy.
+    total_scalar.zeroize();
 
     // Build PublicKeyPackage from all participants' commitments
     peer_commitments.insert(own_identifier, secret.commitment.clone());
@@ -672,6 +700,21 @@ mod tests {
 
     use super::*;
 
+    #[test]
+    fn shamir_share_debug_redacts_value() {
+        let share = ShamirShare {
+            id: 7,
+            value: [0xAB; 32],
+        };
+
+        let rendered = format!("{share:?}");
+
+        // The id is visible; the secret value bytes are not.
+        assert!(rendered.contains("id: 7"));
+        assert!(rendered.contains("<redacted>"));
+        assert!(!rendered.contains("171")); // decimal of 0xAB
+    }
+
     #[test]
     fn scalar_from_be_rejects_invalid_scalar_encoding() {
         assert!(matches!(