crypto: Rotate fallback keys in a time-based manner (#3151)

Fallback keys until now have been rotated on the basis that the
homeserver tells us that a fallback key has been used.

Now this leads to various problems if the server tells us too often that
it has been used, i.e. if we receive the same sync response too often.
It leaves us also open to the homeserver being dishonest and never
telling us that the fallback key has been used.

Let's avoid all these problems by just rotating the fallback key in a
time-based manner.

Signed-off-by: Damir Jelić <[email protected]>
Co-authored-by: Andy Balaam <[email protected]>

Author: poljar

bindings/matrix-sdk-crypto-ffi/src/lib.rs

@@ -245,7 +245,8 @@ async fn migrate_data(
         pickle,
         shared: data.account.shared,
         uploaded_signed_key_count: data.account.uploaded_signed_key_count as u64,
-        creation_local_time: MilliSecondsSinceUnixEpoch(UInt::default()),
+        creation_local_time: MilliSecondsSinceUnixEpoch::now(),
+        fallback_key_creation_timestamp: Some(MilliSecondsSinceUnixEpoch::now()),
     };
     let account = matrix_sdk_crypto::olm::Account::from_pickle(pickled_account)?;
 

crates/matrix-sdk-crypto/CHANGELOG.md

@@ -1,5 +1,11 @@
 # UNRELEASED
 
+Changed:
+
+- Fallback keys are rotated in a time-based manner, instead of waiting for the
+  server to tell us that a fallback key got used.
+  ([#3151](https://github.com/matrix-org/matrix-rust-sdk/pull/3151))
+
 Breaking changes:
 
 - Rename the `OlmMachine::invalidate_group_session` method to

crates/matrix-sdk-crypto/src/dehydrated_devices.rs

@@ -318,7 +318,7 @@ impl DehydratedDevice {
         let mut transaction = self.store.transaction().await;
 
         let account = transaction.account().await?;
-        account.generate_fallback_key_helper();
+        account.generate_fallback_key_if_needed();
 
         let (device_keys, one_time_keys, fallback_keys) = account.keys_for_upload();
 

crates/matrix-sdk-crypto/src/machine.rs

@@ -2305,7 +2305,7 @@ pub(crate) mod tests {
             .store()
             .with_transaction(|mut tr| async {
                 let account = tr.account().await.unwrap();
-                account.generate_fallback_key_helper();
+                account.generate_fallback_key_if_needed();
                 account.update_uploaded_key_count(0);
                 account.generate_one_time_keys_if_needed();
                 let request = machine

crates/matrix-sdk-crypto/src/olm/account.rs

@@ -17,6 +17,7 @@ use std::{
     fmt,
     ops::{Deref, Not as _},
     sync::Arc,
+    time::Duration,
 };
 
 use ruma::{
@@ -327,6 +328,14 @@ pub struct Account {
     /// needs to set this for us, depending on the count we will suggest the
     /// client to upload new keys.
     uploaded_signed_key_count: u64,
+    /// The timestamp of the last time we generated a fallback key. Fallback
+    /// keys are rotated in a time-based manner. This field records when we
+    /// either generated our first fallback key or rotated one.
+    ///
+    /// Will be `None` if we never created a fallback key, or if we're migrating
+    /// from a `AccountPickle` that didn't use time-based fallback key
+    /// rotation.
+    fallback_creation_timestamp: Option<MilliSecondsSinceUnixEpoch>,
 }
 
 impl Deref for Account {
@@ -358,6 +367,9 @@ pub struct PickledAccount {
     /// as creation time of own device
     #[serde(default = "default_account_creation_time")]
     pub creation_local_time: MilliSecondsSinceUnixEpoch,
+    /// The timestamp of the last time we generated a fallback key.
+    #[serde(default)]
+    pub fallback_key_creation_timestamp: Option<MilliSecondsSinceUnixEpoch>,
 }
 
 fn default_account_creation_time() -> MilliSecondsSinceUnixEpoch {
@@ -404,6 +416,7 @@ impl Account {
             inner: Box::new(account),
             shared: false,
             uploaded_signed_key_count: 0,
+            fallback_creation_timestamp: None,
         }
     }
 
@@ -496,11 +509,11 @@ impl Account {
             self.generate_one_time_keys_if_needed();
         }
 
-        if let Some(unused) = unused_fallback_keys {
-            if !unused.contains(&DeviceKeyAlgorithm::SignedCurve25519) {
-                // Generate a new fallback key if we don't have one.
-                self.generate_fallback_key_helper();
-            }
+        // If the server supports fallback keys or if it did so in the past, shown by
+        // the existence of a fallback creation timestamp, generate a new one if
+        // we don't have one, or if the current fallback key expired.
+        if unused_fallback_keys.is_some() || self.fallback_creation_timestamp.is_some() {
+            self.generate_fallback_key_if_needed();
         }
     }
 
@@ -543,17 +556,61 @@ impl Account {
         Some(key_count as u64)
     }
 
-    pub(crate) fn generate_fallback_key_helper(&mut self) {
-        if self.inner.fallback_key().is_empty() {
+    /// Generate a new fallback key iff a unpublished one isn't already inside
+    /// of vodozemac and if the currently active one expired.
+    ///
+    /// The former is checked using [`Account::fallback_key().is_empty()`],
+    /// which is a hashmap that gets cleared by the
+    /// [`Account::mark_keys_as_published()`] call.
+    pub(crate) fn generate_fallback_key_if_needed(&mut self) {
+        if self.inner.fallback_key().is_empty() && self.fallback_key_expired() {
             let removed_fallback_key = self.inner.generate_fallback_key();
+            self.fallback_creation_timestamp = Some(MilliSecondsSinceUnixEpoch::now());
 
             debug!(
                 ?removed_fallback_key,
-                "No unused fallback keys were found on the server, generated a new fallback key.",
+                "The fallback key either expired or we didn't have one: generated a new fallback key.",
             );
         }
     }
 
+    /// Check if our most recent fallback key has expired.
+    ///
+    /// We consider the fallback key to be expired if it's older than a week.
+    /// This is the lower bound for the recommended signed pre-key bundle
+    /// rotation interval in the X3DH spec[1].
+    ///
+    /// [1]: https://signal.org/docs/specifications/x3dh/#publishing-keys
+    fn fallback_key_expired(&self) -> bool {
+        const FALLBACK_KEY_MAX_AGE: Duration = Duration::from_secs(3600 * 24 * 7);
+
+        if let Some(time) = self.fallback_creation_timestamp {
+            // `to_system_time()` returns `None` if the the UNIX_EPOCH + `time` doesn't fit
+            // into a i64. This will likely never happen, but let's rotate the
+            // key in case the values are messed up for some other reason.
+            let Some(system_time) = time.to_system_time() else {
+                return true;
+            };
+
+            // `elapsed()` errors if the `system_time` is in the future, this should mean
+            // that our clock has changed to the past, let's rotate just in case
+            // and then we'll get to a normal time.
+            let Ok(elapsed) = system_time.elapsed() else {
+                return true;
+            };
+
+            // Alright, our times are normal and we know how much time elapsed since the
+            // last time we created/rotated a fallback key.
+            //
+            // If the key is older than a week, then we rotate it.
+            elapsed > FALLBACK_KEY_MAX_AGE
+        } else {
+            // We never created a fallback key, or we're migrating to the time-based
+            // fallback key rotation, so let's generate a new fallback key.
+            true
+        }
+    }
+
     fn fallback_key(&self) -> HashMap<KeyId, Curve25519PublicKey> {
         self.inner.fallback_key()
     }
@@ -595,6 +652,7 @@ impl Account {
             shared: self.shared(),
             uploaded_signed_key_count: self.uploaded_key_count(),
             creation_local_time: self.static_data.creation_local_time,
+            fallback_key_creation_timestamp: self.fallback_creation_timestamp,
         }
     }
 
@@ -651,6 +709,7 @@ impl Account {
             inner: Box::new(account),
             shared: pickle.shared,
             uploaded_signed_key_count: pickle.uploaded_signed_key_count,
+            fallback_creation_timestamp: pickle.fallback_key_creation_timestamp,
         })
     }
 
@@ -1372,6 +1431,7 @@ mod tests {
     use std::{
         collections::{BTreeMap, BTreeSet},
         ops::Deref,
+        time::Duration,
     };
 
     use anyhow::Result;
@@ -1443,30 +1503,59 @@ mod tests {
         // We don't create fallback keys since we don't know if the server
         // supports them, we need to receive a sync response to decide if we're
         // going to create them or not.
-        assert!(fallback_keys.is_empty());
+        assert!(
+            fallback_keys.is_empty(),
+            "We should not upload fallback keys until we know if the server supports them."
+        );
 
         let one_time_keys = BTreeMap::from([(DeviceKeyAlgorithm::SignedCurve25519, 50u8.into())]);
 
         // A `None` here means that the server doesn't support fallback keys, no
         // fallback key gets uploaded.
         account.update_key_counts(&one_time_keys, None);
         let (_, _, fallback_keys) = account.keys_for_upload();
-        assert!(fallback_keys.is_empty());
+        assert!(
+            fallback_keys.is_empty(),
+            "We should not upload a fallback key if we're certain that the server doesn't support \
+             them."
+        );
 
         // The empty array means that the server supports fallback keys but
         // there isn't a unused fallback key on the server. This time we upload
         // a fallback key.
         let unused_fallback_keys = &[];
         account.update_key_counts(&one_time_keys, Some(unused_fallback_keys.as_ref()));
         let (_, _, fallback_keys) = account.keys_for_upload();
-        assert!(!fallback_keys.is_empty());
+        assert!(
+            !fallback_keys.is_empty(),
+            "We should upload the initial fallback key if the server supports them."
+        );
         account.mark_keys_as_published();
 
-        // There's an unused fallback key on the server, nothing to do here.
-        let unused_fallback_keys = &[DeviceKeyAlgorithm::SignedCurve25519];
+        // There's no unused fallback key on the server, but our initial fallback key
+        // did not yet expire.
+        let unused_fallback_keys = &[];
         account.update_key_counts(&one_time_keys, Some(unused_fallback_keys.as_ref()));
         let (_, _, fallback_keys) = account.keys_for_upload();
-        assert!(fallback_keys.is_empty());
+        assert!(
+            fallback_keys.is_empty(),
+            "We should not upload new fallback keys unless our current fallback key expires."
+        );
+
+        let fallback_key_timestamp =
+            account.fallback_creation_timestamp.unwrap().to_system_time().unwrap()
+                - Duration::from_secs(3600 * 24 * 30);
+
+        account.fallback_creation_timestamp =
+            Some(MilliSecondsSinceUnixEpoch::from_system_time(fallback_key_timestamp).unwrap());
+
+        account.update_key_counts(&one_time_keys, None);
+        let (_, _, fallback_keys) = account.keys_for_upload();
+        assert!(
+            !fallback_keys.is_empty(),
+            "Now that our fallback key has expired, we should try to upload a new one, even if the \
+             server supposedly doesn't support fallback keys anymore"
+        );
 
         Ok(())
     }