Adding identity element checks and ensuring non-zero scalar selection

(cherry picked from commit 98f1821897)

src/errors.rs

@@ -73,6 +73,8 @@ pub enum PakeError {
     InvalidLoginError,
     /// Error with serializing / deserializing protocol messages
     SerializationError,
+    /// Identity group element was encountered during deserialization, which is invalid
+    IdentityGroupElementError,
 }
 
 // This is meant to express future(ly) non-trivial ways of converting the

src/group.rs

@@ -12,6 +12,7 @@ use curve25519_dalek::{
     constants::RISTRETTO_BASEPOINT_POINT,
     ristretto::{CompressedRistretto, RistrettoPoint},
     scalar::Scalar,
+    traits::Identity,
 };
 use generic_array::{
     typenum::{U32, U64},
@@ -35,7 +36,7 @@ pub trait Group: Copy + Sized + for<'a> Mul<&'a <Self as Group>::Scalar, Output
         scalar_bits: &GenericArray<u8, Self::ScalarLen>,
     ) -> Result<Self::Scalar, InternalPakeError>;
     /// picks a scalar at random
-    fn random_scalar<R: RngCore + CryptoRng>(rng: &mut R) -> Self::Scalar;
+    fn random_nonzero_scalar<R: RngCore + CryptoRng>(rng: &mut R) -> Self::Scalar;
     /// Serializes a scalar to bytes
     fn scalar_as_bytes(scalar: &Self::Scalar) -> &GenericArray<u8, Self::ScalarLen>;
     /// The multiplicative inverse of this scalar
@@ -64,6 +65,9 @@ pub trait Group: Copy + Sized + for<'a> Mul<&'a <Self as Group>::Scalar, Output
 
     /// 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;
 }
 
 /// The implementation of such a subgroup for Ristretto
@@ -77,20 +81,28 @@ impl Group for RistrettoPoint {
         bits.copy_from_slice(scalar_bits);
         Ok(Scalar::from_bytes_mod_order(bits))
     }
