Fix unchecked access (#477)

* add test that reproduces issue

* remove usages of slicing and unwraps; add clippy lints

* check in aggregate() if identifiers are consistent

frost-core/src/benches.rs

@@ -1,4 +1,5 @@
 //! Ciphersuite-generic benchmark functions.
+#![allow(clippy::unwrap_used)]
 
 use std::collections::{BTreeMap, HashMap};
 

frost-core/src/frost.rs

@@ -13,7 +13,6 @@
 use std::{
     collections::{BTreeMap, BTreeSet, HashMap},
     fmt::{self, Debug},
-    ops::Index,
 };
 
 use derive_getters::Getters;
@@ -89,21 +88,10 @@ where
     pub fn iter(&self) -> impl Iterator<Item = (&Identifier<C>, &BindingFactor<C>)> {
         self.0.iter()
     }
-}
 
-impl<C> Index<Identifier<C>> for BindingFactorList<C>
-where
-    C: Ciphersuite,
-{
-    type Output = BindingFactor<C>;
-
-    // Get the binding factor of a participant in the list.
-    //
-    // [`binding_factor_for_participant`] in the spec
-    //
-    // [`binding_factor_for_participant`]: https://www.ietf.org/archive/id/draft-irtf-cfrg-frost-14.html#section-4.3
-    fn index(&self, identifier: Identifier<C>) -> &Self::Output {
-        &self.0[&identifier]
+    /// Get the [`BindingFactor`] for the given identifier, or None if not found.
+    pub fn get(&self, key: &Identifier<C>) -> Option<&BindingFactor<C>> {
+        self.0.get(key)
     }
 }
 
@@ -267,9 +255,12 @@ where
         }
     }
 
-    /// Get a signing commitment by its participant identifier.
-    pub fn signing_commitment(&self, identifier: &Identifier<C>) -> round1::SigningCommitments<C> {
-        self.signing_commitments[identifier]
+    /// Get a signing commitment by its participant identifier, or None if not found.
+    pub fn signing_commitment(
+        &self,
+        identifier: &Identifier<C>,
+    ) -> Option<round1::SigningCommitments<C>> {
+        self.signing_commitments.get(identifier).copied()
     }
 
     /// Compute the preimages to H1 to compute the per-signer binding factors
@@ -362,7 +353,9 @@ where
             return Err(Error::IdentityCommitment);
         }
 
-        let binding_factor = binding_factor_list[*commitment_identifier].clone();
+        let binding_factor = binding_factor_list
+            .get(commitment_identifier)
+            .ok_or(Error::UnknownIdentifier)?;
 
         // Collect the binding commitments and their binding factors for one big
         // multiscalar multiplication at the end.
@@ -410,6 +403,19 @@ pub fn aggregate<C>(
 where
     C: Ciphersuite,
 {
+    // Check if signing_package.signing_commitments and signature_shares have
+    // the same set of identifiers, and if they are all in pubkeys.signer_pubkeys.
+    if signing_package.signing_commitments().len() != signature_shares.len() {
+        return Err(Error::UnknownIdentifier);
+    }
+    if !signing_package
+        .signing_commitments()
+        .keys()
+        .all(|id| signature_shares.contains_key(id) && pubkeys.signer_pubkeys().contains_key(id))
+    {
+        return Err(Error::UnknownIdentifier);
+    }
+
     // Encodes the signing commitment list produced in round one as part of generating [`BindingFactor`], the
     // binding factor.
     let binding_factor_list: BindingFactorList<C> =
@@ -458,17 +464,20 @@ where
             let signer_pubkey = pubkeys
                 .signer_pubkeys
                 .get(signature_share_identifier)
-                .unwrap();
+                .ok_or(Error::UnknownIdentifier)?;
 
             // Compute Lagrange coefficient.
             let lambda_i = derive_interpolating_value(signature_share_identifier, signing_package)?;
 
-            let binding_factor = binding_factor_list[*signature_share_identifier].clone();
+            let binding_factor = binding_factor_list
+                .get(signature_share_identifier)
+                .ok_or(Error::UnknownIdentifier)?;
 
             // Compute the commitment share.
             let R_share = signing_package
                 .signing_commitment(signature_share_identifier)
-                .to_group_commitment_share(&binding_factor);
+                .ok_or(Error::UnknownIdentifier)?
+                .to_group_commitment_share(binding_factor);
 
             // Compute relation values to verify this signature share.
             signature_share.verify(

frost-core/src/frost/keys.rs

@@ -328,6 +328,12 @@ where
 
         Ok(Self(coefficient_commitments))
     }
+
+    /// Get the first commitment (which is equivalent to the VerifyingKey),
+    /// or an error if the vector is empty.
+    pub(crate) fn first(&self) -> Result<CoefficientCommitment<C>, Error<C>> {
+        self.0.get(0).ok_or(Error::MissingCommitment).copied()
+    }
 }
 
 /// A secret share generated by performing a (t-out-of-n) secret sharing scheme,
@@ -412,7 +418,7 @@ where
         }
 
         let group_public = VerifyingKey {
-            element: self.commitment.0[0].0,
+            element: self.commitment.first()?.0,
         };
 
         Ok((VerifyingShare(result), group_public))
