Bound the spending history by count, so a moved clock cannot erase it

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<dyn SecureStorage>).unwrap();
+        assert!(matches!(
+            mobile.evaluate_policy(a_spend()).unwrap(),
+            PolicyDecision::Deny { .. }
+        ));
+    }
 }

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<dyn SecureStorage>,
 ) -> Result<VelocityTracker, KeepMobileError> {
     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}"),

keep-mobile/src/velocity.rs

@@ -5,7 +5,21 @@ use chrono::{DateTime, Duration, Utc};
 use serde::{Deserialize, Serialize};
 use std::collections::VecDeque;
 
-const MAX_TRACKED_TRANSACTIONS: usize = 10_000;
+/// 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.
+pub(crate) const MAX_TRACKED_TRANSACTIONS: usize = 10_000;
 
 #[derive(Clone, Debug, Serialize, Deserialize)]
 struct TrackedTransaction {
@@ -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,11 @@ impl VelocityTracker {
     }
 
     pub fn from_bytes(data: &[u8]) -> Result<Self, serde_json::Error> {
-        let mut tracker: Self = serde_json::from_slice(data)?;
-        tracker.cleanup_old();
-        Ok(tracker)
+        serde_json::from_slice(data)
+    }
+
+    pub(crate) fn over_capacity(&self) -> bool {
+        self.transactions.len() > MAX_TRACKED_TRANSACTIONS
     }
 }
 
@@ -114,12 +117,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]