return error when deserializing the identity (#671)

frost-core/CHANGELOG.md

@@ -4,6 +4,23 @@ Entries are listed in reverse chronological order.
 
 ## Unreleased
 
+* Changed the `deserialize()` function of Elements and structs containing
+  Elements to return an error if the element is the identity. This is a
+  requirement in the FROST specification that wasn't being followed. We are not
+  aware of any possible security issues that could be caused by this; in the
+  unlikely case that the identity was being serialized, this would be caught by
+  deserialization methods. However, we consider this change the right thing to
+  do as a defense-in-depth mechanism. This entails the following changes:
+  * `Group::serialize()` now returns an error. When implementing it, you must
+    return an error if it attempts to serialize the identity.
+  * `VerifyingShare::serialize()`, `CoefficientCommitment::serialize()`,
+    `VerifiableSecretSharingCommitment::serialize()`,
+    `NonceCommitment::serialize()`, `Signature::serialize()`,
+    `VerifyingKey::serialize()` can now all return an error.
+* Removed `batch::Item::into()` which created a batch Item from a triple of
+  VerifyingKey, Signature and message. Use the new `batch::Item::new()` instead
+  (which can return an error).
+
 ## 1.0.1
 
 * Fixed `no-default-features`, previously it wouldn't compile.

frost-core/src/batch.rs

@@ -25,16 +25,20 @@ pub struct Item<C: Ciphersuite> {
     c: Challenge<C>,
 }
 
-impl<'msg, C, M> From<(VerifyingKey<C>, Signature<C>, &'msg M)> for Item<C>
+impl<C> Item<C>
 where
     C: Ciphersuite,
-    M: AsRef<[u8]>,
 {
-    fn from((vk, sig, msg): (VerifyingKey<C>, Signature<C>, &'msg M)) -> Self {
+    /// Create a new batch [`Item`] from a [`VerifyingKey`], [`Signature`]
+    /// and a message to be verified.
+    pub fn new<M>(vk: VerifyingKey<C>, sig: Signature<C>, msg: M) -> Result<Self, Error<C>>
+    where
+        M: AsRef<[u8]>,
+    {
         // Compute c now to avoid dependency on the msg lifetime.
-        let c = crate::challenge(&sig.R, &vk, msg.as_ref());
+        let c = crate::challenge(&sig.R, &vk, msg.as_ref())?;
 
-        Self { vk, sig, c }
+        Ok(Self { vk, sig, c })
     }
 }
 
@@ -129,7 +133,7 @@ where
             Rs.push(R);
 
             VK_coeffs.push(<<C::Group as Group>::Field>::zero() + (blind * item.c.0));
-            VKs.push(item.vk.element);
+            VKs.push(item.vk.to_element());
         }
 
         let scalars = once(&P_coeff_acc)

frost-core/src/benches.rs

@@ -67,7 +67,8 @@ pub fn bench_batch_verify<C: Ciphersuite, R: RngCore + CryptoRng + Clone>(
                         let msg = b"Bench";
 
                         let Item { vk, sig } = item;
-                        batch.queue((*vk, *sig, msg));
+                        let item = batch::Item::<C>::new(*vk, *sig, msg).unwrap();
+                        batch.queue(item);
                     }
                     batch.verify(&mut rng)
                 })

frost-core/src/keys.rs

@@ -17,11 +17,12 @@ use rand_core::{CryptoRng, RngCore};
 use zeroize::{DefaultIsZeroes, Zeroize};
 
 use crate::{
-    Ciphersuite, Element, Error, Field, Group, Header, Identifier, Scalar, SigningKey, VerifyingKey,
+    serialization::SerializableElement, Ciphersuite, Element, Error, Field, Group, GroupError,
+    Header, Identifier, Scalar, SigningKey, VerifyingKey,
 };
 
 #[cfg(feature = "serde")]
-use crate::serialization::{ElementSerialization, ScalarSerialization};
+use crate::serialization::ScalarSerialization;
 
 #[cfg(feature = "serialization")]
 use crate::serialization::{Deserialize, Serialize};