@@ -520,7 +526,10 @@ fn evaluate_polynomial<C: Ciphersuite>(
         value = value + *coeff;
         value *= ell_scalar;
     }
-    value = value + coefficients[0];
+    value = value
+        + *coefficients
+            .get(0)
+            .expect("coefficients must have at least one element");
     value
 }
 

frost-core/src/frost/keys/dkg.rs

@@ -130,8 +130,8 @@ pub mod round1 {
         C: Ciphersuite,
     {
         fn zeroize(&mut self) {
-            for i in 0..self.coefficients.len() {
-                self.coefficients[i] = <<C::Group as Group>::Field>::zero();
+            for c in self.coefficients.iter_mut() {
+                *c = <<C::Group as Group>::Field>::zero();
             }
         }
     }
@@ -262,8 +262,12 @@ pub fn part1<C: Ciphersuite, R: RngCore + CryptoRng>(
 
     let k = <<C::Group as Group>::Field>::random(&mut rng);
     let R_i = <C::Group>::generator() * k;
-    let c_i = challenge::<C>(identifier, &R_i, &commitment.0[0].0).ok_or(Error::DKGNotSupported)?;
-    let mu_i = k + coefficients[0] * c_i.0;
+    let c_i =
+        challenge::<C>(identifier, &R_i, &commitment.first()?.0).ok_or(Error::DKGNotSupported)?;
+    let a_i0 = *coefficients
+        .get(0)
+        .expect("coefficients must have at least one element");
+    let mu_i = k + a_i0 * c_i.0;
 
     let secret_package = round1::SecretPackage {
         identifier,
@@ -336,7 +340,7 @@ pub fn part2<C: Ciphersuite>(
         // > R_ℓ ? ≟ g^{μ_ℓ} · φ^{-c_ℓ}_{ℓ0}, where c_ℓ = H(ℓ, Φ, φ_{ℓ0}, R_ℓ).
         let R_ell = round1_package.proof_of_knowledge.R;
         let mu_ell = round1_package.proof_of_knowledge.z;
-        let phi_ell0 = round1_package.commitment.0[0].0;
+        let phi_ell0 = round1_package.commitment.first()?.0;
         let c_ell = challenge::<C>(ell, &R_ell, &phi_ell0).ok_or(Error::DKGNotSupported)?;
 
         if R_ell != <C::Group>::generator() * mu_ell - phi_ell0 * c_ell.0 {
@@ -485,11 +489,11 @@ pub fn part3<C: Ciphersuite>(
         // Round 2, Step 4
         //
         // > Each P_i calculates [...] the group’s public key Y = ∏^n_{j=1} φ_{j0}.
-        group_public = group_public + commitment.0[0].0;
+        group_public = group_public + commitment.first()?.0;
     }
 
     signing_share = signing_share + round2_secret_package.secret_share;
-    group_public = group_public + round2_secret_package.commitment.0[0].0;
+    group_public = group_public + round2_secret_package.commitment.first()?.0;
 
     let signing_share = SigningShare(signing_share);
     // Round 2, Step 4

frost-core/src/frost/keys/repairable.rs

@@ -70,7 +70,10 @@ fn compute_last_random_value<C: Ciphersuite>(
         sum_i_deltas = sum_i_deltas + *v;
     }
 
-    out.insert(*helpers.last().unwrap(), lhs - sum_i_deltas);
+    out.insert(
+        *helpers.last().ok_or(Error::IncorrectNumberOfIdentifiers)?,
+        lhs - sum_i_deltas,
+    );
 
     Ok(out)
 }

frost-core/src/frost/round2.rs

@@ -206,8 +206,10 @@ pub fn sign<C: Ciphersuite>(
     // binding factor.
     let binding_factor_list: BindingFactorList<C> =
         compute_binding_factor_list(signing_package, &key_package.group_public, &[]);
-    let binding_factor: frost::BindingFactor<C> =
-        binding_factor_list[key_package.identifier].clone();
+    let binding_factor: frost::BindingFactor<C> = binding_factor_list
+        .get(&key_package.identifier)
+        .ok_or(Error::UnknownIdentifier)?
+        .clone();
 
     // Compute the group commitment from signing commitments produced in round one.
     let group_commitment = compute_group_commitment(signing_package, &binding_factor_list)?;

frost-core/src/lib.rs

@@ -4,6 +4,8 @@
 #![deny(missing_docs)]
 #![doc = include_str!("../README.md")]
 #![forbid(unsafe_code)]
+#![deny(clippy::indexing_slicing)]
+#![deny(clippy::unwrap_used)]
 
 use std::{
     default::Default,

frost-core/src/scalar_mul.rs

@@ -1,5 +1,9 @@
 //! Non-adjacent form (NAF) implementations for fast batch scalar multiplcation
 
+// We expect slicings in this module to never panic due to algorithmic
+// constraints.
+#![allow(clippy::indexing_slicing)]
+
 use std::{
     borrow::Borrow,
     fmt::{Debug, Result},
@@ -166,7 +170,7 @@ pub trait VartimeMultiscalarMul<C: Ciphersuite>: Clone {
             scalars,
             elements.into_iter().map(|e| Some(e.borrow().clone())),
         )
-        .unwrap()
+        .expect("all elements should be Some")
     }
 }
 

frost-core/src/signature.rs

@@ -39,7 +39,12 @@ where
 
         let R_bytes_len = R_bytes.len();
 
-        R_bytes[..].copy_from_slice(&bytes.as_ref()[0..R_bytes_len]);
+        R_bytes[..].copy_from_slice(
+            bytes
+                .as_ref()
+                .get(0..R_bytes_len)
+                .ok_or(Error::MalformedSignature)?,
+        );
 
         let R_serialization = &R_bytes.try_into().map_err(|_| Error::MalformedSignature)?;
 
@@ -50,7 +55,12 @@ where
         let z_bytes_len = z_bytes.len();
 
         // We extract the exact length of bytes we expect, not just the remaining bytes with `bytes[R_bytes_len..]`
-        z_bytes[..].copy_from_slice(&bytes.as_ref()[R_bytes_len..R_bytes_len + z_bytes_len]);
+        z_bytes[..].copy_from_slice(
+            bytes
+                .as_ref()
+                .get(R_bytes_len..R_bytes_len + z_bytes_len)
+                .ok_or(Error::MalformedSignature)?,
+        );
 
         let z_serialization = &z_bytes.try_into().map_err(|_| Error::MalformedSignature)?;
 

frost-core/src/tests.rs

@@ -1,4 +1,6 @@
 //! Test modules
+#![allow(clippy::indexing_slicing)]
+#![allow(clippy::unwrap_used)]
 
 pub mod batch;
 pub mod ciphersuite_generic;

frost-core/src/tests/ciphersuite_generic.rs

@@ -206,7 +206,7 @@ pub fn check_sign<C: Ciphersuite + PartialEq, R: RngCore + CryptoRng>(
     // generates the final signature.
     ////////////////////////////////////////////////////////////////////////////
 
-    check_aggregate_error(
+    check_aggregate_errors(
         signing_package.clone(),
         signature_shares.clone(),
         pubkey_package.clone(),
@@ -242,7 +242,24 @@ pub fn check_sign<C: Ciphersuite + PartialEq, R: RngCore + CryptoRng>(
     )
 }
 
-fn check_aggregate_error<C: Ciphersuite + PartialEq>(
+fn check_aggregate_errors<C: Ciphersuite + PartialEq>(
+    signing_package: frost::SigningPackage<C>,
+    signature_shares: HashMap<frost::Identifier<C>, frost::round2::SignatureShare<C>>,
+    pubkey_package: frost::keys::PublicKeyPackage<C>,
+) {
+    check_aggregate_corrupted_share(
+        signing_package.clone(),
+        signature_shares.clone(),
+        pubkey_package.clone(),
+    );
+    check_aggregate_invalid_share_identifier_for_signer_pubkeys(
+        signing_package.clone(),
+        signature_shares.clone(),
+        pubkey_package.clone(),
+    );
+}
+
+fn check_aggregate_corrupted_share<C: Ciphersuite + PartialEq>(
     signing_package: frost::SigningPackage<C>,
     mut signature_shares: HashMap<frost::Identifier<C>, frost::round2::SignatureShare<C>>,
     pubkey_package: frost::keys::PublicKeyPackage<C>,
@@ -256,6 +273,26 @@ fn check_aggregate_error<C: Ciphersuite + PartialEq>(
     assert_eq!(e, Error::InvalidSignatureShare { culprit: id });
 }
 
+/// Test NCC-E008263-4VP audit finding (PublicKeyPackage).
+/// Note that the SigningPackage part of the finding is not currently reachable
+/// since it's caught by `compute_lagrange_coefficient()`, and the Binding Factor
+/// part can't either since it's caught before by the PublicKeyPackage part.
+fn check_aggregate_invalid_share_identifier_for_signer_pubkeys<C: Ciphersuite + PartialEq>(
+    signing_package: frost::SigningPackage<C>,
+    mut signature_shares: HashMap<frost::Identifier<C>, frost::round2::SignatureShare<C>>,
+    pubkey_package: frost::keys::PublicKeyPackage<C>,
+) {
+    let invalid_identifier = Identifier::derive("invalid identifier".as_bytes()).unwrap();
+    // Insert a new share (copied from other existing share) with an invalid identifier
+    signature_shares.insert(
+        invalid_identifier,
+        *signature_shares.values().next().unwrap(),
+    );
+    // Should error, but not panic
+    frost::aggregate(&signing_package, &signature_shares, &pubkey_package)
+        .expect_err("should not work");
+}
+
 /// Test FROST signing with trusted dealer with a Ciphersuite.
 pub fn check_sign_with_dkg<C: Ciphersuite + PartialEq, R: RngCore + CryptoRng>(
     mut rng: R,