From 1ecfee93542c626d8047fff4341070a2dcc14a95 Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Sat, 17 Oct 2026 17:48:55 -0400 Subject: [PATCH 1/2] Refuse when the stored policy cannot be verified, rather than dropping it --- keep-mobile/src/lib.rs | 701 +++++++++++++++++++++++++++++++-- keep-mobile/src/persistence.rs | 20 +- keep-mobile/src/policy.rs | 374 +++++++++++++++--- keep-mobile/src/storage.rs | 13 + 4 files changed, 1025 insertions(+), 83 deletions(-) diff --git a/keep-mobile/src/lib.rs b/keep-mobile/src/lib.rs index 5147b422..d0c47dee 100644 --- a/keep-mobile/src/lib.rs +++ b/keep-mobile/src/lib.rs @@ -44,7 +44,7 @@ pub use nip55_decision::{ evaluate_nip55_request, nip55_needs_biometric, Nip55DecisionInputs, Nip55Outcome, Nip55VelocityCheck, }; -pub use policy::{PolicyDecision, PolicyInfo, TransactionContext}; +pub use policy::{PolicyDecision, PolicyFault, PolicyInfo, PolicyStatus, TransactionContext}; pub use psbt::{PsbtInfo, PsbtInputSighash, PsbtOutputInfo, PsbtParser}; pub use signing_policy::{ assess_signing_risk, evaluate_sign_policy, evaluate_sign_policy_selection, is_sensitive_kind, @@ -832,22 +832,7 @@ impl KeepMobile { let velocity = Arc::new(std::sync::Mutex::new(velocity)); let mut evaluator = PolicyEvaluator::new(velocity.clone()); - - if let Ok(wardens) = persistence::load_trusted_wardens(&storage) { - evaluator.set_trusted_wardens(wardens); - } - - if let Ok(bundle) = persistence::load_policy(&storage) { - if bundle.verify_signature().is_ok() - && bundle.verify_hash().is_ok() - && bundle.verify_timestamp().is_ok() - && bundle - .verify_trusted_warden(evaluator.trusted_wardens()) - .is_ok() - { - evaluator.set_policy(bundle); - } - } + Self::load_policy_into(&storage, &mut evaluator); let policy = Arc::new(std::sync::RwLock::new(evaluator)); @@ -1852,9 +1837,32 @@ impl KeepMobile { bundle.verify_hash()?; bundle.verify_timestamp()?; + // 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()?; + let mut policy = self.policy_write_lock()?; + // Refused when the stored record cannot say how old it is, rather than + // installed over it. The rollback check below needs a baseline, and a + // record that failed verification is not one: accepting the install + // would let anyone who can corrupt the record roll the policy back to + // an older bundle that still carries a real warden signature. An + // expired bundle is attributed, so it is still a baseline and a + // replacement installs over it normally. + // + // Worth being exact about what this buys: `delete_policy` needs no + // authorization, so this raises the cost of that rollback from nothing + // to one more call rather than closing it. Closing it needs an epoch + // that outlives deletion, which is follow-up work. + if policy.fault().is_some() && policy.rollback_baseline().is_none() { + return Err(KeepMobileError::InvalidPolicy { + msg: "Refusing to replace a policy that cannot be checked against. \ + Delete the stored policy first." + .into(), + }); + } bundle.verify_trusted_warden(policy.trusted_wardens())?; - bundle.verify_version_upgrade(policy.policy())?; + bundle.verify_version_upgrade(policy.rollback_baseline())?; let info = bundle.to_policy_info(); policy.set_policy(bundle.clone()); @@ -1863,6 +1871,9 @@ impl KeepMobile { Ok(info) } + /// 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 { self.policy .read() @@ -1871,9 +1882,31 @@ impl KeepMobile { .map(|b| b.to_policy_info()) } + /// Whether a policy is configured, in force, or stored and untrustworthy. + /// + /// 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()?; + Ok(self.policy_read_lock()?.status()) + } + + /// Removes the stored policy, and is the only way out of a record that will + /// not verify. Deliberately an explicit act: it discards a policy a warden + /// signed, so it should not happen as a side effect of a corrupt record. pub fn delete_policy(&self) -> Result<(), KeepMobileError> { - self.policy_write_lock()?.clear_policy(); - let _ = self.storage.delete_share_by_key(POLICY_STORAGE_KEY.into()); + // The record goes first and the error is surfaced. Clearing memory on a + // delete that did not happen reports success, opens the gate for this + // 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. + 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(); Ok(()) } @@ -1882,6 +1915,7 @@ impl KeepMobile { ctx: TransactionContext, ) -> Result { self.reload_velocity_if_unknown()?; + self.reload_policy_if_transient()?; self.policy_read_lock()?.evaluate(&ctx) } @@ -1914,18 +1948,38 @@ impl KeepMobile { Ok(()) } + /// Trusting a warden re-runs the stored policy against the new set, which + /// is how a policy faulted for an untrusted signer comes back into force + /// after a key rotation without its rules being discarded. pub fn add_trusted_warden(&self, pubkey_hex: String) -> Result<(), KeepMobileError> { let pubkey = network::parse_warden_pubkey(&pubkey_hex)?; let mut policy = self.policy_write_lock()?; - policy.add_trusted_warden(pubkey); - persistence::persist_trusted_wardens(&self.storage, policy.trusted_wardens()) + if policy.trusted_wardens().contains(&pubkey) { + return Ok(()); + } + let mut wardens = policy.trusted_wardens().clone(); + wardens.insert(pubkey); + persistence::persist_trusted_wardens(&self.storage, &wardens)?; + policy.set_trusted_wardens(wardens); + Self::load_policy_into(&self.storage, &mut policy); + Ok(()) } + /// Untrusting a warden re-runs the stored policy too, so a policy that only + /// that warden vouched for stops being in force rather than staying in + /// memory after the signer behind it was withdrawn. pub fn remove_trusted_warden(&self, pubkey_hex: String) -> Result<(), KeepMobileError> { let pubkey = network::parse_warden_pubkey(&pubkey_hex)?; let mut policy = self.policy_write_lock()?; - policy.remove_trusted_warden(&pubkey); - persistence::persist_trusted_wardens(&self.storage, policy.trusted_wardens()) + if !policy.trusted_wardens().contains(&pubkey) { + return Ok(()); + } + let mut wardens = policy.trusted_wardens().clone(); + wardens.remove(&pubkey); + persistence::persist_trusted_wardens(&self.storage, &wardens)?; + policy.set_trusted_wardens(wardens); + Self::load_policy_into(&self.storage, &mut policy); + Ok(()) } pub fn list_trusted_wardens(&self) -> Result, KeepMobileError> { @@ -3951,6 +4005,113 @@ 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". + fn load_policy_into(storage: &Arc, evaluator: &mut PolicyEvaluator) { + match persistence::load_trusted_wardens(storage) { + Ok(wardens) => evaluator.set_trusted_wardens(wardens), + Err(KeepMobileError::StorageNotFound) => { + evaluator.set_trusted_wardens(std::collections::HashSet::new()) + } + // Keep whatever set is already in memory rather than replacing it + // with an empty one. On a fresh evaluator that set is empty anyway, + // so a stored bundle still refuses for want of a signer; on a live + // one it means a warden just added does not disappear because the + // record could not be read back. + Err(e) => tracing::error!( + "Trusted wardens unreadable, keeping the set already in memory; a \ + stored policy needing a warden outside it will refuse: {e}" + ), + } + + let bundle = match persistence::load_policy(storage) { + Ok(bundle) => bundle, + Err(KeepMobileError::StorageNotFound) => { + evaluator.clear_policy(); + return; + } + // A record that will not parse will not parse next time either, so + // it is not the retryable arm. Calling it unreadable would put two + // Keystore decrypts on every decision for the life of the process + // and tell the caller to wait out a fault that never clears. + Err(e @ KeepMobileError::InvalidPolicy { .. }) => { + tracing::error!("Stored policy could not be parsed, every rule will refuse: {e}"); + evaluator.set_policy_faulted(PolicyFault::Unverified, None); + return; + } + Err(e) => { + tracing::error!("Policy unreadable, every rule will refuse: {e}"); + evaluator.set_policy_faulted(PolicyFault::Unreadable, None); + return; + } + }; + + // Attribution before the clock, and in that order deliberately. The + // signature, hash and signer are what make the bundle evidence; only + // once they hold is its own `created_at` worth anything, which is what + // lets a replacement be measured against an expired bundle rather than + // the expired one having to be thrown away first. + let attributed = bundle + .verify_signature() + .and_then(|()| bundle.verify_hash()) + .and_then(|()| bundle.verify_trusted_warden(evaluator.trusted_wardens())); + if let Err(e) = attributed { + tracing::error!("Stored policy did not verify, every rule will refuse: {e}"); + evaluator.set_policy_faulted(PolicyFault::Unverified, None); + return; + } + + match bundle.timestamp_fault() { + None => evaluator.set_policy(bundle), + Some(fault) => { + tracing::error!( + "Stored policy is authentic but outside its validity window ({fault}), \ + every rule will refuse" + ); + evaluator.set_policy_faulted(fault, Some(bundle)); + } + } + } + + /// Reads the stored policy again if reading it again could change the + /// answer. + /// + /// Only the transient faults are retried. A record that will not parse, or + /// that is signed by nobody we trust, answers the same way every time, so + /// 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> { + // 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. + let fault = self.policy_read_lock()?.fault(); + if !fault.is_some_and(PolicyFault::is_transient) { + return Ok(()); + } + let mut policy = self.policy_write_lock()?; + Self::load_policy_into(&self.storage, &mut policy); + Ok(()) + } + /// Reads the stored counters again if they are still unknown. /// /// The fault that made them unknown can be transient: the Android Keystore @@ -6673,6 +6834,8 @@ mod velocity_fault_tests { records: StdMutex>>, unreadable: StdMutex>, unwritable: StdMutex>, + undeletable: StdMutex>, + reads: StdMutex>, } impl FaultingStorage { @@ -6689,6 +6852,16 @@ mod velocity_fault_tests { self.unwritable.lock().unwrap().clear(); } + fn fault_delete(&self, key: &str) { + self.undeletable.lock().unwrap().push(key.into()); + } + + /// How many reads of `key` the backend has served, for pinning that a + /// permanent fault is not re-read on every decision. + fn reads_of(&self, key: &str) -> usize { + *self.reads.lock().unwrap().get(key).unwrap_or(&0) + } + fn plant(&self, key: &str, data: Vec) { self.records.lock().unwrap().insert(key.into(), data); } @@ -6729,6 +6902,7 @@ mod velocity_fault_tests { Ok(()) } fn load_share_by_key(&self, key: String) -> Result, KeepMobileError> { + *self.reads.lock().unwrap().entry(key.clone()).or_insert(0) += 1; if self.unreadable.lock().unwrap().contains(&key) { return Err(KeepMobileError::StorageError { msg: "cannot decrypt right now".into(), @@ -6745,6 +6919,11 @@ mod velocity_fault_tests { Vec::new() } fn delete_share_by_key(&self, key: String) -> Result<(), KeepMobileError> { + if self.undeletable.lock().unwrap().contains(&key) { + return Err(KeepMobileError::StorageError { + msg: "cannot delete right now".into(), + }); + } self.records.lock().unwrap().remove(&key); Ok(()) } @@ -6854,4 +7033,478 @@ mod velocity_fault_tests { "the recovered history must be the stored one, not an empty one" ); } + use crate::policy::{ + create_test_bundle, create_test_bundle_at, create_test_bundle_with, PolicyRules, + }; + + fn plant_policy(storage: &FaultingStorage, rules: &PolicyRules, trust_warden: bool) -> Vec { + let bundle = create_test_bundle(rules); + storage.plant( + POLICY_STORAGE_KEY, + serde_json::to_vec(&bundle).expect("serialize policy"), + ); + if trust_warden { + let hex_list = vec![hex::encode(bundle.warden_pubkey)]; + storage.plant( + TRUSTED_WARDENS_KEY, + serde_json::to_vec(&hex_list).expect("serialize wardens"), + ); + } + bundle.warden_pubkey.to_vec() + } + + fn a_spend() -> TransactionContext { + TransactionContext { + amount_sats: 1, + fee_sats: 1, + destinations: vec![], + has_unknown_destination: false, + } + } + + fn capped_rules() -> PolicyRules { + PolicyRules { + max_amount: Some(100_000), + ..Default::default() + } + } + + /// The headline case, through the real load. One flipped byte in the stored + /// record used to leave the evaluator holding no policy, which allowed + /// everything; a policy whose own tamper evidence disarms the policy is + /// worse than no policy, because the result reads like configuration. + /// + /// Deliberately asserts only that it faults. Where the flipped byte lands + /// decides whether the record stops parsing or survives and fails the + /// signature, and that moves with the generated key, so the arm is pinned + /// by `a_record_whose_rules_were_edited_faults_as_unverified` and + /// `an_unparseable_record_is_not_re_read_on_every_decision` instead. What + /// this one is for is the broad property: any corruption refuses. + #[test] + fn a_tampered_policy_record_refuses_instead_of_vanishing() { + let storage = Arc::new(FaultingStorage::default()); + plant_policy(&storage, &capped_rules(), true); + + let mut tampered = storage.raw(POLICY_STORAGE_KEY).expect("planted"); + let at = tampered.len() / 2; + tampered[at] ^= 0x01; + storage.plant(POLICY_STORAGE_KEY, tampered); + + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + + let PolicyDecision::Deny { reason } = mobile.evaluate_policy(a_spend()).unwrap() else { + panic!("a tampered policy record must refuse, not allow everything"); + }; + assert!(reason.contains("policy"), "{reason}"); + assert!( + matches!( + mobile.policy_status().unwrap(), + PolicyStatus::Faulted { .. } + ), + "the refusal has to be explainable, so it must not report as unconfigured" + ); + assert!(mobile.get_policy_info().is_none()); + } + + /// A verified record still works, so the refusals above cannot be a policy + /// engine that simply says no. + #[test] + fn a_verified_policy_record_is_enforced() { + 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().unwrap(), + PolicyStatus::Active { .. } + )); + assert_eq!( + mobile.evaluate_policy(a_spend()).unwrap(), + PolicyDecision::Allow + ); + let over = TransactionContext { + amount_sats: 200_000, + ..a_spend() + }; + assert!(matches!( + mobile.evaluate_policy(over).unwrap(), + PolicyDecision::Deny { .. } + )); + } + + /// No record at all is the one state that means there are no rules. + #[test] + fn no_policy_record_allows() { + let storage = Arc::new(FaultingStorage::default()); + let mobile = KeepMobile::new(storage as Arc).unwrap(); + assert!(matches!( + mobile.policy_status().unwrap(), + PolicyStatus::NotConfigured + )); + assert_eq!( + mobile.evaluate_policy(a_spend()).unwrap(), + PolicyDecision::Allow + ); + } + + /// An unreadable record is retried, because a Keystore decrypt fault can be + /// transient with the record intact. Until it reads it refuses, and when it + /// reads the real rules come back rather than the policy being discarded. + #[test] + fn an_unreadable_policy_record_refuses_and_then_recovers() { + let storage = Arc::new(FaultingStorage::default()); + plant_policy(&storage, &capped_rules(), true); + storage.fault(POLICY_STORAGE_KEY); + + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + assert!(matches!( + mobile.policy_status().unwrap(), + PolicyStatus::Faulted { + fault: PolicyFault::Unreadable + } + )); + assert!(matches!( + mobile.evaluate_policy(a_spend()).unwrap(), + PolicyDecision::Deny { .. } + )); + + storage.heal(); + assert!(matches!( + mobile.policy_status().unwrap(), + PolicyStatus::Active { .. } + )); + assert_eq!( + mobile.evaluate_policy(a_spend()).unwrap(), + PolicyDecision::Allow + ); + } + + /// A bundle can only have been stored while some warden was trusted, so a + /// stored bundle with the wardens gone is a policy that cannot be + /// attributed to a signer. Unattributable is not absent. + #[test] + fn a_stored_policy_with_the_wardens_lost_refuses() { + let storage = Arc::new(FaultingStorage::default()); + plant_policy(&storage, &capped_rules(), false); + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + + assert!(matches!( + mobile.policy_status().unwrap(), + PolicyStatus::Faulted { + fault: PolicyFault::Unverified + } + )); + assert!(matches!( + mobile.evaluate_policy(a_spend()).unwrap(), + PolicyDecision::Deny { .. } + )); + } + + /// The lockout this refusal could otherwise cause has a way out that keeps + /// the rules: trust the signer and the stored bundle verifies again. This is + /// the warden-rotation path, and it is why refusing an unattributable policy + /// does not require throwing it away. + #[test] + fn trusting_the_warden_brings_the_stored_policy_back_into_force() { + let storage = Arc::new(FaultingStorage::default()); + let warden = plant_policy(&storage, &capped_rules(), false); + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + assert!(matches!( + mobile.policy_status().unwrap(), + PolicyStatus::Faulted { .. } + )); + + mobile.add_trusted_warden(hex::encode(&warden)).unwrap(); + + assert!( + matches!(mobile.policy_status().unwrap(), PolicyStatus::Active { .. }), + "trusting the signer must re-run the stored policy, not leave it faulted" + ); + let over = TransactionContext { + amount_sats: 200_000, + ..a_spend() + }; + assert!(matches!( + mobile.evaluate_policy(over).unwrap(), + PolicyDecision::Deny { .. } + )); + } + + /// Installing over a record that cannot be checked against would hand a + /// rollback to anyone able to corrupt storage: the downgrade check compares + /// against the policy in force, and a faulted record has none, so an older + /// bundle carrying a real warden signature would install cleanly. + #[test] + fn a_policy_cannot_be_installed_over_one_that_will_not_verify() { + let storage = Arc::new(FaultingStorage::default()); + plant_policy(&storage, &capped_rules(), true); + storage.fault(POLICY_STORAGE_KEY); + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + + let replacement = create_test_bundle(&PolicyRules::default()); + let hex_bundle = hex::encode(replacement.to_bytes()); + let err = mobile + .import_policy(hex_bundle) + .expect_err("installing over an unverifiable record must be refused"); + // `InvalidPolicy`'s Display drops `msg`, so the reason is only in the + // field a caller destructures. + let KeepMobileError::InvalidPolicy { msg } = err else { + panic!("expected the install to be refused for the fault, got {err:?}"); + }; + assert!(msg.contains("Delete the stored policy first"), "{msg}"); + + mobile.delete_policy().unwrap(); + assert!(matches!( + mobile.policy_status().unwrap(), + PolicyStatus::NotConfigured + )); + } + /// An unreadable wardens record leaves a stored bundle unattributable on a + /// fresh evaluator, which is a refusal and not an absence. + #[test] + fn an_unreadable_wardens_record_refuses_a_stored_policy() { + let storage = Arc::new(FaultingStorage::default()); + plant_policy(&storage, &capped_rules(), true); + storage.fault(TRUSTED_WARDENS_KEY); + + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + assert!(matches!( + mobile.policy_status().unwrap(), + PolicyStatus::Faulted { + fault: PolicyFault::Unverified + } + )); + assert!(matches!( + mobile.evaluate_policy(a_spend()).unwrap(), + PolicyDecision::Deny { .. } + )); + } + + /// The warden set in memory survives a wardens record that cannot be read + /// back. Without that, adding a warden would persist it and then lose it on + /// the very reload meant to put the policy back in force, so the rotation + /// recovery would fail exactly when storage is already misbehaving. + #[test] + fn adding_a_warden_survives_a_wardens_record_that_cannot_be_read_back() { + let storage = Arc::new(FaultingStorage::default()); + let warden = plant_policy(&storage, &capped_rules(), false); + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + assert!(matches!( + mobile.policy_status().unwrap(), + PolicyStatus::Faulted { .. } + )); + + storage.fault(TRUSTED_WARDENS_KEY); + mobile.add_trusted_warden(hex::encode(&warden)).unwrap(); + + assert!( + matches!(mobile.policy_status().unwrap(), PolicyStatus::Active { .. }), + "the warden just added must not be dropped by the reload that follows it" + ); + assert_eq!( + mobile.list_trusted_wardens().unwrap(), + vec![hex::encode(&warden)] + ); + } + /// Deterministic, unlike flipping a byte at the record's midpoint: a byte + /// inside `rules_json` always survives parsing and always breaks the hash, + /// so this pins the arm and not merely that something faulted. + #[test] + fn a_record_whose_rules_were_edited_faults_as_unverified() { + let storage = Arc::new(FaultingStorage::default()); + plant_policy(&storage, &capped_rules(), true); + + let raw = storage.raw(POLICY_STORAGE_KEY).expect("planted"); + let text = String::from_utf8(raw).expect("json"); + let edited = text.replace("100000", "900000"); + assert_ne!(edited, text, "the fixture must contain the cap to edit"); + storage.plant(POLICY_STORAGE_KEY, edited.into_bytes()); + + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + assert!(matches!( + mobile.policy_status().unwrap(), + PolicyStatus::Faulted { + fault: PolicyFault::Unverified + } + )); + let over = TransactionContext { + amount_sats: 500_000, + ..a_spend() + }; + assert!( + matches!( + mobile.evaluate_policy(over).unwrap(), + PolicyDecision::Deny { .. } + ), + "the raised cap must not take effect, and the record must not read as no policy" + ); + } + + /// A record that will not parse will not parse next time either, so it must + /// not be the retryable arm. Classified as unreadable it would put two + /// Keystore decrypts on every decision for the life of the process, for a + /// condition that cannot resolve. + #[test] + fn an_unparseable_record_is_not_re_read_on_every_decision() { + let storage = Arc::new(FaultingStorage::default()); + storage.plant(POLICY_STORAGE_KEY, b"not json at all".to_vec()); + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + + assert!(matches!( + mobile.policy_status().unwrap(), + PolicyStatus::Faulted { + fault: PolicyFault::Unverified + } + )); + let after_load = storage.reads_of(POLICY_STORAGE_KEY); + for _ in 0..5 { + assert!(matches!( + mobile.evaluate_policy(a_spend()).unwrap(), + PolicyDecision::Deny { .. } + )); + } + assert_eq!( + storage.reads_of(POLICY_STORAGE_KEY), + after_load, + "a permanent fault must not be re-read per decision" + ); + } + + /// A device that boots before its clock syncs dates a good policy in the + /// future. That resolves by itself within seconds, so it refuses meanwhile + /// and keeps looking, rather than holding the refusal for the whole process + /// and offering only deletion as the way out. + #[test] + fn a_policy_dated_ahead_of_the_clock_refuses_and_keeps_retrying() { + let storage = Arc::new(FaultingStorage::default()); + let ahead = chrono::Utc::now().timestamp() as u64 + 7 * 24 * 60 * 60; + let bundle = create_test_bundle_at(&capped_rules(), ahead); + storage.plant(POLICY_STORAGE_KEY, serde_json::to_vec(&bundle).unwrap()); + storage.plant( + TRUSTED_WARDENS_KEY, + serde_json::to_vec(&vec![hex::encode(bundle.warden_pubkey)]).unwrap(), + ); + + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + assert!(matches!( + mobile.policy_status().unwrap(), + PolicyStatus::Faulted { + fault: PolicyFault::NotYetValid + } + )); + assert!(matches!( + mobile.evaluate_policy(a_spend()).unwrap(), + PolicyDecision::Deny { .. } + )); + + let after_load = storage.reads_of(POLICY_STORAGE_KEY); + mobile.evaluate_policy(a_spend()).unwrap(); + assert!( + storage.reads_of(POLICY_STORAGE_KEY) > after_load, + "a fault that resolves on its own has to be looked at again" + ); + } + + /// Renewal must not require destroying the policy first. By the time the + /// clock check fails the signature and signer already held, so the expired + /// bundle is still a sound baseline to measure the replacement against. + #[test] + fn a_replacement_installs_over_an_expired_policy_without_deleting_it() { + let storage = Arc::new(FaultingStorage::default()); + let long_ago = chrono::Utc::now().timestamp() as u64 - 400 * 24 * 60 * 60; + let old = create_test_bundle_at(&capped_rules(), long_ago); + storage.plant(POLICY_STORAGE_KEY, serde_json::to_vec(&old).unwrap()); + storage.plant( + TRUSTED_WARDENS_KEY, + serde_json::to_vec(&vec![hex::encode(old.warden_pubkey)]).unwrap(), + ); + + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + assert!(matches!( + mobile.policy_status().unwrap(), + PolicyStatus::Faulted { + fault: PolicyFault::Expired + } + )); + assert!( + matches!( + mobile.evaluate_policy(a_spend()).unwrap(), + PolicyDecision::Deny { .. } + ), + "stale rules are neither enforced nor ignored" + ); + + let fresh = create_test_bundle(&capped_rules()); + mobile + .add_trusted_warden(hex::encode(fresh.warden_pubkey)) + .unwrap(); + mobile + .import_policy(hex::encode(fresh.to_bytes())) + .expect("a replacement must install over an expired policy"); + + assert!(matches!( + mobile.policy_status().unwrap(), + PolicyStatus::Active { .. } + )); + } + + /// The escape hatch must not report success on a delete that did not + /// happen: memory would open the gate for this process while the record + /// stayed put and faulted again at the next launch. + #[test] + fn a_delete_that_fails_leaves_the_policy_refusing() { + let storage = Arc::new(FaultingStorage::default()); + plant_policy(&storage, &capped_rules(), true); + storage.fault(POLICY_STORAGE_KEY); + storage.fault_delete(POLICY_STORAGE_KEY); + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + + assert!(mobile.delete_policy().is_err()); + assert!( + matches!( + mobile.policy_status().unwrap(), + PolicyStatus::Faulted { .. } + ), + "a failed delete must not leave memory reading as no policy" + ); + } + + /// A transient fault must not push the caller into discarding a + /// warden-signed policy, so an install reads the record again first. + /// + /// Both bundles come from one warden on purpose. Trusting a new warden + /// re-runs the load itself, which would mask whether the install does, so + /// this leaves the install as the only thing that can clear the fault. + #[test] + fn an_install_retries_a_fault_that_has_already_healed() { + let warden = k256::schnorr::SigningKey::random(&mut k256::elliptic_curve::rand_core::OsRng); + let now = chrono::Utc::now().timestamp() as u64; + let stored = create_test_bundle_with(&warden, &capped_rules(), now - 60); + + let storage = Arc::new(FaultingStorage::default()); + storage.plant(POLICY_STORAGE_KEY, serde_json::to_vec(&stored).unwrap()); + storage.plant( + TRUSTED_WARDENS_KEY, + serde_json::to_vec(&vec![hex::encode(stored.warden_pubkey)]).unwrap(), + ); + storage.fault(POLICY_STORAGE_KEY); + + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + assert!(matches!( + mobile.policy_status().unwrap(), + PolicyStatus::Faulted { + fault: PolicyFault::Unreadable + } + )); + + storage.heal(); + let replacement = create_test_bundle_with(&warden, &capped_rules(), now); + mobile + .import_policy(hex::encode(replacement.to_bytes())) + .expect("a healed fault must not force a delete"); + assert!(matches!( + mobile.policy_status().unwrap(), + PolicyStatus::Active { .. } + )); + } } diff --git a/keep-mobile/src/persistence.rs b/keep-mobile/src/persistence.rs index a04259aa..c78d5d01 100644 --- a/keep-mobile/src/persistence.rs +++ b/keep-mobile/src/persistence.rs @@ -8,7 +8,7 @@ use serde::{Deserialize, Serialize}; use zeroize::Zeroizing; use crate::error::KeepMobileError; -use crate::policy::{PolicyBundle, POLICY_PUBKEY_LEN}; +use crate::policy::{PolicyBundle, POLICY_MAX_RULES_LEN, POLICY_PUBKEY_LEN}; use crate::storage::{SecureStorage, ShareMetadataInfo}; use crate::types::{DeviceRegistrationInfo, KeyHealthStatusInfo, WalletDescriptorInfo}; use crate::velocity::VelocityTracker; @@ -54,9 +54,21 @@ pub(crate) fn load_policy( storage: &Arc, ) -> Result { let data = storage.load_share_by_key(POLICY_STORAGE_KEY.into())?; - serde_json::from_slice(&data).map_err(|e| KeepMobileError::InvalidPolicy { - msg: format!("Failed to deserialize policy: {e}"), - }) + // Deliberately without serde's message, for the same reason load_velocity + // drops it: serde quotes the value it tripped on, the record is attacker + // controlled before it has verified, and this error reaches a log. + let bundle: PolicyBundle = + serde_json::from_slice(&data).map_err(|_| KeepMobileError::InvalidPolicy { + msg: "Failed to deserialize policy".into(), + })?; + // The wire format caps the rules and the import path caps its input, but a + // record read straight out of storage has been through neither. + if bundle.rules_json.len() > POLICY_MAX_RULES_LEN { + return Err(KeepMobileError::InvalidPolicy { + msg: "Stored policy rules exceed the maximum length".into(), + }); + } + Ok(bundle) } pub(crate) fn persist_policy( diff --git a/keep-mobile/src/policy.rs b/keep-mobile/src/policy.rs index 8a34696d..192d1bff 100644 --- a/keep-mobile/src/policy.rs +++ b/keep-mobile/src/policy.rs @@ -177,6 +177,88 @@ pub enum PolicyDecision { Deny { reason: String }, } +/// Why a stored policy record could not be turned into rules worth trusting. +/// +/// Every arm refuses. They are kept apart because the way out differs, and two +/// of them clear on their own: a decrypt fault can be transient, and a clock +/// that has not synced yet dates a good policy in the future and corrects +/// itself within seconds. Collapsing those into "did not verify" would hold a +/// refusal for the life of the process over a condition that had already gone. +#[derive(uniffi::Enum, Clone, Copy, Debug, PartialEq, Eq)] +pub enum PolicyFault { + /// The record could not be read. On Android a decrypt fault can be + /// transient with the record intact, so this is retried. + Unreadable, + /// The record was read and could not be attributed: a signature, hash or + /// signer that does not hold. Its contents are evidence of nothing, + /// including its own age, so they cannot serve as a rollback baseline. + /// Retrying cannot help; trusting the signer or deleting the record can. + Unverified, + /// Attributed, and dated further ahead than the clock allows for. A device + /// that boots before its clock syncs does this to a perfectly good policy, + /// so it is retried. + NotYetValid, + /// Attributed, and past its maximum age. Not retried, because it will not + /// come back, but a replacement can be installed over it: by the time this + /// is reached the signature and signer already held, so the bundle's + /// `created_at` is still a sound baseline to measure a downgrade against. + Expired, +} + +impl PolicyFault { + /// Whether reading the record again could change the answer. + pub(crate) fn is_transient(self) -> bool { + matches!(self, Self::Unreadable | Self::NotYetValid) + } +} + +impl std::fmt::Display for PolicyFault { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Unreadable => write!(f, "the stored policy could not be read"), + Self::Unverified => write!(f, "the stored policy did not verify"), + Self::NotYetValid => { + write!(f, "the stored policy is dated ahead of this device's clock") + } + Self::Expired => write!(f, "the stored policy has expired"), + } + } +} + +/// What the stored policy record says, as distinct from what it holds. +/// +/// The distinction [`Self::NotConfigured`] draws against [`Self::Faulted`] is +/// the whole point: no record means no rules, and a record that cannot be +/// trusted means the rules are unknown. Reading the second as the first is what +/// made a single flipped byte switch off every rule. +#[derive(Clone, Debug)] +pub(crate) enum PolicyState { + NotConfigured, + Active(Box), + /// `attributed` carries the bundle only when its signature, hash and signer + /// all held, which is the one case where an unenforceable record can still + /// say truthfully how old it is. + Faulted { + fault: PolicyFault, + attributed: Option>, + }, +} + +/// The stored policy as a caller can act on it. +/// +/// Separate from [`PolicyInfo`] because a faulted record has no info to show +/// and must not be reported as no policy: a UI saying "no policy" while every +/// spend is refused leaves nobody able to work out why. +#[derive(uniffi::Enum, Clone, Debug)] +pub enum PolicyStatus { + /// No record, so no rules. + NotConfigured, + /// A record that was read and verified. + Active { info: PolicyInfo }, + /// A record that exists and cannot be trusted. Every rule refuses. + Faulted { fault: PolicyFault }, +} + #[derive(uniffi::Record, Clone, Debug)] pub struct TransactionContext { pub amount_sats: u64, @@ -353,22 +435,33 @@ impl PolicyBundle { Ok(()) } - pub fn verify_timestamp(&self) -> Result<(), KeepMobileError> { + /// Which validity bound the bundle falls outside, if either. + /// + /// 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. Saturating, so a clock set before + /// 1970 cannot make the comparison wrap. + pub fn timestamp_fault(&self) -> Option { let now = Utc::now().timestamp() as u64; - if self.created_at > now + MAX_POLICY_FUTURE_SECS { - return Err(KeepMobileError::InvalidPolicy { - msg: "Policy timestamp is in the future".into(), - }); + if self.created_at > now.saturating_add(MAX_POLICY_FUTURE_SECS) { + return Some(PolicyFault::NotYetValid); } - if now > self.created_at + MAX_POLICY_AGE_SECS { - return Err(KeepMobileError::InvalidPolicy { - msg: "Policy has expired".into(), - }); + if now > self.created_at.saturating_add(MAX_POLICY_AGE_SECS) { + return Some(PolicyFault::Expired); } - Ok(()) + None + } + + pub fn verify_timestamp(&self) -> Result<(), KeepMobileError> { + match self.timestamp_fault() { + None => Ok(()), + Some(fault) => Err(KeepMobileError::InvalidPolicy { + msg: fault.to_string(), + }), + } } pub fn verify_version_upgrade( @@ -409,7 +502,7 @@ impl PolicyBundle { } pub struct PolicyEvaluator { - policy: Option, + policy: PolicyState, /// `None` means the stored counters could not be read, which is not the /// same as counters that read as empty: the spending history is unknown /// rather than known to be nothing, so the rules that consult it refuse @@ -421,20 +514,12 @@ pub struct PolicyEvaluator { impl PolicyEvaluator { pub fn new(velocity: Arc>>) -> Self { Self { - policy: None, + policy: PolicyState::NotConfigured, velocity, trusted_wardens: HashSet::new(), } } - pub fn add_trusted_warden(&mut self, pubkey: [u8; POLICY_PUBKEY_LEN]) { - self.trusted_wardens.insert(pubkey); - } - - pub fn remove_trusted_warden(&mut self, pubkey: &[u8; POLICY_PUBKEY_LEN]) { - self.trusted_wardens.remove(pubkey); - } - pub fn trusted_wardens(&self) -> &HashSet<[u8; POLICY_PUBKEY_LEN]> { &self.trusted_wardens } @@ -444,20 +529,87 @@ impl PolicyEvaluator { } pub fn set_policy(&mut self, bundle: PolicyBundle) { - self.policy = Some(bundle); + self.policy = PolicyState::Active(Box::new(bundle)); + } + + /// Record that a policy exists and cannot be enforced, which refuses every + /// rule until it reads and verifies again or is deleted outright. + /// + /// `attributed` is the bundle when signature, hash and signer held and only + /// the clock did not, and `None` when the record itself is not evidence. + pub fn set_policy_faulted(&mut self, fault: PolicyFault, attributed: Option) { + self.policy = PolicyState::Faulted { + fault, + attributed: attributed.map(Box::new), + }; } pub fn clear_policy(&mut self) { - self.policy = None; + self.policy = PolicyState::NotConfigured; } + /// The bundle in force, which exists only when one verified. A faulted + /// record answers `None`, so nothing can read its unverified contents as + /// though they meant something, its `created_at` included. pub fn policy(&self) -> Option<&PolicyBundle> { - self.policy.as_ref() + match &self.policy { + PolicyState::Active(bundle) => Some(bundle), + PolicyState::NotConfigured | PolicyState::Faulted { .. } => None, + } + } + + /// The bundle a replacement's `created_at` has to beat, when the stored + /// record can be trusted to supply one. + /// + /// Separate from [`Self::policy`] because these answer different questions. + /// An expired bundle must not be enforced and is still authentic, so it is + /// a baseline but not a policy; one that failed verification is neither. + pub fn rollback_baseline(&self) -> Option<&PolicyBundle> { + match &self.policy { + PolicyState::Active(bundle) => Some(bundle), + PolicyState::Faulted { attributed, .. } => attributed.as_deref(), + PolicyState::NotConfigured => None, + } + } + + pub fn fault(&self) -> Option { + match &self.policy { + PolicyState::Faulted { fault, .. } => Some(*fault), + PolicyState::NotConfigured | PolicyState::Active(_) => None, + } + } + + pub fn status(&self) -> PolicyStatus { + match &self.policy { + PolicyState::NotConfigured => PolicyStatus::NotConfigured, + PolicyState::Active(bundle) => PolicyStatus::Active { + info: bundle.to_policy_info(), + }, + PolicyState::Faulted { fault, .. } => PolicyStatus::Faulted { fault: *fault }, + } } + /// Decide on a transaction against the policy in force. + /// + /// An `Err` is not a decision and must never be read as one. The state + /// machine guarantees that uncertainty answers [`PolicyDecision::Deny`], + /// not that a decision always arrives: a poisoned lock, or rules that will + /// 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 { - let Some(bundle) = &self.policy else { - return Ok(PolicyDecision::Allow); + let bundle = match &self.policy { + PolicyState::NotConfigured => return Ok(PolicyDecision::Allow), + // The reason says only that it did not verify. Which check failed + // is logged where the record is read, because the record's contents + // are attacker-controlled once it has failed verification and this + // string reaches a caller. + PolicyState::Faulted { fault, .. } => { + return Ok(PolicyDecision::Deny { + reason: format!("Cannot evaluate the spending policy: {fault}"), + }) + } + PolicyState::Active(bundle) => bundle, }; let rules = bundle.parse_rules()?; @@ -615,42 +767,62 @@ fn weekday_abbrev(weekday: chrono::Weekday) -> &'static str { } } +/// A bundle signed by a warden generated for the call, so a test that wants +/// it trusted reads `warden_pubkey` off the result. #[cfg(test)] -mod tests { - use super::*; - use k256::schnorr::{signature::Signer, SigningKey}; +pub(crate) fn create_test_bundle(rules: &PolicyRules) -> PolicyBundle { + create_test_bundle_at(rules, Utc::now().timestamp() as u64) +} - fn create_test_bundle(rules: &PolicyRules) -> PolicyBundle { - let rules_json = serde_json::to_string(rules).unwrap(); - let mut rules_hasher = Sha256::new(); - rules_hasher.update(rules_json.as_bytes()); - let policy_hash: [u8; 32] = rules_hasher.finalize().into(); +/// A bundle dated `created_at`, signed over that date, so a test can put one +/// outside its validity window without the signature stopping being genuine. +#[cfg(test)] +pub(crate) fn create_test_bundle_at(rules: &PolicyRules, created_at: u64) -> PolicyBundle { + let key = k256::schnorr::SigningKey::random(&mut k256::elliptic_curve::rand_core::OsRng); + create_test_bundle_with(&key, rules, created_at) +} - let signing_key = SigningKey::random(&mut k256::elliptic_curve::rand_core::OsRng); - let verifying_key = signing_key.verifying_key(); - let mut warden_pubkey = [0u8; 32]; - warden_pubkey.copy_from_slice(&verifying_key.to_bytes()); +/// A bundle signed by a given warden, for a test that needs two bundles from +/// one signer, such as a replacement installing over what is already stored. +#[cfg(test)] +pub(crate) fn create_test_bundle_with( + signing_key: &k256::schnorr::SigningKey, + rules: &PolicyRules, + created_at: u64, +) -> PolicyBundle { + use k256::schnorr::signature::Signer; + + let rules_json = serde_json::to_string(rules).unwrap(); + let mut rules_hasher = Sha256::new(); + rules_hasher.update(rules_json.as_bytes()); + let policy_hash: [u8; 32] = rules_hasher.finalize().into(); + + let verifying_key = signing_key.verifying_key(); + let mut warden_pubkey = [0u8; 32]; + warden_pubkey.copy_from_slice(&verifying_key.to_bytes()); + + let mut bundle = PolicyBundle { + version: POLICY_VERSION, + warden_pubkey, + policy_hash, + rules_json, + created_at, + signature: [0u8; 64], + }; - let created_at = Utc::now().timestamp() as u64; + let signed_data = bundle.signed_data(); + let mut hasher = Sha256::new(); + hasher.update(&signed_data); + let message: [u8; 32] = hasher.finalize().into(); + let signature = signing_key.sign(&message); + bundle.signature.copy_from_slice(&signature.to_bytes()); - let mut bundle = PolicyBundle { - version: POLICY_VERSION, - warden_pubkey, - policy_hash, - rules_json, - created_at, - signature: [0u8; 64], - }; - - let signed_data = bundle.signed_data(); - let mut hasher = Sha256::new(); - hasher.update(&signed_data); - let message: [u8; 32] = hasher.finalize().into(); - let signature = signing_key.sign(&message); - bundle.signature.copy_from_slice(&signature.to_bytes()); + bundle +} - bundle - } +#[cfg(test)] +mod tests { + use super::*; #[test] fn test_bundle_roundtrip() { @@ -1065,4 +1237,96 @@ mod tests { let evaluator = PolicyEvaluator::new(Arc::new(Mutex::new(None))); assert!(evaluator.record_transaction(1_000).is_err()); } + /// The defect this state machine exists for. An unverifiable record used to + /// be dropped on the floor, which left the evaluator indistinguishable from + /// one with no policy, so a single flipped byte in storage switched off + /// max_amount, max_fee, the whitelist and the hour window at once. Tamper + /// evidence that answers "no rules" is worse than none, because it reads as + /// a configuration state rather than an attack. + #[test] + fn a_faulted_policy_refuses_every_rule_rather_than_reading_as_no_policy() { + let rules = PolicyRules { + max_amount: Some(100_000), + ..Default::default() + }; + let mut evaluator = + PolicyEvaluator::new(Arc::new(Mutex::new(Some(VelocityTracker::new())))); + evaluator.set_policy(create_test_bundle(&rules)); + + let ctx = TransactionContext { + amount_sats: 50_000, + fee_sats: 500, + destinations: vec![], + has_unknown_destination: false, + }; + assert_eq!( + evaluator.evaluate(&ctx).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 { + panic!("a policy that cannot be trusted must refuse, not fall back to no policy"); + }; + assert!(reason.contains("did not verify"), "{reason}"); + } + + /// No record is a different statement from a record that cannot be read, + /// and only the first means there are no rules. + #[test] + fn no_policy_still_allows() { + let evaluator = PolicyEvaluator::new(Arc::new(Mutex::new(Some(VelocityTracker::new())))); + let ctx = TransactionContext { + amount_sats: u64::MAX, + fee_sats: u64::MAX, + destinations: vec![], + has_unknown_destination: false, + }; + assert_eq!(evaluator.evaluate(&ctx).unwrap(), PolicyDecision::Allow); + } + + /// A faulted record must not be readable as a policy through any accessor. + /// `policy()` feeds the rollback check, and an unverified bundle's + /// `created_at` is worth nothing, so answering with it would let a corrupted + /// record become the baseline a downgrade is measured against. + #[test] + fn a_faulted_policy_is_not_reachable_as_a_bundle() { + let rules = PolicyRules::default(); + let mut evaluator = + PolicyEvaluator::new(Arc::new(Mutex::new(Some(VelocityTracker::new())))); + evaluator.set_policy(create_test_bundle(&rules)); + assert!(evaluator.policy().is_some()); + assert!(evaluator.fault().is_none()); + + evaluator.set_policy_faulted(PolicyFault::Unreadable, None); + assert!( + evaluator.policy().is_none(), + "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 { .. })); + } + + /// The two faults are reported apart because only one of them is worth + /// retrying, and a caller has to be able to tell a storage fault it can + /// wait out from a verification failure it cannot. + #[test] + 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)); + + evaluator.set_policy_faulted(PolicyFault::Unreadable, None); + let PolicyStatus::Faulted { fault } = evaluator.status() else { + panic!("expected a fault"); + }; + assert_eq!(fault, PolicyFault::Unreadable); + + evaluator.set_policy_faulted(PolicyFault::Unverified, None); + let PolicyStatus::Faulted { fault } = evaluator.status() else { + panic!("expected a fault"); + }; + assert_eq!(fault, PolicyFault::Unverified); + } } diff --git a/keep-mobile/src/storage.rs b/keep-mobile/src/storage.rs index b7468b74..d0c672d0 100644 --- a/keep-mobile/src/storage.rs +++ b/keep-mobile/src/storage.rs @@ -74,6 +74,19 @@ pub trait SecureStorage: Send + Sync { data: Vec, metadata: ShareMetadataInfo, ) -> Result<(), KeepMobileError>; + /// Read the record under `key`. + /// + /// Absence and failure are different answers and an implementation must + /// not collapse them. `StorageNotFound` means there is no record. A record + /// that exists and could not be produced, because it would not decrypt, + /// because a key alias was invalidated, or because only part of it is + /// there, must come back as some other `Err`. + /// + /// This is load bearing rather than tidiness. Callers that hold a record + /// unreadable are built to refuse rather than to proceed: no policy means + /// no rules, and no spending history means nothing spent, so an + /// implementation that reports an unreadable record as absent turns a + /// refusal into exactly the permission it was there to withhold. fn load_share_by_key(&self, key: String) -> Result, KeepMobileError>; fn list_all_shares(&self) -> Vec; /// Delete the record under `key`. From 7af1200d1dc08e424a1a16b14adc5f6f3448866e Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Sat, 17 Oct 2026 20:20:19 -0400 Subject: [PATCH 2/2] Store the policy before enforcing it, and stop a future-dated record wedging imports --- keep-mobile/src/lib.rs | 329 +++++++++++++++++++++++++++++---- keep-mobile/src/persistence.rs | 31 +++- keep-mobile/src/policy.rs | 87 +++++++-- 3 files changed, 394 insertions(+), 53 deletions(-) diff --git a/keep-mobile/src/lib.rs b/keep-mobile/src/lib.rs index d0c47dee..5d09a90f 100644 --- a/keep-mobile/src/lib.rs +++ b/keep-mobile/src/lib.rs @@ -227,7 +227,7 @@ use keep_core::frost::{ }; use keep_frost_net::{KfpNode, KfpNodeEvent, SessionInfo, SigningHooks}; use network::{constant_time_eq, convert_peer_status, parse_loopback_proxy, validate_hex_pubkey}; -use policy::{PolicyBundle, PolicyEvaluator}; +use policy::{PolicyBundle, PolicyEvaluator, POLICY_PUBKEY_LEN}; use serde::{Deserialize, Serialize}; use std::collections::HashMap; use std::sync::Arc; @@ -1850,10 +1850,14 @@ impl KeepMobile { // expired bundle is attributed, so it is still a baseline and a // replacement installs over it normally. // - // Worth being exact about what this buys: `delete_policy` needs no - // authorization, so this raises the cost of that rollback from nothing - // to one more call rather than closing it. Closing it needs an epoch - // that outlives deletion, which is follow-up work. + // Worth being exact about what this buys, because it is less than it + // looks. The baseline is read out of the same record it is protecting, + // so an attacker who can roll the stored bytes back to an older + // warden-signed bundle gets no comparison at all: that bundle loads as + // `Active` on its own terms and this check never runs. It also takes no + // FFI call, so authenticating `delete_policy` would not close it. + // Closing it needs a high-water mark kept outside the record and + // refused below, which is follow-up work. if policy.fault().is_some() && policy.rollback_baseline().is_none() { return Err(KeepMobileError::InvalidPolicy { msg: "Refusing to replace a policy that cannot be checked against. \ @@ -1865,8 +1869,15 @@ impl KeepMobile { bundle.verify_version_upgrade(policy.rollback_baseline())?; let info = bundle.to_policy_info(); - policy.set_policy(bundle.clone()); + // The record goes first, for the same reason it does in + // `delete_policy`. Committing to memory first means a failed write + // returns an error while this process goes on enforcing the bundle it + // just refused to store: a renewal that relaxes a limit takes effect + // unstored, and a `Faulted` deny-all is lifted until the next launch, + // where `reload_policy_if_transient` cannot undo it because the fault + // is already gone from memory. persistence::persist_policy(&self.storage, &bundle)?; + policy.set_policy(bundle.clone()); Ok(info) } @@ -1953,16 +1964,7 @@ impl KeepMobile { /// after a key rotation without its rules being discarded. pub fn add_trusted_warden(&self, pubkey_hex: String) -> Result<(), KeepMobileError> { let pubkey = network::parse_warden_pubkey(&pubkey_hex)?; - let mut policy = self.policy_write_lock()?; - if policy.trusted_wardens().contains(&pubkey) { - return Ok(()); - } - let mut wardens = policy.trusted_wardens().clone(); - wardens.insert(pubkey); - persistence::persist_trusted_wardens(&self.storage, &wardens)?; - policy.set_trusted_wardens(wardens); - Self::load_policy_into(&self.storage, &mut policy); - Ok(()) + self.mutate_trusted_wardens(pubkey, true) } /// Untrusting a warden re-runs the stored policy too, so a policy that only @@ -1970,16 +1972,7 @@ impl KeepMobile { /// memory after the signer behind it was withdrawn. pub fn remove_trusted_warden(&self, pubkey_hex: String) -> Result<(), KeepMobileError> { let pubkey = network::parse_warden_pubkey(&pubkey_hex)?; - let mut policy = self.policy_write_lock()?; - if !policy.trusted_wardens().contains(&pubkey) { - return Ok(()); - } - let mut wardens = policy.trusted_wardens().clone(); - wardens.remove(&pubkey); - persistence::persist_trusted_wardens(&self.storage, &wardens)?; - policy.set_trusted_wardens(wardens); - Self::load_policy_into(&self.storage, &mut policy); - Ok(()) + self.mutate_trusted_wardens(pubkey, false) } pub fn list_trusted_wardens(&self) -> Result, KeepMobileError> { @@ -3517,6 +3510,35 @@ fn build_wallet_policy( } impl KeepMobile { + /// The one implementation of both warden mutations, because the ordering is + /// what is being protected and it was written out twice. + /// + /// The record goes before memory: a set committed to memory that storage + /// never accepted would trust a signer across this process and not the + /// next, and on the insert side that is a bundle attributed to a warden the + /// device has no record of trusting. The reload comes last, so the stored + /// policy is re-attributed against the set that actually persisted. + fn mutate_trusted_wardens( + &self, + pubkey: [u8; POLICY_PUBKEY_LEN], + trust: bool, + ) -> Result<(), KeepMobileError> { + let mut policy = self.policy_write_lock()?; + if policy.trusted_wardens().contains(&pubkey) == trust { + return Ok(()); + } + let mut wardens = policy.trusted_wardens().clone(); + if trust { + wardens.insert(pubkey); + } else { + wardens.remove(&pubkey); + } + persistence::persist_trusted_wardens(&self.storage, &wardens)?; + policy.set_trusted_wardens(wardens); + Self::load_policy_into(&self.storage, &mut policy); + Ok(()) + } + /// Load the persisted NIP-46 bunker permission grants from the global relay /// config, returning them ready to seed `ServerConfig::pre_grants` so /// remembered grants survive a bunker restart. @@ -4036,9 +4058,9 @@ impl KeepMobile { // so a stored bundle still refuses for want of a signer; on a live // one it means a warden just added does not disappear because the // record could not be read back. - Err(e) => tracing::error!( + Err(_) => tracing::error!( "Trusted wardens unreadable, keeping the set already in memory; a \ - stored policy needing a warden outside it will refuse: {e}" + stored policy needing a warden outside it will refuse" ), } @@ -4052,13 +4074,13 @@ impl KeepMobile { // it is not the retryable arm. Calling it unreadable would put two // Keystore decrypts on every decision for the life of the process // and tell the caller to wait out a fault that never clears. - Err(e @ KeepMobileError::InvalidPolicy { .. }) => { - tracing::error!("Stored policy could not be parsed, every rule will refuse: {e}"); + Err(KeepMobileError::InvalidPolicy { .. }) => { + tracing::error!("Stored policy could not be parsed, every rule will refuse"); evaluator.set_policy_faulted(PolicyFault::Unverified, None); return; } - Err(e) => { - tracing::error!("Policy unreadable, every rule will refuse: {e}"); + Err(_) => { + tracing::error!("Policy unreadable, every rule will refuse"); evaluator.set_policy_faulted(PolicyFault::Unreadable, None); return; } @@ -4069,12 +4091,26 @@ impl KeepMobile { // once they hold is its own `created_at` worth anything, which is what // lets a replacement be measured against an expired bundle rather than // the expired one having to be thrown away first. - let attributed = bundle - .verify_signature() - .and_then(|()| bundle.verify_hash()) - .and_then(|()| bundle.verify_trusted_warden(evaluator.trusted_wardens())); - if let Err(e) = attributed { - tracing::error!("Stored policy did not verify, every rule will refuse: {e}"); + // + // Each arm logs a fixed string naming the check, not the error. The + // error's `Display` drops its `msg` anyway, so `{e}` would print + // "Invalid policy bundle" for all three and leave a hash mismatch, a + // forged signature and an untrusted signer indistinguishable; the + // `msg` itself must not be logged, because it can quote the record. + let attributed = if bundle.verify_signature().is_err() { + Err("signature") + } else if bundle.verify_hash().is_err() { + Err("hash") + } else if bundle + .verify_trusted_warden(evaluator.trusted_wardens()) + .is_err() + { + Err("no trusted signer") + } else { + Ok(()) + }; + if let Err(check) = attributed { + tracing::error!("Stored policy did not verify ({check}), every rule will refuse"); evaluator.set_policy_faulted(PolicyFault::Unverified, None); return; } @@ -7035,7 +7071,9 @@ mod velocity_fault_tests { } use crate::policy::{ create_test_bundle, create_test_bundle_at, create_test_bundle_with, PolicyRules, + MAX_POLICY_AGE_SECS, MAX_POLICY_FUTURE_SECS, POLICY_VERSION, }; + use chrono::Utc; fn plant_policy(storage: &FaultingStorage, rules: &PolicyRules, trust_warden: bool) -> Vec { let bundle = create_test_bundle(rules); @@ -7507,4 +7545,219 @@ mod velocity_fault_tests { PolicyStatus::Active { .. } )); } + + /// A policy that could not be stored must not take effect. Committing to + /// memory before the write means a failed persist returns an error while + /// the process enforces the bundle anyway, and on a faulted record that is + /// a deny-all lifted by a write that never landed. + #[test] + fn a_policy_that_cannot_be_stored_does_not_take_effect() { + 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 stale = + create_test_bundle_with(&warden, &capped_rules(), now - MAX_POLICY_AGE_SECS - 3600); + storage.plant( + POLICY_STORAGE_KEY, + serde_json::to_vec(&stale).expect("serialize policy"), + ); + storage.plant( + TRUSTED_WARDENS_KEY, + serde_json::to_vec(&vec![hex::encode(stale.warden_pubkey)]).expect("serialize wardens"), + ); + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + assert!(matches!( + mobile.policy_status().unwrap(), + PolicyStatus::Faulted { + fault: PolicyFault::Expired + } + )); + + storage.fault_write(POLICY_STORAGE_KEY); + let replacement = create_test_bundle_with(&warden, &PolicyRules::default(), now); + mobile + .import_policy(hex::encode(replacement.to_bytes())) + .expect_err("an unstorable policy must not install"); + + // 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(), + PolicyStatus::Faulted { .. } + )); + assert!(matches!( + mobile.evaluate_policy(a_spend()).unwrap(), + PolicyDecision::Deny { .. } + )); + } + + /// The rollback check has to run against the stored bundle, not against + /// nothing. Without the baseline an older warden-signed bundle installs + /// over a newer one. + #[test] + fn an_older_bundle_cannot_be_installed_over_the_stored_one() { + 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 current = create_test_bundle_with(&warden, &capped_rules(), now); + storage.plant( + POLICY_STORAGE_KEY, + serde_json::to_vec(¤t).expect("serialize policy"), + ); + storage.plant( + TRUSTED_WARDENS_KEY, + serde_json::to_vec(&vec![hex::encode(current.warden_pubkey)]) + .expect("serialize wardens"), + ); + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + assert!(matches!( + mobile.policy_status().unwrap(), + PolicyStatus::Active { .. } + )); + + let older = create_test_bundle_with(&warden, &PolicyRules::default(), now - 3600); + let err = mobile + .import_policy(hex::encode(older.to_bytes())) + .expect_err("an older bundle must not install over a newer one"); + let KeepMobileError::InvalidPolicy { msg } = err else { + panic!("expected a downgrade refusal, got {err:?}"); + }; + assert!(msg.contains("Cannot downgrade"), "{msg}"); + } + + /// A warden the device could not record must not be trusted in memory + /// either. One that is would attribute a stored bundle to a signer this + /// process trusts and the next one does not. + #[test] + fn a_warden_that_cannot_be_stored_is_not_trusted() { + let storage = Arc::new(FaultingStorage::default()); + let warden = plant_policy(&storage, &capped_rules(), false); + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + + storage.fault_write(TRUSTED_WARDENS_KEY); + mobile + .add_trusted_warden(hex::encode(&warden)) + .expect_err("a warden that cannot be persisted must not be trusted"); + + assert!( + mobile.list_trusted_wardens().unwrap().is_empty(), + "memory must not hold a warden storage refused" + ); + assert!(matches!( + mobile.policy_status().unwrap(), + PolicyStatus::Faulted { .. } + )); + } + + /// A bundle dated ahead of the clock is attributed and still not a + /// baseline. Treating it as one measures every honestly dated replacement + /// against a time that has not happened, so all of them read as downgrades + /// and the device can only be recovered by discarding the policy. + #[test] + fn a_future_dated_policy_does_not_refuse_its_own_replacement() { + 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 ahead = create_test_bundle_with( + &warden, + &capped_rules(), + now + MAX_POLICY_FUTURE_SECS + 3600, + ); + storage.plant( + POLICY_STORAGE_KEY, + serde_json::to_vec(&ahead).expect("serialize policy"), + ); + storage.plant( + TRUSTED_WARDENS_KEY, + serde_json::to_vec(&vec![hex::encode(ahead.warden_pubkey)]).expect("serialize wardens"), + ); + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + assert!(matches!( + mobile.policy_status().unwrap(), + PolicyStatus::Faulted { + fault: PolicyFault::NotYetValid + } + )); + + let replacement = create_test_bundle_with(&warden, &PolicyRules::default(), now); + let err = mobile + .import_policy(hex::encode(replacement.to_bytes())) + .expect_err("a future-dated record cannot be checked against"); + let KeepMobileError::InvalidPolicy { msg } = err else { + panic!("expected the fault refusal, got {err:?}"); + }; + // The actionable refusal, not "Cannot downgrade": the stored record is + // the problem, and deleting it is what the caller can do about it. + assert!(msg.contains("Delete the stored policy first"), "{msg}"); + + mobile.delete_policy().unwrap(); + mobile + .import_policy(hex::encode(replacement.to_bytes())) + .expect("a correctly dated bundle must install once the record is gone"); + } + + /// An oversized stored record is refused before it is parsed. + /// + /// The padding sits in a field the bundle does not declare, so serde + /// ignores it and the rules themselves stay inside their own cap: without + /// the length check ahead of the parse this record verifies and loads. That + /// is what makes this a test of the pre-parse bound rather than of the + /// rules-field bound, which catches a different record. + #[test] + fn an_oversized_policy_record_is_refused() { + let storage = Arc::new(FaultingStorage::default()); + let bundle = create_test_bundle(&capped_rules()); + let mut record: serde_json::Value = serde_json::to_value(&bundle).unwrap(); + record + .as_object_mut() + .unwrap() + .insert("padding".into(), serde_json::json!("x".repeat(64 * 1024))); + storage.plant(POLICY_STORAGE_KEY, serde_json::to_vec(&record).unwrap()); + storage.plant( + TRUSTED_WARDENS_KEY, + serde_json::to_vec(&vec![hex::encode(bundle.warden_pubkey)]) + .expect("serialize wardens"), + ); + + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + assert!(matches!( + mobile.policy_status().unwrap(), + PolicyStatus::Faulted { + fault: PolicyFault::Unverified + } + )); + } + + /// A record whose version this build does not know faults rather than being + /// enforced under the rule semantics of a different version. + #[test] + fn a_stored_policy_of_an_unknown_version_faults() { + let storage = Arc::new(FaultingStorage::default()); + let warden = k256::schnorr::SigningKey::random(&mut k256::elliptic_curve::rand_core::OsRng); + let mut bundle = + create_test_bundle_with(&warden, &capped_rules(), Utc::now().timestamp() as u64); + bundle.version = POLICY_VERSION + 1; + // Signed at the new version, so the signature holds and the version + // gate is the only thing that can refuse this record. Without the + // re-signing the altered version breaks the signature and the test + // passes whether the gate exists or not. + crate::policy::resign_test_bundle(&warden, &mut bundle); + storage.plant( + POLICY_STORAGE_KEY, + serde_json::to_vec(&bundle).expect("serialize policy"), + ); + storage.plant( + TRUSTED_WARDENS_KEY, + serde_json::to_vec(&vec![hex::encode(bundle.warden_pubkey)]) + .expect("serialize wardens"), + ); + + let mobile = KeepMobile::new(Arc::clone(&storage) as Arc).unwrap(); + assert!(matches!( + mobile.policy_status().unwrap(), + PolicyStatus::Faulted { + fault: PolicyFault::Unverified + } + )); + } } diff --git a/keep-mobile/src/persistence.rs b/keep-mobile/src/persistence.rs index c78d5d01..e0da74c9 100644 --- a/keep-mobile/src/persistence.rs +++ b/keep-mobile/src/persistence.rs @@ -8,7 +8,7 @@ use serde::{Deserialize, Serialize}; use zeroize::Zeroizing; use crate::error::KeepMobileError; -use crate::policy::{PolicyBundle, POLICY_MAX_RULES_LEN, POLICY_PUBKEY_LEN}; +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; @@ -50,10 +50,27 @@ impl Default for StoredProxyConfig { } } +/// Loose ceiling on the stored policy record, applied before it is parsed. +/// +/// The rules are the only variable-length field, so this is their cap plus +/// slack for the JSON envelope: the fixed fields, the hex for the 32-byte +/// pubkey and hash, and the field names. +const POLICY_RECORD_MAX_LEN: usize = POLICY_MAX_RULES_LEN + 1024; + pub(crate) fn load_policy( storage: &Arc, ) -> Result { let data = storage.load_share_by_key(POLICY_STORAGE_KEY.into())?; + // Ahead of the parse, so the cap bounds what serde is asked to allocate + // rather than only what it produced. The JSON carries the fixed fields plus + // hex for the pubkey and hash, so the record cannot be smaller than the + // rules it holds; this bounds it loosely and the exact field check below + // still applies. + if data.len() > POLICY_RECORD_MAX_LEN { + return Err(KeepMobileError::InvalidPolicy { + msg: "Stored policy record exceeds the maximum length".into(), + }); + } // Deliberately without serde's message, for the same reason load_velocity // drops it: serde quotes the value it tripped on, the record is attacker // controlled before it has verified, and this error reaches a log. @@ -61,8 +78,16 @@ pub(crate) fn load_policy( serde_json::from_slice(&data).map_err(|_| KeepMobileError::InvalidPolicy { msg: "Failed to deserialize policy".into(), })?; - // The wire format caps the rules and the import path caps its input, but a - // record read straight out of storage has been through neither. + // The wire format caps the rules and checks the version, and the import + // path caps its input, but a record read straight out of storage has been + // through neither. The version is covered by `signed_data`, so a mismatch + // here is not forgeable; without the check a warden-signed v2 record would + // be enforced under v1 rule semantics instead of faulting. + if bundle.version != POLICY_VERSION { + return Err(KeepMobileError::InvalidPolicy { + msg: "Stored policy has an unsupported version".into(), + }); + } if bundle.rules_json.len() > POLICY_MAX_RULES_LEN { return Err(KeepMobileError::InvalidPolicy { msg: "Stored policy rules exceed the maximum length".into(), diff --git a/keep-mobile/src/policy.rs b/keep-mobile/src/policy.rs index 192d1bff..4bc4e588 100644 --- a/keep-mobile/src/policy.rs +++ b/keep-mobile/src/policy.rs @@ -20,8 +20,8 @@ pub const POLICY_HASH_LEN: usize = 32; const POLICY_HEADER_LEN: usize = 1 + POLICY_PUBKEY_LEN + POLICY_HASH_LEN + 4; const POLICY_MIN_LEN: usize = POLICY_HEADER_LEN + 8 + POLICY_SIGNATURE_LEN; -const MAX_POLICY_AGE_SECS: u64 = 365 * 24 * 60 * 60; -const MAX_POLICY_FUTURE_SECS: u64 = 60 * 60; +pub(crate) const MAX_POLICY_AGE_SECS: u64 = 365 * 24 * 60 * 60; +pub(crate) const MAX_POLICY_FUTURE_SECS: u64 = 60 * 60; #[derive(Clone, Debug, Serialize, Deserialize)] pub struct PolicyBundle { @@ -196,12 +196,15 @@ pub enum PolicyFault { Unverified, /// Attributed, and dated further ahead than the clock allows for. A device /// that boots before its clock syncs does this to a perfectly good policy, - /// so it is retried. + /// so it is retried. Not a rollback baseline: its `created_at` is a time + /// that has not happened, so it is no evidence of age, and treating it as + /// one refuses every correctly dated replacement for being older. NotYetValid, /// Attributed, and past its maximum age. Not retried, because it will not /// come back, but a replacement can be installed over it: by the time this /// is reached the signature and signer already held, so the bundle's - /// `created_at` is still a sound baseline to measure a downgrade against. + /// `created_at` is still a sound baseline to measure a downgrade against, + /// and it is the only fault that supplies one. Expired, } @@ -439,10 +442,23 @@ 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. Saturating, so a clock set before - /// 1970 cannot make the comparison wrap. + /// resolves by itself, expiring does not. pub fn timestamp_fault(&self) -> Option { - let now = Utc::now().timestamp() as u64; + self.timestamp_fault_at(Utc::now().timestamp()) + } + + /// The clock is a parameter so the bounds can be tested at an exact instant + /// either side of them, including a negative one, which `Utc::now` cannot + /// be made to return. + /// + /// 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. + 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) { return Some(PolicyFault::NotYetValid); @@ -567,8 +583,18 @@ impl PolicyEvaluator { pub fn rollback_baseline(&self) -> Option<&PolicyBundle> { match &self.policy { PolicyState::Active(bundle) => Some(bundle), - PolicyState::Faulted { attributed, .. } => attributed.as_deref(), - PolicyState::NotConfigured => None, + // A future-dated bundle is attributed and still not a baseline, so + // this cannot be `attributed.as_deref()` for every fault. Its + // `created_at` is ahead of the clock, so every honestly dated + // replacement reads as a downgrade and is refused, and nothing can + // be installed until the record is deleted outright. Dropping the + // baseline here sends that case to `import_policy`'s own refusal, + // which says what to do about it. + PolicyState::Faulted { + fault: PolicyFault::Expired, + attributed, + } => attributed.as_deref(), + PolicyState::Faulted { .. } | PolicyState::NotConfigured => None, } } @@ -601,9 +627,10 @@ impl PolicyEvaluator { let bundle = match &self.policy { PolicyState::NotConfigured => return Ok(PolicyDecision::Allow), // The reason says only that it did not verify. Which check failed - // is logged where the record is read, because the record's contents - // are attacker-controlled once it has failed verification and this - // string reaches a caller. + // is logged as a fixed string where the record is read, never the + // error's own message: the record's contents are attacker + // controlled once it has failed verification, and this string + // reaches a caller. PolicyState::Faulted { fault, .. } => { return Ok(PolicyDecision::Deny { reason: format!("Cannot evaluate the spending policy: {fault}"), @@ -820,10 +847,46 @@ pub(crate) fn create_test_bundle_with( bundle } +/// Re-signs a bundle after a test has altered a signed field, so a test can +/// isolate a check that would otherwise be masked by the signature failing +/// first. `signed_data` covers the version, so a test for the version gate +/// needs a bundle genuinely signed at that version. +#[cfg(test)] +pub(crate) fn resign_test_bundle( + signing_key: &k256::schnorr::SigningKey, + bundle: &mut PolicyBundle, +) { + use k256::schnorr::signature::Signer; + + let signed_data = bundle.signed_data(); + let mut hasher = Sha256::new(); + hasher.update(&signed_data); + let message: [u8; 32] = hasher.finalize().into(); + let signature = signing_key.sign(&message); + bundle.signature.copy_from_slice(&signature.to_bytes()); +} + #[cfg(test)] mod tests { use super::*; + /// A clock set before 1970 must land on the retried fault, not the + /// permanent one. `as u64` on a negative turns it into roughly 1.8e19, + /// which clears the future bound and trips the age bound, so a perfectly + /// good bundle reads as `Expired` and is never re-read for the life of the + /// process even once the clock is set. + #[test] + fn a_clock_before_1970_is_not_yet_valid_rather_than_expired() { + let rules = PolicyRules::default(); + let bundle = create_test_bundle_at(&rules, 1_700_000_000); + + assert_eq!( + bundle.timestamp_fault_at(-86_400), + Some(PolicyFault::NotYetValid) + ); + assert!(PolicyFault::NotYetValid.is_transient()); + } + #[test] fn test_bundle_roundtrip() { let rules = PolicyRules {