From 51d3e8713c6ec7a8c22f5375a1ed8810cadd104f Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Sun, 18 Oct 2026 08:59:51 -0400 Subject: [PATCH 1/7] Judge policy validity against a time floor the device clock cannot lower --- keep-mobile/src/clock_floor.rs | 186 ++++++++++++++++++++++ keep-mobile/src/lib.rs | 276 ++++++++++++++++++++++++++------- keep-mobile/src/persistence.rs | 40 ++++- keep-mobile/src/policy.rs | 23 +-- 4 files changed, 452 insertions(+), 73 deletions(-) create mode 100644 keep-mobile/src/clock_floor.rs diff --git a/keep-mobile/src/clock_floor.rs b/keep-mobile/src/clock_floor.rs new file mode 100644 index 00000000..8e8b36ed --- /dev/null +++ b/keep-mobile/src/clock_floor.rs @@ -0,0 +1,186 @@ +// SPDX-FileCopyrightText: © 2026 PrivKey LLC +// SPDX-License-Identifier: MIT + +//! A lower bound on the current time that the device's own clock cannot lower. +//! +//! The spending policy expires, and expiry is the state a warden stops renewing +//! into: past it every rule refuses. Deciding that from the wall clock alone +//! means the date setting decides whether the policy is in force, so setting +//! the clock back inside the validity window turns an expired policy back into +//! an enforced one, with no root and no prompt. +//! +//! What closes it is a time floor: the latest wall reading the app has ever +//! seen, kept in storage and consulted on every decision. Expiry is evaluated +//! at `max(now, floor)`, so winding the clock back leaves the floor in charge +//! and an expired policy stays expired. The floor can only ever make the +//! evaluated time later, which can expire a bundle but never un-expire one, so +//! every way it can be wrong refuses rather than permits. +//! +//! This works here and would not work against a record rollback: that needs +//! root or offline access to the data partition, which is also enough to +//! rewrite this record, whereas the date setting is not. +//! +//! The reason the floor is bounded rather than simply raised to whatever the +//! clock says is the failure [`crate::audit`]'s `monotonic_timestamp` is +//! written around. One reading from a clock set to 2099 would park the floor +//! there, and from then on every bundle is expired, including a replacement a +//! warden signs today, so the policy refuses forever with no way back. The +//! floor therefore advances only as far as monotonic time proves has elapsed: a +//! jump to 2099 moves it by seconds, and the poisoning is not available. +//! +//! The monotonic reading comes from the caller, as it already does for +//! [`crate::signing_policy::SigningRateLimiter`], because only the platform has +//! it. On Android it is `SystemClock.elapsedRealtime()`, which counts across +//! sleep and resets on reboot. + +use serde::{Deserialize, Serialize}; + +/// Slack allowed on top of the elapsed monotonic time when advancing the floor. +/// +/// Covers the gap between the two readings being taken and a second of +/// rounding, nothing more. It does not need to cover a legitimate forward +/// correction, such as a device whose battery died resyncing by NTP: the floor +/// is a lower bound, so lagging a real correction is harmless, and the +/// evaluated time is `max(now, floor)`, which is the corrected reading itself. +const ADVANCE_SLACK_SECS: u64 = 60; + +#[derive(Clone, Copy, Debug, Serialize, Deserialize)] +pub(crate) struct ClockFloor { + /// The latest wall-clock second the app accepts as already past. + pub(crate) floor_secs: u64, + /// The monotonic reading taken alongside `floor_secs`, for measuring how + /// much time has since been proven to pass. + pub(crate) anchor_elapsed_ms: u64, +} + +impl ClockFloor { + pub(crate) fn new(now_secs: u64, now_elapsed_ms: u64) -> Self { + Self { + floor_secs: now_secs, + anchor_elapsed_ms: now_elapsed_ms, + } + } + + /// The floor after observing `now_secs` alongside `now_elapsed_ms`, and + /// whether it changed enough to be worth storing. + /// + /// `None` for the monotonic reading means the caller had none, which is the + /// case at construction. The floor is then consulted and not advanced: + /// raising it needs proof that time passed, and reading it does not. + pub(crate) fn observe(self, now_secs: u64, now_elapsed_ms: Option) -> (Self, bool) { + let Some(now_elapsed_ms) = now_elapsed_ms else { + return (self, false); + }; + + // A monotonic reading below the anchor means the device rebooted, since + // the counter restarts there, so nothing can be proven about the gap. + // The anchor is moved to the new reading and the floor is left alone. + // That is safe because a stale floor is still a lower bound on real + // time: when the clock reads later than the floor, the clock is used. + // The cost is that the floor lags by however long the device was off, + // so a rewind that far back is not detected. + if now_elapsed_ms < self.anchor_elapsed_ms { + return ( + Self { + floor_secs: self.floor_secs, + anchor_elapsed_ms: now_elapsed_ms, + }, + true, + ); + } + + let proven_secs = (now_elapsed_ms - self.anchor_elapsed_ms) / 1_000; + let ceiling = self + .floor_secs + .saturating_add(proven_secs) + .saturating_add(ADVANCE_SLACK_SECS); + let advanced = Self { + floor_secs: self.floor_secs.max(now_secs.min(ceiling)), + anchor_elapsed_ms: now_elapsed_ms, + }; + let changed = advanced.floor_secs != self.floor_secs + || advanced.anchor_elapsed_ms != self.anchor_elapsed_ms; + (advanced, changed) + } + + /// The time to evaluate validity at: never earlier than the floor. + pub(crate) fn effective_secs(&self, now_secs: u64) -> u64 { + now_secs.max(self.floor_secs) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + const MIN: u64 = 60; + + #[test] + fn winding_the_clock_back_does_not_move_the_evaluated_time_back() { + let floor = ClockFloor::new(1_000_000, 0); + let (floor, _) = floor.observe(1_000_000 + 10 * MIN, Some(10 * MIN * 1_000)); + + // The attack: the date is set back a week. The floor is what validity + // is judged against, so an expired policy stays expired. + assert_eq!( + floor.effective_secs(1_000_000 - 7 * 24 * 3600), + 1_000_000 + 10 * MIN + ); + } + + #[test] + fn a_clock_set_far_forward_cannot_park_the_floor_there() { + let floor = ClockFloor::new(1_000_000, 0); + + // One reading from a clock set decades ahead, a second of monotonic + // time after the anchor. + let (poisoned, _) = floor.observe(4_000_000_000, Some(1_000)); + + // The floor moved by the proven second plus the slack, not by decades, + // so a policy signed today is still inside its window afterwards. + assert!( + poisoned.floor_secs <= 1_000_000 + 1 + ADVANCE_SLACK_SECS, + "floor advanced to {}", + poisoned.floor_secs + ); + } + + #[test] + fn the_floor_tracks_time_that_monotonic_proves_passed() { + let floor = ClockFloor::new(1_000_000, 5_000); + let (floor, changed) = floor.observe(1_000_000 + 30 * MIN, Some(5_000 + 30 * MIN * 1_000)); + + assert!(changed); + assert_eq!(floor.floor_secs, 1_000_000 + 30 * MIN); + assert_eq!(floor.anchor_elapsed_ms, 5_000 + 30 * MIN * 1_000); + } + + #[test] + fn a_reboot_re_anchors_without_advancing_the_floor() { + let floor = ClockFloor::new(1_000_000, 9_000_000); + let (floor, changed) = floor.observe(1_000_000 + 24 * 3600, Some(12_000)); + + assert!(changed, "the anchor has to follow the restarted counter"); + assert_eq!( + floor.floor_secs, 1_000_000, + "nothing proves the gap, so the floor cannot advance" + ); + assert_eq!(floor.anchor_elapsed_ms, 12_000); + // Still usable: the clock reads later than the floor, so the clock wins. + assert_eq!( + floor.effective_secs(1_000_000 + 24 * 3600), + 1_000_000 + 24 * 3600 + ); + } + + #[test] + fn no_monotonic_reading_consults_the_floor_without_advancing_it() { + let floor = ClockFloor::new(1_000_000, 4_000); + let (same, changed) = floor.observe(1_000_000 + 3600, None); + + assert!(!changed); + assert_eq!(same.floor_secs, 1_000_000); + assert_eq!(same.anchor_elapsed_ms, 4_000); + assert_eq!(same.effective_secs(999_000), 1_000_000); + } +} diff --git a/keep-mobile/src/lib.rs b/keep-mobile/src/lib.rs index e59c8562..e87d1e29 100644 --- a/keep-mobile/src/lib.rs +++ b/keep-mobile/src/lib.rs @@ -5,6 +5,7 @@ mod approver; mod audit; +mod clock_floor; mod dkg; mod error; mod network; @@ -349,6 +350,7 @@ fn lock_pre_approved( const POLICY_STORAGE_KEY: &str = "__keep_policy_v1"; const VELOCITY_STORAGE_KEY: &str = "__keep_velocity_v1"; const TRUSTED_WARDENS_KEY: &str = "__keep_trusted_wardens_v1"; +const CLOCK_FLOOR_STORAGE_KEY: &str = "__keep_clock_floor_v1"; const CERT_PINS_STORAGE_KEY: &str = "__keep_cert_pins_v1"; const DESCRIPTOR_INDEX_KEY: &str = "__keep_descriptor_index_v1"; const DESCRIPTOR_KEY_PREFIX: &str = "__keep_descriptor_"; @@ -832,7 +834,20 @@ impl KeepMobile { let velocity = Arc::new(std::sync::Mutex::new(velocity)); let mut evaluator = PolicyEvaluator::new(velocity.clone()); - Self::load_policy_into(&storage, &mut evaluator); + // The floor is consulted and not advanced here: no monotonic reading is + // available at construction, and advancing needs one. A floor that + // cannot be read faults the policy instead of failing construction, + // because refusing to open the app is not better than refusing every + // spend, and the fault is retried before each decision. + match Self::startup_policy_now(&storage) { + Ok(now_secs) => Self::load_policy_into(&storage, &mut evaluator, now_secs), + Err(_) => { + tracing::error!( + "Time floor unreadable, every policy rule will refuse until it is read" + ); + evaluator.set_policy_faulted(PolicyFault::Unreadable, None); + } + } let policy = Arc::new(std::sync::RwLock::new(evaluator)); @@ -1835,11 +1850,15 @@ impl KeepMobile { let bundle = PolicyBundle::from_bytes(&bundle_bytes)?; bundle.verify_signature()?; bundle.verify_hash()?; - bundle.verify_timestamp()?; + // Judged against the floor, not the raw clock, so a bundle that is + // already expired cannot be installed by winding the date back to + // inside its window. + let now_secs = self.policy_now(None)?; + bundle.verify_timestamp_at(now_secs)?; // A transient fault must not push the caller into discarding a // warden-signed policy, so the record gets another read first. - self.reload_policy_if_transient()?; + self.reload_policy_if_transient(None)?; let mut policy = self.policy_write_lock()?; // Refused when the stored record cannot say how old it is, rather than @@ -1898,8 +1917,13 @@ impl KeepMobile { /// A faulted record refuses every spend, so reporting it as no policy would /// leave a caller unable to explain the refusals. Retries an unreadable /// record first, so a transient read fault is not reported as a fault. - pub fn policy_status(&self) -> Result { - self.reload_policy_if_transient()?; + /// `now_elapsed_ms` is the platform's monotonic clock, as + /// `SigningRateLimiter::check_and_record` already takes: on Android, + /// `SystemClock.elapsedRealtime()`. It is what lets the stored time floor + /// advance, so passing a stale or invented value holds the floor still + /// rather than moving it anywhere useful. + pub fn policy_status(&self, now_elapsed_ms: u64) -> Result { + self.reload_policy_if_transient(Some(now_elapsed_ms))?; Ok(self.policy_read_lock()?.status()) } @@ -1921,12 +1945,15 @@ impl KeepMobile { Ok(()) } + /// `now_elapsed_ms` is the platform's monotonic clock; see + /// [`Self::policy_status`]. pub fn evaluate_policy( &self, ctx: TransactionContext, + now_elapsed_ms: u64, ) -> Result { self.reload_velocity_if_unknown()?; - self.reload_policy_if_transient()?; + self.reload_policy_if_transient(Some(now_elapsed_ms))?; self.policy_read_lock()?.evaluate(&ctx) } @@ -3540,6 +3567,7 @@ impl KeepMobile { pubkey: [u8; POLICY_PUBKEY_LEN], trust: bool, ) -> Result<(), KeepMobileError> { + let now_secs = self.policy_now(None)?; let mut policy = self.policy_write_lock()?; if policy.trusted_wardens().contains(&pubkey) == trust { return Ok(()); @@ -3552,7 +3580,7 @@ impl KeepMobile { } persistence::persist_trusted_wardens(&self.storage, &wardens)?; policy.set_trusted_wardens(wardens); - Self::load_policy_into(&self.storage, &mut policy); + Self::load_policy_into(&self.storage, &mut policy, now_secs); Ok(()) } @@ -4064,7 +4092,52 @@ impl KeepMobile { /// warden was trusted, so an empty set with a bundle present means the /// record was lost, which is exactly the case that must not read as "no /// policy". - fn load_policy_into(storage: &Arc, evaluator: &mut PolicyEvaluator) { + /// The time to judge policy validity at, never earlier than the floor. + /// + /// `now_elapsed_ms` is the platform's monotonic reading, as + /// [`SigningRateLimiter::check_and_record`] already takes. `None` means the + /// caller has none, which consults the floor without advancing it: raising + /// it needs proof that time passed, and reading it does not. + /// + /// A floor that cannot be read is an error, not an absence. Nothing then + /// says the clock was not wound back, and the callers turn that into a + /// refusal, because the alternative is trusting the one input this exists + /// to distrust. + /// + /// [`SigningRateLimiter::check_and_record`]: crate::SigningRateLimiter::check_and_record + fn policy_now(&self, now_elapsed_ms: Option) -> Result { + Self::floor_now(&self.storage, now_elapsed_ms) + } + + /// [`Self::policy_now`] before there is a `self`, for the load the + /// constructor does. + fn startup_policy_now(storage: &Arc) -> Result { + Self::floor_now(storage, None) + } + + fn floor_now( + storage: &Arc, + now_elapsed_ms: Option, + ) -> Result { + let now_secs = u64::try_from(chrono::Utc::now().timestamp()).unwrap_or(0); + let stored = persistence::load_clock_floor(storage)?; + let floor = stored + .unwrap_or_else(|| clock_floor::ClockFloor::new(now_secs, now_elapsed_ms.unwrap_or(0))); + let (observed, changed) = floor.observe(now_secs, now_elapsed_ms); + // A first write counts as changed, so the floor exists from here on. + if changed || stored.is_none() { + persistence::persist_clock_floor(storage, &observed)?; + } + // Back to the signed seconds the validity checks take. The advance is + // bounded, so the floor cannot reach the saturating case. + Ok(i64::try_from(observed.effective_secs(now_secs)).unwrap_or(i64::MAX)) + } + + fn load_policy_into( + storage: &Arc, + evaluator: &mut PolicyEvaluator, + now_secs: i64, + ) { match persistence::load_trusted_wardens(storage) { Ok(wardens) => evaluator.set_trusted_wardens(wardens), Err(KeepMobileError::StorageNotFound) => { @@ -4132,7 +4205,7 @@ impl KeepMobile { return; } - match bundle.timestamp_fault() { + match bundle.timestamp_fault_at(now_secs) { None => evaluator.set_policy(bundle), Some(fault) => { tracing::error!( @@ -4152,7 +4225,10 @@ impl KeepMobile { /// retrying it would be two storage reads per decision for nothing; that /// one changes when the trusted wardens do, and both warden mutations /// re-run the load themselves. - fn reload_policy_if_transient(&self) -> Result<(), KeepMobileError> { + fn reload_policy_if_transient( + &self, + now_elapsed_ms: Option, + ) -> Result<(), KeepMobileError> { // Bound to a local first. Folding this into the `if` condition, or into // a `match`, keeps the read guard alive across `policy_write_lock` and // self-deadlocks on a lock that is not reentrant. @@ -4160,8 +4236,9 @@ impl KeepMobile { if !fault.is_some_and(PolicyFault::is_transient) { return Ok(()); } + let now_secs = self.policy_now(now_elapsed_ms)?; let mut policy = self.policy_write_lock()?; - Self::load_policy_into(&self.storage, &mut policy); + Self::load_policy_into(&self.storage, &mut policy, now_secs); Ok(()) } @@ -7067,12 +7144,15 @@ mod velocity_fault_tests { storage.heal(); mobile - .evaluate_policy(TransactionContext { - amount_sats: 1, - fee_sats: 1, - destinations: vec![], - has_unknown_destination: false, - }) + .evaluate_policy( + TransactionContext { + amount_sats: 1, + fee_sats: 1, + destinations: vec![], + has_unknown_destination: false, + }, + 0, + ) .unwrap(); assert_eq!( @@ -7147,13 +7227,13 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); - let PolicyDecision::Deny { reason } = mobile.evaluate_policy(a_spend()).unwrap() else { + let PolicyDecision::Deny { reason } = mobile.evaluate_policy(a_spend(), 0).unwrap() else { panic!("a tampered policy record must refuse, not allow everything"); }; assert!(reason.contains("policy"), "{reason}"); assert!( matches!( - mobile.policy_status().unwrap(), + mobile.policy_status(0).unwrap(), PolicyStatus::Faulted { .. } ), "the refusal has to be explainable, so it must not report as unconfigured" @@ -7170,11 +7250,11 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status().unwrap(), + mobile.policy_status(0).unwrap(), PolicyStatus::Active { .. } )); assert_eq!( - mobile.evaluate_policy(a_spend()).unwrap(), + mobile.evaluate_policy(a_spend(), 0).unwrap(), PolicyDecision::Allow ); let over = TransactionContext { @@ -7182,7 +7262,7 @@ mod velocity_fault_tests { ..a_spend() }; assert!(matches!( - mobile.evaluate_policy(over).unwrap(), + mobile.evaluate_policy(over, 0).unwrap(), PolicyDecision::Deny { .. } )); } @@ -7193,11 +7273,11 @@ mod velocity_fault_tests { let storage = Arc::new(FaultingStorage::default()); let mobile = KeepMobile::new(storage as Arc).unwrap(); assert!(matches!( - mobile.policy_status().unwrap(), + mobile.policy_status(0).unwrap(), PolicyStatus::NotConfigured )); assert_eq!( - mobile.evaluate_policy(a_spend()).unwrap(), + mobile.evaluate_policy(a_spend(), 0).unwrap(), PolicyDecision::Allow ); } @@ -7213,23 +7293,23 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status().unwrap(), + mobile.policy_status(0).unwrap(), PolicyStatus::Faulted { fault: PolicyFault::Unreadable } )); assert!(matches!( - mobile.evaluate_policy(a_spend()).unwrap(), + mobile.evaluate_policy(a_spend(), 0).unwrap(), PolicyDecision::Deny { .. } )); storage.heal(); assert!(matches!( - mobile.policy_status().unwrap(), + mobile.policy_status(0).unwrap(), PolicyStatus::Active { .. } )); assert_eq!( - mobile.evaluate_policy(a_spend()).unwrap(), + mobile.evaluate_policy(a_spend(), 0).unwrap(), PolicyDecision::Allow ); } @@ -7244,13 +7324,13 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status().unwrap(), + mobile.policy_status(0).unwrap(), PolicyStatus::Faulted { fault: PolicyFault::Unverified } )); assert!(matches!( - mobile.evaluate_policy(a_spend()).unwrap(), + mobile.evaluate_policy(a_spend(), 0).unwrap(), PolicyDecision::Deny { .. } )); } @@ -7265,14 +7345,17 @@ mod velocity_fault_tests { let warden = plant_policy(&storage, &capped_rules(), false); let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status().unwrap(), + mobile.policy_status(0).unwrap(), PolicyStatus::Faulted { .. } )); mobile.add_trusted_warden(hex::encode(&warden)).unwrap(); assert!( - matches!(mobile.policy_status().unwrap(), PolicyStatus::Active { .. }), + matches!( + mobile.policy_status(0).unwrap(), + PolicyStatus::Active { .. } + ), "trusting the signer must re-run the stored policy, not leave it faulted" ); let over = TransactionContext { @@ -7280,7 +7363,7 @@ mod velocity_fault_tests { ..a_spend() }; assert!(matches!( - mobile.evaluate_policy(over).unwrap(), + mobile.evaluate_policy(over, 0).unwrap(), PolicyDecision::Deny { .. } )); } @@ -7310,7 +7393,7 @@ mod velocity_fault_tests { mobile.delete_policy().unwrap(); assert!(matches!( - mobile.policy_status().unwrap(), + mobile.policy_status(0).unwrap(), PolicyStatus::NotConfigured )); } @@ -7324,13 +7407,13 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status().unwrap(), + mobile.policy_status(0).unwrap(), PolicyStatus::Faulted { fault: PolicyFault::Unverified } )); assert!(matches!( - mobile.evaluate_policy(a_spend()).unwrap(), + mobile.evaluate_policy(a_spend(), 0).unwrap(), PolicyDecision::Deny { .. } )); } @@ -7345,7 +7428,7 @@ mod velocity_fault_tests { let warden = plant_policy(&storage, &capped_rules(), false); let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status().unwrap(), + mobile.policy_status(0).unwrap(), PolicyStatus::Faulted { .. } )); @@ -7353,7 +7436,10 @@ mod velocity_fault_tests { mobile.add_trusted_warden(hex::encode(&warden)).unwrap(); assert!( - matches!(mobile.policy_status().unwrap(), PolicyStatus::Active { .. }), + matches!( + mobile.policy_status(0).unwrap(), + PolicyStatus::Active { .. } + ), "the warden just added must not be dropped by the reload that follows it" ); assert_eq!( @@ -7377,7 +7463,7 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status().unwrap(), + mobile.policy_status(0).unwrap(), PolicyStatus::Faulted { fault: PolicyFault::Unverified } @@ -7388,7 +7474,7 @@ mod velocity_fault_tests { }; assert!( matches!( - mobile.evaluate_policy(over).unwrap(), + mobile.evaluate_policy(over, 0).unwrap(), PolicyDecision::Deny { .. } ), "the raised cap must not take effect, and the record must not read as no policy" @@ -7406,7 +7492,7 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status().unwrap(), + mobile.policy_status(0).unwrap(), PolicyStatus::Faulted { fault: PolicyFault::Unverified } @@ -7414,7 +7500,7 @@ mod velocity_fault_tests { let after_load = storage.reads_of(POLICY_STORAGE_KEY); for _ in 0..5 { assert!(matches!( - mobile.evaluate_policy(a_spend()).unwrap(), + mobile.evaluate_policy(a_spend(), 0).unwrap(), PolicyDecision::Deny { .. } )); } @@ -7442,18 +7528,18 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status().unwrap(), + mobile.policy_status(0).unwrap(), PolicyStatus::Faulted { fault: PolicyFault::NotYetValid } )); assert!(matches!( - mobile.evaluate_policy(a_spend()).unwrap(), + mobile.evaluate_policy(a_spend(), 0).unwrap(), PolicyDecision::Deny { .. } )); let after_load = storage.reads_of(POLICY_STORAGE_KEY); - mobile.evaluate_policy(a_spend()).unwrap(); + mobile.evaluate_policy(a_spend(), 0).unwrap(); assert!( storage.reads_of(POLICY_STORAGE_KEY) > after_load, "a fault that resolves on its own has to be looked at again" @@ -7476,14 +7562,14 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status().unwrap(), + mobile.policy_status(0).unwrap(), PolicyStatus::Faulted { fault: PolicyFault::Expired } )); assert!( matches!( - mobile.evaluate_policy(a_spend()).unwrap(), + mobile.evaluate_policy(a_spend(), 0).unwrap(), PolicyDecision::Deny { .. } ), "stale rules are neither enforced nor ignored" @@ -7498,7 +7584,7 @@ mod velocity_fault_tests { .expect("a replacement must install over an expired policy"); assert!(matches!( - mobile.policy_status().unwrap(), + mobile.policy_status(0).unwrap(), PolicyStatus::Active { .. } )); } @@ -7517,7 +7603,7 @@ mod velocity_fault_tests { assert!(mobile.delete_policy().is_err()); assert!( matches!( - mobile.policy_status().unwrap(), + mobile.policy_status(0).unwrap(), PolicyStatus::Faulted { .. } ), "a failed delete must not leave memory reading as no policy" @@ -7546,7 +7632,7 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status().unwrap(), + mobile.policy_status(0).unwrap(), PolicyStatus::Faulted { fault: PolicyFault::Unreadable } @@ -7558,7 +7644,7 @@ mod velocity_fault_tests { .import_policy(hex::encode(replacement.to_bytes())) .expect("a healed fault must not force a delete"); assert!(matches!( - mobile.policy_status().unwrap(), + mobile.policy_status(0).unwrap(), PolicyStatus::Active { .. } )); } @@ -7584,7 +7670,7 @@ mod velocity_fault_tests { ); let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status().unwrap(), + mobile.policy_status(0).unwrap(), PolicyStatus::Faulted { fault: PolicyFault::Expired } @@ -7599,11 +7685,11 @@ mod velocity_fault_tests { // The refusal has to outlive the failed write, or the gate is open for // the life of the process on a policy storage never accepted. assert!(matches!( - mobile.policy_status().unwrap(), + mobile.policy_status(0).unwrap(), PolicyStatus::Faulted { .. } )); assert!(matches!( - mobile.evaluate_policy(a_spend()).unwrap(), + mobile.evaluate_policy(a_spend(), 0).unwrap(), PolicyDecision::Deny { .. } )); } @@ -7628,7 +7714,7 @@ mod velocity_fault_tests { ); let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status().unwrap(), + mobile.policy_status(0).unwrap(), PolicyStatus::Active { .. } )); @@ -7661,7 +7747,7 @@ mod velocity_fault_tests { "memory must not hold a warden storage refused" ); assert!(matches!( - mobile.policy_status().unwrap(), + mobile.policy_status(0).unwrap(), PolicyStatus::Faulted { .. } )); } @@ -7690,7 +7776,7 @@ mod velocity_fault_tests { ); let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status().unwrap(), + mobile.policy_status(0).unwrap(), PolicyStatus::Faulted { fault: PolicyFault::NotYetValid } @@ -7738,7 +7824,7 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status().unwrap(), + mobile.policy_status(0).unwrap(), PolicyStatus::Faulted { fault: PolicyFault::Unverified } @@ -7771,7 +7857,7 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status().unwrap(), + mobile.policy_status(0).unwrap(), PolicyStatus::Faulted { fault: PolicyFault::Unverified } @@ -7814,8 +7900,78 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.evaluate_policy(a_spend()).unwrap(), + mobile.evaluate_policy(a_spend(), 0).unwrap(), PolicyDecision::Deny { .. } )); } + + /// Winding the device clock back must not put an expired policy back in + /// force. The stored floor is the time already known to have passed, so + /// validity is judged against it rather than against the date setting. + /// + /// The attack cannot be staged by moving the real clock under test, so it + /// is staged from the other side: the bundle is dated now, and the floor + /// says a time past its window has already been observed, which is the same + /// relation as a bundle whose window closed while the clock was later and + /// has now been wound back inside it. + #[test] + fn a_clock_wound_back_does_not_revive_an_expired_policy() { + let now = Utc::now().timestamp() as u64; + let storage = Arc::new(FaultingStorage::default()); + plant_policy(&storage, &capped_rules(), true); + // Past the thirty-day window, and the anchor is ahead of the reading + // the test passes in, so the reload takes the reboot path and leaves + // the floor where it was planted. + storage.plant( + CLOCK_FLOOR_STORAGE_KEY, + serde_json::to_vec(&crate::clock_floor::ClockFloor { + floor_secs: now + 40 * 24 * 3600, + anchor_elapsed_ms: 5_000, + }) + .unwrap(), + ); + + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + assert!( + matches!( + mobile.policy_status(0).unwrap(), + PolicyStatus::Faulted { + fault: PolicyFault::Expired + } + ), + "a bundle whose window closed against the floor must stay expired" + ); + assert!(matches!( + mobile.evaluate_policy(a_spend(), 0).unwrap(), + PolicyDecision::Deny { .. } + )); + + // And it cannot be reinstalled by the same wound-back clock: the import + // is judged against the floor too. + let bundle = create_test_bundle(&capped_rules()); + mobile + .import_policy(hex::encode(bundle.to_bytes())) + .expect_err("an import judged against the floor is expired too"); + } + + /// A floor that cannot be read leaves the policy refusing rather than + /// enforcing, because nothing then says the clock was not wound back. + #[test] + fn an_unreadable_floor_refuses_every_rule() { + let storage = Arc::new(FaultingStorage::default()); + plant_policy(&storage, &capped_rules(), true); + storage.plant(CLOCK_FLOOR_STORAGE_KEY, b"not json".to_vec()); + + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + assert!(matches!( + mobile.policy_status(0).unwrap_or(PolicyStatus::Faulted { + fault: PolicyFault::Unreadable + }), + PolicyStatus::Faulted { .. } + )); + assert!(mobile + .evaluate_policy(a_spend(), 0) + .map(|d| matches!(d, PolicyDecision::Deny { .. })) + .unwrap_or(true)); + } } diff --git a/keep-mobile/src/persistence.rs b/keep-mobile/src/persistence.rs index a048e5a7..061cd34e 100644 --- a/keep-mobile/src/persistence.rs +++ b/keep-mobile/src/persistence.rs @@ -7,14 +7,16 @@ use std::sync::Arc; use serde::{Deserialize, Serialize}; use zeroize::Zeroizing; +use crate::clock_floor::ClockFloor; use crate::error::KeepMobileError; use crate::policy::{PolicyBundle, POLICY_MAX_RULES_LEN, POLICY_PUBKEY_LEN, POLICY_VERSION}; use crate::storage::{SecureStorage, ShareMetadataInfo}; use crate::types::{DeviceRegistrationInfo, KeyHealthStatusInfo, WalletDescriptorInfo}; use crate::velocity::{VelocityTracker, MAX_TRACKED_TRANSACTIONS}; use crate::{ - CERT_PINS_STORAGE_KEY, DESCRIPTOR_INDEX_KEY, DESCRIPTOR_KEY_PREFIX, HEALTH_STATUS_INDEX_KEY, - HEALTH_STATUS_KEY_PREFIX, POLICY_STORAGE_KEY, TRUSTED_WARDENS_KEY, VELOCITY_STORAGE_KEY, + CERT_PINS_STORAGE_KEY, CLOCK_FLOOR_STORAGE_KEY, DESCRIPTOR_INDEX_KEY, DESCRIPTOR_KEY_PREFIX, + HEALTH_STATUS_INDEX_KEY, HEALTH_STATUS_KEY_PREFIX, POLICY_STORAGE_KEY, TRUSTED_WARDENS_KEY, + VELOCITY_STORAGE_KEY, }; #[derive(Serialize, Deserialize, Default)] @@ -115,6 +117,40 @@ pub(crate) fn persist_policy( /// at a generous 128 bytes per entry against a worst case nearer 75. const VELOCITY_RECORD_MAX_LEN: usize = 128 * MAX_TRACKED_TRANSACTIONS + 1024; +/// Reads the stored time floor, or `None` when none has been written yet. +/// +/// A read failure is an error rather than an absence, and the caller refuses on +/// it, for the reason the whole module exists: without the floor there is +/// nothing to say the clock was not wound back. +pub(crate) fn load_clock_floor( + storage: &Arc, +) -> Result, KeepMobileError> { + match storage.load_share_by_key(CLOCK_FLOOR_STORAGE_KEY.into()) { + Ok(data) => serde_json::from_slice(&data) + .map(Some) + // Without serde's message, which quotes the value it tripped on. + .map_err(|_| KeepMobileError::StorageError { + msg: "Failed to deserialize the clock floor".into(), + }), + Err(KeepMobileError::StorageNotFound) => Ok(None), + Err(e) => Err(e), + } +} + +pub(crate) fn persist_clock_floor( + storage: &Arc, + floor: &ClockFloor, +) -> Result<(), KeepMobileError> { + let data = serde_json::to_vec(floor).map_err(|e| KeepMobileError::StorageError { + msg: format!("Failed to serialize the clock floor: {e}"), + })?; + storage.store_share_by_key( + CLOCK_FLOOR_STORAGE_KEY.into(), + data, + storage_metadata("clock_floor"), + ) +} + pub(crate) fn load_velocity( storage: &Arc, ) -> Result { diff --git a/keep-mobile/src/policy.rs b/keep-mobile/src/policy.rs index 65e4e904..0f444ffc 100644 --- a/keep-mobile/src/policy.rs +++ b/keep-mobile/src/policy.rs @@ -466,11 +466,10 @@ impl PolicyBundle { /// Reported apart from [`Self::verify_timestamp`]'s single error because /// the two call for opposite handling: being dated ahead of the clock /// resolves by itself, expiring does not. - pub fn timestamp_fault(&self) -> Option { - self.timestamp_fault_at(Utc::now().timestamp()) - } - - /// The clock is a parameter so the bounds can be tested at an exact instant + /// The clock is a parameter, not read here, and that is what keeps the + /// stored time floor on the path: a bundle's validity is judged at the time + /// the caller establishes, so no call site can fall back to the raw device + /// clock by accident. It also lets the bounds be tested at an exact instant /// either side of them, including a negative one, which `Utc::now` cannot /// be made to return. /// @@ -480,7 +479,7 @@ impl PolicyBundle { /// the arm that is never retried. Clamping sends it to `NotYetValid` /// instead, which is both true of a pre-1970 clock and retried once the /// clock is set. - fn timestamp_fault_at(&self, now_secs: i64) -> Option { + pub(crate) fn timestamp_fault_at(&self, now_secs: i64) -> Option { let now = u64::try_from(now_secs).unwrap_or(0); if self.created_at > now.saturating_add(MAX_POLICY_FUTURE_SECS) { @@ -494,8 +493,10 @@ impl PolicyBundle { None } - pub fn verify_timestamp(&self) -> Result<(), KeepMobileError> { - match self.timestamp_fault() { + /// Judged at the instant the caller establishes, so an import is measured + /// against the stored time floor rather than the raw clock. + pub(crate) fn verify_timestamp_at(&self, now_secs: i64) -> Result<(), KeepMobileError> { + match self.timestamp_fault_at(now_secs) { None => Ok(()), Some(fault) => Err(KeepMobileError::InvalidPolicy { msg: fault.to_string(), @@ -1169,13 +1170,13 @@ mod tests { let rules = PolicyRules::default(); let mut bundle = create_test_bundle(&rules); - assert!(bundle.verify_timestamp().is_ok()); + assert!(bundle.verify_timestamp_at(Utc::now().timestamp()).is_ok()); bundle.created_at = Utc::now().timestamp() as u64 + MAX_POLICY_FUTURE_SECS + 3600; - assert!(bundle.verify_timestamp().is_err()); + assert!(bundle.verify_timestamp_at(Utc::now().timestamp()).is_err()); bundle.created_at = Utc::now().timestamp() as u64 - MAX_POLICY_AGE_SECS - 3600; - assert!(bundle.verify_timestamp().is_err()); + assert!(bundle.verify_timestamp_at(Utc::now().timestamp()).is_err()); } #[test] From b3146b9812eb5305226c5434089b3d6b3de18405 Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Sun, 18 Oct 2026 09:15:20 -0400 Subject: [PATCH 2/7] Advance the floor on every decision and judge expiry when the decision is made --- keep-mobile/src/clock_floor.rs | 237 ++++++++++++++++++++++----------- keep-mobile/src/lib.rs | 190 ++++++++++++++++++++------ keep-mobile/src/persistence.rs | 8 ++ keep-mobile/src/policy.rs | 175 +++++++++++++++++++----- 4 files changed, 457 insertions(+), 153 deletions(-) diff --git a/keep-mobile/src/clock_floor.rs b/keep-mobile/src/clock_floor.rs index 8e8b36ed..7df949cc 100644 --- a/keep-mobile/src/clock_floor.rs +++ b/keep-mobile/src/clock_floor.rs @@ -9,24 +9,33 @@ //! the clock back inside the validity window turns an expired policy back into //! an enforced one, with no root and no prompt. //! -//! What closes it is a time floor: the latest wall reading the app has ever -//! seen, kept in storage and consulted on every decision. Expiry is evaluated -//! at `max(now, floor)`, so winding the clock back leaves the floor in charge -//! and an expired policy stays expired. The floor can only ever make the -//! evaluated time later, which can expire a bundle but never un-expire one, so -//! every way it can be wrong refuses rather than permits. +//! What closes it is a time floor: the latest wall reading the app has seen, +//! kept in storage and consulted on every decision. Expiry is judged at +//! `max(now, floor)`, so winding the clock back leaves the floor in charge and +//! an expired policy stays expired. //! //! This works here and would not work against a record rollback: that needs //! root or offline access to the data partition, which is also enough to //! rewrite this record, whereas the date setting is not. //! +//! The floor applies to expiry only, not to the not-yet-valid bound. A floor +//! ahead of the real clock makes a bundle look older, which can only expire +//! one; applied to the future bound it would make a future-dated bundle look +//! current, which would un-refuse it. So [`PolicyClock`] carries both readings +//! and each bound uses the one that can only refuse. +//! //! The reason the floor is bounded rather than simply raised to whatever the //! clock says is the failure [`crate::audit`]'s `monotonic_timestamp` is -//! written around. One reading from a clock set to 2099 would park the floor +//! written around. One reading from a clock set far ahead would park the floor //! there, and from then on every bundle is expired, including a replacement a //! warden signs today, so the policy refuses forever with no way back. The -//! floor therefore advances only as far as monotonic time proves has elapsed: a -//! jump to 2099 moves it by seconds, and the poisoning is not available. +//! floor therefore advances only by as much as monotonic time proves has +//! elapsed since the reading it was taken with. +//! +//! That proof is why there is no slack: a tolerance granted per observation is +//! granted again on the next one, so a caller that asks repeatedly walks the +//! floor forward at the tolerance per call regardless of how little time +//! passed. Advancing strictly by proven elapsed time has no such rate. //! //! The monotonic reading comes from the caller, as it already does for //! [`crate::signing_policy::SigningRateLimiter`], because only the platform has @@ -35,14 +44,14 @@ use serde::{Deserialize, Serialize}; -/// Slack allowed on top of the elapsed monotonic time when advancing the floor. -/// -/// Covers the gap between the two readings being taken and a second of -/// rounding, nothing more. It does not need to cover a legitimate forward -/// correction, such as a device whose battery died resyncing by NTP: the floor -/// is a lower bound, so lagging a real correction is harmless, and the -/// evaluated time is `max(now, floor)`, which is the corrected reading itself. -const ADVANCE_SLACK_SECS: u64 = 60; +/// The two readings a validity decision needs. +#[derive(Clone, Copy, Debug)] +pub(crate) struct PolicyClock { + /// What the device clock says, used for the not-yet-valid bound. + pub(crate) clock_secs: i64, + /// Never earlier than the stored floor, used for the expiry bound. + pub(crate) effective_secs: i64, +} #[derive(Clone, Copy, Debug, Serialize, Deserialize)] pub(crate) struct ClockFloor { @@ -50,11 +59,17 @@ pub(crate) struct ClockFloor { pub(crate) floor_secs: u64, /// The monotonic reading taken alongside `floor_secs`, for measuring how /// much time has since been proven to pass. - pub(crate) anchor_elapsed_ms: u64, + /// + /// `None` when the floor was written without one, which is the case at + /// construction. An advance needs a reading to measure from, so the first + /// one only anchors. Storing a zero instead would claim the floor was taken + /// at boot, and the next reading would then credit the whole device uptime + /// as elapsed and advance the floor by it. + pub(crate) anchor_elapsed_ms: Option, } impl ClockFloor { - pub(crate) fn new(now_secs: u64, now_elapsed_ms: u64) -> Self { + pub(crate) fn new(now_secs: u64, now_elapsed_ms: Option) -> Self { Self { floor_secs: now_secs, anchor_elapsed_ms: now_elapsed_ms, @@ -64,48 +79,66 @@ impl ClockFloor { /// The floor after observing `now_secs` alongside `now_elapsed_ms`, and /// whether it changed enough to be worth storing. /// - /// `None` for the monotonic reading means the caller had none, which is the - /// case at construction. The floor is then consulted and not advanced: - /// raising it needs proof that time passed, and reading it does not. + /// `None` for the monotonic reading means the caller had none. The floor is + /// then consulted and not advanced: raising it needs proof that time + /// passed, and reading it does not. pub(crate) fn observe(self, now_secs: u64, now_elapsed_ms: Option) -> (Self, bool) { let Some(now_elapsed_ms) = now_elapsed_ms else { return (self, false); }; - // A monotonic reading below the anchor means the device rebooted, since - // the counter restarts there, so nothing can be proven about the gap. - // The anchor is moved to the new reading and the floor is left alone. - // That is safe because a stale floor is still a lower bound on real - // time: when the clock reads later than the floor, the clock is used. - // The cost is that the floor lags by however long the device was off, - // so a rewind that far back is not detected. - if now_elapsed_ms < self.anchor_elapsed_ms { - return ( - Self { - floor_secs: self.floor_secs, - anchor_elapsed_ms: now_elapsed_ms, - }, - true, - ); + // No anchor yet, or a reading below the one held, which means the device + // rebooted since the counter restarts there. Either way nothing can be + // proven about the gap, so the reading is adopted as the anchor and the + // floor is left alone. + // + // Safe because the floor only has to be a lower bound on real time, and + // a stale lower bound is still valid: when the clock reads later than + // the floor, the clock is used. The cost is that the floor lags by + // however long the device was off, so a rewind that far back is not + // detected. + let anchor = match self.anchor_elapsed_ms { + Some(anchor) if now_elapsed_ms >= anchor => anchor, + _ => { + return ( + Self { + floor_secs: self.floor_secs, + anchor_elapsed_ms: Some(now_elapsed_ms), + }, + true, + ) + } + }; + + let proven_secs = (now_elapsed_ms - anchor) / 1_000; + if proven_secs == 0 { + // Nothing proven, so nothing to advance by, and the anchor stays + // where it is so the next observation measures from the same point + // rather than restarting the measurement. Not storing this is also + // what keeps a decision from writing to storage every time. + return (self, false); } - let proven_secs = (now_elapsed_ms - self.anchor_elapsed_ms) / 1_000; - let ceiling = self - .floor_secs - .saturating_add(proven_secs) - .saturating_add(ADVANCE_SLACK_SECS); + let ceiling = self.floor_secs.saturating_add(proven_secs); let advanced = Self { floor_secs: self.floor_secs.max(now_secs.min(ceiling)), - anchor_elapsed_ms: now_elapsed_ms, + anchor_elapsed_ms: Some(now_elapsed_ms), }; - let changed = advanced.floor_secs != self.floor_secs - || advanced.anchor_elapsed_ms != self.anchor_elapsed_ms; - (advanced, changed) + (advanced, true) } - /// The time to evaluate validity at: never earlier than the floor. - pub(crate) fn effective_secs(&self, now_secs: u64) -> u64 { - now_secs.max(self.floor_secs) + /// The readings to judge validity with: the clock as given, and an expiry + /// time never earlier than the floor. + pub(crate) fn clock_at(&self, now_secs: u64) -> PolicyClock { + let effective = now_secs.max(self.floor_secs); + PolicyClock { + clock_secs: i64::try_from(now_secs).unwrap_or(i64::MAX), + // Saturating rather than unreachable: a floor this code produced is + // bounded, but one read back from a corrupt or tampered record can + // carry any `u64`. Saturating sends that to "everything expired", + // which refuses. + effective_secs: i64::try_from(effective).unwrap_or(i64::MAX), + } } } @@ -114,73 +147,123 @@ mod tests { use super::*; const MIN: u64 = 60; + const DAY: u64 = 24 * 3600; #[test] - fn winding_the_clock_back_does_not_move_the_evaluated_time_back() { - let floor = ClockFloor::new(1_000_000, 0); + fn winding_the_clock_back_does_not_move_the_expiry_time_back() { + let floor = ClockFloor::new(1_000_000, Some(0)); let (floor, _) = floor.observe(1_000_000 + 10 * MIN, Some(10 * MIN * 1_000)); - // The attack: the date is set back a week. The floor is what validity - // is judged against, so an expired policy stays expired. - assert_eq!( - floor.effective_secs(1_000_000 - 7 * 24 * 3600), - 1_000_000 + 10 * MIN - ); + // The attack: the date is set back a week. Expiry is judged against the + // floor, so an expired policy stays expired. + let clock = floor.clock_at(1_000_000 - 7 * DAY); + assert_eq!(clock.effective_secs, (1_000_000 + 10 * MIN) as i64); + // The clock reading itself is reported as it is, because the + // not-yet-valid bound must not be moved later. + assert_eq!(clock.clock_secs, (1_000_000 - 7 * DAY) as i64); } #[test] fn a_clock_set_far_forward_cannot_park_the_floor_there() { - let floor = ClockFloor::new(1_000_000, 0); + let floor = ClockFloor::new(1_000_000, Some(5_000)); - // One reading from a clock set decades ahead, a second of monotonic + // One reading from a clock set decades ahead, one second of monotonic // time after the anchor. - let (poisoned, _) = floor.observe(4_000_000_000, Some(1_000)); - - // The floor moved by the proven second plus the slack, not by decades, - // so a policy signed today is still inside its window afterwards. - assert!( - poisoned.floor_secs <= 1_000_000 + 1 + ADVANCE_SLACK_SECS, - "floor advanced to {}", - poisoned.floor_secs + let (poisoned, _) = floor.observe(4_000_000_000, Some(6_000)); + assert_eq!( + poisoned.floor_secs, 1_000_001, + "the floor may only advance by the proven second" + ); + + // And the same reading on a device that has been up for weeks, which is + // the case a zero anchor used to credit in full. + let (poisoned, _) = floor.observe(4_000_000_000, Some(5_000 + 40 * DAY * 1_000)); + assert_eq!( + poisoned.floor_secs, + 1_000_000 + 40 * DAY, + "uptime past the anchor is proven time, but no more than that" ); } + #[test] + fn a_floor_written_without_a_reading_only_anchors_on_the_next_one() { + // What construction writes: a floor with no anchor. + let floor = ClockFloor::new(1_000_000, None); + assert_eq!(floor.anchor_elapsed_ms, None); + + // A device up for 40 days, clock set far forward. Crediting the uptime + // here would move the floor past every validity window and refuse the + // warden replacement that would fix it. + let (anchored, changed) = floor.observe(4_000_000_000, Some(40 * DAY * 1_000)); + assert!(changed); + assert_eq!( + anchored.floor_secs, 1_000_000, + "an unanchored floor cannot advance, only anchor" + ); + assert_eq!(anchored.anchor_elapsed_ms, Some(40 * DAY * 1_000)); + } + + #[test] + fn repeated_observations_do_not_walk_the_floor_forward() { + let mut floor = ClockFloor::new(1_000_000, Some(1_000)); + + // The clock is set a month ahead and the caller asks 10,000 times + // within the same monotonic second. Granting any tolerance per call + // would walk the floor there; proven time grants none. + for _ in 0..10_000 { + let (next, changed) = floor.observe(1_000_000 + 30 * DAY, Some(1_000)); + assert!(!changed, "nothing proven, so nothing to store"); + floor = next; + } + assert_eq!(floor.floor_secs, 1_000_000); + assert_eq!(floor.anchor_elapsed_ms, Some(1_000)); + } + #[test] fn the_floor_tracks_time_that_monotonic_proves_passed() { - let floor = ClockFloor::new(1_000_000, 5_000); + let floor = ClockFloor::new(1_000_000, Some(5_000)); let (floor, changed) = floor.observe(1_000_000 + 30 * MIN, Some(5_000 + 30 * MIN * 1_000)); assert!(changed); assert_eq!(floor.floor_secs, 1_000_000 + 30 * MIN); - assert_eq!(floor.anchor_elapsed_ms, 5_000 + 30 * MIN * 1_000); + assert_eq!(floor.anchor_elapsed_ms, Some(5_000 + 30 * MIN * 1_000)); } #[test] fn a_reboot_re_anchors_without_advancing_the_floor() { - let floor = ClockFloor::new(1_000_000, 9_000_000); - let (floor, changed) = floor.observe(1_000_000 + 24 * 3600, Some(12_000)); + let floor = ClockFloor::new(1_000_000, Some(9_000_000)); + let (floor, changed) = floor.observe(1_000_000 + DAY, Some(12_000)); assert!(changed, "the anchor has to follow the restarted counter"); assert_eq!( floor.floor_secs, 1_000_000, "nothing proves the gap, so the floor cannot advance" ); - assert_eq!(floor.anchor_elapsed_ms, 12_000); + assert_eq!(floor.anchor_elapsed_ms, Some(12_000)); // Still usable: the clock reads later than the floor, so the clock wins. assert_eq!( - floor.effective_secs(1_000_000 + 24 * 3600), - 1_000_000 + 24 * 3600 + floor.clock_at(1_000_000 + DAY).effective_secs, + (1_000_000 + DAY) as i64 ); } #[test] fn no_monotonic_reading_consults_the_floor_without_advancing_it() { - let floor = ClockFloor::new(1_000_000, 4_000); + let floor = ClockFloor::new(1_000_000, Some(4_000)); let (same, changed) = floor.observe(1_000_000 + 3600, None); assert!(!changed); assert_eq!(same.floor_secs, 1_000_000); - assert_eq!(same.anchor_elapsed_ms, 4_000); - assert_eq!(same.effective_secs(999_000), 1_000_000); + assert_eq!(same.anchor_elapsed_ms, Some(4_000)); + assert_eq!(same.clock_at(999_000).effective_secs, 1_000_000); + } + + #[test] + fn a_corrupt_floor_saturates_to_everything_expired() { + let floor = ClockFloor { + floor_secs: u64::MAX, + anchor_elapsed_ms: Some(0), + }; + assert_eq!(floor.clock_at(1_000_000).effective_secs, i64::MAX); } } diff --git a/keep-mobile/src/lib.rs b/keep-mobile/src/lib.rs index e87d1e29..518e7f47 100644 --- a/keep-mobile/src/lib.rs +++ b/keep-mobile/src/lib.rs @@ -840,7 +840,7 @@ impl KeepMobile { // because refusing to open the app is not better than refusing every // spend, and the fault is retried before each decision. match Self::startup_policy_now(&storage) { - Ok(now_secs) => Self::load_policy_into(&storage, &mut evaluator, now_secs), + Ok(clock) => Self::load_policy_into(&storage, &mut evaluator, clock), Err(_) => { tracing::error!( "Time floor unreadable, every policy rule will refuse until it is read" @@ -1853,12 +1853,12 @@ impl KeepMobile { // Judged against the floor, not the raw clock, so a bundle that is // already expired cannot be installed by winding the date back to // inside its window. - let now_secs = self.policy_now(None)?; - bundle.verify_timestamp_at(now_secs)?; + let clock = self.policy_now(None)?; + bundle.verify_timestamp_at(clock)?; // A transient fault must not push the caller into discarding a // warden-signed policy, so the record gets another read first. - self.reload_policy_if_transient(None)?; + self.reload_policy_if_transient(clock)?; let mut policy = self.policy_write_lock()?; // Refused when the stored record cannot say how old it is, rather than @@ -1923,7 +1923,22 @@ impl KeepMobile { /// advance, so passing a stale or invented value holds the floor still /// rather than moving it anywhere useful. pub fn policy_status(&self, now_elapsed_ms: u64) -> Result { - self.reload_policy_if_transient(Some(now_elapsed_ms))?; + // The floor is advanced here, before anything can return early. It is + // what makes winding the clock back detectable, and a floor that only + // moved while the policy happened to be faulted would never move on a + // working install, which is the case the attack is run against. + let clock = match self.policy_now(Some(now_elapsed_ms)) { + Ok(clock) => clock, + // Reported as a fault rather than an error: a caller rendering + // status must be able to say the policy cannot be trusted, and + // without the floor nothing says the clock was not wound back. + Err(_) => { + return Ok(PolicyStatus::Faulted { + fault: PolicyFault::Unreadable, + }) + } + }; + self.reload_policy_if_transient(clock)?; Ok(self.policy_read_lock()?.status()) } @@ -1953,8 +1968,23 @@ impl KeepMobile { now_elapsed_ms: u64, ) -> Result { self.reload_velocity_if_unknown()?; - self.reload_policy_if_transient(Some(now_elapsed_ms))?; - self.policy_read_lock()?.evaluate(&ctx) + // As in `policy_status`: advanced before any early return, so the floor + // tracks on an install where nothing is faulted. + let clock = match self.policy_now(Some(now_elapsed_ms)) { + Ok(clock) => clock, + // A refusal, not an error. The contract on `evaluate` says an error + // is not a decision, so a floor that cannot be read would leave the + // guarantee resting on the foreign caller honoring that; answering + // `Deny` here keeps it on this side of the boundary. + Err(_) => { + return Ok(PolicyDecision::Deny { + reason: "Cannot evaluate the spending policy: the time floor is unreadable" + .into(), + }) + } + }; + self.reload_policy_if_transient(clock)?; + self.policy_read_lock()?.evaluate(&ctx, clock) } /// Counts a spend against the velocity limits. @@ -3567,7 +3597,7 @@ impl KeepMobile { pubkey: [u8; POLICY_PUBKEY_LEN], trust: bool, ) -> Result<(), KeepMobileError> { - let now_secs = self.policy_now(None)?; + let clock = self.policy_now(None)?; let mut policy = self.policy_write_lock()?; if policy.trusted_wardens().contains(&pubkey) == trust { return Ok(()); @@ -3580,7 +3610,7 @@ impl KeepMobile { } persistence::persist_trusted_wardens(&self.storage, &wardens)?; policy.set_trusted_wardens(wardens); - Self::load_policy_into(&self.storage, &mut policy, now_secs); + Self::load_policy_into(&self.storage, &mut policy, clock); Ok(()) } @@ -4072,26 +4102,6 @@ impl KeepMobile { }) } - /// Loads the stored wardens and policy into `evaluator`, deciding which of - /// the three states the records put it in. - /// - /// An absent record and an unreadable one are different statements. - /// `load_share_by_key` answers `StorageNotFound` for absent, and a fault - /// means the backend tried and could not, which on Android means the record - /// is there and did not decrypt. So: - /// - /// - no policy record: no rules, nothing to enforce or to be unsure about. - /// - policy unreadable: the rules are unknown, so every rule refuses. - /// - policy present, wardens unreadable: the bundle cannot be attributed to - /// a signer, and an unattributable policy is not an absent one. - /// - policy present and wardens known: verified, or faulted. - /// - /// Wardens absent and wardens unreadable both land on a refusal for a - /// stored bundle on a fresh evaluator, because `verify_trusted_warden` - /// refuses an empty set. A bundle can only have been stored while some - /// warden was trusted, so an empty set with a bundle present means the - /// record was lost, which is exactly the case that must not read as "no - /// policy". /// The time to judge policy validity at, never earlier than the floor. /// /// `now_elapsed_ms` is the platform's monotonic reading, as @@ -4105,38 +4115,61 @@ impl KeepMobile { /// to distrust. /// /// [`SigningRateLimiter::check_and_record`]: crate::SigningRateLimiter::check_and_record - fn policy_now(&self, now_elapsed_ms: Option) -> Result { + fn policy_now( + &self, + now_elapsed_ms: Option, + ) -> Result { Self::floor_now(&self.storage, now_elapsed_ms) } /// [`Self::policy_now`] before there is a `self`, for the load the /// constructor does. - fn startup_policy_now(storage: &Arc) -> Result { + fn startup_policy_now( + storage: &Arc, + ) -> Result { Self::floor_now(storage, None) } fn floor_now( storage: &Arc, now_elapsed_ms: Option, - ) -> Result { + ) -> Result { let now_secs = u64::try_from(chrono::Utc::now().timestamp()).unwrap_or(0); let stored = persistence::load_clock_floor(storage)?; - let floor = stored - .unwrap_or_else(|| clock_floor::ClockFloor::new(now_secs, now_elapsed_ms.unwrap_or(0))); + let floor = + stored.unwrap_or_else(|| clock_floor::ClockFloor::new(now_secs, now_elapsed_ms)); let (observed, changed) = floor.observe(now_secs, now_elapsed_ms); // A first write counts as changed, so the floor exists from here on. if changed || stored.is_none() { persistence::persist_clock_floor(storage, &observed)?; } - // Back to the signed seconds the validity checks take. The advance is - // bounded, so the floor cannot reach the saturating case. - Ok(i64::try_from(observed.effective_secs(now_secs)).unwrap_or(i64::MAX)) + Ok(observed.clock_at(now_secs)) } + /// Loads the stored wardens and policy into `evaluator`, deciding which of + /// the three states the records put it in. + /// + /// An absent record and an unreadable one are different statements. + /// `load_share_by_key` answers `StorageNotFound` for absent, and a fault + /// means the backend tried and could not, which on Android means the record + /// is there and did not decrypt. So: + /// + /// - no policy record: no rules, nothing to enforce or to be unsure about. + /// - policy unreadable: the rules are unknown, so every rule refuses. + /// - policy present, wardens unreadable: the bundle cannot be attributed to + /// a signer, and an unattributable policy is not an absent one. + /// - policy present and wardens known: verified, or faulted. + /// + /// Wardens absent and wardens unreadable both land on a refusal for a + /// stored bundle on a fresh evaluator, because `verify_trusted_warden` + /// refuses an empty set. A bundle can only have been stored while some + /// warden was trusted, so an empty set with a bundle present means the + /// record was lost, which is exactly the case that must not read as "no + /// policy". fn load_policy_into( storage: &Arc, evaluator: &mut PolicyEvaluator, - now_secs: i64, + clock: clock_floor::PolicyClock, ) { match persistence::load_trusted_wardens(storage) { Ok(wardens) => evaluator.set_trusted_wardens(wardens), @@ -4205,7 +4238,7 @@ impl KeepMobile { return; } - match bundle.timestamp_fault_at(now_secs) { + match bundle.timestamp_fault_at(clock) { None => evaluator.set_policy(bundle), Some(fault) => { tracing::error!( @@ -4227,7 +4260,7 @@ impl KeepMobile { /// re-run the load themselves. fn reload_policy_if_transient( &self, - now_elapsed_ms: Option, + clock: clock_floor::PolicyClock, ) -> Result<(), KeepMobileError> { // Bound to a local first. Folding this into the `if` condition, or into // a `match`, keeps the read guard alive across `policy_write_lock` and @@ -4236,9 +4269,8 @@ impl KeepMobile { if !fault.is_some_and(PolicyFault::is_transient) { return Ok(()); } - let now_secs = self.policy_now(now_elapsed_ms)?; let mut policy = self.policy_write_lock()?; - Self::load_policy_into(&self.storage, &mut policy, now_secs); + Self::load_policy_into(&self.storage, &mut policy, clock); Ok(()) } @@ -7926,7 +7958,7 @@ mod velocity_fault_tests { CLOCK_FLOOR_STORAGE_KEY, serde_json::to_vec(&crate::clock_floor::ClockFloor { floor_secs: now + 40 * 24 * 3600, - anchor_elapsed_ms: 5_000, + anchor_elapsed_ms: Some(5_000), }) .unwrap(), ); @@ -7974,4 +8006,76 @@ mod velocity_fault_tests { .map(|d| matches!(d, PolicyDecision::Deny { .. })) .unwrap_or(true)); } + + /// The floor has to advance on an install where nothing is wrong, which is + /// the only kind the attack is run against. A floor that moved only while + /// the policy happened to be faulted would never move at all on a working + /// device, so the record would keep the value it was written with at first + /// launch and a wound-back clock would sail past it. + #[test] + fn the_floor_advances_on_an_install_with_nothing_faulted() { + let storage = Arc::new(FaultingStorage::default()); + plant_policy(&storage, &capped_rules(), true); + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + + // Nothing is faulted, so the reload path returns without doing + // anything; only a floor advanced ahead of that check can move. + assert!(matches!( + mobile.policy_status(1_000).unwrap(), + PolicyStatus::Active { .. } + )); + + let written: crate::clock_floor::ClockFloor = + serde_json::from_slice(&storage.raw(CLOCK_FLOOR_STORAGE_KEY).expect("floor written")) + .unwrap(); + assert_eq!( + written.anchor_elapsed_ms, + Some(1_000), + "the reading has to reach the floor on a healthy install" + ); + + // And a later reading is adopted, so the measurement follows the device + // rather than staying pinned to the first call. + mobile.evaluate_policy(a_spend(), 3_601_000).unwrap(); + let written: crate::clock_floor::ClockFloor = + serde_json::from_slice(&storage.raw(CLOCK_FLOOR_STORAGE_KEY).unwrap()).unwrap(); + assert_eq!(written.anchor_elapsed_ms, Some(3_601_000)); + } + + /// A policy that was in force when it was read must stop being enforced + /// once it expires, without waiting for anything to re-read it. Nothing + /// re-reads a policy that is not faulted, so judging validity only where + /// the record is loaded leaves a running process enforcing retired rules + /// for as long as it stays up. + #[test] + fn an_active_policy_stops_being_enforced_once_it_expires() { + let now = Utc::now().timestamp() as u64; + let storage = Arc::new(FaultingStorage::default()); + plant_policy(&storage, &capped_rules(), true); + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + assert!(matches!( + mobile.policy_status(0).unwrap(), + PolicyStatus::Active { .. } + )); + + // Time moves past the window. Staged through the floor because the real + // clock cannot be moved under test; the anchor is ahead of the reading + // passed in, so the observation leaves the planted floor alone. + storage.plant( + CLOCK_FLOOR_STORAGE_KEY, + serde_json::to_vec(&crate::clock_floor::ClockFloor { + floor_secs: now + 40 * 24 * 3600, + anchor_elapsed_ms: Some(9_000_000), + }) + .unwrap(), + ); + + assert!( + matches!( + mobile.evaluate_policy(a_spend(), 0).unwrap(), + PolicyDecision::Deny { .. } + ), + "an expired bundle must refuse even while memory still holds it as active" + ); + } } diff --git a/keep-mobile/src/persistence.rs b/keep-mobile/src/persistence.rs index 061cd34e..bf8c0cdf 100644 --- a/keep-mobile/src/persistence.rs +++ b/keep-mobile/src/persistence.rs @@ -117,6 +117,8 @@ pub(crate) fn persist_policy( /// at a generous 128 bytes per entry against a worst case nearer 75. const VELOCITY_RECORD_MAX_LEN: usize = 128 * MAX_TRACKED_TRANSACTIONS + 1024; +const CLOCK_FLOOR_RECORD_MAX_LEN: usize = 256; + /// Reads the stored time floor, or `None` when none has been written yet. /// /// A read failure is an error rather than an absence, and the caller refuses on @@ -126,6 +128,12 @@ pub(crate) fn load_clock_floor( storage: &Arc, ) -> Result, KeepMobileError> { match storage.load_share_by_key(CLOCK_FLOOR_STORAGE_KEY.into()) { + // Bounded before the parse, as the policy and counter records are. Two + // integer fields, so the ceiling is generous and still refuses anything + // that is not this record. + Ok(data) if data.len() > CLOCK_FLOOR_RECORD_MAX_LEN => Err(KeepMobileError::StorageError { + msg: "Clock floor record exceeds the maximum length".into(), + }), Ok(data) => serde_json::from_slice(&data) .map(Some) // Without serde's message, which quotes the value it tripped on. diff --git a/keep-mobile/src/policy.rs b/keep-mobile/src/policy.rs index 0f444ffc..872295d5 100644 --- a/keep-mobile/src/policy.rs +++ b/keep-mobile/src/policy.rs @@ -1,6 +1,7 @@ // SPDX-FileCopyrightText: © 2026 PrivKey LLC // SPDX-License-Identifier: MIT +use crate::clock_floor::PolicyClock; use crate::error::KeepMobileError; use crate::velocity::VelocityTracker; use chrono::{Datelike, Timelike, Utc}; @@ -463,30 +464,38 @@ impl PolicyBundle { /// Which validity bound the bundle falls outside, if either. /// - /// Reported apart from [`Self::verify_timestamp`]'s single error because + /// Reported apart from [`Self::verify_timestamp_at`]'s single error because /// the two call for opposite handling: being dated ahead of the clock /// resolves by itself, expiring does not. - /// The clock is a parameter, not read here, and that is what keeps the + /// + /// Both readings are parameters, not read here, and that is what keeps the /// stored time floor on the path: a bundle's validity is judged at the time /// the caller establishes, so no call site can fall back to the raw device /// clock by accident. It also lets the bounds be tested at an exact instant /// either side of them, including a negative one, which `Utc::now` cannot /// be made to return. /// + /// Each bound uses the reading that can only refuse. Expiry takes the + /// floored time, because a later time can only make a bundle more expired. + /// The future bound takes the raw clock, because a floor ahead of the clock + /// would make a future-dated bundle look current and un-refuse it, which is + /// the one direction the floor must not be allowed to move a decision. + /// /// A clock before 1970 clamps to the epoch rather than casting: `as u64` on /// a negative turns it into roughly 1.8e19, which clears the future bound /// and trips the age bound, landing a perfectly good bundle on `Expired`, /// the arm that is never retried. Clamping sends it to `NotYetValid` /// instead, which is both true of a pre-1970 clock and retried once the /// clock is set. - pub(crate) fn timestamp_fault_at(&self, now_secs: i64) -> Option { - let now = u64::try_from(now_secs).unwrap_or(0); + pub(crate) fn timestamp_fault_at(&self, clock: PolicyClock) -> Option { + let now = u64::try_from(clock.clock_secs).unwrap_or(0); + let effective = u64::try_from(clock.effective_secs).unwrap_or(0); if self.created_at > now.saturating_add(MAX_POLICY_FUTURE_SECS) { return Some(PolicyFault::NotYetValid); } - if now > self.created_at.saturating_add(MAX_POLICY_AGE_SECS) { + if effective > self.created_at.saturating_add(MAX_POLICY_AGE_SECS) { return Some(PolicyFault::Expired); } @@ -495,8 +504,8 @@ impl PolicyBundle { /// Judged at the instant the caller establishes, so an import is measured /// against the stored time floor rather than the raw clock. - pub(crate) fn verify_timestamp_at(&self, now_secs: i64) -> Result<(), KeepMobileError> { - match self.timestamp_fault_at(now_secs) { + pub(crate) fn verify_timestamp_at(&self, clock: PolicyClock) -> Result<(), KeepMobileError> { + match self.timestamp_fault_at(clock) { None => Ok(()), Some(fault) => Err(KeepMobileError::InvalidPolicy { msg: fault.to_string(), @@ -647,7 +656,11 @@ impl PolicyEvaluator { /// not parse out of a bundle that verified, come back as `Err`. A caller /// that treats the error as anything but a refusal gives back exactly the /// fail-open this type exists to close. - pub fn evaluate(&self, ctx: &TransactionContext) -> Result { + pub fn evaluate( + &self, + ctx: &TransactionContext, + clock: PolicyClock, + ) -> Result { let bundle = match &self.policy { PolicyState::NotConfigured => return Ok(PolicyDecision::Allow), // The reason says only that it did not verify. Which check failed @@ -663,6 +676,17 @@ impl PolicyEvaluator { PolicyState::Active(bundle) => bundle, }; + // Validity is a function of time, so it is judged here and not only + // where the record was read. A process that loaded a bundle inside its + // window would otherwise keep enforcing it after it expired, for as + // long as it stayed running, because nothing re-reads a policy that is + // not faulted. + if let Some(fault) = bundle.timestamp_fault_at(clock) { + return Ok(PolicyDecision::Deny { + reason: format!("Cannot evaluate the spending policy: {fault}"), + }); + } + let rules = bundle.parse_rules()?; if let Some(max_amount) = rules.max_amount { @@ -894,6 +918,15 @@ pub(crate) fn resign_test_bundle( mod tests { use super::*; + /// A clock with no floor ahead of it, for the bounds that do not involve + /// one. The floor's own effect is covered in `clock_floor`. + fn at(now_secs: i64) -> PolicyClock { + PolicyClock { + clock_secs: now_secs, + effective_secs: now_secs, + } + } + /// The validity window is pinned in days here rather than against /// [`MAX_POLICY_AGE_SECS`], because it is the value that bounds how far a /// rollback can reach and every other test expresses its dates in terms of @@ -905,12 +938,12 @@ mod tests { let bundle = create_test_bundle_at(&PolicyRules::default(), created_at as u64); assert_eq!( - bundle.timestamp_fault_at(created_at + 29 * DAY), + bundle.timestamp_fault_at(at(created_at + 29 * DAY)), None, "a bundle inside the window is enforceable" ); assert_eq!( - bundle.timestamp_fault_at(created_at + 31 * DAY), + bundle.timestamp_fault_at(at(created_at + 31 * DAY)), Some(PolicyFault::Expired), "past the window it must refuse rather than enforce retired rules" ); @@ -927,7 +960,7 @@ mod tests { let bundle = create_test_bundle_at(&rules, 1_700_000_000); assert_eq!( - bundle.timestamp_fault_at(-86_400), + bundle.timestamp_fault_at(at(-86_400)), Some(PolicyFault::NotYetValid) ); assert!(PolicyFault::NotYetValid.is_transient()); @@ -1007,7 +1040,12 @@ mod tests { destinations: vec![], has_unknown_destination: false, }; - assert_eq!(evaluator.evaluate(&ctx).unwrap(), PolicyDecision::Allow); + assert_eq!( + evaluator + .evaluate(&ctx, at(Utc::now().timestamp())) + .unwrap(), + PolicyDecision::Allow + ); let ctx = TransactionContext { amount_sats: 150_000, @@ -1016,7 +1054,9 @@ mod tests { has_unknown_destination: false, }; assert!(matches!( - evaluator.evaluate(&ctx).unwrap(), + evaluator + .evaluate(&ctx, at(Utc::now().timestamp())) + .unwrap(), PolicyDecision::Deny { .. } )); } @@ -1039,7 +1079,12 @@ mod tests { destinations: vec![], has_unknown_destination: false, }; - assert_eq!(evaluator.evaluate(&ctx).unwrap(), PolicyDecision::Allow); + assert_eq!( + evaluator + .evaluate(&ctx, at(Utc::now().timestamp())) + .unwrap(), + PolicyDecision::Allow + ); let ctx = TransactionContext { amount_sats: 50_000, @@ -1048,7 +1093,9 @@ mod tests { has_unknown_destination: false, }; assert!(matches!( - evaluator.evaluate(&ctx).unwrap(), + evaluator + .evaluate(&ctx, at(Utc::now().timestamp())) + .unwrap(), PolicyDecision::Deny { .. } )); } @@ -1071,7 +1118,12 @@ mod tests { destinations: vec!["bc1allowed".to_string()], has_unknown_destination: false, }; - assert_eq!(evaluator.evaluate(&ctx).unwrap(), PolicyDecision::Allow); + assert_eq!( + evaluator + .evaluate(&ctx, at(Utc::now().timestamp())) + .unwrap(), + PolicyDecision::Allow + ); let ctx = TransactionContext { amount_sats: 50_000, @@ -1080,7 +1132,9 @@ mod tests { has_unknown_destination: false, }; assert!(matches!( - evaluator.evaluate(&ctx).unwrap(), + evaluator + .evaluate(&ctx, at(Utc::now().timestamp())) + .unwrap(), PolicyDecision::Deny { .. } )); } @@ -1103,7 +1157,12 @@ mod tests { destinations: vec!["bc1allowed".to_string()], has_unknown_destination: false, }; - assert_eq!(evaluator.evaluate(&ctx).unwrap(), PolicyDecision::Allow); + assert_eq!( + evaluator + .evaluate(&ctx, at(Utc::now().timestamp())) + .unwrap(), + PolicyDecision::Allow + ); let ctx = TransactionContext { amount_sats: 50_000, @@ -1112,7 +1171,9 @@ mod tests { has_unknown_destination: false, }; assert!(matches!( - evaluator.evaluate(&ctx).unwrap(), + evaluator + .evaluate(&ctx, at(Utc::now().timestamp())) + .unwrap(), PolicyDecision::Deny { .. } )); } @@ -1128,7 +1189,12 @@ mod tests { destinations: vec!["any".to_string()], has_unknown_destination: false, }; - assert_eq!(evaluator.evaluate(&ctx).unwrap(), PolicyDecision::Allow); + assert_eq!( + evaluator + .evaluate(&ctx, at(Utc::now().timestamp())) + .unwrap(), + PolicyDecision::Allow + ); } #[test] @@ -1170,13 +1236,19 @@ mod tests { let rules = PolicyRules::default(); let mut bundle = create_test_bundle(&rules); - assert!(bundle.verify_timestamp_at(Utc::now().timestamp()).is_ok()); + assert!(bundle + .verify_timestamp_at(at(Utc::now().timestamp())) + .is_ok()); bundle.created_at = Utc::now().timestamp() as u64 + MAX_POLICY_FUTURE_SECS + 3600; - assert!(bundle.verify_timestamp_at(Utc::now().timestamp()).is_err()); + assert!(bundle + .verify_timestamp_at(at(Utc::now().timestamp())) + .is_err()); bundle.created_at = Utc::now().timestamp() as u64 - MAX_POLICY_AGE_SECS - 3600; - assert!(bundle.verify_timestamp_at(Utc::now().timestamp()).is_err()); + assert!(bundle + .verify_timestamp_at(at(Utc::now().timestamp())) + .is_err()); } #[test] @@ -1209,7 +1281,12 @@ mod tests { destinations: vec!["bc1qallowed".to_string()], has_unknown_destination: false, }; - assert_eq!(evaluator.evaluate(&ctx).unwrap(), PolicyDecision::Allow); + assert_eq!( + evaluator + .evaluate(&ctx, at(Utc::now().timestamp())) + .unwrap(), + PolicyDecision::Allow + ); let ctx = TransactionContext { amount_sats: 50_000, @@ -1217,7 +1294,12 @@ mod tests { destinations: vec!["BC1QALLOWED".to_string()], has_unknown_destination: false, }; - assert_eq!(evaluator.evaluate(&ctx).unwrap(), PolicyDecision::Allow); + assert_eq!( + evaluator + .evaluate(&ctx, at(Utc::now().timestamp())) + .unwrap(), + PolicyDecision::Allow + ); } #[test] @@ -1239,7 +1321,9 @@ mod tests { has_unknown_destination: false, }; assert!(matches!( - evaluator.evaluate(&ctx).unwrap(), + evaluator + .evaluate(&ctx, at(Utc::now().timestamp())) + .unwrap(), PolicyDecision::Deny { .. } )); } @@ -1280,7 +1364,12 @@ mod tests { destinations: vec![], has_unknown_destination: false, }; - assert_eq!(evaluator.evaluate(&ctx).unwrap(), PolicyDecision::Allow); + assert_eq!( + evaluator + .evaluate(&ctx, at(Utc::now().timestamp())) + .unwrap(), + PolicyDecision::Allow + ); let ctx = TransactionContext { amount_sats: 2_000, @@ -1289,7 +1378,9 @@ mod tests { has_unknown_destination: false, }; assert!(matches!( - evaluator.evaluate(&ctx).unwrap(), + evaluator + .evaluate(&ctx, at(Utc::now().timestamp())) + .unwrap(), PolicyDecision::Deny { .. } )); } @@ -1312,7 +1403,10 @@ mod tests { destinations: vec![], has_unknown_destination: false, }; - let PolicyDecision::Deny { reason } = evaluator.evaluate(&ctx).unwrap() else { + let PolicyDecision::Deny { reason } = evaluator + .evaluate(&ctx, at(Utc::now().timestamp())) + .unwrap() + else { panic!("an unreadable history must not pass a daily limit"); }; assert!(reason.contains("unreadable"), "{reason}"); @@ -1335,7 +1429,12 @@ mod tests { destinations: vec![], has_unknown_destination: false, }; - assert_eq!(evaluator.evaluate(&ctx).unwrap(), PolicyDecision::Allow); + assert_eq!( + evaluator + .evaluate(&ctx, at(Utc::now().timestamp())) + .unwrap(), + PolicyDecision::Allow + ); } // Recording into a tracker conjured at record time would make the counters @@ -1369,13 +1468,18 @@ mod tests { has_unknown_destination: false, }; assert_eq!( - evaluator.evaluate(&ctx).unwrap(), + evaluator + .evaluate(&ctx, at(Utc::now().timestamp())) + .unwrap(), PolicyDecision::Allow, "under the limit, so a verified policy allows it" ); evaluator.set_policy_faulted(PolicyFault::Unverified, None); - let PolicyDecision::Deny { reason } = evaluator.evaluate(&ctx).unwrap() else { + let PolicyDecision::Deny { reason } = evaluator + .evaluate(&ctx, at(Utc::now().timestamp())) + .unwrap() + else { panic!("a policy that cannot be trusted must refuse, not fall back to no policy"); }; assert!(reason.contains("did not verify"), "{reason}"); @@ -1392,7 +1496,12 @@ mod tests { destinations: vec![], has_unknown_destination: false, }; - assert_eq!(evaluator.evaluate(&ctx).unwrap(), PolicyDecision::Allow); + assert_eq!( + evaluator + .evaluate(&ctx, at(Utc::now().timestamp())) + .unwrap(), + PolicyDecision::Allow + ); } /// A faulted record must not be readable as a policy through any accessor. From d00e48706862d8dec7966991bee570a88232f7dc Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Sun, 18 Oct 2026 09:26:47 -0400 Subject: [PATCH 3/7] Make status agree with the decision, and refuse on a floor fault only when a policy exists --- keep-mobile/src/clock_floor.rs | 76 +++++++++++++- keep-mobile/src/lib.rs | 175 ++++++++++++++++++++++++++++----- keep-mobile/src/policy.rs | 37 +++++-- 3 files changed, 250 insertions(+), 38 deletions(-) diff --git a/keep-mobile/src/clock_floor.rs b/keep-mobile/src/clock_floor.rs index 7df949cc..33af2f88 100644 --- a/keep-mobile/src/clock_floor.rs +++ b/keep-mobile/src/clock_floor.rs @@ -120,11 +120,32 @@ impl ClockFloor { } let ceiling = self.floor_secs.saturating_add(proven_secs); - let advanced = Self { - floor_secs: self.floor_secs.max(now_secs.min(ceiling)), - anchor_elapsed_ms: Some(now_elapsed_ms), - }; - (advanced, true) + let floor_secs = self.floor_secs.max(now_secs.min(ceiling)); + // The anchor moves by the whole seconds that were credited, not to the + // reading itself, so the remainder below a second is carried instead of + // discarded. Moving it to the reading would throw away up to 999 ms on + // every advance, and a caller asking just under every two seconds would + // then credit one second per two and hold the floor at half of real + // time. + let anchor_elapsed_ms = anchor.saturating_add(proven_secs.saturating_mul(1_000)); + // Stored only when the floor itself moved. When it did not, which is + // every call while the clock reads behind the floor, keeping the old + // anchor is what lets the proven time accrue rather than being consumed + // by a call that could not use it. It also keeps a decision from + // writing to storage every time one is more than a second after the + // last. + let changed = floor_secs != self.floor_secs; + ( + Self { + floor_secs, + anchor_elapsed_ms: if changed { + Some(anchor_elapsed_ms) + } else { + self.anchor_elapsed_ms + }, + }, + changed, + ) } /// The readings to judge validity with: the clock as given, and an expiry @@ -227,6 +248,51 @@ mod tests { assert!(changed); assert_eq!(floor.floor_secs, 1_000_000 + 30 * MIN); assert_eq!(floor.anchor_elapsed_ms, Some(5_000 + 30 * MIN * 1_000)); + // Exactly the credited seconds, so nothing is carried here. + } + + /// Time below a whole second is carried, not discarded. Moving the anchor + /// to the reading would throw away the remainder on every advance, so a + /// caller asking just under every two seconds would credit one second per + /// two and hold the floor at half of real time. + #[test] + fn accrual_below_a_second_is_carried_rather_than_discarded() { + let mut floor = ClockFloor::new(1_000_000, Some(0)); + + // Ten asks at 1,900 ms apart: 19 s of real time, and each ask on its + // own proves only one whole second. + for i in 1..=10u64 { + let reading = i * 1_900; + let (next, _) = floor.observe(1_000_000 + 19, Some(reading)); + floor = next; + } + + assert_eq!( + floor.floor_secs, 1_000_019, + "the carried remainders have to add up to the real elapsed time" + ); + } + + /// A floor that did not move is not written, which keeps a decision off + /// storage and keeps the unused proven time accruing. + #[test] + fn an_advance_that_cannot_happen_is_not_stored() { + let floor = ClockFloor::new(1_000_000, Some(0)); + + // The clock reads behind the floor, so there is nothing to advance to. + let (same, changed) = floor.observe(999_000, Some(60_000)); + assert!(!changed, "nothing moved, so nothing to store"); + assert_eq!(same.floor_secs, 1_000_000); + assert_eq!( + same.anchor_elapsed_ms, + Some(0), + "the anchor stays so the minute still counts once the clock is back" + ); + + // Once the clock is restored the whole minute is still available. + let (advanced, changed) = same.observe(1_000_060, Some(60_000)); + assert!(changed); + assert_eq!(advanced.floor_secs, 1_000_060); } #[test] diff --git a/keep-mobile/src/lib.rs b/keep-mobile/src/lib.rs index 518e7f47..c8b84346 100644 --- a/keep-mobile/src/lib.rs +++ b/keep-mobile/src/lib.rs @@ -841,12 +841,20 @@ impl KeepMobile { // spend, and the fault is retried before each decision. match Self::startup_policy_now(&storage) { Ok(clock) => Self::load_policy_into(&storage, &mut evaluator, clock), - Err(_) => { - tracing::error!( - "Time floor unreadable, every policy rule will refuse until it is read" - ); - evaluator.set_policy_faulted(PolicyFault::Unreadable, None); - } + // Faulted only when there is a policy for the floor to protect. A + // device with no policy record has nothing to wind the clock back + // into, and nothing deletes the floor record, so faulting here + // would leave such a device refusing permanently with no policy to + // delete to recover. + Err(_) => match persistence::load_policy(&storage) { + Err(KeepMobileError::StorageNotFound) => evaluator.clear_policy(), + _ => { + tracing::error!( + "Time floor unreadable, every policy rule will refuse until it is read" + ); + evaluator.set_policy_faulted(PolicyFault::Unreadable, None); + } + }, } let policy = Arc::new(std::sync::RwLock::new(evaluator)); @@ -1904,14 +1912,56 @@ impl KeepMobile { /// The policy in force, which is `None` both when none is configured and /// when the stored one cannot be trusted. Use [`Self::policy_status`] to /// tell those apart; they behave as opposites. - pub fn get_policy_info(&self) -> Option { + /// + /// Takes the clock so "in force" means what it says: a bundle whose window + /// closed is not in force, even while this process still holds it, and + /// reporting its limits would describe rules nothing will honor. + pub fn get_policy_info(&self, now_elapsed_ms: u64) -> Option { + let clock = self.policy_now(Some(now_elapsed_ms)).ok()?; self.policy .read() .ok()? - .policy() + .policy_at(clock) .map(|b| b.to_policy_info()) } + /// How a floor that cannot be read answers, which depends on whether there + /// is a policy for it to protect. + /// + /// With one, the floor is the only thing saying the clock was not wound + /// back, so its absence refuses. With none, there is nothing to wind back + /// into: the device would otherwise be left refusing permanently over a + /// record that guards nothing, and nothing deletes it. + fn floor_configured(&self) -> Result { + Ok(!matches!( + self.policy_read_lock()?.status(clock_floor::PolicyClock { + clock_secs: 0, + effective_secs: 0, + }), + PolicyStatus::NotConfigured + )) + } + + fn floor_fault_status(&self, e: KeepMobileError) -> Result { + if self.floor_configured()? { + tracing::error!("Time floor unreadable, the stored policy will refuse: {e}"); + return Ok(PolicyStatus::Faulted { + fault: PolicyFault::Unreadable, + }); + } + Ok(PolicyStatus::NotConfigured) + } + + fn floor_fault_decision(&self, e: KeepMobileError) -> Result { + if self.floor_configured()? { + tracing::error!("Time floor unreadable, refusing the spend: {e}"); + return Ok(PolicyDecision::Deny { + reason: "Cannot evaluate the spending policy: the time floor is unreadable".into(), + }); + } + Ok(PolicyDecision::Allow) + } + /// Whether a policy is configured, in force, or stored and untrustworthy. /// /// A faulted record refuses every spend, so reporting it as no policy would @@ -1932,14 +1982,16 @@ impl KeepMobile { // Reported as a fault rather than an error: a caller rendering // status must be able to say the policy cannot be trusted, and // without the floor nothing says the clock was not wound back. - Err(_) => { - return Ok(PolicyStatus::Faulted { - fault: PolicyFault::Unreadable, - }) - } + // + // Only when there is a policy to protect. With none configured the + // floor guards nothing, and refusing on it would turn an + // unreadable record into a permanent refusal on a device that never + // had a policy: nothing deletes this record, so it would outlive + // every restart with no policy to delete. + Err(e) => return self.floor_fault_status(e), }; self.reload_policy_if_transient(clock)?; - Ok(self.policy_read_lock()?.status()) + Ok(self.policy_read_lock()?.status(clock)) } /// Removes the stored policy, and is the only way out of a record that will @@ -1975,13 +2027,10 @@ impl KeepMobile { // A refusal, not an error. The contract on `evaluate` says an error // is not a decision, so a floor that cannot be read would leave the // guarantee resting on the foreign caller honoring that; answering - // `Deny` here keeps it on this side of the boundary. - Err(_) => { - return Ok(PolicyDecision::Deny { - reason: "Cannot evaluate the spending policy: the time floor is unreadable" - .into(), - }) - } + // `Deny` here keeps it on this side of the boundary. Only when + // there is a policy to protect, for the reason given on + // `floor_fault_status`. + Err(e) => return self.floor_fault_decision(e), }; self.reload_policy_if_transient(clock)?; self.policy_read_lock()?.evaluate(&ctx, clock) @@ -7270,7 +7319,7 @@ mod velocity_fault_tests { ), "the refusal has to be explainable, so it must not report as unconfigured" ); - assert!(mobile.get_policy_info().is_none()); + assert!(mobile.get_policy_info(0).is_none()); } /// A verified record still works, so the refusals above cannot be a policy @@ -8034,12 +8083,14 @@ mod velocity_fault_tests { "the reading has to reach the floor on a healthy install" ); - // And a later reading is adopted, so the measurement follows the device - // rather than staying pinned to the first call. + // A later decision cannot move the floor backwards. It also cannot move + // it ahead of the real clock, so on a healthy install it tracks rather + // than climbing; the advance itself is covered where the clock can be + // controlled, in `clock_floor`. mobile.evaluate_policy(a_spend(), 3_601_000).unwrap(); - let written: crate::clock_floor::ClockFloor = + let later: crate::clock_floor::ClockFloor = serde_json::from_slice(&storage.raw(CLOCK_FLOOR_STORAGE_KEY).unwrap()).unwrap(); - assert_eq!(written.anchor_elapsed_ms, Some(3_601_000)); + assert!(later.floor_secs >= written.floor_secs); } /// A policy that was in force when it was read must stop being enforced @@ -8078,4 +8129,76 @@ mod velocity_fault_tests { "an expired bundle must refuse even while memory still holds it as active" ); } + + /// Status and the decision have to agree. Reporting the stored state alone + /// said `Active`, with the retired limits, about a bundle the decision path + /// had already started refusing because its window closed while the process + /// stayed up, which leaves a caller unable to explain the refusals. + #[test] + fn status_and_the_decision_agree_once_a_policy_expires() { + let now = Utc::now().timestamp() as u64; + let storage = Arc::new(FaultingStorage::default()); + plant_policy(&storage, &capped_rules(), true); + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + assert!(matches!( + mobile.policy_status(0).unwrap(), + PolicyStatus::Active { .. } + )); + + storage.plant( + CLOCK_FLOOR_STORAGE_KEY, + serde_json::to_vec(&crate::clock_floor::ClockFloor { + floor_secs: now + 40 * 24 * 3600, + anchor_elapsed_ms: Some(9_000_000), + }) + .unwrap(), + ); + + assert!(matches!( + mobile.evaluate_policy(a_spend(), 0).unwrap(), + PolicyDecision::Deny { .. } + )); + assert!( + matches!( + mobile.policy_status(0).unwrap(), + PolicyStatus::Faulted { + fault: PolicyFault::Expired + } + ), + "status must say why the spend was refused" + ); + assert!( + mobile.get_policy_info(0).is_none(), + "a bundle out of its window is not the policy in force" + ); + } + + /// With no policy configured the floor guards nothing, so a record that + /// cannot be read must not refuse. Nothing deletes it, so refusing would + /// outlive every restart on a device that never had a policy, with no + /// policy to delete to recover. + #[test] + fn an_unreadable_floor_does_not_refuse_when_no_policy_is_configured() { + let storage = Arc::new(FaultingStorage::default()); + storage.plant(CLOCK_FLOOR_STORAGE_KEY, b"not json".to_vec()); + + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + assert!(matches!( + mobile.policy_status(0).unwrap(), + PolicyStatus::NotConfigured + )); + assert!(matches!( + mobile.evaluate_policy(a_spend(), 0).unwrap(), + PolicyDecision::Allow + )); + + // And with a policy present the same unreadable record refuses, because + // then it is the only thing saying the clock was not wound back. + plant_policy(&storage, &capped_rules(), true); + let guarded = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + assert!(matches!( + guarded.evaluate_policy(a_spend(), 0).unwrap(), + PolicyDecision::Deny { .. } + )); + } } diff --git a/keep-mobile/src/policy.rs b/keep-mobile/src/policy.rs index 872295d5..9a831c84 100644 --- a/keep-mobile/src/policy.rs +++ b/keep-mobile/src/policy.rs @@ -638,16 +638,33 @@ impl PolicyEvaluator { } } - pub fn status(&self) -> PolicyStatus { + /// Takes the clock for the same reason [`Self::evaluate`] does, so the two + /// cannot disagree. Reporting the stored state alone would say `Active`, + /// with the retired limits, about a bundle `evaluate` has started refusing + /// because its window closed while this process stayed up. A caller shown + /// an active policy while every spend is denied is left unable to explain + /// the refusals, which is the failure the `Faulted` state exists to + /// prevent. + pub fn status(&self, clock: PolicyClock) -> PolicyStatus { match &self.policy { PolicyState::NotConfigured => PolicyStatus::NotConfigured, - PolicyState::Active(bundle) => PolicyStatus::Active { - info: bundle.to_policy_info(), + PolicyState::Active(bundle) => match bundle.timestamp_fault_at(clock) { + Some(fault) => PolicyStatus::Faulted { fault }, + None => PolicyStatus::Active { + info: bundle.to_policy_info(), + }, }, PolicyState::Faulted { fault, .. } => PolicyStatus::Faulted { fault: *fault }, } } + /// The bundle in force at `clock`, which is `None` once its window closes + /// even while this evaluator still holds it. + pub(crate) fn policy_at(&self, clock: PolicyClock) -> Option<&PolicyBundle> { + self.policy() + .filter(|bundle| bundle.timestamp_fault_at(clock).is_none()) + } + /// Decide on a transaction against the policy in force. /// /// An `Err` is not a decision and must never be read as one. The state @@ -1523,7 +1540,10 @@ mod tests { "an unverified bundle must not be readable as the policy in force" ); assert_eq!(evaluator.fault(), Some(PolicyFault::Unreadable)); - assert!(matches!(evaluator.status(), PolicyStatus::Faulted { .. })); + assert!(matches!( + evaluator.status(at(Utc::now().timestamp())), + PolicyStatus::Faulted { .. } + )); } /// The two faults are reported apart because only one of them is worth @@ -1533,16 +1553,19 @@ mod tests { fn the_two_faults_are_reported_apart() { let mut evaluator = PolicyEvaluator::new(Arc::new(Mutex::new(Some(VelocityTracker::new())))); - assert!(matches!(evaluator.status(), PolicyStatus::NotConfigured)); + assert!(matches!( + evaluator.status(at(Utc::now().timestamp())), + PolicyStatus::NotConfigured + )); evaluator.set_policy_faulted(PolicyFault::Unreadable, None); - let PolicyStatus::Faulted { fault } = evaluator.status() else { + let PolicyStatus::Faulted { fault } = evaluator.status(at(Utc::now().timestamp())) else { panic!("expected a fault"); }; assert_eq!(fault, PolicyFault::Unreadable); evaluator.set_policy_faulted(PolicyFault::Unverified, None); - let PolicyStatus::Faulted { fault } = evaluator.status() else { + let PolicyStatus::Faulted { fault } = evaluator.status(at(Utc::now().timestamp())) else { panic!("expected a fault"); }; assert_eq!(fault, PolicyFault::Unverified); From 6319e6e3fec5e1306ff15fa314e21ae38cf0687c Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Sun, 18 Oct 2026 10:09:13 -0400 Subject: [PATCH 4/7] Recover from a corrupt time floor, and stop a zero reading or a nudge spending its proof --- keep-mobile/src/clock_floor.rs | 158 ++++++++++++---- keep-mobile/src/lib.rs | 334 ++++++++++++++++++++++----------- keep-mobile/src/persistence.rs | 39 ++-- keep-mobile/src/policy.rs | 6 + 4 files changed, 372 insertions(+), 165 deletions(-) diff --git a/keep-mobile/src/clock_floor.rs b/keep-mobile/src/clock_floor.rs index 33af2f88..5996d9a3 100644 --- a/keep-mobile/src/clock_floor.rs +++ b/keep-mobile/src/clock_floor.rs @@ -53,6 +53,26 @@ pub(crate) struct PolicyClock { pub(crate) effective_secs: i64, } +/// How far the floor has to move before it is worth storing. +const PERSIST_QUANTUM_SECS: u64 = 60; + +/// A monotonic reading, or `None` when there is no usable one. +/// +/// Zero is treated as no reading. `elapsedRealtime()` cannot be zero by the time +/// an app is running, so a zero is a caller with no monotonic source that put +/// something in the argument rather than a real reading. Taken at face value it +/// is the worst value there is: it is below any stored anchor, so it re-anchors +/// the floor to boot, and the next genuine reading then credits the whole +/// device uptime as elapsed and advances the floor by weeks. The `Option` anchor +/// closes that on construction; this closes it on every other call. +/// +/// A caller that fabricates a small nonzero reading can still do it. That is a +/// caller lying about the monotonic clock, which this cannot detect, and why +/// the readings have to come from the platform. +fn reading(now_elapsed_ms: Option) -> Option { + now_elapsed_ms.filter(|&ms| ms > 0) +} + #[derive(Clone, Copy, Debug, Serialize, Deserialize)] pub(crate) struct ClockFloor { /// The latest wall-clock second the app accepts as already past. @@ -72,7 +92,7 @@ impl ClockFloor { pub(crate) fn new(now_secs: u64, now_elapsed_ms: Option) -> Self { Self { floor_secs: now_secs, - anchor_elapsed_ms: now_elapsed_ms, + anchor_elapsed_ms: reading(now_elapsed_ms), } } @@ -83,7 +103,7 @@ impl ClockFloor { /// then consulted and not advanced: raising it needs proof that time /// passed, and reading it does not. pub(crate) fn observe(self, now_secs: u64, now_elapsed_ms: Option) -> (Self, bool) { - let Some(now_elapsed_ms) = now_elapsed_ms else { + let Some(now_elapsed_ms) = reading(now_elapsed_ms) else { return (self, false); }; @@ -114,38 +134,40 @@ impl ClockFloor { if proven_secs == 0 { // Nothing proven, so nothing to advance by, and the anchor stays // where it is so the next observation measures from the same point - // rather than restarting the measurement. Not storing this is also - // what keeps a decision from writing to storage every time. + // rather than restarting the measurement. return (self, false); } let ceiling = self.floor_secs.saturating_add(proven_secs); let floor_secs = self.floor_secs.max(now_secs.min(ceiling)); - // The anchor moves by the whole seconds that were credited, not to the - // reading itself, so the remainder below a second is carried instead of - // discarded. Moving it to the reading would throw away up to 999 ms on - // every advance, and a caller asking just under every two seconds would - // then credit one second per two and hold the floor at half of real - // time. - let anchor_elapsed_ms = anchor.saturating_add(proven_secs.saturating_mul(1_000)); - // Stored only when the floor itself moved. When it did not, which is - // every call while the clock reads behind the floor, keeping the old - // anchor is what lets the proven time accrue rather than being consumed - // by a call that could not use it. It also keeps a decision from - // writing to storage every time one is more than a second after the - // last. - let changed = floor_secs != self.floor_secs; - ( - Self { - floor_secs, - anchor_elapsed_ms: if changed { - Some(anchor_elapsed_ms) - } else { - self.anchor_elapsed_ms - }, - }, - changed, - ) + let credited_secs = floor_secs - self.floor_secs; + + // The anchor moves by what was credited to the floor, not by what was + // proven. When the clock reading is the binding constraint the two + // differ, and moving by the proof would spend all of it on the little + // the clock allowed: one call that nudges the floor a second after a + // long idle would burn the whole accrual, and a later call could then + // raise the floor no further. Moving by the credit keeps the surplus + // for when the clock catches up, and still carries the remainder below + // a second, because only whole seconds are credited. + // + // The total advance still never exceeds the monotonic time proven since + // the original anchor: every second credited is a second of proof + // spent, and proof is only spent by crediting it. + let advanced = Self { + floor_secs, + anchor_elapsed_ms: Some(anchor.saturating_add(credited_secs.saturating_mul(1_000))), + }; + + // Worth storing once the floor has moved by a quantum, not on every + // second it moves. On a healthy device the floor follows the wall clock + // and so moves every second, and storing each of those would be an + // encrypted read and write per decision for as long as anything polls. + // Skipping a small move is safe: the stored pair stays consistent, and + // the next call measures from the older anchor and credits the same + // time again. The cost is that a rewind shorter than the quantum, made + // between two stores, is not caught. + (advanced, credited_secs >= PERSIST_QUANTUM_SECS) } /// The readings to judge validity with: the clock as given, and an expiry @@ -169,11 +191,14 @@ mod tests { const MIN: u64 = 60; const DAY: u64 = 24 * 3600; + /// A genuine post-boot monotonic reading. Zero is not one: it is read as no + /// reading at all, so tests that need a real anchor start here. + const BOOT: u64 = 1_000; #[test] fn winding_the_clock_back_does_not_move_the_expiry_time_back() { - let floor = ClockFloor::new(1_000_000, Some(0)); - let (floor, _) = floor.observe(1_000_000 + 10 * MIN, Some(10 * MIN * 1_000)); + let floor = ClockFloor::new(1_000_000, Some(BOOT)); + let (floor, _) = floor.observe(1_000_000 + 10 * MIN, Some(BOOT + 10 * MIN * 1_000)); // The attack: the date is set back a week. Expiry is judged against the // floor, so an expired policy stays expired. @@ -257,12 +282,12 @@ mod tests { /// two and hold the floor at half of real time. #[test] fn accrual_below_a_second_is_carried_rather_than_discarded() { - let mut floor = ClockFloor::new(1_000_000, Some(0)); + let mut floor = ClockFloor::new(1_000_000, Some(BOOT)); // Ten asks at 1,900 ms apart: 19 s of real time, and each ask on its // own proves only one whole second. for i in 1..=10u64 { - let reading = i * 1_900; + let reading = BOOT + i * 1_900; let (next, _) = floor.observe(1_000_000 + 19, Some(reading)); floor = next; } @@ -277,24 +302,83 @@ mod tests { /// storage and keeps the unused proven time accruing. #[test] fn an_advance_that_cannot_happen_is_not_stored() { - let floor = ClockFloor::new(1_000_000, Some(0)); + let floor = ClockFloor::new(1_000_000, Some(BOOT)); // The clock reads behind the floor, so there is nothing to advance to. - let (same, changed) = floor.observe(999_000, Some(60_000)); + let (same, changed) = floor.observe(999_000, Some(BOOT + 60_000)); assert!(!changed, "nothing moved, so nothing to store"); assert_eq!(same.floor_secs, 1_000_000); assert_eq!( same.anchor_elapsed_ms, - Some(0), + Some(BOOT), "the anchor stays so the minute still counts once the clock is back" ); // Once the clock is restored the whole minute is still available. - let (advanced, changed) = same.observe(1_000_060, Some(60_000)); + let (advanced, changed) = same.observe(1_000_060, Some(BOOT + 60_000)); assert!(changed); assert_eq!(advanced.floor_secs, 1_000_060); } + /// Proof the clock did not let the floor use is kept, not spent. One call + /// that nudges the floor a second after a long idle used to consume the + /// whole accrual, after which the floor could rise no further even once + /// the clock caught up, which is the difference between catching and + /// missing a rewind made after that. + #[test] + fn proof_the_clock_did_not_allow_is_carried_to_a_later_call() { + let floor = ClockFloor::new(1_000, Some(BOOT)); + + // A minute proven, but the clock reads only a second past the floor. + let (nudged, _) = floor.observe(1_001, Some(BOOT + 60_000)); + assert_eq!(nudged.floor_secs, 1_001); + + // The clock catches up within the same monotonic reading: the other + // 59 seconds of proof are still there to credit. + let (caught_up, _) = nudged.observe(2_000_000, Some(BOOT + 60_000)); + assert_eq!(caught_up.floor_secs, 1_060); + } + + /// A zero reading is no reading. Taken at face value it is below any + /// stored anchor, so it re-anchors the floor to boot, and the next genuine + /// reading then credits the whole device uptime: the floor parks weeks + /// ahead and every bundle, including one a warden signs today, is expired. + #[test] + fn a_zero_reading_cannot_re_anchor_the_floor_to_boot() { + let floor = ClockFloor::new(1_000_000, Some(5_000)); + + let (held, changed) = floor.observe(1_000_000, Some(0)); + assert!(!changed, "a zero is no reading, so nothing to store"); + assert_eq!(held.anchor_elapsed_ms, Some(5_000)); + + // The next genuine reading on a device up for 40 days, clock set far + // forward. Measured from the real anchor, not from a planted zero. + let (after, _) = held.observe(4_000_000_000, Some(5_000 + 1_000)); + assert_eq!(after.floor_secs, 1_000_001); + + // And construction reads zero the same way. + assert_eq!(ClockFloor::new(1_000_000, Some(0)).anchor_elapsed_ms, None); + } + + /// A move smaller than the quantum is used for the decision but not + /// stored, so a device whose floor follows the clock does not write on + /// every call. + #[test] + fn only_a_move_of_a_quantum_or_more_is_worth_storing() { + let floor = ClockFloor::new(1_000_000, Some(BOOT)); + + let (small, store) = floor.observe(1_000_030, Some(BOOT + 30_000)); + assert!(!store, "thirty seconds is under the quantum"); + assert_eq!(small.floor_secs, 1_000_030, "still used for this decision"); + + let (large, store) = floor.observe( + 1_000_000 + PERSIST_QUANTUM_SECS, + Some(BOOT + PERSIST_QUANTUM_SECS * 1_000), + ); + assert!(store); + assert_eq!(large.floor_secs, 1_000_000 + PERSIST_QUANTUM_SECS); + } + #[test] fn a_reboot_re_anchors_without_advancing_the_floor() { let floor = ClockFloor::new(1_000_000, Some(9_000_000)); diff --git a/keep-mobile/src/lib.rs b/keep-mobile/src/lib.rs index c8b84346..416fbdfa 100644 --- a/keep-mobile/src/lib.rs +++ b/keep-mobile/src/lib.rs @@ -639,6 +639,8 @@ pub struct KeepMobile { dkg_in_flight: Arc, pub(crate) runtime: tokio::runtime::Runtime, policy: Arc>, + /// Serializes the time floor's read-modify-write; see `policy_now`. + floor_lock: std::sync::Mutex<()>, velocity: Arc>>, descriptor_callbacks: Arc>>>, health_callbacks: Arc>>>, @@ -843,18 +845,32 @@ impl KeepMobile { Ok(clock) => Self::load_policy_into(&storage, &mut evaluator, clock), // Faulted only when there is a policy for the floor to protect. A // device with no policy record has nothing to wind the clock back - // into, and nothing deletes the floor record, so faulting here - // would leave such a device refusing permanently with no policy to - // delete to recover. - Err(_) => match persistence::load_policy(&storage) { - Err(KeepMobileError::StorageNotFound) => evaluator.clear_policy(), - _ => { - tracing::error!( - "Time floor unreadable, every policy rule will refuse until it is read" - ); - evaluator.set_policy_faulted(PolicyFault::Unreadable, None); + // into, so it starts with no policy, and the floor is retried on + // the next decision. A corrupt floor never reaches here on such a + // device, because it is replaced instead; what does is a read that + // did not complete. + Err(_) => { + // The wardens are loaded either way, because the floor is no + // reason to lose them. With the policy left out of memory, + // nothing re-reads them: on a device with no policy nothing is + // faulted, and on one with a policy the retry needs the floor + // first. Skipping them left the set empty for the life of the + // process while the record held wardens, so deleting the policy + // to recover from a corrupt floor ended with no trusted signer + // and the replacement refused. + if let Ok(wardens) = persistence::load_trusted_wardens(&storage) { + evaluator.set_trusted_wardens(wardens); } - }, + match persistence::load_policy(&storage) { + Err(KeepMobileError::StorageNotFound) => evaluator.clear_policy(), + _ => { + tracing::error!( + "Time floor unreadable, every policy rule will refuse until it is read" + ); + evaluator.set_policy_faulted(PolicyFault::Unreadable, None); + } + } + } } let policy = Arc::new(std::sync::RwLock::new(evaluator)); @@ -870,6 +886,7 @@ impl KeepMobile { dkg_in_flight: Arc::new(std::sync::atomic::AtomicBool::new(false)), runtime, policy, + floor_lock: std::sync::Mutex::new(()), velocity, descriptor_callbacks: Arc::new(RwLock::new(None)), health_callbacks: Arc::new(RwLock::new(None)), @@ -1916,10 +1933,13 @@ impl KeepMobile { /// Takes the clock so "in force" means what it says: a bundle whose window /// closed is not in force, even while this process still holds it, and /// reporting its limits would describe rules nothing will honor. - pub fn get_policy_info(&self, now_elapsed_ms: u64) -> Option { - let clock = self.policy_now(Some(now_elapsed_ms)).ok()?; - self.policy - .read() + pub fn get_policy_info(&self, now_elapsed_ms: Option) -> Option { + let clock = self.policy_now(now_elapsed_ms).ok()?; + // Healed first, as `policy_status` does, so the two cannot answer + // differently on the same tick: without it a transient fault that + // status would have cleared into `Active` still reads as no policy here. + self.reload_policy_if_transient(clock).ok()?; + self.policy_read_lock() .ok()? .policy_at(clock) .map(|b| b.to_policy_info()) @@ -1932,18 +1952,12 @@ impl KeepMobile { /// back, so its absence refuses. With none, there is nothing to wind back /// into: the device would otherwise be left refusing permanently over a /// record that guards nothing, and nothing deletes it. - fn floor_configured(&self) -> Result { - Ok(!matches!( - self.policy_read_lock()?.status(clock_floor::PolicyClock { - clock_secs: 0, - effective_secs: 0, - }), - PolicyStatus::NotConfigured - )) + fn policy_configured(&self) -> Result { + Ok(self.policy_read_lock()?.is_configured()) } fn floor_fault_status(&self, e: KeepMobileError) -> Result { - if self.floor_configured()? { + if self.policy_configured()? { tracing::error!("Time floor unreadable, the stored policy will refuse: {e}"); return Ok(PolicyStatus::Faulted { fault: PolicyFault::Unreadable, @@ -1953,7 +1967,7 @@ impl KeepMobile { } fn floor_fault_decision(&self, e: KeepMobileError) -> Result { - if self.floor_configured()? { + if self.policy_configured()? { tracing::error!("Time floor unreadable, refusing the spend: {e}"); return Ok(PolicyDecision::Deny { reason: "Cannot evaluate the spending policy: the time floor is unreadable".into(), @@ -1970,14 +1984,19 @@ impl KeepMobile { /// `now_elapsed_ms` is the platform's monotonic clock, as /// `SigningRateLimiter::check_and_record` already takes: on Android, /// `SystemClock.elapsedRealtime()`. It is what lets the stored time floor - /// advance, so passing a stale or invented value holds the floor still - /// rather than moving it anywhere useful. - pub fn policy_status(&self, now_elapsed_ms: u64) -> Result { + /// advance. Pass `None` when there is no such reading, never a stand-in: + /// `None` holds the floor where it is, which is safe, whereas an invented + /// small value re-anchors it and the next real reading credits the whole + /// device uptime. Zero is read as `None` for that reason. + pub fn policy_status( + &self, + now_elapsed_ms: Option, + ) -> Result { // The floor is advanced here, before anything can return early. It is // what makes winding the clock back detectable, and a floor that only // moved while the policy happened to be faulted would never move on a // working install, which is the case the attack is run against. - let clock = match self.policy_now(Some(now_elapsed_ms)) { + let clock = match self.policy_now(now_elapsed_ms) { Ok(clock) => clock, // Reported as a fault rather than an error: a caller rendering // status must be able to say the policy cannot be trusted, and @@ -2017,12 +2036,12 @@ impl KeepMobile { pub fn evaluate_policy( &self, ctx: TransactionContext, - now_elapsed_ms: u64, + now_elapsed_ms: Option, ) -> Result { self.reload_velocity_if_unknown()?; // As in `policy_status`: advanced before any early return, so the floor // tracks on an install where nothing is faulted. - let clock = match self.policy_now(Some(now_elapsed_ms)) { + let clock = match self.policy_now(now_elapsed_ms) { Ok(clock) => clock, // A refusal, not an error. The contract on `evaluate` says an error // is not a decision, so a floor that cannot be read would leave the @@ -4168,28 +4187,69 @@ impl KeepMobile { &self, now_elapsed_ms: Option, ) -> Result { - Self::floor_now(&self.storage, now_elapsed_ms) + // One read-modify-write at a time. Two calls racing through the load + // and the store would each observe the same old floor and the later + // store would win, losing the other's advance; with the floor resting + // on that record, a lost advance is a wider rewind window. Taken before + // the policy lock and never while it is held, so the order is fixed. + let _floor = self + .floor_lock + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + Self::floor_now(&self.storage, now_elapsed_ms, || { + // A poisoned lock answers "configured", which keeps the corrupt + // record and refuses rather than replacing it. + !self.policy_configured().unwrap_or(true) + }) } /// [`Self::policy_now`] before there is a `self`, for the load the - /// constructor does. + /// constructor does. No lock: nothing can call in before construction + /// returns. fn startup_policy_now( storage: &Arc, ) -> Result { - Self::floor_now(storage, None) + Self::floor_now(storage, None, || { + matches!( + persistence::load_policy(storage), + Err(KeepMobileError::StorageNotFound) + ) + }) } + /// `no_policy` is asked only when the stored floor is corrupt, and decides + /// whether it may be replaced. + /// + /// With no policy configured the floor guards nothing, so a corrupt record + /// is replaced with a fresh one. Refusing instead would leave it in place + /// forever, since nothing else rewrites it, and the device could never + /// install a policy again: deleting the old policy, which is the way out of + /// every other fault, would not help. With a policy configured it refuses, + /// because then the record is the only thing saying the clock was not wound + /// back; deleting the policy is the way out, after which the next call + /// replaces it. fn floor_now( storage: &Arc, now_elapsed_ms: Option, + no_policy: impl FnOnce() -> bool, ) -> Result { let now_secs = u64::try_from(chrono::Utc::now().timestamp()).unwrap_or(0); - let stored = persistence::load_clock_floor(storage)?; - let floor = - stored.unwrap_or_else(|| clock_floor::ClockFloor::new(now_secs, now_elapsed_ms)); + let fresh = || clock_floor::ClockFloor::new(now_secs, now_elapsed_ms); + let (floor, first_write) = match persistence::load_clock_floor(storage)? { + persistence::ClockFloorRead::Found(floor) => (floor, false), + persistence::ClockFloorRead::Absent => (fresh(), true), + persistence::ClockFloorRead::Corrupt if no_policy() => { + tracing::warn!("Replacing a corrupt time floor; no policy depends on it"); + (fresh(), true) + } + persistence::ClockFloorRead::Corrupt => { + return Err(KeepMobileError::StorageError { + msg: "The time floor is corrupt and a policy depends on it".into(), + }) + } + }; let (observed, changed) = floor.observe(now_secs, now_elapsed_ms); - // A first write counts as changed, so the floor exists from here on. - if changed || stored.is_none() { + if changed || first_write { persistence::persist_clock_floor(storage, &observed)?; } Ok(observed.clock_at(now_secs)) @@ -7232,7 +7292,7 @@ mod velocity_fault_tests { destinations: vec![], has_unknown_destination: false, }, - 0, + None, ) .unwrap(); @@ -7308,18 +7368,19 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); - let PolicyDecision::Deny { reason } = mobile.evaluate_policy(a_spend(), 0).unwrap() else { + let PolicyDecision::Deny { reason } = mobile.evaluate_policy(a_spend(), None).unwrap() + else { panic!("a tampered policy record must refuse, not allow everything"); }; assert!(reason.contains("policy"), "{reason}"); assert!( matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Faulted { .. } ), "the refusal has to be explainable, so it must not report as unconfigured" ); - assert!(mobile.get_policy_info(0).is_none()); + assert!(mobile.get_policy_info(None).is_none()); } /// A verified record still works, so the refusals above cannot be a policy @@ -7331,11 +7392,11 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Active { .. } )); assert_eq!( - mobile.evaluate_policy(a_spend(), 0).unwrap(), + mobile.evaluate_policy(a_spend(), None).unwrap(), PolicyDecision::Allow ); let over = TransactionContext { @@ -7343,7 +7404,7 @@ mod velocity_fault_tests { ..a_spend() }; assert!(matches!( - mobile.evaluate_policy(over, 0).unwrap(), + mobile.evaluate_policy(over, None).unwrap(), PolicyDecision::Deny { .. } )); } @@ -7354,11 +7415,11 @@ mod velocity_fault_tests { let storage = Arc::new(FaultingStorage::default()); let mobile = KeepMobile::new(storage as Arc).unwrap(); assert!(matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::NotConfigured )); assert_eq!( - mobile.evaluate_policy(a_spend(), 0).unwrap(), + mobile.evaluate_policy(a_spend(), None).unwrap(), PolicyDecision::Allow ); } @@ -7374,23 +7435,23 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Faulted { fault: PolicyFault::Unreadable } )); assert!(matches!( - mobile.evaluate_policy(a_spend(), 0).unwrap(), + mobile.evaluate_policy(a_spend(), None).unwrap(), PolicyDecision::Deny { .. } )); storage.heal(); assert!(matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Active { .. } )); assert_eq!( - mobile.evaluate_policy(a_spend(), 0).unwrap(), + mobile.evaluate_policy(a_spend(), None).unwrap(), PolicyDecision::Allow ); } @@ -7405,13 +7466,13 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Faulted { fault: PolicyFault::Unverified } )); assert!(matches!( - mobile.evaluate_policy(a_spend(), 0).unwrap(), + mobile.evaluate_policy(a_spend(), None).unwrap(), PolicyDecision::Deny { .. } )); } @@ -7426,7 +7487,7 @@ mod velocity_fault_tests { let warden = plant_policy(&storage, &capped_rules(), false); let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Faulted { .. } )); @@ -7434,7 +7495,7 @@ mod velocity_fault_tests { assert!( matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Active { .. } ), "trusting the signer must re-run the stored policy, not leave it faulted" @@ -7444,7 +7505,7 @@ mod velocity_fault_tests { ..a_spend() }; assert!(matches!( - mobile.evaluate_policy(over, 0).unwrap(), + mobile.evaluate_policy(over, None).unwrap(), PolicyDecision::Deny { .. } )); } @@ -7474,7 +7535,7 @@ mod velocity_fault_tests { mobile.delete_policy().unwrap(); assert!(matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::NotConfigured )); } @@ -7488,13 +7549,13 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Faulted { fault: PolicyFault::Unverified } )); assert!(matches!( - mobile.evaluate_policy(a_spend(), 0).unwrap(), + mobile.evaluate_policy(a_spend(), None).unwrap(), PolicyDecision::Deny { .. } )); } @@ -7509,7 +7570,7 @@ mod velocity_fault_tests { let warden = plant_policy(&storage, &capped_rules(), false); let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Faulted { .. } )); @@ -7518,7 +7579,7 @@ mod velocity_fault_tests { assert!( matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Active { .. } ), "the warden just added must not be dropped by the reload that follows it" @@ -7544,7 +7605,7 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Faulted { fault: PolicyFault::Unverified } @@ -7555,7 +7616,7 @@ mod velocity_fault_tests { }; assert!( matches!( - mobile.evaluate_policy(over, 0).unwrap(), + mobile.evaluate_policy(over, None).unwrap(), PolicyDecision::Deny { .. } ), "the raised cap must not take effect, and the record must not read as no policy" @@ -7573,7 +7634,7 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Faulted { fault: PolicyFault::Unverified } @@ -7581,7 +7642,7 @@ mod velocity_fault_tests { let after_load = storage.reads_of(POLICY_STORAGE_KEY); for _ in 0..5 { assert!(matches!( - mobile.evaluate_policy(a_spend(), 0).unwrap(), + mobile.evaluate_policy(a_spend(), None).unwrap(), PolicyDecision::Deny { .. } )); } @@ -7609,18 +7670,18 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Faulted { fault: PolicyFault::NotYetValid } )); assert!(matches!( - mobile.evaluate_policy(a_spend(), 0).unwrap(), + mobile.evaluate_policy(a_spend(), None).unwrap(), PolicyDecision::Deny { .. } )); let after_load = storage.reads_of(POLICY_STORAGE_KEY); - mobile.evaluate_policy(a_spend(), 0).unwrap(); + mobile.evaluate_policy(a_spend(), None).unwrap(); assert!( storage.reads_of(POLICY_STORAGE_KEY) > after_load, "a fault that resolves on its own has to be looked at again" @@ -7643,14 +7704,14 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Faulted { fault: PolicyFault::Expired } )); assert!( matches!( - mobile.evaluate_policy(a_spend(), 0).unwrap(), + mobile.evaluate_policy(a_spend(), None).unwrap(), PolicyDecision::Deny { .. } ), "stale rules are neither enforced nor ignored" @@ -7665,7 +7726,7 @@ mod velocity_fault_tests { .expect("a replacement must install over an expired policy"); assert!(matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Active { .. } )); } @@ -7684,7 +7745,7 @@ mod velocity_fault_tests { assert!(mobile.delete_policy().is_err()); assert!( matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Faulted { .. } ), "a failed delete must not leave memory reading as no policy" @@ -7713,7 +7774,7 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Faulted { fault: PolicyFault::Unreadable } @@ -7725,7 +7786,7 @@ mod velocity_fault_tests { .import_policy(hex::encode(replacement.to_bytes())) .expect("a healed fault must not force a delete"); assert!(matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Active { .. } )); } @@ -7751,7 +7812,7 @@ mod velocity_fault_tests { ); let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Faulted { fault: PolicyFault::Expired } @@ -7766,11 +7827,11 @@ mod velocity_fault_tests { // The refusal has to outlive the failed write, or the gate is open for // the life of the process on a policy storage never accepted. assert!(matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Faulted { .. } )); assert!(matches!( - mobile.evaluate_policy(a_spend(), 0).unwrap(), + mobile.evaluate_policy(a_spend(), None).unwrap(), PolicyDecision::Deny { .. } )); } @@ -7795,7 +7856,7 @@ mod velocity_fault_tests { ); let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Active { .. } )); @@ -7828,7 +7889,7 @@ mod velocity_fault_tests { "memory must not hold a warden storage refused" ); assert!(matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Faulted { .. } )); } @@ -7857,7 +7918,7 @@ mod velocity_fault_tests { ); let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Faulted { fault: PolicyFault::NotYetValid } @@ -7905,7 +7966,7 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Faulted { fault: PolicyFault::Unverified } @@ -7938,7 +7999,7 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Faulted { fault: PolicyFault::Unverified } @@ -7981,7 +8042,7 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.evaluate_policy(a_spend(), 0).unwrap(), + mobile.evaluate_policy(a_spend(), None).unwrap(), PolicyDecision::Deny { .. } )); } @@ -8015,7 +8076,7 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!( matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Faulted { fault: PolicyFault::Expired } @@ -8023,7 +8084,7 @@ mod velocity_fault_tests { "a bundle whose window closed against the floor must stay expired" ); assert!(matches!( - mobile.evaluate_policy(a_spend(), 0).unwrap(), + mobile.evaluate_policy(a_spend(), None).unwrap(), PolicyDecision::Deny { .. } )); @@ -8044,16 +8105,18 @@ mod velocity_fault_tests { storage.plant(CLOCK_FLOOR_STORAGE_KEY, b"not json".to_vec()); let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + // Unwrapped rather than defaulted: the distinction this pins is a + // refusal as opposed to an error, and a default would pass on either. assert!(matches!( - mobile.policy_status(0).unwrap_or(PolicyStatus::Faulted { + mobile.policy_status(None).unwrap(), + PolicyStatus::Faulted { fault: PolicyFault::Unreadable - }), - PolicyStatus::Faulted { .. } + } + )); + assert!(matches!( + mobile.evaluate_policy(a_spend(), None).unwrap(), + PolicyDecision::Deny { .. } )); - assert!(mobile - .evaluate_policy(a_spend(), 0) - .map(|d| matches!(d, PolicyDecision::Deny { .. })) - .unwrap_or(true)); } /// The floor has to advance on an install where nothing is wrong, which is @@ -8070,7 +8133,7 @@ mod velocity_fault_tests { // Nothing is faulted, so the reload path returns without doing // anything; only a floor advanced ahead of that check can move. assert!(matches!( - mobile.policy_status(1_000).unwrap(), + mobile.policy_status(Some(1_000)).unwrap(), PolicyStatus::Active { .. } )); @@ -8083,14 +8146,15 @@ mod velocity_fault_tests { "the reading has to reach the floor on a healthy install" ); - // A later decision cannot move the floor backwards. It also cannot move - // it ahead of the real clock, so on a healthy install it tracks rather - // than climbing; the advance itself is covered where the clock can be - // controlled, in `clock_floor`. - mobile.evaluate_policy(a_spend(), 3_601_000).unwrap(); + // An hour of monotonic time later the floor still cannot pass the real + // clock, which has barely moved, so it advances by less than the + // quantum and the record is left as it was: no write, and the stored + // anchor stays put so that hour remains available to credit later. + mobile.evaluate_policy(a_spend(), Some(3_601_000)).unwrap(); let later: crate::clock_floor::ClockFloor = serde_json::from_slice(&storage.raw(CLOCK_FLOOR_STORAGE_KEY).unwrap()).unwrap(); - assert!(later.floor_secs >= written.floor_secs); + assert_eq!(later.anchor_elapsed_ms, written.anchor_elapsed_ms); + assert_eq!(later.floor_secs, written.floor_secs); } /// A policy that was in force when it was read must stop being enforced @@ -8105,7 +8169,7 @@ mod velocity_fault_tests { plant_policy(&storage, &capped_rules(), true); let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Active { .. } )); @@ -8123,7 +8187,7 @@ mod velocity_fault_tests { assert!( matches!( - mobile.evaluate_policy(a_spend(), 0).unwrap(), + mobile.evaluate_policy(a_spend(), None).unwrap(), PolicyDecision::Deny { .. } ), "an expired bundle must refuse even while memory still holds it as active" @@ -8141,7 +8205,7 @@ mod velocity_fault_tests { plant_policy(&storage, &capped_rules(), true); let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Active { .. } )); @@ -8155,12 +8219,12 @@ mod velocity_fault_tests { ); assert!(matches!( - mobile.evaluate_policy(a_spend(), 0).unwrap(), + mobile.evaluate_policy(a_spend(), None).unwrap(), PolicyDecision::Deny { .. } )); assert!( matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::Faulted { fault: PolicyFault::Expired } @@ -8168,7 +8232,7 @@ mod velocity_fault_tests { "status must say why the spend was refused" ); assert!( - mobile.get_policy_info(0).is_none(), + mobile.get_policy_info(None).is_none(), "a bundle out of its window is not the policy in force" ); } @@ -8184,21 +8248,69 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - mobile.policy_status(0).unwrap(), + mobile.policy_status(None).unwrap(), PolicyStatus::NotConfigured )); assert!(matches!( - mobile.evaluate_policy(a_spend(), 0).unwrap(), + mobile.evaluate_policy(a_spend(), None).unwrap(), PolicyDecision::Allow )); - // And with a policy present the same unreadable record refuses, because - // then it is the only thing saying the clock was not wound back. + // The corrupt record was replaced, because nothing depended on it. + assert!(matches!( + crate::persistence::load_clock_floor(&(Arc::clone(&storage) as Arc)), + Ok(crate::persistence::ClockFloorRead::Found(_)) + )); + + // And with a policy present a corrupt record refuses, because then it + // is the only thing saying the clock was not wound back. plant_policy(&storage, &capped_rules(), true); + storage.plant(CLOCK_FLOOR_STORAGE_KEY, b"not json".to_vec()); let guarded = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( - guarded.evaluate_policy(a_spend(), 0).unwrap(), + guarded.evaluate_policy(a_spend(), None).unwrap(), + PolicyDecision::Deny { .. } + )); + } + + /// A corrupt floor must not leave a device unable to install a policy + /// ever again. Nothing rewrites the record, so before the fix the way out + /// of every other fault, deleting the policy, left it in place and every + /// later import refused on it for good. + #[test] + fn a_corrupt_floor_has_a_way_out_through_deleting_the_policy() { + let now = Utc::now().timestamp() as u64; + let storage = Arc::new(FaultingStorage::default()); + let warden = k256::schnorr::SigningKey::random(&mut k256::elliptic_curve::rand_core::OsRng); + let stored = create_test_bundle_with(&warden, &capped_rules(), now); + storage.plant( + POLICY_STORAGE_KEY, + serde_json::to_vec(&stored).expect("serialize policy"), + ); + storage.plant( + TRUSTED_WARDENS_KEY, + serde_json::to_vec(&vec![hex::encode(stored.warden_pubkey)]) + .expect("serialize wardens"), + ); + storage.plant(CLOCK_FLOOR_STORAGE_KEY, b"not json".to_vec()); + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + + // A policy depends on the floor, so it refuses rather than replacing it. + assert!(matches!( + mobile.evaluate_policy(a_spend(), None).unwrap(), PolicyDecision::Deny { .. } )); + + // Deleting the policy is the deliberate act that lets it go; the + // warden is still trusted, so a replacement installs. + mobile.delete_policy().unwrap(); + let replacement = create_test_bundle_with(&warden, &capped_rules(), now); + mobile + .import_policy(hex::encode(replacement.to_bytes())) + .expect("a device with no policy must be able to install one"); + assert!(matches!( + mobile.policy_status(None).unwrap(), + PolicyStatus::Active { .. } + )); } } diff --git a/keep-mobile/src/persistence.rs b/keep-mobile/src/persistence.rs index bf8c0cdf..31460a03 100644 --- a/keep-mobile/src/persistence.rs +++ b/keep-mobile/src/persistence.rs @@ -119,28 +119,33 @@ const VELOCITY_RECORD_MAX_LEN: usize = 128 * MAX_TRACKED_TRANSACTIONS + 1024; const CLOCK_FLOOR_RECORD_MAX_LEN: usize = 256; -/// Reads the stored time floor, or `None` when none has been written yet. +/// What reading the stored time floor found. /// -/// A read failure is an error rather than an absence, and the caller refuses on -/// it, for the reason the whole module exists: without the floor there is -/// nothing to say the clock was not wound back. +/// A record that is there and will not parse is a different statement from one +/// that could not be read. A read failure may clear on the next attempt, so it +/// refuses and is retried. A corrupt record will not, and treating it as a read +/// failure left it in place forever: nothing rewrites it, so a device that hit +/// it could never install a policy again, even after deleting the old one. +/// Reporting it apart lets the caller replace it where that is safe. +pub(crate) enum ClockFloorRead { + Found(ClockFloor), + Absent, + Corrupt, +} + +/// Reads the stored time floor. `Err` is a read that did not complete, which +/// the caller refuses on. pub(crate) fn load_clock_floor( storage: &Arc, -) -> Result, KeepMobileError> { +) -> Result { match storage.load_share_by_key(CLOCK_FLOOR_STORAGE_KEY.into()) { // Bounded before the parse, as the policy and counter records are. Two - // integer fields, so the ceiling is generous and still refuses anything - // that is not this record. - Ok(data) if data.len() > CLOCK_FLOOR_RECORD_MAX_LEN => Err(KeepMobileError::StorageError { - msg: "Clock floor record exceeds the maximum length".into(), - }), - Ok(data) => serde_json::from_slice(&data) - .map(Some) - // Without serde's message, which quotes the value it tripped on. - .map_err(|_| KeepMobileError::StorageError { - msg: "Failed to deserialize the clock floor".into(), - }), - Err(KeepMobileError::StorageNotFound) => Ok(None), + // integer fields, so anything this long is not this record. + Ok(data) if data.len() > CLOCK_FLOOR_RECORD_MAX_LEN => Ok(ClockFloorRead::Corrupt), + Ok(data) => Ok(serde_json::from_slice(&data) + .map(ClockFloorRead::Found) + .unwrap_or(ClockFloorRead::Corrupt)), + Err(KeepMobileError::StorageNotFound) => Ok(ClockFloorRead::Absent), Err(e) => Err(e), } } diff --git a/keep-mobile/src/policy.rs b/keep-mobile/src/policy.rs index 9a831c84..b09947b2 100644 --- a/keep-mobile/src/policy.rs +++ b/keep-mobile/src/policy.rs @@ -658,6 +658,12 @@ impl PolicyEvaluator { } } + /// Whether any policy record is held, in force or not. Asked without a + /// clock because the answer does not depend on one. + pub(crate) fn is_configured(&self) -> bool { + !matches!(self.policy, PolicyState::NotConfigured) + } + /// The bundle in force at `clock`, which is `None` once its window closes /// even while this evaluator still holds it. pub(crate) fn policy_at(&self, clock: PolicyClock) -> Option<&PolicyBundle> { From dbe1509031cb837ceae49c27df10703dcd95e4e8 Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Sun, 18 Oct 2026 10:21:44 -0400 Subject: [PATCH 5/7] Replace an unreadable time floor too, report a corrupt one as untrusted, lock it process-wide --- keep-mobile/src/lib.rs | 169 ++++++++++++++++++++++++++++++++------ keep-mobile/src/policy.rs | 11 +++ 2 files changed, 157 insertions(+), 23 deletions(-) diff --git a/keep-mobile/src/lib.rs b/keep-mobile/src/lib.rs index 416fbdfa..a4d1753e 100644 --- a/keep-mobile/src/lib.rs +++ b/keep-mobile/src/lib.rs @@ -351,6 +351,26 @@ const POLICY_STORAGE_KEY: &str = "__keep_policy_v1"; const VELOCITY_STORAGE_KEY: &str = "__keep_velocity_v1"; const TRUSTED_WARDENS_KEY: &str = "__keep_trusted_wardens_v1"; const CLOCK_FLOOR_STORAGE_KEY: &str = "__keep_clock_floor_v1"; + +/// The fault a time-floor error stands for. A corrupt record will not clear, so +/// it is the fault that is not retried and that says deleting the policy is the +/// way out; reporting it as unreadable would tell the caller to wait. A failed +/// read may clear, so it stays the retried one. +fn floor_fault(e: &KeepMobileError) -> PolicyFault { + match e { + KeepMobileError::InvalidPolicy { .. } => PolicyFault::ClockUntrusted, + _ => PolicyFault::Unreadable, + } +} + +/// Serializes the time floor's read-modify-write across the whole process. +/// +/// Process-wide rather than per instance because the record is: two instances +/// on the same storage would each hold their own lock and still race the load +/// and the store, and the later store would erase the other's advance. Taken +/// before any policy lock and never while one is held. It cannot cover another +/// process, which would need a lock in the storage itself. +static FLOOR_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); const CERT_PINS_STORAGE_KEY: &str = "__keep_cert_pins_v1"; const DESCRIPTOR_INDEX_KEY: &str = "__keep_descriptor_index_v1"; const DESCRIPTOR_KEY_PREFIX: &str = "__keep_descriptor_"; @@ -639,8 +659,6 @@ pub struct KeepMobile { dkg_in_flight: Arc, pub(crate) runtime: tokio::runtime::Runtime, policy: Arc>, - /// Serializes the time floor's read-modify-write; see `policy_now`. - floor_lock: std::sync::Mutex<()>, velocity: Arc>>, descriptor_callbacks: Arc>>>, health_callbacks: Arc>>>, @@ -843,6 +861,15 @@ impl KeepMobile { // spend, and the fault is retried before each decision. match Self::startup_policy_now(&storage) { Ok(clock) => Self::load_policy_into(&storage, &mut evaluator, clock), + Err(e) if matches!(e, KeepMobileError::InvalidPolicy { .. }) => { + if let Ok(wardens) = persistence::load_trusted_wardens(&storage) { + evaluator.set_trusted_wardens(wardens); + } + tracing::error!( + "Time floor corrupt, every policy rule will refuse until the policy is deleted" + ); + evaluator.set_policy_faulted(floor_fault(&e), None); + } // Faulted only when there is a policy for the floor to protect. A // device with no policy record has nothing to wind the clock back // into, so it starts with no policy, and the floor is retried on @@ -886,7 +913,6 @@ impl KeepMobile { dkg_in_flight: Arc::new(std::sync::atomic::AtomicBool::new(false)), runtime, policy, - floor_lock: std::sync::Mutex::new(()), velocity, descriptor_callbacks: Arc::new(RwLock::new(None)), health_callbacks: Arc::new(RwLock::new(None)), @@ -1958,19 +1984,19 @@ impl KeepMobile { fn floor_fault_status(&self, e: KeepMobileError) -> Result { if self.policy_configured()? { - tracing::error!("Time floor unreadable, the stored policy will refuse: {e}"); - return Ok(PolicyStatus::Faulted { - fault: PolicyFault::Unreadable, - }); + let fault = floor_fault(&e); + tracing::error!("Time floor unusable ({fault}), the stored policy will refuse"); + return Ok(PolicyStatus::Faulted { fault }); } Ok(PolicyStatus::NotConfigured) } fn floor_fault_decision(&self, e: KeepMobileError) -> Result { if self.policy_configured()? { - tracing::error!("Time floor unreadable, refusing the spend: {e}"); + let fault = floor_fault(&e); + tracing::error!("Time floor unusable ({fault}), refusing the spend"); return Ok(PolicyDecision::Deny { - reason: "Cannot evaluate the spending policy: the time floor is unreadable".into(), + reason: format!("Cannot evaluate the spending policy: {fault}"), }); } Ok(PolicyDecision::Allow) @@ -4192,8 +4218,7 @@ impl KeepMobile { // store would win, losing the other's advance; with the floor resting // on that record, a lost advance is a wider rewind window. Taken before // the policy lock and never while it is held, so the order is fixed. - let _floor = self - .floor_lock + let _floor = FLOOR_LOCK .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()); Self::floor_now(&self.storage, now_elapsed_ms, || { @@ -4204,11 +4229,15 @@ impl KeepMobile { } /// [`Self::policy_now`] before there is a `self`, for the load the - /// constructor does. No lock: nothing can call in before construction - /// returns. + /// constructor does. Locked like any other call: nothing can call into + /// this instance before it exists, but another instance on the same + /// storage can be running. fn startup_policy_now( storage: &Arc, ) -> Result { + let _floor = FLOOR_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); Self::floor_now(storage, None, || { matches!( persistence::load_policy(storage), @@ -4235,18 +4264,32 @@ impl KeepMobile { ) -> Result { let now_secs = u64::try_from(chrono::Utc::now().timestamp()).unwrap_or(0); let fresh = || clock_floor::ClockFloor::new(now_secs, now_elapsed_ms); - let (floor, first_write) = match persistence::load_clock_floor(storage)? { - persistence::ClockFloorRead::Found(floor) => (floor, false), - persistence::ClockFloorRead::Absent => (fresh(), true), - persistence::ClockFloorRead::Corrupt if no_policy() => { - tracing::warn!("Replacing a corrupt time floor; no policy depends on it"); + let (floor, first_write) = match persistence::load_clock_floor(storage) { + Ok(persistence::ClockFloorRead::Found(floor)) => (floor, false), + Ok(persistence::ClockFloorRead::Absent) => (fresh(), true), + // A record that will not parse, or will not read, is replaced when + // no policy depends on it, and both for the same reason. A read + // that fails is not only transient: a record whose key alias was + // invalidated comes back as an error (see `load_share_by_key`) and + // never recovers, so refusing on it would lock a device out of + // installing a policy for good, exactly as a corrupt one did. With + // no policy the floor's only job is judging an import, and + // installing any bundle is never looser than having none. + _ if no_policy() => { + tracing::warn!("Replacing an unusable time floor; no policy depends on it"); (fresh(), true) } - persistence::ClockFloorRead::Corrupt => { - return Err(KeepMobileError::StorageError { + // With a policy the record is the only thing saying the clock was + // not wound back, so it refuses. A corrupt one is reported as + // `InvalidPolicy`, which the callers map to a fault that is not + // retried, because it will not clear; a failed read keeps its own + // error, which they map to one that is. + Ok(persistence::ClockFloorRead::Corrupt) => { + return Err(KeepMobileError::InvalidPolicy { msg: "The time floor is corrupt and a policy depends on it".into(), }) } + Err(e) => return Err(e), }; let (observed, changed) = floor.observe(now_secs, now_elapsed_ms); if changed || first_write { @@ -8096,10 +8139,12 @@ mod velocity_fault_tests { .expect_err("an import judged against the floor is expired too"); } - /// A floor that cannot be read leaves the policy refusing rather than - /// enforcing, because nothing then says the clock was not wound back. + /// A corrupt floor with a policy depending on it refuses, and says so as a + /// fault that is not retried. Reporting it as unreadable would tell a caller + /// to wait for a fault that never clears, when deleting the policy is the + /// only way out. #[test] - fn an_unreadable_floor_refuses_every_rule() { + fn a_corrupt_floor_refuses_as_untrusted_while_a_policy_depends_on_it() { let storage = Arc::new(FaultingStorage::default()); plant_policy(&storage, &capped_rules(), true); storage.plant(CLOCK_FLOOR_STORAGE_KEY, b"not json".to_vec()); @@ -8107,6 +8152,37 @@ mod velocity_fault_tests { let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); // Unwrapped rather than defaulted: the distinction this pins is a // refusal as opposed to an error, and a default would pass on either. + let status = mobile.policy_status(None).unwrap(); + assert!( + matches!( + status, + PolicyStatus::Faulted { + fault: PolicyFault::ClockUntrusted + } + ), + "{status:?}" + ); + assert!(!PolicyFault::ClockUntrusted.is_transient()); + assert!(matches!( + mobile.evaluate_policy(a_spend(), None).unwrap(), + PolicyDecision::Deny { .. } + )); + } + + /// A floor that cannot be read, as opposed to one that will not parse, may + /// clear, so with a policy depending on it it refuses as the fault that is + /// retried. + #[test] + fn an_unreadable_floor_refuses_as_retryable_while_a_policy_depends_on_it() { + let storage = Arc::new(FaultingStorage::default()); + plant_policy(&storage, &capped_rules(), true); + storage.plant( + CLOCK_FLOOR_STORAGE_KEY, + serde_json::to_vec(&crate::clock_floor::ClockFloor::new(1, None)).unwrap(), + ); + storage.fault(CLOCK_FLOOR_STORAGE_KEY); + + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); assert!(matches!( mobile.policy_status(None).unwrap(), PolicyStatus::Faulted { @@ -8119,6 +8195,53 @@ mod velocity_fault_tests { )); } + /// An unreadable floor must not lock a device with no policy out of ever + /// installing one. A read that fails is not always transient: a record + /// whose key alias was invalidated comes back as an error and never + /// recovers, so refusing on it was the corrupt-record lockout again + /// through a different door. + #[test] + fn an_unreadable_floor_does_not_block_installing_a_first_policy() { + let now = Utc::now().timestamp() as u64; + let storage = Arc::new(FaultingStorage::default()); + let warden = k256::schnorr::SigningKey::random(&mut k256::elliptic_curve::rand_core::OsRng); + let bundle = create_test_bundle_with(&warden, &capped_rules(), now); + storage.plant( + TRUSTED_WARDENS_KEY, + serde_json::to_vec(&vec![hex::encode(bundle.warden_pubkey)]) + .expect("serialize wardens"), + ); + storage.plant( + CLOCK_FLOOR_STORAGE_KEY, + serde_json::to_vec(&crate::clock_floor::ClockFloor::new(1, None)).unwrap(), + ); + storage.fault(CLOCK_FLOOR_STORAGE_KEY); + + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + mobile + .import_policy(hex::encode(bundle.to_bytes())) + .expect("no policy depends on the floor, so it must not block the first one"); + + // Now one does. A floor that still will not read is the only thing that + // would say the clock was not wound back, so the policy refuses on it + // rather than enforcing unchecked, and it is the retried fault. + assert!(matches!( + mobile.policy_status(None).unwrap(), + PolicyStatus::Faulted { + fault: PolicyFault::Unreadable + } + )); + + // And once the record reads again, which is what the replacement write + // is for, the policy is in force. Whether that write repairs a read on + // a real invalidated key alias is the platform's to answer. + storage.heal(); + assert!(matches!( + mobile.policy_status(None).unwrap(), + PolicyStatus::Active { .. } + )); + } + /// The floor has to advance on an install where nothing is wrong, which is /// the only kind the attack is run against. A floor that moved only while /// the policy happened to be faulted would never move at all on a working diff --git a/keep-mobile/src/policy.rs b/keep-mobile/src/policy.rs index b09947b2..bfd2c4ef 100644 --- a/keep-mobile/src/policy.rs +++ b/keep-mobile/src/policy.rs @@ -230,6 +230,13 @@ pub enum PolicyFault { /// `created_at` is still a sound baseline to measure a downgrade against, /// and it is the only fault that supplies one. Expired, + /// The policy itself is fine, but the stored time floor it is judged + /// against is corrupt, so nothing says the clock was not wound back. Not + /// retried, because a record that will not parse will not parse next time + /// either, and reporting it as [`Self::Unreadable`] would tell a caller to + /// wait for a fault that never clears. Deleting the policy is the way out: + /// with no policy the floor guards nothing and is replaced. + ClockUntrusted, } impl PolicyFault { @@ -248,6 +255,10 @@ impl std::fmt::Display for PolicyFault { write!(f, "the stored policy is dated ahead of this device's clock") } Self::Expired => write!(f, "the stored policy has expired"), + Self::ClockUntrusted => write!( + f, + "this device's record of the current time is corrupt; delete the policy to reset it" + ), } } } From cdea2b1b25b12c49f0a80f23176c01837a3e79ca Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Sun, 18 Oct 2026 10:26:02 -0400 Subject: [PATCH 6/7] Keep a storage error from reading as a corrupt time floor --- keep-mobile/src/lib.rs | 61 +++++++++++++++++++++++++++++++++++++-- keep-mobile/src/policy.rs | 6 ++-- 2 files changed, 63 insertions(+), 4 deletions(-) diff --git a/keep-mobile/src/lib.rs b/keep-mobile/src/lib.rs index a4d1753e..386997cb 100644 --- a/keep-mobile/src/lib.rs +++ b/keep-mobile/src/lib.rs @@ -356,6 +356,11 @@ const CLOCK_FLOOR_STORAGE_KEY: &str = "__keep_clock_floor_v1"; /// it is the fault that is not retried and that says deleting the policy is the /// way out; reporting it as unreadable would tell the caller to wait. A failed /// read may clear, so it stays the retried one. +/// +/// Sound only because `floor_now` produces `InvalidPolicy` for a corrupt record +/// and nothing else: storage errors are passed through `floor_storage_error` +/// on the way out, so a storage implementation returning that variant cannot be +/// read as corruption and turn a transient fault into one that is never retried. fn floor_fault(e: &KeepMobileError) -> PolicyFault { match e { KeepMobileError::InvalidPolicy { .. } => PolicyFault::ClockUntrusted, @@ -363,6 +368,19 @@ fn floor_fault(e: &KeepMobileError) -> PolicyFault { } } +/// A storage error reaching the time floor, with `InvalidPolicy` taken out of +/// it. That variant is how `floor_now` says a record is corrupt, and the storage +/// error type admits any variant, so one coming from storage is restated as the +/// read failure it is. +fn floor_storage_error(e: KeepMobileError) -> KeepMobileError { + match e { + KeepMobileError::InvalidPolicy { .. } => KeepMobileError::StorageError { + msg: "The time floor could not be read or written".into(), + }, + other => other, + } +} + /// Serializes the time floor's read-modify-write across the whole process. /// /// Process-wide rather than per instance because the record is: two instances @@ -4289,11 +4307,11 @@ impl KeepMobile { msg: "The time floor is corrupt and a policy depends on it".into(), }) } - Err(e) => return Err(e), + Err(e) => return Err(floor_storage_error(e)), }; let (observed, changed) = floor.observe(now_secs, now_elapsed_ms); if changed || first_write { - persistence::persist_clock_floor(storage, &observed)?; + persistence::persist_clock_floor(storage, &observed).map_err(floor_storage_error)?; } Ok(observed.clock_at(now_secs)) } @@ -7147,6 +7165,9 @@ mod velocity_fault_tests { struct FaultingStorage { records: StdMutex>>, unreadable: StdMutex>, + /// Reads of these fault with `InvalidPolicy`, which the storage error + /// type admits, to pin that a caller does not read meaning into it. + misreported: StdMutex>, unwritable: StdMutex>, undeletable: StdMutex>, reads: StdMutex>, @@ -7157,6 +7178,10 @@ mod velocity_fault_tests { self.unreadable.lock().unwrap().push(key.into()); } + fn misreport(&self, key: &str) { + self.misreported.lock().unwrap().push(key.into()); + } + fn fault_write(&self, key: &str) { self.unwritable.lock().unwrap().push(key.into()); } @@ -7222,6 +7247,11 @@ mod velocity_fault_tests { msg: "cannot decrypt right now".into(), }); } + if self.misreported.lock().unwrap().contains(&key) { + return Err(KeepMobileError::InvalidPolicy { + msg: "reported by storage".into(), + }); + } self.records .lock() .unwrap() @@ -8436,4 +8466,31 @@ mod velocity_fault_tests { PolicyStatus::Active { .. } )); } + + /// A read failure the storage reports with `InvalidPolicy` is still a read + /// failure. That variant is how the floor says its record is corrupt, which + /// is a fault that is never retried, so taking it at face value from + /// storage would hold a transient fault for the life of the process. + #[test] + fn a_storage_error_is_not_mistaken_for_a_corrupt_floor() { + let storage = Arc::new(FaultingStorage::default()); + plant_policy(&storage, &capped_rules(), true); + storage.plant( + CLOCK_FLOOR_STORAGE_KEY, + serde_json::to_vec(&crate::clock_floor::ClockFloor::new(1, None)).unwrap(), + ); + storage.misreport(CLOCK_FLOOR_STORAGE_KEY); + + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + let status = mobile.policy_status(None).unwrap(); + assert!( + matches!( + status, + PolicyStatus::Faulted { + fault: PolicyFault::Unreadable + } + ), + "{status:?}" + ); + } } diff --git a/keep-mobile/src/policy.rs b/keep-mobile/src/policy.rs index bfd2c4ef..0fbd1c29 100644 --- a/keep-mobile/src/policy.rs +++ b/keep-mobile/src/policy.rs @@ -230,8 +230,10 @@ pub enum PolicyFault { /// `created_at` is still a sound baseline to measure a downgrade against, /// and it is the only fault that supplies one. Expired, - /// The policy itself is fine, but the stored time floor it is judged - /// against is corrupt, so nothing says the clock was not wound back. Not + /// The stored time floor the policy is judged against is corrupt, so + /// nothing says the clock was not wound back. The policy may well be fine; + /// it was not checked, because its validity cannot be judged without a + /// floor. Not /// retried, because a record that will not parse will not parse next time /// either, and reporting it as [`Self::Unreadable`] would tell a caller to /// wait for a fault that never clears. Deleting the policy is the way out: From 4ec85139b98803392c354afaeff726cf4c668122 Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Sun, 18 Oct 2026 11:46:15 -0400 Subject: [PATCH 7/7] Reset the time floor when the policy is deleted --- keep-mobile/src/lib.rs | 57 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/keep-mobile/src/lib.rs b/keep-mobile/src/lib.rs index 386997cb..bf03ad92 100644 --- a/keep-mobile/src/lib.rs +++ b/keep-mobile/src/lib.rs @@ -2066,12 +2066,40 @@ impl KeepMobile { // process, and faults again at the next launch. One guard spans both, // so a reload cannot land between them and re-arm the fault after this // has already returned. + // + // The floor lock is taken first, the order `policy_now` uses. + let _floor = FLOOR_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); let mut policy = self.policy_write_lock()?; match self.storage.delete_share_by_key(POLICY_STORAGE_KEY.into()) { Ok(()) | Err(KeepMobileError::StorageNotFound) => {} Err(e) => return Err(e), } policy.clear_policy(); + + // The time floor goes with it. A floor is seeded from the wall clock + // when it is first written, before any monotonic reading can bound it, + // so a device whose clock was far ahead at that moment holds a floor in + // the future: every bundle reads as expired, including one a warden + // signs today, and nothing lowers it. Without this, deleting the policy, + // the way out of every other fault, left that in place, and no policy + // could be installed until real time caught up. + // + // Safe because with no policy the floor guards nothing. Resetting it + // does let an older bundle install that the old floor would have + // refused, but the deletion above already left the device with no + // policy, which is looser than any bundle; the gap is that deleting is + // unauthenticated, not this. A floor that will not delete is reported + // and does not undo the deletion: the policy is gone either way, and + // calling this again retries it. + match self + .storage + .delete_share_by_key(CLOCK_FLOOR_STORAGE_KEY.into()) + { + Ok(()) | Err(KeepMobileError::StorageNotFound) => {} + Err(_) => tracing::warn!("The time floor could not be reset; delete the policy again"), + } Ok(()) } @@ -8493,4 +8521,33 @@ mod velocity_fault_tests { "{status:?}" ); } + + /// A floor seeded from a clock that was far ahead must have a way out. It is + /// written from the wall clock before anything can bound it, so it holds a + /// time in the future that nothing lowers, and every bundle, including one + /// signed today, reads as expired. Deleting the policy is that way out. + #[test] + fn deleting_the_policy_resets_a_floor_seeded_from_a_clock_far_ahead() { + let now = Utc::now().timestamp() as u64; + let storage = Arc::new(FaultingStorage::default()); + let warden = k256::schnorr::SigningKey::random(&mut k256::elliptic_curve::rand_core::OsRng); + let bundle = create_test_bundle_with(&warden, &capped_rules(), now); + storage.plant( + TRUSTED_WARDENS_KEY, + serde_json::to_vec(&vec![hex::encode(bundle.warden_pubkey)]).unwrap(), + ); + storage.plant( + CLOCK_FLOOR_STORAGE_KEY, + serde_json::to_vec(&crate::clock_floor::ClockFloor::new( + now + 40 * 24 * 3600, + None, + )) + .unwrap(), + ); + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + let _ = mobile.delete_policy(); + mobile + .import_policy(hex::encode(bundle.to_bytes())) + .expect("deleting the policy must be a way out of a floor parked ahead"); + } }