From 5c1d6f9051726d878d9e8e1362a3d2af88904c85 Mon Sep 17 00:00:00 2001 From: Bohdan Ohorodnii <273991985+varex83agent@users.noreply.github.com> Date: Tue, 30 Jun 2026 18:53:59 +0200 Subject: [PATCH] fix(crypto): harden secret key material against Debug leakage and zeroize gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses five P0 secret-key-material findings across frost, crypto, and k1util — closing log-leakage holes, wiping in-memory secrets, and fixing one Charon parity divergence in on-disk key permissions. - frost: replace derived `Debug` on `SigningShare`, `KeyPackage` and `ShamirShare` with redacting manual impls so secret scalars/shares can no longer reach logs/panics (mirrors the existing `BlsSignature` style). - frost: explicitly zeroize the ephemeral Schnorr nonce and the reconstructed signing-key scalars in kryptology `round1`/`round2`. `Scalar` deliberately keeps `Copy` (removing it ripples across the crate for no guarantee — the inner `blst_fr` is a `Copy` C struct); rationale documented on the type. - crypto: declare the `zeroize` dep and wrap the raw `ikm`/candidate byte buffers in `Zeroizing`. blst's `SecretKey` is `#[zeroize(drop)]`, so the `BlstSecretKey` intermediates already wipe on drop. - k1util: write the secp256k1 private key with mode `0o600`, matching Charon `app/k1util/k1util.go` Save (v1.7.1); previously used the default umask mode. No wire-format, signature, or DKG-output changes; only Debug output, drop behaviour, and on-disk file mode. Adds unit tests for each redaction and the 0o600 permission (incl. tightening an existing looser file). Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@users.noreply.github.com> --- Cargo.lock | 1 + crates/crypto/Cargo.toml | 1 + crates/crypto/src/blst_impl.rs | 31 ++++++++++----- crates/frost/src/curve.rs | 21 ++++++++++ crates/frost/src/frost_core.rs | 57 +++++++++++++++++++++++++- crates/frost/src/kryptology.rs | 59 +++++++++++++++++++++++---- crates/k1util/src/k1util.rs | 73 +++++++++++++++++++++++++++++++++- 7 files changed, 221 insertions(+), 22 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 373f60d8..84c831c3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5683,6 +5683,7 @@ dependencies = [ "rand_core 0.6.4", "test-case", "thiserror 2.0.18", + "zeroize", ] [[package]] diff --git a/crates/crypto/Cargo.toml b/crates/crypto/Cargo.toml index f4d66349..f9929e9b 100644 --- a/crates/crypto/Cargo.toml +++ b/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) diff --git a/crates/crypto/src/blst_impl.rs b/crates/crypto/src/blst_impl.rs index 62605594..a37f4f9c 100644 --- a/crates/crypto/src/blst_impl.rs +++ b/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 { - 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 { 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 = 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 = shares .values() .map(|bytes| { diff --git a/crates/frost/src/curve.rs b/crates/frost/src/curve.rs index 2c690628..c430519b 100644 --- a/crates/frost/src/curve.rs +++ b/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); diff --git a/crates/frost/src/frost_core.rs b/crates/frost/src/frost_core.rs index 07585f12..96429f07 100644 --- a/crates/frost/src/frost_core.rs +++ b/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(&"").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", &"") + .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("")); + // 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: \"\"")); + 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(); diff --git a/crates/frost/src/kryptology.rs b/crates/frost/src/kryptology.rs index 08f94dba..dd7ffb1f 100644 --- a/crates/frost/src/kryptology.rs +++ b/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", &"") + .finish() + } +} + /// Secret state held by a participant between round 1 and round 2. /// /// # Security @@ -327,7 +338,7 @@ pub fn round1( }; // 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( 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,7 +359,7 @@ pub fn round1( 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 { @@ -353,6 +367,9 @@ pub fn round1( 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( 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 = @@ -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("")); + assert!(!rendered.contains("171")); // decimal of 0xAB + } + #[test] fn scalar_from_be_rejects_invalid_scalar_encoding() { assert!(matches!( diff --git a/crates/k1util/src/k1util.rs b/crates/k1util/src/k1util.rs index 704e930e..d7e9d353 100644 --- a/crates/k1util/src/k1util.rs +++ b/crates/k1util/src/k1util.rs @@ -199,10 +199,43 @@ pub fn load(file: &Path) -> Result { } /// Save saves a secret key to a file. +/// +/// On unix the file is created with mode `0o600` (owner read/write only), +/// matching Charon's `app/k1util/k1util.go` `Save` which writes via +/// `os.WriteFile(file, ..., 0o600)`. This prevents the private key from being +/// world-readable. pub fn save(key: &SecretKey, file: &Path) -> Result<()> { let encoded = hex::encode(key.to_bytes()); - std::fs::write(file, encoded).map_err(K1UtilError::FailedToWriteFile)?; + #[cfg(unix)] + { + use std::{ + io::Write as _, + os::unix::fs::{OpenOptionsExt as _, PermissionsExt as _}, + }; + + // Create with `0o600` so the key is never momentarily world-readable. + let mut f = std::fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .mode(0o600) + .open(file) + .map_err(K1UtilError::FailedToWriteFile)?; + + // `mode(0o600)` only applies on creation; force it for an existing file + // too so an overwrite can never leave looser permissions in place. + f.set_permissions(std::fs::Permissions::from_mode(0o600)) + .map_err(K1UtilError::FailedToWriteFile)?; + + f.write_all(encoded.as_bytes()) + .map_err(K1UtilError::FailedToWriteFile)?; + } + + #[cfg(not(unix))] + { + std::fs::write(file, encoded).map_err(K1UtilError::FailedToWriteFile)?; + } Ok(()) } @@ -282,6 +315,44 @@ mod tests { ); } + #[cfg(unix)] + #[test] + fn save_writes_private_key_with_owner_only_permissions() { + use std::os::unix::fs::PermissionsExt; + + let key = SecretKey::random(&mut OsRng); + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("charon-enr-private-key"); + + save(&key, &path).unwrap(); + + let mode = std::fs::metadata(&path).unwrap().permissions().mode(); + assert_eq!(mode & 0o777, 0o600, "private key file must be mode 0o600"); + + // The saved key must still round-trip through `load`. + assert_eq!(load(&path).unwrap(), key, "saved key should round-trip"); + } + + #[cfg(unix)] + #[test] + fn save_tightens_permissions_when_overwriting_existing_file() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("charon-enr-private-key"); + + // Pre-create a world-readable file at the target path. + std::fs::write(&path, "stale").unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).unwrap(); + + let key = SecretKey::random(&mut OsRng); + save(&key, &path).unwrap(); + + let mode = std::fs::metadata(&path).unwrap().permissions().mode(); + assert_eq!(mode & 0o777, 0o600, "overwrite must tighten to mode 0o600"); + assert_eq!(load(&path).unwrap(), key); + } + #[test] fn load_nonexistent_file() { let file = Path::new("nonexistent-file");