protocol: Check that base keys are torsion free and in range

Co-authored-by: Jordan Rose <[email protected]>

Author: rolfe-signal

Cargo.lock

@@ -9,7 +9,7 @@ mod utils;
 use std::cmp::Ordering;
 use std::fmt;
 
-use curve25519_dalek::scalar;
+use curve25519_dalek::{MontgomeryPoint, scalar};
 use rand::{CryptoRng, Rng};
 use subtle::ConstantTimeEq;
 
@@ -150,6 +150,33 @@ impl PublicKey {
             PublicKeyData::DjbPublicKey(_) => KeyType::Djb,
         }
     }
+
+    fn is_torsion_free(&self) -> bool {
+        match &self.key {
+            PublicKeyData::DjbPublicKey(k) => {
+                let mont_point = MontgomeryPoint(*k);
+                mont_point
+                    .to_edwards(0)
+                    .is_some_and(|ed| ed.is_torsion_free())
+            }
+        }
+    }
+
+    fn scalar_is_in_range(&self) -> bool {
+        match &self.key {
+            PublicKeyData::DjbPublicKey(k) => {
+                // it is not true that the scalar is greater than 2^255 - 19
+                // specifically, it is not true that either the high bit is set
+                // or that the high 247 bits are all 1 and the bottom byte is >(2^8 - 19)
+                !(k[31] & 0b1000_0000_u8 != 0
+                    || (k[0] >= 0u8.wrapping_sub(19) && k[1..31] == [0xFFu8; 30] && k[31] == 0x7F))
+            }
+        }
+    }
+
+    pub fn is_canonical(&self) -> bool {
+        self.is_torsion_free() && self.scalar_is_in_range()
+    }
 }
 
 impl TryFrom<&[u8]> for PublicKey {
@@ -356,6 +383,8 @@ impl TryFrom<PrivateKey> for KeyPair {
 #[cfg(test)]
 mod tests {
     use assert_matches::assert_matches;
+    use const_str::hex;
+    use curve25519_dalek::constants::EIGHT_TORSION;
     use rand::TryRngCore as _;
     use rand::rngs::OsRng;
 
@@ -433,4 +462,100 @@ mod tests {
         let error = Box::new(error) as Box<dyn std::error::Error>;
         assert_matches!(error.downcast_ref(), Some(CurveError::BadKeyType(_)));
     }
+
+    #[test]
+    fn honest_keys_are_torsion_free() {
+        let mut csprng = OsRng.unwrap_err();
+        let key_pair = KeyPair::generate(&mut csprng);
+        assert!(key_pair.public_key.is_torsion_free());
+    }
+
+    #[test]
+    fn tweaked_keys_are_not_torsion_free() {
+        let mut csprng = OsRng.unwrap_err();
+        let key_pair = KeyPair::generate(&mut csprng);
+        let pk_bytes: [u8; 32] = key_pair.public_key.public_key_bytes().try_into().unwrap();
+        let mont_pt = MontgomeryPoint(pk_bytes);
+        let ed_pt = mont_pt.to_edwards(0).unwrap();
+        for t in EIGHT_TORSION.iter().skip(1) {
+            let tweaked = ed_pt + *t; // add a torsion point
+            let tweaked_mont = tweaked.to_montgomery();
+            let tweaked_pk_bytes: [u8; 32] = tweaked_mont.to_bytes();
+            let tweaked_pk = PublicKey::from_djb_public_key_bytes(&tweaked_pk_bytes).unwrap();
+            assert!(!tweaked_pk.is_torsion_free());
+        }
+    }
+
+    #[test]
+    fn keys_with_the_high_bit_set_are_out_of_range() {
+        assert!(
+            PublicKey::from_djb_public_key_bytes(&[0; 32])
+                .expect("structurally valid")
+                .scalar_is_in_range(),
+            "0 should be in range"
+        );
+        assert!(
+            !PublicKey::from_djb_public_key_bytes(&hex!(
+                "0000000000000000000000000000000000000000000000000000000000000080"
+            ))
+            .expect("structurally valid")
+            .scalar_is_in_range(),
+            "2^255 should be out of range"
+        );
+        assert!(
+            !PublicKey::from_djb_public_key_bytes(&[0xFF; 32])
+                .expect("structurally valid")
+                .scalar_is_in_range(),
+            "2^256 - 1 should be out of range"
+        );
+        {
+            let mut csprng = OsRng.unwrap_err();
+            let key_pair = KeyPair::generate(&mut csprng);
+            assert!(key_pair.public_key.scalar_is_in_range());
+            let mut pk_bytes: [u8; 32] = key_pair.public_key.public_key_bytes().try_into().unwrap();
+            assert!(pk_bytes[31] & 0x80 == 0);
+            pk_bytes[31] |= 0x80;
+            assert!(
+                !PublicKey::from_djb_public_key_bytes(&pk_bytes)
+                    .expect("structurally valid")
+                    .scalar_is_in_range(),
+                ">2^255 should be out of range"
+            );
+        }
+    }
+
+    #[test]
+    fn keys_above_the_prime_modulus_are_out_of_range() {
+        // Curve25519 scalars use a little-endian representation.
+        let two_to_the_255_minus_one =
+            hex!("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff7f");
+
+        for i in 1..=19 {
+            let mut pk_bytes = two_to_the_255_minus_one;
+            pk_bytes[0] -= i;
+            pk_bytes[0] += 1; // because our original literal was 2^255 - 1
+            assert!(
+                !PublicKey::from_djb_public_key_bytes(&pk_bytes)
+                    .expect("structurally valid")
+                    .scalar_is_in_range(),
+                "2^255 - {i} should be out of range",
+            );
+
+            let mut canonical_representative = [0; 32];
+            canonical_representative[0] = 19 - i;
+
+            assert_eq!(
+                MontgomeryPoint(pk_bytes),
+                MontgomeryPoint(canonical_representative)
+            );
+        }
+
+        let mut pk_bytes = two_to_the_255_minus_one;
+        pk_bytes[0] -= 19; // resulting in the value 2^255 - 20
+        assert!(
+            PublicKey::from_djb_public_key_bytes(&pk_bytes)
+                .expect("structurally valid")
+                .scalar_is_in_range()
+        );
+    }
 }

rust/core/src/curve.rs

@@ -56,6 +56,7 @@ mlkem1024 = []
 [dev-dependencies]
 clap = { workspace = true, features = ["derive"] }
 criterion = { workspace = true }
+curve25519-dalek = { workspace = true, features = ["digest"] }
 env_logger = { workspace = true }
 futures-util = { workspace = true }
 proptest = { workspace = true }

rust/protocol/Cargo.toml

@@ -139,6 +139,14 @@ pub(crate) fn initialize_alice_session<R: Rng + CryptoRng>(
 pub(crate) fn initialize_bob_session(
     parameters: &BobSignalProtocolParameters,
 ) -> Result<SessionState> {
+    // validate their base key
+    if !parameters.their_base_key().is_canonical() {
+        return Err(SignalProtocolError::InvalidMessage(
+            crate::CiphertextMessageType::PreKey,
+            "incoming base key is invalid",
+        ));
+    }
+
     let local_identity = parameters.our_identity_key_pair().identity_key();
 
     let mut secrets = Vec::with_capacity(32 * 6);

rust/protocol/src/ratchet.rs

@@ -6,6 +6,8 @@
 use libsignal_protocol::*;
 
 mod support;
+use curve25519_dalek::constants::EIGHT_TORSION;
+use curve25519_dalek::montgomery::MontgomeryPoint;
 use rand::TryRngCore as _;
 use support::*;
 
@@ -77,3 +79,135 @@ fn test_alice_and_bob_agree_on_chain_keys_with_kyber() -> Result<(), SignalProto
 
     Ok(())
 }
+
+#[test]
+fn test_bob_rejects_torsioned_basekey() -> Result<(), SignalProtocolError> {
+    let mut csprng = rand::rngs::OsRng.unwrap_err();
+
+    let alice_identity_key_pair = IdentityKeyPair::generate(&mut csprng);
+    let alice_base_key_pair = KeyPair::generate(&mut csprng);
+
+    let bob_ephemeral_key_pair = KeyPair::generate(&mut csprng);
+    let bob_identity_key_pair = IdentityKeyPair::generate(&mut csprng);
+    let bob_signed_pre_key_pair = KeyPair::generate(&mut csprng);
+
+    let bob_kyber_pre_key_pair = kem::KeyPair::generate(kem::KeyType::Kyber1024, &mut csprng);
+
+    let alice_parameters = AliceSignalProtocolParameters::new(
+        alice_identity_key_pair,
+        alice_base_key_pair,
+        *bob_identity_key_pair.identity_key(),
+        bob_signed_pre_key_pair.public_key,
+        bob_ephemeral_key_pair.public_key,
+        bob_kyber_pre_key_pair.public_key.clone(),
+        UsePQRatchet::Yes,
+    );
+
+    let alice_record = initialize_alice_session_record(&alice_parameters, &mut csprng)?;
+
+    assert_eq!(
+        KYBER_AWARE_MESSAGE_VERSION,
+        alice_record.session_version().expect("must have a version")
+    );
+
+    let kyber_ciphertext = alice_record
+        .get_kyber_ciphertext()
+        .expect("must have session")
+        .expect("must have kyber ciphertext")
+        .clone()
+        .into_boxed_slice();
+
+    let tweaked_alice_base_key = {
+        let pk_bytes: [u8; 32] = alice_base_key_pair
+            .public_key
+            .public_key_bytes()
+            .try_into()
+            .unwrap();
+        let mont_pt = MontgomeryPoint(pk_bytes);
+        let ed_pt = mont_pt.to_edwards(0).unwrap();
+        let tweaked_ed = ed_pt + EIGHT_TORSION[1];
+        let tweaked_mont = tweaked_ed.to_montgomery();
+        let tweaked_pk_bytes: [u8; 32] = tweaked_mont.to_bytes();
+        PublicKey::from_djb_public_key_bytes(&tweaked_pk_bytes).unwrap()
+    };
+
+    let bob_parameters = BobSignalProtocolParameters::new(
+        bob_identity_key_pair,
+        bob_signed_pre_key_pair,
+        None,
+        bob_ephemeral_key_pair,
+        bob_kyber_pre_key_pair,
+        *alice_identity_key_pair.identity_key(),
+        tweaked_alice_base_key,
+        &kyber_ciphertext,
+        UsePQRatchet::Yes,
+    );
+
+    assert!(initialize_bob_session_record(&bob_parameters).is_err());
+
+    Ok(())
+}
+
+#[test]
+fn test_bob_rejects_highbit_basekey() -> Result<(), SignalProtocolError> {
+    let mut csprng = rand::rngs::OsRng.unwrap_err();
+
+    let alice_identity_key_pair = IdentityKeyPair::generate(&mut csprng);
+    let alice_base_key_pair = KeyPair::generate(&mut csprng);
+
+    let bob_ephemeral_key_pair = KeyPair::generate(&mut csprng);
+    let bob_identity_key_pair = IdentityKeyPair::generate(&mut csprng);
+    let bob_signed_pre_key_pair = KeyPair::generate(&mut csprng);
+
+    let bob_kyber_pre_key_pair = kem::KeyPair::generate(kem::KeyType::Kyber1024, &mut csprng);
+
+    let alice_parameters = AliceSignalProtocolParameters::new(
+        alice_identity_key_pair,
+        alice_base_key_pair,
+        *bob_identity_key_pair.identity_key(),
+        bob_signed_pre_key_pair.public_key,
+        bob_ephemeral_key_pair.public_key,
+        bob_kyber_pre_key_pair.public_key.clone(),
+        UsePQRatchet::Yes,
+    );
+
+    let alice_record = initialize_alice_session_record(&alice_parameters, &mut csprng)?;
+
+    assert_eq!(
+        KYBER_AWARE_MESSAGE_VERSION,
+        alice_record.session_version().expect("must have a version")
+    );
+
+    let kyber_ciphertext = alice_record
+        .get_kyber_ciphertext()
+        .expect("must have session")
+        .expect("must have kyber ciphertext")
+        .clone()
+        .into_boxed_slice();
+
+    let tweaked_alice_base_key = {
+        let mut pk_bytes: [u8; 32] = alice_base_key_pair
+            .public_key
+            .public_key_bytes()
+            .try_into()
+            .unwrap();
+        pk_bytes[31] |= 0b1000_0000_u8;
+        PublicKey::from_djb_public_key_bytes(&pk_bytes).unwrap()
+    };
+
+    let bob_parameters = BobSignalProtocolParameters::new(
+        bob_identity_key_pair,
+        bob_signed_pre_key_pair,
+        None,
+        bob_ephemeral_key_pair,
+        bob_kyber_pre_key_pair,
+        *alice_identity_key_pair.identity_key(),
+        tweaked_alice_base_key,
+        &kyber_ciphertext,
+        UsePQRatchet::Yes,
+    );
+
+    assert!(initialize_bob_session_record(&bob_parameters).is_err());
+
+    Ok(())
+}