Adding deserialization checks (#10)

Cargo.toml

@@ -33,9 +33,9 @@ num-traits = { version = "0.2", default-features = false, optional = true }
 once_cell = { version = "1", default-features = false, optional = true }
 p256_ = { package = "p256", version = "0.9", default-features = false, features = ["arithmetic", "zeroize"], optional = true }
 rand = { version = "0.8", default-features = false }
+serde = { version = "1", default-features = false, features = ["alloc", "derive"], optional = true }
 subtle = { version = "2.3", default-features = false }
 zeroize = { version = "1", features = ["zeroize_derive"] }
-serde = { version = "1", default-features = false, features = ["alloc", "derive"], optional = true }
 
 [target.'cfg(target_arch = "wasm32")'.dependencies]
 getrandom = { version = "0.2", features = ["js"], optional = true }

src/errors.rs

@@ -15,7 +15,7 @@ use displaydoc::Display;
 pub enum InternalError {
     /// Could not parse byte sequence for key
     InvalidByteSequence,
-    /// Could not decompress point.
+    /// Could not deserialize element, or deserialized to the identity element
     PointError,
     /// Computing the hash-to-curve function failed
     HashToCurveError,
@@ -32,6 +32,8 @@ pub enum InternalError {
     ProofVerificationError,
     /// Encountered insufficient bytes when attempting to deserialize
     SizeError,
+    /// Encountered a zero scalar
+    ZeroScalarError,
 }
 
 impl Debug for InternalError {
@@ -47,6 +49,7 @@ impl Debug for InternalError {
                 .finish(),
             Self::ProofVerificationError => f.debug_tuple("ProofVerificationError").finish(),
             Self::SizeError => f.debug_tuple("SizeError").finish(),
+            Self::ZeroScalarError => f.debug_tuple("ZeroScalarError").finish(),
         }
     }
 }

src/group/mod.rs

@@ -31,7 +31,7 @@ pub trait Group:
     const SUITE_ID: usize;
 
     /// transforms a password and domain separation tag (DST) into a curve point
-    fn map_to_curve<H: Hash>(msg: &[u8], dst: &[u8]) -> Result<Self, InternalError>;
+    fn hash_to_curve<H: Hash>(msg: &[u8], dst: &[u8]) -> Result<Self, InternalError>;
 
     /// Hashes a slice of pseudo-random bytes to a scalar
     fn hash_to_scalar<H: Hash>(input: &[u8], dst: &[u8]) -> Result<Self::Scalar, InternalError>;
@@ -44,10 +44,25 @@ pub trait Group:
         + for<'a> Mul<&'a Self::Scalar, Output = Self::Scalar>;
     /// The byte length necessary to represent scalars
     type ScalarLen: ArrayLength<u8> + 'static;
-    /// Return a scalar from its fixed-length bytes representation
-    fn from_scalar_slice(
+
+    /// Return a scalar from its fixed-length bytes representation, without
+    /// checking if the scalar is zero.
+    fn from_scalar_slice_unchecked(
         scalar_bits: &GenericArray<u8, Self::ScalarLen>,
     ) -> Result<Self::Scalar, InternalError>;
+
+    /// Return a scalar from its fixed-length bytes representation. If the scalar
+    /// is zero, then return an error.
+    fn from_scalar_slice(
+        scalar_bits: &GenericArray<u8, Self::ScalarLen>,
+    ) -> Result<Self::Scalar, InternalError> {
+        let scalar = Self::from_scalar_slice_unchecked(scalar_bits)?;
+        if Self::ct_equal_scalar(&scalar, &Self::scalar_zero()) {
+            return Err(InternalError::ZeroScalarError);
+        }
+        Ok(scalar)
+    }
+
     /// picks a scalar at random
     fn random_nonzero_scalar<R: RngCore + CryptoRng>(rng: &mut R) -> Self::Scalar;
     /// Serializes a scalar to bytes
@@ -57,19 +72,35 @@ pub trait Group:
 
     /// The byte length necessary to represent group elements
     type ElemLen: ArrayLength<u8> + 'static;
-    /// Return an element from its fixed-length bytes representation
-    fn from_element_slice(
+
+    /// Return an element from its fixed-length bytes representation. This is
+    /// the unchecked version, which does not check for deserializing the identity
+    /// element
+    fn from_element_slice_unchecked(
         element_bits: &GenericArray<u8, Self::ElemLen>,
     ) -> Result<Self, InternalError>;
+
+    /// Return an element from its fixed-length bytes representation. If the element
+    /// is the identity element, return an error.
+    fn from_element_slice(
+        element_bits: &GenericArray<u8, Self::ElemLen>,
+    ) -> Result<Self, InternalError> {
+        let elem = Self::from_element_slice_unchecked(element_bits)?;
+
+        if Self::ct_equal(&elem, &<Self as Group>::identity()) {
+            // found the identity element
+            return Err(InternalError::PointError);
+        }
+
+        Ok(elem)
+    }
+
     /// Serializes the `self` group element
     fn to_arr(&self) -> GenericArray<u8, Self::ElemLen>;
 
     /// Get the base point for the group
     fn base_point() -> Self;
 
-    /// Multiply the point by a scalar, represented as a slice
-    fn mult_by_slice(&self, scalar: &GenericArray<u8, Self::ScalarLen>) -> Self;
-
     /// Returns if the group element is equal to the identity (1)
     fn is_identity(&self) -> bool {
         self.ct_equal(&<Self as Group>::identity())
@@ -78,9 +109,15 @@ pub trait Group:
     /// Returns the identity group element
     fn identity() -> Self;
 
+    /// Returns the scalar representing zero
+    fn scalar_zero() -> Self::Scalar;
+
     /// Compares in constant time if the group elements are equal
     fn ct_equal(&self, other: &Self) -> bool;
 
     /// Compares in constant time if the scalars are equal
     fn ct_equal_scalar(s1: &Self::Scalar, s2: &Self::Scalar) -> bool;
 }
+
+#[cfg(test)]
+mod tests;

src/group/p256.rs

@@ -17,7 +17,7 @@ use generic_array::typenum::{U32, U33};
 use generic_array::{ArrayLength, GenericArray};
 use num_bigint::{BigInt, Sign};
 use num_integer::Integer;
-use num_traits::{One, ToPrimitive};
+use num_traits::{One, ToPrimitive, Zero};
 use once_cell::unsync::Lazy;
 use p256_::elliptic_curve::group::prime::PrimeCurveAffine;
 use p256_::elliptic_curve::group::GroupEncoding;
@@ -35,7 +35,7 @@ impl Group for ProjectivePoint {
 
     // Implements the `hash_to_curve()` function from
     // https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-hash-to-curve-11#section-3
-    fn map_to_curve<H: Hash>(msg: &[u8], dst: &[u8]) -> Result<Self, InternalError> {
+    fn hash_to_curve<H: Hash>(msg: &[u8], dst: &[u8]) -> Result<Self, InternalError> {
         // https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-hash-to-curve-11#section-8.2
         // `p: 2^256 - 2^224 + 2^192 + 2^96 - 1`
         const P: Lazy<BigInt> = Lazy::new(|| {
@@ -63,9 +63,9 @@ impl Group for ProjectivePoint {
         // `hash_to_field` calls `expand_message` with a `len_in_bytes` of `count * L`
         let uniform_bytes = super::expand::expand_message_xmd::<H>(msg, dst, 2 * L)?;
 
-        // map to curve
-        let (q0x, q0y) = map_to_curve_simple_swu(&uniform_bytes[..L], &A, &B, &P, &Z);
-        let (q1x, q1y) = map_to_curve_simple_swu(&uniform_bytes[L..], &A, &B, &P, &Z);
+        // hash to curve
+        let (q0x, q0y) = hash_to_curve_simple_swu(&uniform_bytes[..L], &A, &B, &P, &Z);
+        let (q1x, q1y) = hash_to_curve_simple_swu(&uniform_bytes[L..], &A, &B, &P, &Z);
 
         // convert to `p256` types
         let p0 = AffinePoint::from_encoded_point(&EncodedPoint::from_affine_coordinates(
@@ -111,7 +111,7 @@ impl Group for ProjectivePoint {
     type Scalar = p256_::Scalar;
     type ScalarLen = U32;
 
-    fn from_scalar_slice(
+    fn from_scalar_slice_unchecked(
         scalar_bits: &GenericArray<u8, Self::ScalarLen>,
     ) -> Result<Self::Scalar, InternalError> {
         Ok(Self::Scalar::from_bytes_reduced(scalar_bits))
@@ -129,7 +129,7 @@ impl Group for ProjectivePoint {
         scalar.invert().unwrap_or(Self::Scalar::zero())
     }
 
-    fn from_element_slice(
+    fn from_element_slice_unchecked(
         element_bits: &GenericArray<u8, Self::ElemLen>,
     ) -> Result<Self, InternalError> {
         Option::from(Self::from_bytes(element_bits)).ok_or(InternalError::PointError)
@@ -145,14 +145,14 @@ impl Group for ProjectivePoint {
         Self::generator()
     }
 
-    fn mult_by_slice(&self, scalar: &GenericArray<u8, Self::ScalarLen>) -> Self {
-        self * &Self::Scalar::from_bytes_reduced(scalar)
-    }
-
     fn identity() -> Self {
         Self::identity()
     }
 
+    fn scalar_zero() -> Self::Scalar {
+        Self::Scalar::zero()
+    }
+
     fn ct_equal(&self, other: &Self) -> bool {
         self.ct_eq(other).into()
     }
@@ -162,10 +162,10 @@ impl Group for ProjectivePoint {
     }
 }
 
-/// Corresponds to the map_to_curve_simple_swu() function defined in
+/// Corresponds to the hash_to_curve_simple_swu() function defined in
 /// <https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-hash-to-curve-11#appendix-F.2>
 #[allow(clippy::many_single_char_names)]
-fn map_to_curve_simple_swu<N: ArrayLength<u8>>(
+fn hash_to_curve_simple_swu<N: ArrayLength<u8>>(
     u: &[u8],
     a: &BigInt,
     b: &BigInt,
@@ -311,7 +311,7 @@ fn map_to_curve_simple_swu<N: ArrayLength<u8>>(
         }
 
         fn is_zero(&self) -> bool {
-            self.number.is_one()
+            self.number.is_zero()
         }
 
         /// Corresponds to the is_square() function defined in
@@ -321,7 +321,7 @@ fn map_to_curve_simple_swu<N: ArrayLength<u8>>(
             let exponent = (self.f.0 - 1) >> 1;
 
             let result = self.pow_internal(&exponent);
-            result.number.is_one() || result.is_zero()
+            result.is_zero() || result.number.is_one()
         }
 
         fn to_bytes<N: ArrayLength<u8>>(&self) -> GenericArray<u8, N> {
@@ -413,7 +413,7 @@ mod tests {
     }
 
     #[test]
-    fn map_to_curve_simple_swu() {
+    fn hash_to_curve_simple_swu() {
         const P: Lazy<BigInt> = Lazy::new(|| {
             BigInt::from_str(
                 "115792089210356248762697446949407573530086143415290314195533631308867097853951",
@@ -515,8 +515,8 @@ mod tests {
             assert_eq!(BigInt::parse_bytes(tv.u0.as_bytes(), 16).unwrap(), u0);
             assert_eq!(BigInt::parse_bytes(tv.u1.as_bytes(), 16).unwrap(), u1);
 
-            let (q0x, q0y) = super::map_to_curve_simple_swu(&u0.to_bytes_be().1, &A, &B, &P, &Z);
-            let (q1x, q1y) = super::map_to_curve_simple_swu(&u1.to_bytes_be().1, &A, &B, &P, &Z);
+            let (q0x, q0y) = super::hash_to_curve_simple_swu(&u0.to_bytes_be().1, &A, &B, &P, &Z);
+            let (q1x, q1y) = super::hash_to_curve_simple_swu(&u1.to_bytes_be().1, &A, &B, &P, &Z);
 
             assert_eq!(tv.q0x, hex::encode(q0x));
             assert_eq!(tv.q0y, hex::encode(q0y));

src/group/ristretto.rs

@@ -23,7 +23,7 @@ impl Group for RistrettoPoint {
 
     // Implements the `hash_to_ristretto255()` function from
     // https://www.ietf.org/archive/id/draft-irtf-cfrg-hash-to-curve-10.txt
-    fn map_to_curve<H: Hash>(msg: &[u8], dst: &[u8]) -> Result<Self, InternalError> {
+    fn hash_to_curve<H: Hash>(msg: &[u8], dst: &[u8]) -> Result<Self, InternalError> {
         let uniform_bytes = super::expand::expand_message_xmd::<H>(msg, dst, 64)?;
 
         Ok(RistrettoPoint::from_uniform_bytes(
@@ -49,11 +49,12 @@ impl Group for RistrettoPoint {
 
     type Scalar = Scalar;
     type ScalarLen = U32;
-    fn from_scalar_slice(
+    fn from_scalar_slice_unchecked(
         scalar_bits: &GenericArray<u8, Self::ScalarLen>,
     ) -> Result<Self::Scalar, InternalError> {
         Ok(Scalar::from_bytes_mod_order(*scalar_bits.as_ref()))
     }
+
     fn random_nonzero_scalar<R: RngCore + CryptoRng>(rng: &mut R) -> Self::Scalar {
         loop {
             let scalar = {
@@ -89,7 +90,7 @@ impl Group for RistrettoPoint {
 
     // The byte length necessary to represent group elements
     type ElemLen = U32;
-    fn from_element_slice(
+    fn from_element_slice_unchecked(
         element_bits: &GenericArray<u8, Self::ElemLen>,
     ) -> Result<Self, InternalError> {
         CompressedRistretto::from_slice(element_bits)
@@ -105,14 +106,14 @@ impl Group for RistrettoPoint {
         RISTRETTO_BASEPOINT_POINT
     }
 
-    fn mult_by_slice(&self, scalar: &GenericArray<u8, Self::ScalarLen>) -> Self {
-        self * Scalar::from_bits(*scalar.as_ref())
-    }
-
     fn identity() -> Self {
         <Self as Identity>::identity()
     }
 
+    fn scalar_zero() -> Self::Scalar {
+        Self::Scalar::zero()
+    }
+
     fn ct_equal(&self, other: &Self) -> bool {
         ConstantTimeEq::ct_eq(self, other).into()
     }

src/group/tests.rs

@@ -0,0 +1,55 @@
+// Copyright (c) Facebook, Inc. and its affiliates.
+//
+// This source code is licensed under the MIT license found in the
+// LICENSE file in the root directory of this source tree.
+
+//! Includes a series of tests for the group implementations
+
+use crate::errors::InternalError;
+use crate::group::Group;
+use crate::CipherSuite;
+
+// Test that the deserialization of a group element should throw an error
+// if the identity element can be deserialized properly
+
+#[test]
+fn test_group_properties() -> Result<(), InternalError> {
+    use crate::tests::Ristretto255Sha512;
+
+    test_identity_element_error::<Ristretto255Sha512>()?;
+    test_zero_scalar_error::<Ristretto255Sha512>()?;
+
+    #[cfg(feature = "p256")]
+    {
+        use crate::tests::P256Sha256;
+
+        test_identity_element_error::<P256Sha256>()?;
+        test_zero_scalar_error::<P256Sha256>()?;
+    }
+
+    Ok(())
+}
+
+// Checks that the identity element cannot be deserialized
+fn test_identity_element_error<CS: CipherSuite>() -> Result<(), InternalError> {
+    let identity = CS::Group::identity();
+    let result = CS::Group::from_element_slice(&identity.to_arr());
+    assert!(match result {
+        Err(InternalError::PointError) => true,
+        _ => false,
+    });
+
+    Ok(())
+}
+
+// Checks that the zero scalar cannot be deserialized
+fn test_zero_scalar_error<CS: CipherSuite>() -> Result<(), InternalError> {
+    let zero_scalar = CS::Group::scalar_zero();
+    let result = CS::Group::from_scalar_slice(&CS::Group::scalar_as_bytes(zero_scalar));
+    assert!(match result {
+        Err(InternalError::ZeroScalarError) => true,
+        _ => false,
+    });
+
+    Ok(())
+}

src/lib.rs

@@ -6,7 +6,7 @@
 //! An implementation of a verifiable oblivious pseudorandom function (VOPRF)
 //!
 //! Note: This implementation is in sync with
-//! [draft-irtf-cfrg-opaque-07](https://www.ietf.org/archive/id/draft-irtf-cfrg-opaque-07.html),
+//! [draft-irtf-cfrg-voprf-07](https://www.ietf.org/archive/id/draft-irtf-cfrg-voprf-07.html),
 //! but this specification is subject to change, until the final version
 //! published by the IETF.
 //!

src/tests/mod.rs

@@ -7,3 +7,18 @@ mod mock_rng;
 mod parser;
 mod voprf_test_vectors;
 mod voprf_vectors;
+
+/// Ciphersuite definitions for tests
+pub(crate) struct Ristretto255Sha512;
+impl crate::CipherSuite for Ristretto255Sha512 {
+    type Group = curve25519_dalek::ristretto::RistrettoPoint;
+    type Hash = sha2::Sha512;
+}
+
+#[cfg(feature = "p256")]
+pub(crate) struct P256Sha256;
+#[cfg(feature = "p256")]
+impl crate::CipherSuite for P256Sha256 {
+    type Group = p256_::ProjectivePoint;
+    type Hash = sha2::Sha256;
+}

src/tests/voprf_test_vectors.rs

@@ -15,10 +15,8 @@ use crate::{
 };
 use alloc::string::ToString;
 use alloc::vec::Vec;
-use curve25519_dalek::ristretto::RistrettoPoint;
 use generic_array::GenericArray;
 use json::JsonValue;
-use sha2::Sha512;
 
 #[derive(Debug)]
 struct VOPRFTestVectorParameters {
@@ -82,15 +80,11 @@ macro_rules! json_to_test_vectors {
 
 #[test]
 fn test_vectors() -> Result<(), InternalError> {
-    struct Ristretto255Sha512;
-    impl CipherSuite for Ristretto255Sha512 {
-        type Group = RistrettoPoint;
-        type Hash = Sha512;
-    }
-
     let rfc = json::parse(rfc_to_json(super::voprf_vectors::VECTORS).as_str())
         .expect("Could not parse json");
 
+    use crate::tests::Ristretto255Sha512;
+
     let ristretto_base_tvs = json_to_test_vectors!(
         rfc,
         String::from("ristretto255, SHA-512"),
@@ -115,11 +109,7 @@ fn test_vectors() -> Result<(), InternalError> {
 
     #[cfg(feature = "p256")]
     {
-        struct P256Sha256;
-        impl CipherSuite for P256Sha256 {
-            type Group = p256_::ProjectivePoint;
-            type Hash = sha2::Sha256;
-        }
+        use crate::tests::P256Sha256;
 
         let p256_base_tvs =
             json_to_test_vectors!(rfc, String::from("P-256, SHA-256"), String::from("Base"));

src/tests/voprf_vectors.rs

@@ -4,7 +4,7 @@
 // LICENSE file in the root directory of this source tree.
 
 //! The VOPRF test vectors taken from:
-//! https://github.com/cfrg/draft-irtf-cfrg-opaque/blob/master/draft-irtf-cfrg-opaque.md
+//! https://github.com/cfrg/draft-irtf-cfrg-voprf/blob/master/draft-irtf-cfrg-voprf.md
 
 pub(crate) static VECTORS: &str = r#"
 ## OPRF(ristretto255, SHA-512)

src/voprf.rs

@@ -528,8 +528,8 @@ fn blind<CS: CipherSuite, R: RngCore + CryptoRng>(
     // Choose a random scalar that must be non-zero
     let blind = <CS::Group as Group>::random_nonzero_scalar(blinding_factor_rng);
     let dst = [STR_HASH_TO_GROUP, &get_context_string::<CS>(mode)?].concat();
-    let mapped_point = <CS::Group as Group>::map_to_curve::<CS::Hash>(input, &dst)?;
-    let blinded_element = mapped_point * &blind;
+    let hashed_point = <CS::Group as Group>::hash_to_curve::<CS::Hash>(input, &dst)?;
+    let blinded_element = hashed_point * &blind;
     Ok((blind, blinded_element))
 }
 
@@ -744,7 +744,6 @@ fn get_context_string<CS: CipherSuite>(mode: Mode) -> Result<alloc::vec::Vec<u8>
 // Tests //
 // ===== //
 ///////////
-
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -770,7 +769,7 @@ mod tests {
             &get_context_string::<Ristretto255Sha512>(Mode::Base).unwrap(),
         ]
         .concat();
-        let point = RistrettoPoint::map_to_curve::<Sha512>(input, &dst).unwrap();
+        let point = RistrettoPoint::hash_to_curve::<Sha512>(input, &dst).unwrap();
         let scalar =
             RistrettoPoint::from_scalar_slice(GenericArray::from_slice(&oprf_key[..])).unwrap();
 
@@ -842,7 +841,7 @@ mod tests {
             &get_context_string::<Ristretto255Sha512>(Mode::Base).unwrap(),
         ]
         .concat();
-        let point = RistrettoPoint::map_to_curve::<Sha512>(&input, &dst).unwrap();
+        let point = RistrettoPoint::hash_to_curve::<Sha512>(&input, &dst).unwrap();
         let res2 = finalize_after_unblind::<Ristretto255Sha512>(
             &[(input.to_vec(), point)],
             info,