diff --git a/CHANGELOG.md b/CHANGELOG.md index 37d3dc44..96a63cc7 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/bindings/react-native/src/types.ts b/bindings/react-native/src/types.ts index 4fae8d07..f4a728cf 100644 --- a/bindings/react-native/src/types.ts +++ b/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'; diff --git a/crates/offline-protocol-mls/src/error.rs b/crates/offline-protocol-mls/src/error.rs index a7681136..2720a90b 100644 --- a/crates/offline-protocol-mls/src/error.rs +++ b/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", diff --git a/crates/offline-protocol-mls/src/manager.rs b/crates/offline-protocol-mls/src/manager.rs index 77a9fd3b..a31b1454 100644 --- a/crates/offline-protocol-mls/src/manager.rs +++ b/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 { + 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 { + 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> { + let storage: Arc = 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] diff --git a/crates/offline-protocol/src/events.rs b/crates/offline-protocol/src/events.rs index 4337b647..db72b9cc 100644 --- a/crates/offline-protocol/src/events.rs +++ b/crates/offline-protocol/src/events.rs @@ -331,6 +331,33 @@ pub enum SecurityWarningCode { /// carrier available, and this warning explains a transport that stays /// down while its socket connects fine. GatewayAddressDeclarationRefused, + /// A peer's key package was refused because this device's clock is + /// outside its validity window: the window has not started yet, or has + /// already closed. + /// + /// Like [`Self::StaleControlFrame`], the first thing to check is a clock, + /// and it is usually this device's. A package is valid from an hour before + /// it was minted until 30 days after, by the receiver's clock. So a device + /// more than an hour behind a peer cannot start a session with it, though + /// the peer, whose clock accepts this device's package, still can. Once + /// the gap passes 30 days each side refuses the other's package and no + /// session forms at all: messages and connection requests wait for as long + /// as the clocks disagree, and both sides report this. Phones that have not + /// been online are where this happens. Many different peers is this + /// device's clock; one peer while others are fine is that peer's. + /// + /// A window that has already closed is not always a clock: a relay can + /// hold a package for days, and the receiver anchors its cached expiry to + /// when the frame arrived. That package is discarded, as an expired one + /// is, and the `reason` says either cause; the next package the peer sends + /// replaces it. A window that has not started is kept, since it becomes + /// valid once the clocks agree. + /// + /// Raised on every route that admits a pending package: when it arrives, + /// and on the first send after a restart. The package has not proved its + /// sender when the window is checked, so the peer named is a claim, and + /// the event is reported at most once per peer. + KeyPackageOutsideValidityWindow, } impl SecurityWarningCode { @@ -354,6 +381,7 @@ impl SecurityWarningCode { Self::StaleControlFrame => "STALE_CONTROL_FRAME", Self::GatewayAddressBindingMismatch => "GATEWAY_ADDRESS_BINDING_MISMATCH", Self::GatewayAddressDeclarationRefused => "GATEWAY_ADDRESS_DECLARATION_REFUSED", + Self::KeyPackageOutsideValidityWindow => "KEY_PACKAGE_OUTSIDE_VALIDITY_WINDOW", } } } @@ -4234,6 +4262,7 @@ mod tests { SecurityWarningCode::StaleControlFrame, SecurityWarningCode::GatewayAddressBindingMismatch, SecurityWarningCode::GatewayAddressDeclarationRefused, + SecurityWarningCode::KeyPackageOutsideValidityWindow, ]; for code in all { // serde renders a unit enum variant as a quoted JSON string. @@ -4263,7 +4292,8 @@ mod tests { | SecurityWarningCode::GroupLeafIdentityUnproven | SecurityWarningCode::StaleControlFrame | SecurityWarningCode::GatewayAddressBindingMismatch - | SecurityWarningCode::GatewayAddressDeclarationRefused => {} + | SecurityWarningCode::GatewayAddressDeclarationRefused + | SecurityWarningCode::KeyPackageOutsideValidityWindow => {} } } } diff --git a/crates/offline-protocol/src/protocol/mod.rs b/crates/offline-protocol/src/protocol/mod.rs index 041abaae..41d27679 100644 --- a/crates/offline-protocol/src/protocol/mod.rs +++ b/crates/offline-protocol/src/protocol/mod.rs @@ -2971,12 +2971,7 @@ impl OfflineProtocol { self.pending_key_packages.remove(peer_id); self.delete_peer_key_package_from_storage(peer_id); } else { - { - let manager = mls - .read() - .map_err(|_| Error::Other("MLS lock poisoned".to_string()))?; - manager.import_key_package(peer_id, &received_pkg.key_package_data)?; - } + self.import_pending_key_package(&mls, peer_id, &received_pkg.key_package_data)?; // Create session and get welcome message let welcome = { @@ -3036,6 +3031,69 @@ impl OfflineProtocol { Err(Error::SessionNotReady(self.establishment_state(peer_id)?)) } + /// Imports `peer_id`'s pending key package into MLS, reporting a refusal + /// by this device's clock. + /// + /// The one place the engine admits a pending package, so that the refusal + /// is reported whichever route reached it: `establish_secure_session`, + /// taken when a package arrives, and the send path's + /// `ensure_session_establishment`, taken on the first send after a + /// restart or with `auto_key_exchange` off. Reported on only the first, + /// the restart case left a message queued "anyway" with nothing to say why, + /// which is the failure the code exists for. The throttle makes a second + /// route free: one event per peer either way. + /// + /// A window that has not started keeps the package, since it becomes valid + /// once the clocks agree. A window that has closed discards it, exactly as + /// an expired cached package is discarded, because it may be a package a + /// relay held for days rather than a clock, and kept it would fail every + /// attempt until its cached expiry. The next package the peer sends + /// replaces it. + pub(super) fn import_pending_key_package( + &mut self, + mls: &Arc>, + peer_id: &str, + key_package_data: &[u8], + ) -> Result<()> { + let imported = { + let manager = mls + .read() + .map_err(|_| Error::Other("MLS lock poisoned".to_string()))?; + manager.import_key_package(peer_id, key_package_data) + }; + if let Err(offline_protocol_mls::MlsError::KeyPackageOutsideValidityWindow { expired }) = + &imported + { + let reason = if *expired { + warn!( + peer_id = %peer_id, + "Peer key package's validity window has closed by this device's \ + clock; discarding it until the peer sends a fresh one" + ); + self.pending_key_packages.remove(peer_id); + self.delete_peer_key_package_from_storage(peer_id); + "The peer's key package has expired by this device's clock. Either \ + this device's date is ahead, or the package is older than its 30-day \ + window; it is discarded and the next one the peer sends replaces it." + } else { + warn!( + peer_id = %peer_id, + "Peer key package's validity window has not started by this \ + device's clock; this device cannot start a session with the peer \ + until the clocks agree" + ); + "The peer's key package is not valid yet at this device's time. \ + Check this device's date and time, then the peer's." + }; + self.warn_control_gate_rejection( + peer_id, + crate::events::SecurityWarningCode::KeyPackageOutsideValidityWindow, + reason, + ); + } + Ok(imported?) + } + /// Checks if a pending key package is available for a peer. /// /// This can be used to check if session establishment is possible diff --git a/crates/offline-protocol/src/protocol/security.rs b/crates/offline-protocol/src/protocol/security.rs index f57dfc39..835974a8 100644 --- a/crates/offline-protocol/src/protocol/security.rs +++ b/crates/offline-protocol/src/protocol/security.rs @@ -899,6 +899,10 @@ impl OfflineProtocol { SecurityWarningCode::UnsignedControlRejected => 1 << 2, SecurityWarningCode::SenderAddressMismatch => 1 << 3, SecurityWarningCode::StaleControlFrame => 1 << 4, + // Not a gate rejection, but the same exposure: the key package has + // not proved its sender when its window is checked, so the peer id + // is attacker-chosen and needs the same once-per-peer bound. + SecurityWarningCode::KeyPackageOutsideValidityWindow => 1 << 5, _ => 0, } } diff --git a/crates/offline-protocol/src/protocol/send.rs b/crates/offline-protocol/src/protocol/send.rs index 0d9c69c7..9ae73ef1 100644 --- a/crates/offline-protocol/src/protocol/send.rs +++ b/crates/offline-protocol/src/protocol/send.rs @@ -898,12 +898,11 @@ impl OfflineProtocol { self.pending_key_packages.remove(recipient); self.delete_peer_key_package_from_storage(recipient); } else { - { - let manager = mls - .read() - .map_err(|_| Error::Other("MLS lock poisoned".to_string()))?; - manager.import_key_package(recipient, &received_pkg.key_package_data)?; - } + self.import_pending_key_package( + mls, + recipient, + &received_pkg.key_package_data, + )?; // Create session and send welcome message let welcome = { diff --git a/crates/offline-protocol/src/protocol/tests/mod.rs b/crates/offline-protocol/src/protocol/tests/mod.rs index 330b78bd..3b8436be 100644 --- a/crates/offline-protocol/src/protocol/tests/mod.rs +++ b/crates/offline-protocol/src/protocol/tests/mod.rs @@ -40217,6 +40217,231 @@ fn the_ratchet_survives_a_restart() { ); } +/// A peer's key package whose window this device's clock is outside of +/// blocks every session with that peer, and used to say so only in a debug +/// line: two offline phones a year apart sat on a pending connection request +/// with nothing to tell either user to check the date. It is now reported +/// under its own code, once per peer however often the session is retried. +#[test] +fn a_key_package_outside_its_window_by_our_clock_is_reported_once_per_peer() { + let mut alice = protocol_with_mls("alice"); + let warnings: Arc>> = Arc::new(Mutex::new(Vec::new())); + { + let sink = Arc::clone(&warnings); + alice.on_event(move |e| { + if let Event::SecurityWarning { + peer_id, + reason_code, + .. + } = e + { + sink.lock().unwrap().push((peer_id, reason_code)); + } + }); + } + + // Bob's clock runs a year ahead of Alice's. + let now = Utc::now().timestamp() as u64; + let year = 365 * 24 * 3600; + let bob = id("bob"); + let bytes = offline_protocol_mls::MlsManager::key_package_with_window_for_testing( + &bob, + now + year - 3600, + now + year + 30 * 24 * 3600, + ) + .unwrap(); + alice.pending_key_packages.insert( + bob.clone(), + ReceivedKeyPackage { + key_package_data: bytes, + local_expires_at_ms: u64::MAX, + }, + ); + + for _ in 0..3 { + let err = alice + .establish_secure_session(&bob) + .expect_err("no session can form from a package that is not valid yet"); + assert!( + matches!( + err, + Error::Mls( + offline_protocol_mls::MlsError::KeyPackageOutsideValidityWindow { + expired: false + } + ) + ), + "refused for the wrong reason: {err:?}" + ); + } + + let seen = warnings.lock().unwrap().clone(); + assert_eq!( + seen, + vec![( + bob.clone(), + SecurityWarningCode::KeyPackageOutsideValidityWindow + )], + "exactly one warning naming the peer, whatever the retry count" + ); + assert!( + alice.has_pending_key_package(&bob), + "a window that has not started becomes valid once the clocks agree, so \ + the package is kept" + ); +} + +/// Collects `(peer_id, code, reason)` for every security warning `protocol` +/// raises. +fn collect_security_warnings( + protocol: &mut OfflineProtocol, +) -> Arc>> { + let warnings = Arc::new(Mutex::new(Vec::new())); + let sink = Arc::clone(&warnings); + protocol.on_event(move |e| { + if let Event::SecurityWarning { + peer_id, + reason_code, + reason, + } = e + { + sink.lock().unwrap().push((peer_id, reason_code, reason)); + } + }); + warnings +} + +/// Places a key package for `peer` whose window runs from `not_before` to +/// `not_after` in `protocol`'s pending map, as a frame that arrived would. +fn insert_pending_key_package_with_window( + protocol: &mut OfflineProtocol, + peer: &str, + not_before: u64, + not_after: u64, +) { + let bytes = offline_protocol_mls::MlsManager::key_package_with_window_for_testing( + peer, not_before, not_after, + ) + .unwrap(); + let pkg = ReceivedKeyPackage { + key_package_data: bytes, + local_expires_at_ms: u64::MAX, + }; + protocol.persist_peer_key_package(peer, &pkg); + protocol.pending_key_packages.insert(peer.to_string(), pkg); +} + +/// The send path imports a pending package through its own route, and that +/// route is the one taken on the first send after a restart (the warning +/// throttle is in memory, the package is restored from storage) or whenever +/// `auto_key_exchange` is off. Reported only on the arrival route, the message +/// queued "anyway" and the app heard nothing: the failure the code exists for. +#[test] +fn a_send_reports_a_key_package_outside_its_window_once() { + let mut config = create_test_config_for_user("alice"); + config.encryption.auto_key_exchange = false; + // The stock default: a send to a peer with no session queues behind + // establishment and kicks it, rather than going out in the clear. + config.encryption.require_encryption = true; + config.encryption.store_pending = true; + let mut alice = OfflineProtocol::new(config).unwrap(); + alice + .initialize_mls_for_test(Arc::new(crate::mls::InMemoryStorage::new())) + .unwrap(); + alice.start().unwrap(); + let warnings = collect_security_warnings(&mut alice); + + let now = Utc::now().timestamp() as u64; + let year = 365 * 24 * 3600; + let bob = id("bob"); + insert_pending_key_package_with_window( + &mut alice, + &bob, + now + year - 3600, + now + year + 30 * 24 * 3600, + ); + + for _ in 0..3 { + alice + .send_message(&bob, "hello", None, None::) + .expect("the message queues behind establishment"); + } + + let seen: Vec<_> = warnings + .lock() + .unwrap() + .iter() + .map(|(peer, code, _)| (peer.clone(), *code)) + .collect(); + assert_eq!( + seen, + vec![( + bob.clone(), + SecurityWarningCode::KeyPackageOutsideValidityWindow + )], + "the send path reports the refusal, once however many sends retry it" + ); + assert!(alice.has_pending_key_package(&bob)); +} + +/// A window that has already closed is not necessarily a clock: a relay can +/// hold a package for days, and the cached expiry is anchored to when the +/// frame arrived. So it is reported without telling the user their clock is +/// wrong, and discarded like any expired package, rather than kept to fail +/// every attempt until the cached expiry. +#[test] +fn a_key_package_whose_window_closed_is_reported_and_discarded() { + let mut alice = protocol_with_mls("alice"); + let warnings = collect_security_warnings(&mut alice); + + let now = Utc::now().timestamp() as u64; + let day = 24 * 3600; + let bob = id("bob"); + insert_pending_key_package_with_window(&mut alice, &bob, now - 60 * day, now - 30 * day); + assert!( + alice.load_peer_key_package_from_storage(&bob).is_some(), + "negative control: the package starts out persisted" + ); + let err = alice + .establish_secure_session(&bob) + .expect_err("no session can form from a package whose window closed"); + assert!( + matches!( + err, + Error::Mls( + offline_protocol_mls::MlsError::KeyPackageOutsideValidityWindow { expired: true } + ) + ), + "refused for the wrong reason: {err:?}" + ); + + assert!( + !alice.has_pending_key_package(&bob), + "a closed window never reopens on a correct clock, so the package goes" + ); + assert!( + alice.load_peer_key_package_from_storage(&bob).is_none(), + "and its durable copy with it, or the next launch restores it" + ); + let seen = warnings.lock().unwrap().clone(); + assert_eq!(seen.len(), 1, "one warning: {seen:?}"); + let (peer, code, reason) = &seen[0]; + assert_eq!( + (peer, *code), + (&bob, SecurityWarningCode::KeyPackageOutsideValidityWindow) + ); + assert!( + reason.contains("older than its 30-day window"), + "the reason names the stale package, not only the clock: {reason}" + ); + + // Once gone, the next attempt is plain "not ready", not the same refusal. + assert!(matches!( + alice.establish_secure_session(&bob), + Err(Error::SessionNotReady(_)) + )); +} + /// The refusal is reported under its own code, so an integrator can tell a /// clock fault from the signature failures it would otherwise be filed under. #[test] diff --git a/docs/security/threat-model.md b/docs/security/threat-model.md index d93b39ec..abb46feb 100644 --- a/docs/security/threat-model.md +++ b/docs/security/threat-model.md @@ -393,7 +393,15 @@ takes its own control plane down, key package exchange included. **Mitigation:** the refusal is reported under its own `STALE_CONTROL_FRAME` code so that a run of them across many peers reads as a local fault rather than an attack, and `security.control_freshness_enforced` turns enforcement off without -a new binary. A leaf node is required to have a time source at pairing for this +a new binary. A key package's validity window is judged the same way and fails +the same way: it runs from an hour before minting to 30 days after, so a device +more than an hour behind its peer cannot start a session (the peer still can), +and past 30 days neither side can and no session forms. The refusal is +reported under `KEY_PACKAGE_OUTSIDE_VALIDITY_WINDOW`, once per peer, on every +route that admits a pending package, and the window itself is not relaxed. A +window that has already closed is not always a clock (a relay may have held the +package for days), so that package is discarded and the peer's next one +replaces it. A leaf node is required to have a time source at pairing for this among other reasons; see [leaf provisioning](../spec/leaf-provisioning.md). diff --git a/docs/spec/local-api.md b/docs/spec/local-api.md index 45beb92a..926008fa 100644 --- a/docs/spec/local-api.md +++ b/docs/spec/local-api.md @@ -716,7 +716,7 @@ server-side rule an implementation MAY add, and the reference server does. | `DorsReasonCode` | `INITIAL_SELECTION`, `PRIMARY_SELECTED`, `PRIMARY_SUCCESS`, `FALLBACK_SUCCESS`, `ESCALATION_APPLIED`, `CURRENT_UNAVAILABLE` | | `DorsEscalationPhase` | `TRIGGERED`, `APPLIED` | | `DorsEscalationReasonCode` | `FALLBACK_SUCCESS`, `RETRY_THRESHOLD`, `POOR_SIGNAL`, `CONGESTION`, `LOW_TTL`, `LOW_SUCCESS_RATE` | -| `SecurityWarningCode` | `SENDER_ADDRESS_MISMATCH`, `TRANSPORT_IDENTITY_MISMATCH`, `CONTROL_SIGNATURE_INVALID`, `UNSIGNED_CONTROL_REJECTED`, `MEDIA_SENDER_GROUP_MISMATCH`, `PLAINTEXT_SEND`, `PLAINTEXT_RECEIVE_REJECTED`, `SESSION_SENDER_GROUP_MISMATCH`, `SESSION_REKEY_TRIGGERED`, `NOSTR_KEY_PACKAGE_SLOT_EXHAUSTED`, `PUSH_KEY_PACKAGE_POOL_EXHAUSTED`, `RELAY_ADDRESS_BINDING_MISMATCH`, `RELAY_ADDRESS_DECLARATION_REFUSED`, `GROUP_LEAF_IDENTITY_UNPROVEN`, `STALE_CONTROL_FRAME`, `GATEWAY_ADDRESS_BINDING_MISMATCH`, `GATEWAY_ADDRESS_DECLARATION_REFUSED` | +| `SecurityWarningCode` | `SENDER_ADDRESS_MISMATCH`, `TRANSPORT_IDENTITY_MISMATCH`, `CONTROL_SIGNATURE_INVALID`, `UNSIGNED_CONTROL_REJECTED`, `MEDIA_SENDER_GROUP_MISMATCH`, `PLAINTEXT_SEND`, `PLAINTEXT_RECEIVE_REJECTED`, `SESSION_SENDER_GROUP_MISMATCH`, `SESSION_REKEY_TRIGGERED`, `NOSTR_KEY_PACKAGE_SLOT_EXHAUSTED`, `PUSH_KEY_PACKAGE_POOL_EXHAUSTED`, `RELAY_ADDRESS_BINDING_MISMATCH`, `RELAY_ADDRESS_DECLARATION_REFUSED`, `GROUP_LEAF_IDENTITY_UNPROVEN`, `STALE_CONTROL_FRAME`, `GATEWAY_ADDRESS_BINDING_MISMATCH`, `GATEWAY_ADDRESS_DECLARATION_REFUSED`, `KEY_PACKAGE_OUTSIDE_VALIDITY_WINDOW` | The transport name is spelled two ways by the engine, and a client accepts both. `message_received`, `message_delivered`, the DORS events and a