hardening

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "multi-key"
-version = "1.0.6"
+version = "1.0.7"
 edition = "2021"
 authors = ["Dave Grantham <dwg@linuxprogrammer.org>"]
 description = "Multikey self-describing cryptographic key data"
@@ -20,25 +20,25 @@ wasm = ["getrandom/wasm_js"]
 legacy_chacha20_fallback = []
 
 [dependencies]
-aes-gcm = "0.10"
-bcrypt-pbkdf = "0.10"
+aes-gcm = "0.11"
+bcrypt-pbkdf = "0.11"
 blake3 = { version = "1.5.1", features = ["traits-preview", "zeroize"] }
 ciborium = "0.2"
 curve25519-dalek = { version = "5.0.0", features = ["group"] }
 # blsful configured per-target below (blst for native, rust for wasm)
 frodo-kem-rs = { version = "0.7", default-features = false, features = ["frodo640aes", "frodo976aes", "frodo1344aes", "frodo640shake", "frodo976shake", "frodo1344shake"] }
-chacha20 = "0.9"
-chacha20poly1305 = "0.10"
+chacha20 = "0.10"
+chacha20poly1305 = "0.11"
 ed25519-dalek = { version = "3.0", features = ["pkcs8", "rand_core", "alloc"] }
 elliptic-curve = "0.14"
 fn-dsa = "0.3"
 getrandom = { version = "0.4", features = ["sys_rng"] }
 hex = "0.4"
-hkdf = "0.12"
+hkdf = "0.13"
 k256 = { version = "0.14", features = ["ecdh"] }
 mceliece348864 = { version = "1.0" }
 ml-dsa = "0.1.1"
-ml-kem = { version = "0.2", features = ["deterministic"] }
+ml-kem = "0.3"
 multi-base = "1.0"
 multi-codec = "1.0"
 multi-hash = "1.0"
@@ -48,7 +48,7 @@ multi-util = "1.0"
 p256 = { version = "0.14", features = ["ecdsa", "ecdh"] }
 p384 = { version = "0.14", features = ["ecdsa", "ecdh"] }
 p521 = { version = "0.14", features = ["ecdsa", "ecdh"] }
-poly1305 = "0.8"
+poly1305 = "0.9"
 pq-mayo = "0.5.0"
 # RustCrypto 0.14 stack (vsss-rs 6, elliptic-curve 0.14, ed25519-dalek 3) is on
 # rand_core 0.10; pin the rand ecosystem to the matching majors so RNGs thread
@@ -66,11 +66,12 @@ rsa = { version = "0.10.0-rc.18", features = ["sha2"] }
 sec1 = "0.8"
 serde = { version = "1.0", default-features = false, features = ["alloc", "derive"], optional = true }
 serde_cbor = "0.11"
-sha2 = "0.10"
+sha2 = "0.11"
 signature = "3"
 slh-dsa = "0.2.0-rc.5"
 sntrup = "0.3.0"
 ssh-encoding = "0.3"
+subtle = "2"
 thiserror = { version = "2.0" }
 typenum = "1.17"
 unsigned-varint = { version = "0.8", features = ["std"] }

src/error.rs

@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: Apache-2.0
 /// Errors created by this library
