fix(protocol,mls): report a key package refused by this device's clock

CHANGELOG.md

@@ -345,6 +345,25 @@ archived by series under [docs/changelog/](docs/changelog/); see the
 
 ### Fixed
 
+- **A key package refused by this device's clock is reported.** A peer's key
+  package is valid from an hour before it was minted until 30 days after,
+  judged by the receiver's clock. A device more than an hour behind a peer
+  cannot start a session with it, and past 30 days neither side can, so no
+  session forms and messages and connection requests stay pending forever.
+  The only trace was a debug line. Two Android phones that had never been
+  online (one set to 2024, the other to 2025) reproduced it. The refusal
+  now raises a `security_warning` with the new code
+  `KEY_PACKAGE_OUTSIDE_VALIDITY_WINDOW`, once per peer, and is logged at
+  `warn`, whether the package was refused on arrival or on the first send
+  after a restart. `MlsError::KeyPackageOutsideValidityWindow { expired }`
+  replaces the `InvalidKeyPackage` text for this one refusal on every route
+  that admits a peer's package. A package whose window has already closed is
+  discarded like any expired package, because it may be an old package a relay
+  held rather than a clock, and kept it failed every attempt until its cached
+  expiry; one whose window has not started is kept. The window itself is
+  unchanged. TypeScript's
+  `SecurityWarningCode` union gains the code, so an exhaustive `switch` over it
+  needs a new arm.
 - **An old message no longer comes back as a push notification, again and
   again.** A direct message the relay had pushed stayed in the outbox well past
   a week, because each probe re-send refreshed its lifetime and so did each

bindings/react-native/src/types.ts

@@ -2518,7 +2518,8 @@ export type SecurityWarningCode =
   | 'GROUP_LEAF_IDENTITY_UNPROVEN'
   | 'STALE_CONTROL_FRAME'
   | 'GATEWAY_ADDRESS_BINDING_MISMATCH'
-  | 'GATEWAY_ADDRESS_DECLARATION_REFUSED';
+  | 'GATEWAY_ADDRESS_DECLARATION_REFUSED'
+  | 'KEY_PACKAGE_OUTSIDE_VALIDITY_WINDOW';
 
 /**
  * A security-relevant anomaly was detected for a peer.
@@ -2597,6 +2598,22 @@ export type SecurityWarningCode =
  * frames or running a broken build. Nothing is torn down — the frame is
  * dropped, unacknowledged, and a peer whose frame was genuinely just slow
  * re-sends a fresh one.
+ *
+ * `KEY_PACKAGE_OUTSIDE_VALIDITY_WINDOW` is the same kind of finding for key
+ * packages, and again the first thing to check is a clock. A peer's key
+ * package is valid from an hour before it was made until 30 days after,
+ * judged by this device's clock. A device more than an hour behind a peer
+ * cannot start a session with it (the peer still can, and usually does).
+ * Past 30 days each side refuses the other's package, both report this, and
+ * no encrypted session forms: messages and connection requests wait until
+ * the clocks agree. This is what happens between phones that have not been
+ * online to set their time. Many `peer_id`s is this device's clock; one is that
+ * peer's. A window that has already closed may instead be an old package a
+ * relay held for days; that package is discarded and the next one the peer
+ * sends replaces it, and `reason` names both causes. Reported once per peer,
+ * whether the refusal came when the package arrived or on a later send, and
+ * `peer_id` is a claim, since the package has not proved its sender when its
+ * window is checked. Show the user a prompt to check the date and time.
  */
 export interface SecurityWarningEvent extends BaseEvent {
   type: 'security_warning';

crates/offline-protocol-mls/src/error.rs

@@ -73,6 +73,40 @@ pub enum MlsError {
     #[error("Invalid key package: {0}")]
     InvalidKeyPackage(String),
 
+    /// A peer's key package is well formed, but this device's clock falls
+    /// outside its validity window: before its `not_before`, or after its
+    /// `not_after`.
+    ///
+    /// Split from [`Self::InvalidKeyPackage`] because it has a different
+    /// remedy, and that remedy is usually a clock rather than the peer. The
+    /// window is judged against this device's own time, and a package starts
+    /// only an hour before it was minted, so a device whose clock runs more
+    /// than an hour behind its peer's refuses every package that peer sends.
+    /// Past the 30-day lifetime the peer refuses this device's packages too,
+    /// and no session forms from either side. Reported to the application as
+    /// `KEY_PACKAGE_OUTSIDE_VALIDITY_WINDOW`; before that it was a debug line,
+    /// and two phones a year apart sat on a pending connection request with
+    /// nothing anywhere to say why.
+    ///
+    /// `expired` says which end refused it, because the two have different
+    /// remedies. A window that has not started (`false`) is a clock, this
+    /// device's or the peer's, and the package becomes valid once the clocks
+    /// agree. A window that has closed (`true`) is this device's clock running
+    /// ahead *or* a package that is simply old: a relay can hold a package for
+    /// days, and the receiver's cached expiry is anchored to when the frame
+    /// arrived, not to when the package was minted. Blaming the clock for the
+    /// second, and keeping the package, would fail every attempt with a
+    /// correct clock until the cached expiry.
+    #[error(
+        "Key package is outside its validity window by this device's clock: {}",
+        if *.expired { "it has already closed" } else { "it has not started yet" }
+    )]
+    KeyPackageOutsideValidityWindow {
+        /// `true` when the window closed before this device's `now`, `false`
+        /// when it has not opened yet.
+        expired: bool,
+    },
+
     /// Failed to create a group.
     #[error("Group creation failed: {0}")]
     GroupCreation(String),