-    fn random_scalar<R: RngCore + CryptoRng>(rng: &mut R) -> Self::Scalar {
+    fn random_nonzero_scalar<R: RngCore + CryptoRng>(rng: &mut R) -> Self::Scalar {
-        #[cfg(not(test))]
+        loop {
-        {
+            let scalar = {
-            let mut scalar_bytes = [0u8; 64];
+                #[cfg(not(test))]
-            rng.fill_bytes(&mut scalar_bytes);
+                {
-            Scalar::from_bytes_mod_order_wide(&scalar_bytes)
+                    let mut scalar_bytes = [0u8; 64];
-        }
+                    rng.fill_bytes(&mut scalar_bytes);
+                    Scalar::from_bytes_mod_order_wide(&scalar_bytes)
+                }
 
-        // Tests need an exact conversion from bytes to scalar, sampling only 32 bytes from rng
+                // Tests need an exact conversion from bytes to scalar, sampling only 32 bytes from rng
-        #[cfg(test)]
+                #[cfg(test)]
-        {
+                {
-            let mut scalar_bytes = [0u8; 32];
+                    let mut scalar_bytes = [0u8; 32];
-            rng.fill_bytes(&mut scalar_bytes);
+                    rng.fill_bytes(&mut scalar_bytes);
-            Scalar::from_bytes_mod_order(scalar_bytes)
+                    Scalar::from_bytes_mod_order(scalar_bytes)
+                }
+            };
+
+            if scalar != Scalar::zero() {
+                break scalar;
+            }
         }
     }
     fn scalar_as_bytes(scalar: &Self::Scalar) -> &GenericArray<u8, Self::ScalarLen> {
@@ -134,4 +146,9 @@ impl Group for RistrettoPoint {
         let arr: [u8; 32] = scalar.as_slice().try_into().expect("Wrong length");
         self * Scalar::from_bits(arr)
     }
+
+    /// Returns if the group element is equal to the identity (1)
+    fn is_identity(&self) -> bool {
+        self == &Self::identity()
+    }
 }

src/keypair.rs

@@ -69,7 +69,7 @@ impl<G: Group> KeyPair<G> {
 
     /// Generating a random key pair given a cryptographic rng
     pub(crate) fn generate_random<R: RngCore + CryptoRng>(rng: &mut R) -> Self {
-        let sk = G::random_scalar(rng);
+        let sk = G::random_nonzero_scalar(rng);
         let sk_bytes = G::scalar_as_bytes(&sk);
         let pk = G::base_point().mult_by_slice(&sk_bytes);
         Self {

src/messages.rs

@@ -29,6 +29,21 @@ pub struct RegistrationRequest<CS: CipherSuite> {
     pub(crate) alpha: CS::Group,
 }
 
+impl<CS: CipherSuite> RegistrationRequest<CS> {
+    /// Only used for testing purposes
+    #[cfg(test)]
+    pub fn get_alpha_for_testing(&self) -> CS::Group {
+        self.alpha
+    }
+}
+
+// Cannot be derived because it would require for CS to be Clone.
+impl<CS: CipherSuite> Clone for RegistrationRequest<CS> {
+    fn clone(&self) -> Self {
+        Self { alpha: self.alpha }
+    }
+}
+
 impl<CS: CipherSuite> RegistrationRequest<CS> {
     /// Serialization into bytes
     pub fn serialize(&self) -> Vec<u8> {
@@ -43,6 +58,11 @@ impl<CS: CipherSuite> RegistrationRequest<CS> {
         // correct subgroup
         let arr = GenericArray::from_slice(checked_slice);
         let alpha = CS::Group::from_element_slice(arr)?;
+
+        // Throw an error if the identity group element is encountered
+        if alpha.is_identity() {
+            return Err(PakeError::IdentityGroupElementError.into());
+        }
         Ok(Self { alpha })
     }
 }
@@ -74,8 +94,17 @@ impl<CS: CipherSuite> RegistrationResponse<CS> {
         let arr = GenericArray::from_slice(&checked_slice[..elem_len]);
         let beta = CS::Group::from_element_slice(arr)?;
 
+        // Throw an error if the identity group element is encountered
+        if beta.is_identity() {
+            return Err(PakeError::IdentityGroupElementError.into());
+        }
+
+        // Ensure that public key is valid
+        let server_s_pk =
+            KeyPair::<CS::Group>::check_public_key(Key::from_bytes(&checked_slice[elem_len..])?)?;
+
         Ok(Self {
-            server_s_pk: checked_slice[elem_len..].to_vec(),
+            server_s_pk: server_s_pk.to_arr().to_vec(),
             beta,
         })
     }
@@ -150,6 +179,11 @@ impl<CS: CipherSuite> CredentialRequest<CS> {
         let arr = GenericArray::from_slice(&checked_slice[..elem_len]);
         let alpha = CS::Group::from_element_slice(arr)?;
 
+        // Throw an error if the identity group element is encountered
+        if alpha.is_identity() {
+            return Err(PakeError::IdentityGroupElementError.into());
+        }
+
         let ke1_message =
             <CS::KeyExchange as KeyExchange<CS::Hash, CS::Group>>::KE1Message::try_from(
                 &checked_slice[elem_len..],
@@ -206,6 +240,11 @@ impl<CS: CipherSuite> CredentialResponse<CS> {
         let arr = GenericArray::from_slice(beta_bytes);
         let beta = CS::Group::from_element_slice(arr)?;
 
+        // Throw an error if the identity group element is encountered
+        if beta.is_identity() {
+            return Err(PakeError::IdentityGroupElementError.into());
+        }
+
         let unchecked_server_s_pk = Key::from_bytes(&checked_slice[elem_len..elem_len + key_len])?;
         let server_s_pk = KeyPair::<CS::Group>::check_public_key(unchecked_server_s_pk)?;
 

src/opaque.rs

@@ -328,7 +328,7 @@ impl<CS: CipherSuite> ServerRegistration<CS> {
         server_s_pk: &Key,
     ) -> Result<ServerRegistrationStartResult<CS>, ProtocolError> {
         // RFC: generate oprf_key (salt) and v_u = g^oprf_key
-        let oprf_key = CS::Group::random_scalar(rng);
+        let oprf_key = CS::Group::random_nonzero_scalar(rng);
 
         // Compute beta = alpha^oprf_key
         let beta = oprf::evaluate::<CS::Group>(message.alpha, &oprf_key);

src/oprf.rs

@@ -29,7 +29,8 @@ pub(crate) fn blind<R: RngCore + CryptoRng, G: GroupWithMapToCurve, H: Hash>(
     input: &[u8],
     blinding_factor_rng: &mut R,
 ) -> Result<(Token<G>, G), ProtocolError> {
-    let blind = G::random_scalar(blinding_factor_rng);
+    // Choose a random scalar that must be non-zero
+    let blind = G::random_nonzero_scalar(blinding_factor_rng);
     let dst = [STR_VOPRF, &G::get_context_string(MODE_BASE)?].concat();
     let mapped_point = G::map_to_curve::<H>(input, &dst)?;
     let blind_token = mapped_point * &blind;

src/serialization/tests.rs

@@ -6,6 +6,7 @@
 use crate::{
     ciphersuite::CipherSuite,
     envelope::{Envelope, InnerEnvelopeMode},
+    errors::*,
     group::Group,
     key_exchange::{
         traits::{KeyExchange, ToBytes},
@@ -16,7 +17,7 @@ use crate::{
     *,
 };
 
-use curve25519_dalek::ristretto::RistrettoPoint;
+use curve25519_dalek::{ristretto::RistrettoPoint, traits::Identity};
 use generic_array::typenum::Unsigned;
 use generic_bytes::SizedBytes;
 use proptest::{collection::vec, prelude::*};
@@ -54,7 +55,7 @@ fn random_ristretto_point() -> RistrettoPoint {
 fn client_registration_roundtrip() {
     let pw = b"hunter2";
     let mut rng = OsRng;
-    let sc = <RistrettoPoint as Group>::random_scalar(&mut rng);
+    let sc = <RistrettoPoint as Group>::random_nonzero_scalar(&mut rng);
 
     // serialization order: scalar, password
     let bytes: Vec<u8> = [&sc.as_bytes()[..], &pw[..]].concat();
@@ -68,7 +69,7 @@ fn server_registration_roundtrip() {
     // If we don't have envelope and client_pk, the server registration just
     // contains the prf key
     let mut rng = OsRng;
-    let oprf_key = <RistrettoPoint as Group>::random_scalar(&mut rng);
+    let oprf_key = <RistrettoPoint as Group>::random_nonzero_scalar(&mut rng);
     let mut oprf_bytes: Vec<u8> = vec![];
     oprf_bytes.extend_from_slice(oprf_key.as_bytes());
     let reg = ServerRegistration::<Default>::deserialize(&oprf_bytes[..]).unwrap();
@@ -107,6 +108,17 @@ fn registration_request_roundtrip() {
     let r1 = RegistrationRequest::<Default>::deserialize(input.as_slice()).unwrap();
     let r1_bytes = r1.serialize();
     assert_eq!(input, r1_bytes);
+
+    // Assert that identity group element is rejected
+    let identity = RistrettoPoint::identity();
+    let identity_bytes = identity.to_arr().to_vec();
+
+    assert!(
+        match RegistrationRequest::<Default>::deserialize(identity_bytes.as_slice()) {
+            Err(ProtocolError::VerificationError(PakeError::IdentityGroupElementError)) => true,
+            _ => false,
+        }
+    );
 }
 
 #[test]
@@ -124,6 +136,17 @@ fn registration_response_roundtrip() {
     let r2 = RegistrationResponse::<Default>::deserialize(input.as_slice()).unwrap();
     let r2_bytes = r2.serialize();
     assert_eq!(input, r2_bytes);
+
+    // Assert that identity group element is rejected
+    let identity = RistrettoPoint::identity();
+    let identity_bytes = identity.to_arr().to_vec();
+
+    assert!(match RegistrationResponse::<Default>::deserialize(
+        &[identity_bytes, pubkey_bytes.to_vec()].concat()
+    ) {
+        Err(ProtocolError::VerificationError(PakeError::IdentityGroupElementError)) => true,
+        _ => false,
+    });
 }
 
 #[test]
@@ -184,6 +207,17 @@ fn credential_request_roundtrip() {
     let l1 = CredentialRequest::<Default>::deserialize(input.as_slice()).unwrap();
     let l1_bytes = l1.serialize().unwrap();
     assert_eq!(input, l1_bytes);
+
+    // Assert that identity group element is rejected
+    let identity = RistrettoPoint::identity();
+    let identity_bytes = identity.to_arr().to_vec();
+
+    assert!(match CredentialRequest::<Default>::deserialize(
+        &[identity_bytes, ke1m.to_vec()].concat()
+    ) {
+        Err(ProtocolError::VerificationError(PakeError::IdentityGroupElementError)) => true,
+        _ => false,
+    });
 }
 
 #[test]
@@ -227,15 +261,34 @@ fn credential_response_roundtrip() {
     ]
     .concat();
 
+    let serialized_envelope = envelope.serialize();
+
     let mut input = Vec::new();
     input.extend_from_slice(pt_bytes.as_slice());
     input.extend_from_slice(&pubkey_bytes.as_slice());
-    input.extend_from_slice(&envelope.serialize());
+    input.extend_from_slice(&serialized_envelope);
     input.extend_from_slice(&ke2m[..]);
 
     let l2 = CredentialResponse::<Default>::deserialize(&input).unwrap();
     let l2_bytes = l2.serialize().unwrap();
     assert_eq!(input, l2_bytes);
+
+    // Assert that identity group element is rejected
+    let identity = RistrettoPoint::identity();
+    let identity_bytes = identity.to_arr().to_vec();
+
+    assert!(match CredentialResponse::<Default>::deserialize(
+        &[
+            identity_bytes,
+            pubkey_bytes.to_vec(),
+            serialized_envelope,
+            ke2m.to_vec()
+        ]
+        .concat()
+    ) {
+        Err(ProtocolError::VerificationError(PakeError::IdentityGroupElementError)) => true,
+        _ => false,
+    });
 }
 
 #[test]
@@ -255,7 +308,7 @@ fn login_third_message_roundtrip() {
 fn client_login_roundtrip() {
     let pw = b"hunter2";
     let mut rng = OsRng;
-    let sc = <RistrettoPoint as Group>::random_scalar(&mut rng);
+    let sc = <RistrettoPoint as Group>::random_nonzero_scalar(&mut rng);
 
     let client_e_kp = Default::generate_random_keypair(&mut rng);
     let mut client_nonce = vec![0u8; NonceLen::to_usize()];

src/tests/full_test.rs

@@ -16,7 +16,7 @@ use crate::{
     tests::mock_rng::CycleRng,
     *,
 };
-use curve25519_dalek::ristretto::RistrettoPoint;
+use curve25519_dalek::{ristretto::RistrettoPoint, traits::Identity};
 use generic_array::typenum::Unsigned;
 use generic_bytes::SizedBytes;
 use rand::{rngs::OsRng, RngCore};
@@ -285,7 +285,7 @@ fn generate_parameters<CS: CipherSuite>() -> TestVectorParameters {
     let mut server_nonce = vec![0u8; NonceLen::to_usize()];
     rng.fill_bytes(&mut server_nonce);
 
-    let blinding_factor = CS::Group::random_scalar(&mut rng);
+    let blinding_factor = CS::Group::random_nonzero_scalar(&mut rng);
     let blinding_factor_bytes = CS::Group::scalar_as_bytes(&blinding_factor).clone();
 
     let info1 = b"info1";
@@ -1030,3 +1030,23 @@ fn test_zeroize_server_login_finish() -> Result<(), ProtocolError> {
 
     Ok(())
 }
+
+#[test]
+fn test_scalar_always_nonzero() -> Result<(), ProtocolError> {
+    // Start out with a bunch of zeros to force resampling of scalar
+    let mut client_registration_rng = CycleRng::new([vec![0u8; 128], vec![1u8; 128]].concat());
+    let client_registration_start_result =
+        ClientRegistration::<RistrettoSha5123dhNoSlowHash>::start(
+            &mut client_registration_rng,
+            STR_PASSWORD.as_bytes(),
+        )?;
+
+    assert_ne!(
+        RistrettoPoint::identity(),
+        client_registration_start_result
+            .message
+            .get_alpha_for_testing()
+    );
+
+    Ok(())
+}