From 12bde6ca6b85a221344b89e927d43fce243a8e76 Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Sat, 17 Oct 2026 21:57:42 -0400 Subject: [PATCH 1/2] Bound the spending history by count, so a moved clock cannot erase it --- keep-mobile/src/velocity.rs | 88 +++++++++++++++++++++++++++++-------- 1 file changed, 69 insertions(+), 19 deletions(-) diff --git a/keep-mobile/src/velocity.rs b/keep-mobile/src/velocity.rs index e735ee44..9e279c0b 100644 --- a/keep-mobile/src/velocity.rs +++ b/keep-mobile/src/velocity.rs @@ -5,6 +5,20 @@ use chrono::{DateTime, Duration, Utc}; use serde::{Deserialize, Serialize}; use std::collections::VecDeque; +/// The only bound on the history, and deliberately a count rather than an age. +/// +/// Dropping entries older than the longest limit window looks equivalent and is +/// not: the cutoff comes from the device wall clock, and a clock moved forward +/// past that window matches every entry. Reading the record applied the prune, +/// and the next spend wrote the pruned history back, so a clock pushed eight +/// days ahead and then corrected left `daily_total` and `weekly_total` reading +/// zero permanently, with the limits they back bypassed and no way to recover +/// the history they were counting. +/// +/// A count cannot be moved by the clock. The totals already select their own +/// window when they are asked, so nothing needed the stored history to be +/// pruned by time for the limits to be right; the prune only bounded the size, +/// which this does without trusting anything. const MAX_TRACKED_TRANSACTIONS: usize = 10_000; #[derive(Clone, Debug, Serialize, Deserialize)] @@ -26,8 +40,6 @@ impl VelocityTracker { } pub fn record(&mut self, amount_sats: u64) { - self.cleanup_old(); - if self.transactions.len() >= MAX_TRACKED_TRANSACTIONS { self.transactions.pop_front(); } @@ -56,17 +68,6 @@ impl VelocityTracker { .fold(0u64, |acc, amt| acc.saturating_add(amt)) } - pub fn cleanup_old(&mut self) { - let cutoff = Utc::now() - Duration::days(7); - while let Some(front) = self.transactions.front() { - if front.timestamp <= cutoff { - self.transactions.pop_front(); - } else { - break; - } - } - } - #[allow(dead_code)] pub fn transaction_count(&self) -> usize { self.transactions.len() @@ -77,9 +78,7 @@ impl VelocityTracker { } pub fn from_bytes(data: &[u8]) -> Result { - let mut tracker: Self = serde_json::from_slice(data)?; - tracker.cleanup_old(); - Ok(tracker) + serde_json::from_slice(data) } } @@ -114,12 +113,63 @@ mod tests { assert_eq!(tracker.daily_total(), u64::MAX); } + /// Reading the record must not discard what it holds, whatever the clock + /// says about the age of the entries. + /// + /// A spend recorded an hour ago looks eight days old to a device whose + /// clock was pushed eight days forward, which is what the entries here + /// stand in for: the age cannot be produced by moving the real clock in a + /// test, so it is written into the timestamps instead. Pruning on load + /// dropped exactly these, and the next spend persisted the result, so the + /// loss outlived the clock being corrected and the daily and weekly limits + /// were left counting nothing. #[test] - fn test_cleanup() { + fn a_clock_moved_past_the_window_does_not_erase_the_history() { let mut tracker = VelocityTracker::new(); + tracker.transactions.push_back(TrackedTransaction { + timestamp: Utc::now() - Duration::days(8), + amount_sats: 1000, + }); + tracker.transactions.push_back(TrackedTransaction { + timestamp: Utc::now() - Duration::days(9), + amount_sats: 2000, + }); + let stored = tracker.to_bytes().expect("serialize"); + + let reloaded = VelocityTracker::from_bytes(&stored).expect("deserialize"); + assert_eq!( + reloaded.transaction_count(), + 2, + "reading the record must not discard what it holds" + ); + + // Recording a spend persists the history as it stands, so a write + // cannot launder the loss into storage either. + let mut reloaded = reloaded; + reloaded.record(500); + let round_tripped = VelocityTracker::from_bytes(&reloaded.to_bytes().expect("serialize")) + .expect("deserialize"); + assert_eq!(round_tripped.transaction_count(), 3); + } + + /// Entries outside a window are not counted by it, which is the job the + /// prune was wrongly credited with: the totals select their own window, so + /// the stored history never had to be trimmed for them to be right. + #[test] + fn the_totals_select_their_own_window() { + let mut tracker = VelocityTracker::new(); + tracker.transactions.push_back(TrackedTransaction { + timestamp: Utc::now() - Duration::days(3), + amount_sats: 5000, + }); tracker.record(1000); - tracker.cleanup_old(); - assert_eq!(tracker.transaction_count(), 1); + + assert_eq!( + tracker.daily_total(), + 1000, + "a 3-day-old spend is not today" + ); + assert_eq!(tracker.weekly_total(), 6000, "but it is inside the week"); } #[test] From 4f5119b6de859a48fc11bf401f2f655b3077aee9 Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Sat, 17 Oct 2026 22:08:14 -0400 Subject: [PATCH 2/2] Refuse a counters record over the cap rather than trimming it on read --- keep-mobile/src/lib.rs | 41 ++++++++++++++++++++++++++++++++++ keep-mobile/src/persistence.rs | 34 ++++++++++++++++++++++++---- keep-mobile/src/velocity.rs | 6 ++++- 3 files changed, 76 insertions(+), 5 deletions(-) diff --git a/keep-mobile/src/lib.rs b/keep-mobile/src/lib.rs index 5d09a90f..c561ffae 100644 --- a/keep-mobile/src/lib.rs +++ b/keep-mobile/src/lib.rs @@ -7760,4 +7760,45 @@ mod velocity_fault_tests { } )); } + + /// A counters record holding more than the cap is refused, not trimmed. + /// Trimming on load is the shape that let a moved clock erase the history, + /// and the counters already deny every rule that depends on them while they + /// are unknown. + #[test] + fn an_over_capacity_velocity_record_refuses_rather_than_being_trimmed() { + use crate::velocity::MAX_TRACKED_TRANSACTIONS; + + let storage = Arc::new(FaultingStorage::default()); + let mut tracker = VelocityTracker::new(); + for _ in 0..MAX_TRACKED_TRANSACTIONS + 1 { + tracker.record(1); + } + // `record` enforces the cap, so the over-capacity record has to be + // built by hand: this is a record that arrived from somewhere the cap + // was not applied. + let mut raw: serde_json::Value = + serde_json::from_slice(&tracker.to_bytes().expect("serialize")).unwrap(); + let entries = raw + .get_mut("transactions") + .and_then(|t| t.as_array_mut()) + .expect("transactions array"); + let one = entries[0].clone(); + while entries.len() <= MAX_TRACKED_TRANSACTIONS { + entries.push(one.clone()); + } + storage.plant(VELOCITY_STORAGE_KEY, serde_json::to_vec(&raw).unwrap()); + + let rules = PolicyRules { + daily_limit: Some(100_000), + ..Default::default() + }; + plant_policy(&storage, &rules, true); + + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + assert!(matches!( + mobile.evaluate_policy(a_spend()).unwrap(), + PolicyDecision::Deny { .. } + )); + } } diff --git a/keep-mobile/src/persistence.rs b/keep-mobile/src/persistence.rs index e0da74c9..a048e5a7 100644 --- a/keep-mobile/src/persistence.rs +++ b/keep-mobile/src/persistence.rs @@ -11,7 +11,7 @@ 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; +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, @@ -106,17 +106,43 @@ pub(crate) fn persist_policy( storage.store_share_by_key(POLICY_STORAGE_KEY.into(), data, storage_metadata("policy")) } +/// Loose ceiling on the stored counters, applied before they are parsed. +/// +/// `record` is the only place the entry cap is enforced, and it evicts one +/// entry per call, so a record that arrives over the cap would be loaded whole +/// and would not come back under it for thousands of spends. The count is +/// checked after the parse; this bounds what serde is asked to allocate first, +/// at a generous 128 bytes per entry against a worst case nearer 75. +const VELOCITY_RECORD_MAX_LEN: usize = 128 * MAX_TRACKED_TRANSACTIONS + 1024; + pub(crate) fn load_velocity( storage: &Arc, ) -> Result { match storage.load_share_by_key(VELOCITY_STORAGE_KEY.into()) { + Ok(data) if data.len() > VELOCITY_RECORD_MAX_LEN => Err(KeepMobileError::StorageError { + msg: "Velocity record exceeds the maximum length".into(), + }), // Deliberately without serde's message. It quotes the value it tripped // on, which here is an amount or a timestamp out of the record, and this // error reaches a log. What a caller can act on is that the record did // not parse, which is the same either way. - Ok(data) => VelocityTracker::from_bytes(&data).map_err(|_| KeepMobileError::StorageError { - msg: "Failed to deserialize velocity".into(), - }), + Ok(data) => { + let tracker = + VelocityTracker::from_bytes(&data).map_err(|_| KeepMobileError::StorageError { + msg: "Failed to deserialize velocity".into(), + })?; + // Refused, not trimmed. Trimming here would put a destructive write + // back on the read path, which is the shape that made a moved clock + // erase the history in the first place; see the note on + // `load_descriptors`. Refusing leaves the counters unknown, and + // unknown already denies every rule that depends on them. + if tracker.over_capacity() { + return Err(KeepMobileError::StorageError { + msg: "Velocity record holds more entries than the cap allows".into(), + }); + } + Ok(tracker) + } Err(KeepMobileError::StorageNotFound) => Ok(VelocityTracker::new()), Err(e) => Err(KeepMobileError::StorageError { msg: format!("Failed to load velocity tracker: {e}"), diff --git a/keep-mobile/src/velocity.rs b/keep-mobile/src/velocity.rs index 9e279c0b..8bbabef6 100644 --- a/keep-mobile/src/velocity.rs +++ b/keep-mobile/src/velocity.rs @@ -19,7 +19,7 @@ use std::collections::VecDeque; /// window when they are asked, so nothing needed the stored history to be /// pruned by time for the limits to be right; the prune only bounded the size, /// which this does without trusting anything. -const MAX_TRACKED_TRANSACTIONS: usize = 10_000; +pub(crate) const MAX_TRACKED_TRANSACTIONS: usize = 10_000; #[derive(Clone, Debug, Serialize, Deserialize)] struct TrackedTransaction { @@ -80,6 +80,10 @@ impl VelocityTracker { pub fn from_bytes(data: &[u8]) -> Result { serde_json::from_slice(data) } + + pub(crate) fn over_capacity(&self) -> bool { + self.transactions.len() > MAX_TRACKED_TRANSACTIONS + } } impl Default for VelocityTracker {