+#[must_use]
 #[derive(Debug, thiserror::Error)]
 #[non_exhaustive]
 pub enum Error {
@@ -59,6 +60,14 @@ pub enum Error {
     /// Duplicate attribute error
     #[error("Duplicate Multikey attribute: {0}")]
     DuplicateAttribute(u8),
+    /// Attribute count exceeds the configured maximum
+    ///
+    /// Returned by [`crate::mk::Multikey::try_decode_from`] when the number of
+    /// attributes declared in the wire data exceeds
+    /// [`crate::mk::MAX_ATTRIBUTES`]. Bounds the work a crafted input can
+    /// force the decoder to perform and mitigates CWE-400.
+    #[error("attribute count {0} exceeds maximum {1}")]
+    TooManyAttributes(usize, usize),
     /// Incorrect Multikey sigil
     #[error("Missing Multikey sigil")]
     MissingSigil,
@@ -288,6 +297,13 @@ pub enum SealError {
     /// Key derivation failed
     #[error("Key derivation failed: {0}")]
     KeyDerivationFailed(String),
+    /// RNG failure during sealing
+    ///
+    /// Returned when the OS RNG (`getrandom`) fails to produce randomness
+    /// (e.g. on constrained targets without entropy). Propagated from the
+    /// sealing path instead of panicking.
+    #[error("RNG failure: {0}")]
+    RngFailure(String),
 }
 
 /// Sign errors created by this library
@@ -303,6 +319,13 @@ pub enum SignError {
     /// Missing scheme
     #[error("Missing signature scheme")]
     MissingScheme,
+    /// RNG failure during signing
+    ///
+    /// Returned when the OS RNG (`getrandom`) fails to produce randomness
+    /// (e.g. on constrained targets without entropy). Propagated from the
+    /// signing path instead of panicking.
+    #[error("RNG failure: {0}")]
+    RngFailure(String),
 }
 
 /// Threshold errors created by this library
@@ -363,6 +386,7 @@ pub enum VerifyError {
 
 impl Error {
     /// Get the error kind as a string
+    #[must_use]
     pub fn kind(&self) -> &str {
         match self {
             Self::Attributes(_) => "Attributes",
@@ -383,6 +407,7 @@ impl Error {
             Self::Multihash(_) => "Multihash",
             Self::Utf8(_) => "Utf8",
             Self::DuplicateAttribute(_) => "DuplicateAttribute",
+            Self::TooManyAttributes(_, _) => "TooManyAttributes",
             Self::MissingSigil => "MissingSigil",
             Self::UnsupportedAlgorithm(_) => "UnsupportedAlgorithm",
         }

src/lib.rs

@@ -72,6 +72,7 @@
 
 #![warn(missing_docs)]
 #![deny(
+    unsafe_code,
     trivial_casts,
     trivial_numeric_casts,
     unused_import_braces,
@@ -116,7 +117,7 @@ pub mod keysplit;
 pub mod mk;
 pub use mk::{
     Builder, EncodedMultikey, Multikey, FN_DSA_KEY_CODECS, KEY_CODECS, KEY_SHARE_CODECS,
-    ML_DSA_KEY_CODECS, ML_KEM_KEY_CODECS, SLH_DSA_KEY_CODECS, X25519_KEY_CODECS,
+    MAX_ATTRIBUTES, ML_DSA_KEY_CODECS, ML_KEM_KEY_CODECS, SLH_DSA_KEY_CODECS, X25519_KEY_CODECS,
 };
 
 /// Nonce type

src/mk.rs

@@ -30,6 +30,7 @@ use ssh_key::{
     EcdsaCurve, PrivateKey, PublicKey,
 };
 use std::{collections::BTreeMap, fmt};
+use subtle::ConstantTimeEq;
 use zeroize::Zeroizing;
 
 /// the list of key codecs supported for key generation
@@ -128,6 +129,14 @@ pub const KEY_SHARE_CODECS: [Codec; 4] = [
 /// the multicodec sigil for multikey
 pub const SIGIL: Codec = Codec::Multikey;
 
+/// Maximum number of attributes a single decoded [`Multikey`] will accept.
+///
+/// Every legitimate multikey carries at most a handful of attributes (key
+/// data, threshold metadata, cipher info, …). The 256 ceiling comfortably
+/// covers every codec this crate emits while bounding the work a crafted
+/// input can force the decoder to perform (mitigates CWE-400).
+pub const MAX_ATTRIBUTES: usize = 256;
+
 /// A base encoded Multikey structure
 pub type EncodedMultikey = BaseEncoded<Multikey>;
 
@@ -140,6 +149,17 @@ pub struct Multikey {
     /// key codec
     pub(crate) codec: Codec,
     /// the comment for the key
+    ///
+    /// # Zeroization
+    ///
+    /// The comment is **not** zeroized on drop. It is stored as a plain
+    /// [`String`] so it can be serialized, compared, and surfaced in SSH/key
+    /// output without the deref-coercion friction a [`Zeroizing<String>`]
+    /// wrapper would introduce across the ~120 call sites that read it. If a
+    /// caller places sensitive material in the comment, they must zeroize that
+    /// material themselves before it leaves scope; the key *material* (in
+    /// `attributes`) is wrapped in [`Zeroizing`] and is zeroized on drop. See
+    /// the security audit finding R6 for the rationale.
     pub comment: String,
     /// codec-specific attributes, sorted by key
     pub attributes: Attributes,
@@ -169,6 +189,26 @@ impl EncodingInfo for Multikey {
     }
 }
 
+/// Constant-time equality for [`Multikey`] (R7).
+///
+/// The derived [`PartialEq`] short-circuits on the first differing byte,
+/// which leaks timing information when key material (or metadata derived
+/// from it) is compared in a security-sensitive context. This
+/// [`ConstantTimeEq`] impl compares the canonical wire encoding
+/// (`sigil || codec || comment || attributes`) in constant time so callers
+/// that need a side-channel-resistant comparison (e.g. comparing two
+/// fingerprints or two secret keys) can use `mk1.ct_eq(&mk2)` instead of
+/// `mk1 == mk2`. The wire encoding is what gets hashed into a fingerprint,
+/// so comparing it is equivalent to comparing the canonical form.
+impl ConstantTimeEq for Multikey {
+    fn ct_eq(&self, other: &Self) -> subtle::Choice {
+        // Compare the canonical wire encoding byte-for-byte in constant time.
+        let a: Vec<u8> = self.clone().into();
+        let b: Vec<u8> = other.clone().into();
+        a.ct_eq(&b)
+    }
+}
+
 impl From<Multikey> for Vec<u8> {
     fn from(mk: Multikey) -> Self {
         let mut v = Vec::default();
@@ -214,6 +254,11 @@ impl<'a> TryDecodeFrom<'a> for Multikey {
         let comment = String::from_utf8(comment.to_inner())?;
         // decode the number of codec-specific attributes
         let (num_attr, ptr) = Varuint::<usize>::try_decode_from(ptr)?;
+        // reject attribute counts that exceed the configured maximum to bound
+        // the work a crafted input can force the decoder to perform (CWE-400)
+        if *num_attr > MAX_ATTRIBUTES {
+            return Err(Error::TooManyAttributes(*num_attr, MAX_ATTRIBUTES));
+        }
         // decode the codec-specific values
         let (attributes, ptr) = match *num_attr {
             0 => (Attributes::default(), ptr),
@@ -1442,23 +1487,31 @@ impl Builder {
                 out
             }
             Codec::Rsa2048Priv => {
-                let key = ::rsa::RsaPrivateKey::new(&mut rsa::OsRng, 2048)
+                // Probe the OS RNG up front so an entropy failure surfaces as an
+                // error instead of a panic inside UnwrapErr during keygen.
+                rsa::OsRng::check_entropy()
+                    .map_err(|e| ConversionsError::SecretKeyFailure(e.to_string()))?;
+                let key = ::rsa::RsaPrivateKey::new(&mut rand_core::UnwrapErr(rsa::OsRng), 2048)
                     .map_err(|e| ConversionsError::SecretKeyFailure(e.to_string()))?;
                 ::rsa::pkcs1::EncodeRsaPrivateKey::to_pkcs1_der(&key)
                     .map_err(|e| ConversionsError::SecretKeyFailure(e.to_string()))?
                     .as_bytes()
                     .to_vec()
             }
             Codec::Rsa3072Priv => {
-                let key = ::rsa::RsaPrivateKey::new(&mut rsa::OsRng, 3072)
+                rsa::OsRng::check_entropy()
+                    .map_err(|e| ConversionsError::SecretKeyFailure(e.to_string()))?;
+                let key = ::rsa::RsaPrivateKey::new(&mut rand_core::UnwrapErr(rsa::OsRng), 3072)
                     .map_err(|e| ConversionsError::SecretKeyFailure(e.to_string()))?;
                 ::rsa::pkcs1::EncodeRsaPrivateKey::to_pkcs1_der(&key)
                     .map_err(|e| ConversionsError::SecretKeyFailure(e.to_string()))?
                     .as_bytes()
                     .to_vec()
             }
             Codec::Rsa4096Priv => {
-                let key = ::rsa::RsaPrivateKey::new(&mut rsa::OsRng, 4096)
+                rsa::OsRng::check_entropy()
+                    .map_err(|e| ConversionsError::SecretKeyFailure(e.to_string()))?;
+                let key = ::rsa::RsaPrivateKey::new(&mut rand_core::UnwrapErr(rsa::OsRng), 4096)
                     .map_err(|e| ConversionsError::SecretKeyFailure(e.to_string()))?;
                 ::rsa::pkcs1::EncodeRsaPrivateKey::to_pkcs1_der(&key)
                     .map_err(|e| ConversionsError::SecretKeyFailure(e.to_string()))?

src/views/bls12381.rs

@@ -165,7 +165,9 @@ impl<'a> TryDecodeFrom<'a> for ThresholdData {
                 let mut p = ptr;
                 for _ in 0..*num_shares {
                     let (share, ptr) = KeyShare::try_decode_from(p)?;
-                    shares.insert(share.0, share);
+                    if shares.insert(share.0, share).is_some() {
+                        return Err(ThresholdError::DuplicateShare.into());
+                    }
                     p = ptr;
                 }
                 (shares, p)
@@ -923,23 +925,28 @@ impl<'a> ThresholdView for View<'a> {
         let threshold_data: Vec<u8> = {
             let av = self.mk.threshold_attr_view()?;
             let mut tdata = match av.threshold_data() {
-                Ok(b) => ThresholdData::try_from(b).unwrap_or_default(),
+                Ok(b) => ThresholdData::try_from(b)
+                    .map_err(|e| ThresholdError::ShareCombineFailed(e.to_string()))?,
                 Err(_) => ThresholdData::default(),
             };
+            // detect a duplicate share identifier and refuse to silently overwrite it
+            if tdata.0.contains_key(&identifier) {
+                return Err(ThresholdError::DuplicateShare.into());
+            }
             // insert the share data
             tdata.0.insert(identifier, key_share);
             tdata.into()
         };
 
-        // if this multikey doesn't already have the threshold/limi set, then
+        // if this multikey doesn't already have the threshold/limit set, then
         // set it to match the values from the first share
         let av = share.threshold_attr_view()?;
         let threshold = av.threshold().unwrap_or(threshold);
         let limit = av.limit().unwrap_or(limit);
         let comment = if self.mk.comment.is_empty() {
             share.comment.clone()
         } else {
-            String::default()
+            self.mk.comment.clone()
         };
 
         Builder::new(self.mk.codec)
@@ -957,7 +964,8 @@ impl<'a> ThresholdView for View<'a> {
             let av = self.mk.threshold_attr_view()?;
             (
                 match av.threshold_data() {
-                    Ok(b) => ThresholdData::try_from(b).unwrap_or_default(),
+                    Ok(b) => ThresholdData::try_from(b)
+                        .map_err(|e| ThresholdError::ShareCombineFailed(e.to_string()))?,
                     Err(_) => ThresholdData::default(),
                 },
                 av.threshold()?,
@@ -1147,6 +1155,17 @@ impl<'a> ThresholdView for View<'a> {
 
 impl<'a> VerifyView for View<'a> {
     /// try to verify a Multisig using the Multikey
+    ///
+    /// # Curve selection (M1)
+    ///
+    /// The BLS curve (G1 vs G2) is selected by the multikey codec tag
+    /// (`Bls12381G1Pub` / `Bls12381G2Pub` / `*Share`), not by the signature
+    /// byte length. The 48-byte (G1 point) and 96-byte (G2 point) lengths are
+    /// used only to slice the canonical signature into the correct group
+    /// encoding for the curve already chosen by the codec. A 48-byte G2 or
+    /// 96-byte G1 signature is therefore rejected with a deserialization
+    /// error rather than being misverified on the wrong curve. This avoids the
+    /// downgrade heuristic described in the security audit (M1).
     fn verify(&self, multisig: &Multisig, msg: Option<&[u8]>) -> Result<(), Error> {
         let attr = self.mk.attr_view()?;
         let pubmk = if attr.is_secret_key() {

src/views/dkg_threshold.rs

@@ -13,6 +13,8 @@
 use crate::error::AttributesError;
 use crate::Error;
 use crate::{AttrId, AttrView, Multikey, ThresholdAttrView, ThresholdKeyView};
+use multi_trait::TryDecodeFrom;
+use multi_util::Varuint;
 
 /// Read-only DKG threshold view over a [`Multikey`].
 pub(crate) struct View<'a> {
@@ -29,9 +31,16 @@ impl<'a> TryFrom<&'a Multikey> for View<'a> {
 
 impl<'a> AttrView for View<'a> {
     fn is_encrypted(&self) -> bool {
-        // Encryption status is recorded on the multikey itself, not on the view;
-        // a DKG share is conceptually a secret-bearing share, so report false
-        // here and let the multikey's own AttrView impl be authoritative.
+        // A DKG share may itself be encrypted (the `KeyIsEncrypted` attribute is
+        // stamped on the multikey). Earlier revisions hardcoded `false`, which
+        // meant callers using `threshold_attr_view()` alone saw an unencrypted
+        // share even when it was sealed. Read the attribute directly so the
+        // status is authoritative regardless of which view the caller holds.
+        if let Some(v) = self.mk.attributes.get(&AttrId::KeyIsEncrypted) {
+            if let Ok((b, _)) = Varuint::<bool>::try_decode_from(v.as_slice()) {
+                return b.to_inner();
+            }
+        }
         false
     }
 
@@ -40,7 +49,11 @@ impl<'a> AttrView for View<'a> {
     }
 
     fn is_secret_key(&self) -> bool {
-        false
+        // A DKG private share is a secret-bearing key share; treat it as a
+        // secret key for status purposes so callers gating on `is_secret_key()`
+        // (e.g. before signing or exporting) do not silently treat a
+        // secret share as public.
+        true
     }
 
     fn is_secret_key_share(&self) -> bool {