@@ -507,6 +541,9 @@ impl MlsError {
             Self::CredentialCreation(_) => "MLS credential creation failed",
             Self::KeyPackageCreation(_) => "Key package creation failed",
             Self::InvalidKeyPackage(_) => "The peer's key package was malformed or unusable",
+            Self::KeyPackageOutsideValidityWindow { .. } => {
+                "The peer's key package is not valid at this device's time"
+            }
             Self::GroupCreation(_) => "MLS group creation failed",
             Self::GroupNotFound(_) => "No local MLS group state for the requested conversation",
             Self::AddMember(_) => "Adding a member to the MLS group failed",

crates/offline-protocol-mls/src/manager.rs

@@ -673,11 +673,7 @@ impl MlsManager {
         offline_protocol_core::validate_id_chars(user_id, "User ID")
             .map_err(|e| MlsError::InvalidUserId(e.to_string()))?;
 
-        let key_package_in = KeyPackageIn::tls_deserialize_exact(key_package_data)
-            .map_err(|e| MlsError::InvalidKeyPackage(e.to_string()))?;
-        let key_package = key_package_in
-            .validate(self.provider.crypto(), ProtocolVersion::Mls10)
-            .map_err(|e| MlsError::InvalidKeyPackage(e.to_string()))?;
+        let key_package = self.validate_peer_key_package(key_package_data)?;
 
         Self::verify_credential_identity(&key_package, user_id)?;
         Self::verify_address_binding(&key_package, user_id)?;
@@ -698,13 +694,8 @@ impl MlsManager {
             .load(key_type, user_id)?
             .ok_or_else(|| MlsError::NoKeyPackage(user_id.to_string()))?;
 
-        let key_package_in = KeyPackageIn::tls_deserialize_exact(&data)
-            .map_err(|e| MlsError::InvalidKeyPackage(e.to_string()))?;
-
         // Validate using the crypto backend
-        let key_package = key_package_in
-            .validate(self.provider.crypto(), ProtocolVersion::Mls10)
-            .map_err(|e| MlsError::InvalidKeyPackage(e.to_string()))?;
+        let key_package = self.validate_peer_key_package(&data)?;
 
         // Defense in depth: entries written to storage out-of-band must not
         // come back attributed to the wrong user. Both checks run again here
@@ -718,6 +709,84 @@ impl MlsManager {
         Ok(key_package)
     }
 
+    /// Validates a key package this install did not mint.
+    ///
+    /// The one place a peer's package meets OpenMLS's checks, so that a
+    /// package refused for its validity window comes back as
+    /// [`MlsError::KeyPackageOutsideValidityWindow`] on every route that
+    /// admits one (import, the cache read, adding a group member) rather than
+    /// as free text on some of them, so the engine can report it. The window
+    /// is the only refusal whose usual cause is a clock.
+    ///
+    /// Takes the bytes rather than a parsed package because `validate`
+    /// consumes the package, and which end of the window refused it is read
+    /// back from the bytes only on that failure, so the accepting path pays
+    /// nothing for it.
+    fn validate_peer_key_package(&self, key_package_data: &[u8]) -> Result<KeyPackage> {
+        KeyPackageIn::tls_deserialize_exact(key_package_data)
+            .map_err(|e| MlsError::InvalidKeyPackage(e.to_string()))?
+            .validate(self.provider.crypto(), ProtocolVersion::Mls10)
+            .map_err(|e| match e {
+                KeyPackageVerifyError::InvalidLifetime => {
+                    MlsError::KeyPackageOutsideValidityWindow {
+                        expired: Self::window_has_closed(key_package_data),
+                    }
+                }
+                other => MlsError::InvalidKeyPackage(other.to_string()),
+            })
+    }
+
+    /// Whether a package OpenMLS refused for its lifetime was refused because
+    /// the window has already closed, rather than because it has not opened.
+    ///
+    /// OpenMLS exposes no lifetime on an unvalidated `KeyPackageIn` outside
+    /// its `test-utils` feature, so the window is read from its serde form: the
+    /// leaf node's `Lifetime` is the only `{not_before, not_after}` object in a
+    /// key package. Read through OpenMLS's own types rather than by walking the
+    /// TLS encoding by hand, which would be a second key package codec. If the
+    /// shape ever changes the answer is `false`, the refusal is reported as a
+    /// clock and the package is kept, which is how this refusal was handled
+    /// before the two ends were told apart; the tests on both ends pin it.
+    ///
+    /// OpenMLS admits `not_before < now < not_after`, so a refused package
+    /// whose `not_after` is at or before `now` closed, and any other did not
+    /// start. A clock before the epoch reads as "not started", which is right:
+    /// that clock is behind.
+    fn window_has_closed(key_package_data: &[u8]) -> bool {
+        fn find_lifetime(value: &serde_json::Value) -> Option<u64> {
+            match value {
+                serde_json::Value::Object(map) => {
+                    if map
+                        .get("not_before")
+                        .and_then(serde_json::Value::as_u64)
+                        .is_some()
+                    {
+                        if let Some(not_after) =
+                            map.get("not_after").and_then(serde_json::Value::as_u64)
+                        {
+                            return Some(not_after);
+                        }
+                    }
+                    map.values().find_map(find_lifetime)
+                }
+                serde_json::Value::Array(items) => items.iter().find_map(find_lifetime),
+                _ => None,
+            }
+        }
+
+        let Some(not_after) = KeyPackageIn::tls_deserialize_exact(key_package_data)
+            .ok()
+            .and_then(|package| serde_json::to_value(&package).ok())
+            .and_then(|value| find_lifetime(&value))
+        else {
+            return false;
+        };
+        std::time::SystemTime::now()
+            .duration_since(std::time::UNIX_EPOCH)
+            .map(|now| now.as_secs() >= not_after)
+            .unwrap_or(false)
+    }
+
     /// Requires a key package's validity window to be no wider than this
     /// install admits.
     ///
@@ -1104,10 +1173,7 @@ impl MlsManager {
         invitee_user_id: &str,
         member_key_package: &[u8],
     ) -> Result<(WelcomeMessage, EncryptedMessage)> {
-        let key_package = KeyPackageIn::tls_deserialize_exact(member_key_package)
-            .map_err(|e| MlsError::InvalidKeyPackage(e.to_string()))?
-            .validate(self.provider.crypto(), ProtocolVersion::Mls10)
-            .map_err(|e| MlsError::InvalidKeyPackage(e.to_string()))?;
+        let key_package = self.validate_peer_key_package(member_key_package)?;
 
         Self::verify_credential_identity(&key_package, invitee_user_id)?;
         Self::verify_address_binding(&key_package, invitee_user_id)?;
@@ -2041,6 +2107,37 @@ impl MlsManager {
             .tls_serialize_detached()
             .map_err(|e| MlsError::Serialization(e.to_string()))
     }
+
+    /// Mints a key package claiming `claimed` whose validity window runs from
+    /// `not_before` to `not_after` (seconds since the epoch): what a peer
+    /// whose clock is somewhere else entirely would send.
+    ///
+    /// The window is checked before the identity binding, so the claimed
+    /// identity is never proved here, which is also what makes the refusal
+    /// it provokes attacker-reachable.
+    pub fn key_package_with_window_for_testing(
+        claimed: &str,
+        not_before: u64,
+        not_after: u64,
+    ) -> Result<Vec<u8>> {
+        let storage: Arc<dyn MlsStorage> = Arc::new(crate::storage::InMemoryStorage::new());
+        let provider = MlsProvider::new(MlsStorageAdapter::new(storage));
+        let keys = SignatureKeyPair::new(DEFAULT_CIPHERSUITE.signature_algorithm())
+            .map_err(|e| MlsError::CryptoGeneration(format!("{:?}", e)))?;
+        keys.store(provider.storage())
+            .map_err(|e| MlsError::CryptoGeneration(format!("storing key: {:?}", e)))?;
+        let credential = CredentialWithKey {
+            credential: Credential::new(CredentialType::Basic, claimed.as_bytes().to_vec()),
+            signature_key: keys.public().into(),
+        };
+        KeyPackage::builder()
+            .key_package_lifetime(Lifetime::init(not_before, not_after))
+            .build(DEFAULT_CIPHERSUITE, &provider, &keys, credential)
+            .map_err(|e| MlsError::KeyPackageCreation(e.to_string()))?
+            .key_package()
+            .tls_serialize_detached()
+            .map_err(|e| MlsError::Serialization(e.to_string()))
+    }
 }
 
 /// Returns `true` when `bytes` parse as a well-formed MLS wire message
@@ -4889,6 +4986,74 @@ mod tests {
             .as_secs()
     }
 
+    /// A peer whose clock runs a year ahead of ours mints a window that has
+    /// not started yet by our clock. It is refused, and refused as a window
+    /// problem rather than as a malformed package, because that is what tells
+    /// an application to look at a clock. Two phones a year apart reproduced
+    /// this, and the only trace was a debug line.
+    #[test]
+    fn a_window_that_has_not_started_by_our_clock_is_reported_as_such() {
+        let (manager, _) = create_addressed_manager("alice");
+        let year = 365 * 24 * 60 * 60;
+        let (peer, bytes) = key_package_with_window(
+            "bob",
+            Lifetime::init(
+                now_secs() + year - 3600,
+                now_secs() + year + 30 * 24 * 60 * 60,
+            ),
+        );
+
+        let err = manager
+            .import_key_package(&peer, &bytes)
+            .expect_err("a window that starts in a year is not valid now");
+        assert!(
+            matches!(
+                err,
+                MlsError::KeyPackageOutsideValidityWindow { expired: false }
+            ),
+            "refused for the wrong reason: {err:?}"
+        );
+    }
+
+    /// The other side of the same skew: a peer whose clock runs behind mints a
+    /// window that has already closed by ours.
+    #[test]
+    fn a_window_that_closed_by_our_clock_is_reported_as_such() {
+        let (manager, _) = create_addressed_manager("alice");
+        let day = 24 * 60 * 60;
+        let (peer, bytes) = key_package_with_window(
+            "bob",
+            Lifetime::init(now_secs() - 60 * day, now_secs() - 30 * day),
+        );
+
+        let err = manager
+            .import_key_package(&peer, &bytes)
+            .expect_err("a window that closed a month ago is not valid now");
+        assert!(
+            matches!(
+                err,
+                MlsError::KeyPackageOutsideValidityWindow { expired: true }
+            ),
+            "refused for the wrong reason: {err:?}"
+        );
+    }
+
+    /// Every other refusal keeps its old type and text: only the window moved.
+    /// The width cap in particular stays `InvalidKeyPackage`, because a package
+    /// claiming a year is a peer's policy, not anyone's clock.
+    #[test]
+    fn only_the_window_refusal_changes_type() {
+        let (manager, _) = create_addressed_manager("alice");
+        let (peer, bytes) = key_package_with_window("bob", Lifetime::new(365 * 24 * 60 * 60));
+        let err = manager.import_key_package(&peer, &bytes).unwrap_err();
+        assert!(matches!(&err, MlsError::InvalidKeyPackage(m) if m.contains("wider than")));
+
+        let err = manager
+            .import_key_package(&peer, b"not a key package")
+            .unwrap_err();
+        assert!(matches!(err, MlsError::InvalidKeyPackage(_)), "{err:?}");
+    }
+
     /// The case the issue was filed on: mls-rs hands out a year by default, and
     /// OpenMLS validation admits it because it never applies its own cap.
     #[test]