@@ -39,7 +40,7 @@ pub(crate) fn sum_commitments<C: Ciphersuite>(
     commitments: &[&VerifiableSecretSharingCommitment<C>],
 ) -> Result<VerifiableSecretSharingCommitment<C>, Error<C>> {
     let mut group_commitment = vec![
-        CoefficientCommitment(<C::Group>::identity());
+        CoefficientCommitment::new(<C::Group>::identity());
         commitments
             .first()
             .ok_or(Error::IncorrectNumberOfCommitments)?
@@ -48,7 +49,7 @@ pub(crate) fn sum_commitments<C: Ciphersuite>(
     ];
     for commitment in commitments {
         for (i, c) in group_commitment.iter_mut().enumerate() {
-            *c = CoefficientCommitment(
+            *c = CoefficientCommitment::new(
                 c.value()
                     + commitment
                         .0
@@ -187,9 +188,8 @@ where
 #[derive(Copy, Clone, PartialEq, Eq)]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 #[cfg_attr(feature = "serde", serde(bound = "C: Ciphersuite"))]
-#[cfg_attr(feature = "serde", serde(try_from = "ElementSerialization<C>"))]
-#[cfg_attr(feature = "serde", serde(into = "ElementSerialization<C>"))]
-pub struct VerifyingShare<C>(pub(super) Element<C>)
+#[cfg_attr(feature = "serde", serde(transparent))]
+pub struct VerifyingShare<C>(pub(super) SerializableElement<C>)
 where
     C: Ciphersuite;
 
@@ -198,27 +198,30 @@ where
     C: Ciphersuite,
 {
     /// Create a new [`VerifyingShare`] from a element.
-    #[cfg(feature = "internals")]
-    pub fn new(element: Element<C>) -> Self {
-        Self(element)
+    #[cfg_attr(feature = "internals", visibility::make(pub))]
+    #[cfg_attr(docsrs, doc(cfg(feature = "internals")))]
+    pub(crate) fn new(element: Element<C>) -> Self {
+        Self(SerializableElement(element))
     }
 
     /// Get the inner element.
-    #[cfg(feature = "internals")]
-    pub fn to_element(&self) -> Element<C> {
-        self.0
+    #[cfg_attr(feature = "internals", visibility::make(pub))]
+    #[cfg_attr(docsrs, doc(cfg(feature = "internals")))]
+    #[allow(dead_code)]
+    pub(crate) fn to_element(&self) -> Element<C> {
+        self.0 .0
     }
 
     /// Deserialize from bytes
     pub fn deserialize(bytes: <C::Group as Group>::Serialization) -> Result<Self, Error<C>> {
         <C::Group as Group>::deserialize(&bytes)
-            .map(|element| Self(element))
+            .map(|element| Self(SerializableElement(element)))
             .map_err(|e| e.into())
     }
 
     /// Serialize to bytes
-    pub fn serialize(&self) -> <C::Group as Group>::Serialization {
-        <C::Group as Group>::serialize(&self.0)
+    pub fn serialize(&self) -> Result<<C::Group as Group>::Serialization, Error<C>> {
+        Ok(<C::Group as Group>::serialize(&self.0 .0)?)
     }
 
     /// Computes a verifying share for a peer given the group commitment.
@@ -238,7 +241,7 @@ where
         // Y_i = ∏_{k=0}^{t−1} (∏_{j=1}^n φ_{jk})^{i^k mod q}
         // i.e. we can operate on the sum of all φ_j commitments, which is
         // what is passed to the functions.
-        VerifyingShare(evaluate_vss(identifier, commitment))
+        VerifyingShare::new(evaluate_vss(identifier, commitment))
     }
 }
 
@@ -248,7 +251,12 @@ where
 {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         f.debug_tuple("VerifyingShare")
-            .field(&hex::encode(self.serialize()))
+            .field(
+                &self
+                    .serialize()
+                    .map(hex::encode)
+                    .unwrap_or("<invalid>".to_string()),
+            )
             .finish()
     }
 }
@@ -258,29 +266,7 @@ where
     C: Ciphersuite,
 {
     fn from(secret: SigningShare<C>) -> VerifyingShare<C> {
-        VerifyingShare(<C::Group>::generator() * secret.0 as Scalar<C>)
-    }
-}
-
-#[cfg(feature = "serde")]
-impl<C> TryFrom<ElementSerialization<C>> for VerifyingShare<C>
-where
-    C: Ciphersuite,
-{
-    type Error = Error<C>;
-
-    fn try_from(value: ElementSerialization<C>) -> Result<Self, Self::Error> {
-        Self::deserialize(value.0)
-    }
-}
-
-#[cfg(feature = "serde")]
-impl<C> From<VerifyingShare<C>> for ElementSerialization<C>
-where
-    C: Ciphersuite,
-{
-    fn from(value: VerifyingShare<C>) -> Self {
-        Self(value.serialize())
+        VerifyingShare::new(<C::Group>::generator() * secret.0 as Scalar<C>)
     }
 }
 
@@ -291,9 +277,7 @@ where
 #[derive(Clone, Copy, PartialEq, Eq)]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 #[cfg_attr(feature = "serde", serde(bound = "C: Ciphersuite"))]
-#[cfg_attr(feature = "serde", serde(try_from = "ElementSerialization<C>"))]
-#[cfg_attr(feature = "serde", serde(into = "ElementSerialization<C>"))]
-pub struct CoefficientCommitment<C: Ciphersuite>(pub(crate) Element<C>);
+pub struct CoefficientCommitment<C: Ciphersuite>(pub(crate) SerializableElement<C>);
 
 impl<C> CoefficientCommitment<C>
 where
@@ -302,12 +286,12 @@ where
     /// Create a new CoefficientCommitment.
     #[cfg_attr(feature = "internals", visibility::make(pub))]
     pub(crate) fn new(value: Element<C>) -> Self {
-        Self(value)
+        Self(SerializableElement(value))
     }
 
     /// returns serialized element
-    pub fn serialize(&self) -> <C::Group as Group>::Serialization {
-        <C::Group>::serialize(&self.0)
+    pub fn serialize(&self) -> Result<<C::Group as Group>::Serialization, Error<C>> {
+        Ok(<C::Group>::serialize(&self.0 .0)?)
     }
 
     /// Creates a new commitment from a coefficient input
@@ -319,7 +303,7 @@ where
 
     /// Returns inner element value
     pub fn value(&self) -> Element<C> {
-        self.0
+        self.0 .0
     }
 }
 
@@ -329,33 +313,16 @@ where
 {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> std::fmt::Result {
         f.debug_tuple("CoefficientCommitment")
-            .field(&hex::encode(self.serialize()))
+            .field(
+                &self
+                    .serialize()
+                    .map(hex::encode)
+                    .unwrap_or("<invalid>".to_string()),
+            )
             .finish()
     }
 }
 
-#[cfg(feature = "serde")]
-impl<C> TryFrom<ElementSerialization<C>> for CoefficientCommitment<C>
-where
-    C: Ciphersuite,
-{
-    type Error = Error<C>;
-
-    fn try_from(value: ElementSerialization<C>) -> Result<Self, Self::Error> {
-        Self::deserialize(value.0)
-    }
-}
-
-#[cfg(feature = "serde")]
-impl<C> From<CoefficientCommitment<C>> for ElementSerialization<C>
-where
-    C: Ciphersuite,
-{
-    fn from(value: CoefficientCommitment<C>) -> Self {
-        Self(value.serialize())
-    }
-}
-
 /// Contains the commitments to the coefficients for our secret polynomial _f_,
 /// used to generate participants' key shares.
 ///
@@ -386,11 +353,12 @@ where
     }
 
     /// Returns serialized coefficent commitments
-    pub fn serialize(&self) -> Vec<<C::Group as Group>::Serialization> {
-        self.0
+    pub fn serialize(&self) -> Result<Vec<<C::Group as Group>::Serialization>, Error<C>> {
+        Ok(self
+            .0
             .iter()
-            .map(|cc| <<C as Ciphersuite>::Group as Group>::serialize(&cc.0))
-            .collect()
+            .map(|cc| <<C as Ciphersuite>::Group as Group>::serialize(&cc.value()))
+            .collect::<Result<_, GroupError>>()?)
     }
 
     /// Returns VerifiableSecretSharingCommitment from a vector of serialized CoefficientCommitments
@@ -409,7 +377,7 @@ where
     /// element in the vector), or an error if the vector is empty.
     pub(crate) fn verifying_key(&self) -> Result<VerifyingKey<C>, Error<C>> {
         Ok(VerifyingKey::new(
-            self.0.first().ok_or(Error::MissingCommitment)?.0,
+            self.0.first().ok_or(Error::MissingCommitment)?.0 .0,
         ))
     }
 
@@ -489,7 +457,10 @@ where
             return Err(Error::InvalidSecretShare);
         }
 
-        Ok((VerifyingShare(result), self.commitment.verifying_key()?))
+        Ok((
+            VerifyingShare::new(result),
+            self.commitment.verifying_key()?,
+        ))
     }
 }
 
@@ -634,7 +605,9 @@ fn evaluate_vss<C: Ciphersuite>(
 
     let (_, result) = commitment.0.iter().fold(
         (<<C::Group as Group>::Field>::one(), <C::Group>::identity()),
-        |(i_to_the_k, sum_so_far), comm_k| (i * i_to_the_k, sum_so_far + comm_k.0 * i_to_the_k),
+        |(i_to_the_k, sum_so_far), comm_k| {
+            (i * i_to_the_k, sum_so_far + comm_k.value() * i_to_the_k)
+        },
     );
     result
 }
@@ -863,7 +836,7 @@ pub(crate) fn generate_secret_polynomial<C: Ciphersuite>(
     // Create the vector of commitments
     let commitment: Vec<_> = coefficients
         .iter()
-        .map(|c| CoefficientCommitment(<C::Group as Group>::generator() * *c))
+        .map(|c| CoefficientCommitment::new(<C::Group as Group>::generator() * *c))
         .collect();
     let commitment: VerifiableSecretSharingCommitment<C> =
         VerifiableSecretSharingCommitment(commitment);