From 4e83143337be2a892c41db8f8bc1ac50008733dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kyle=20=F0=9F=90=86?= Date: Fri, 7 Aug 2026 15:32:55 -0400 Subject: [PATCH 1/2] Lock a file the nonce store's own rename cannot unlink --- keep-cli/src/signer/nonce_store.rs | 116 +++++++++++++++++++++++++---- 1 file changed, 101 insertions(+), 15 deletions(-) diff --git a/keep-cli/src/signer/nonce_store.rs b/keep-cli/src/signer/nonce_store.rs index 1fadfe6b..1da74544 100644 --- a/keep-cli/src/signer/nonce_store.rs +++ b/keep-cli/src/signer/nonce_store.rs @@ -5,7 +5,7 @@ use fs2::FileExt; use serde::{Deserialize, Serialize}; use std::collections::HashMap; use std::fs::OpenOptions; -use std::io::{Read, Write}; +use std::io::Write; use std::path::{Path, PathBuf}; #[derive(Debug, Clone, Serialize, Deserialize)] @@ -84,23 +84,57 @@ impl NonceStore { std::fs::create_dir_all(parent).context("Failed to create nonce store directory")?; } - let file = OpenOptions::new() - .read(true) - .write(true) - .create(true) - .truncate(false) - .open(&self.path) - .context("Failed to open nonce store")?; - - file.lock_exclusive() + // Lock a dedicated sibling, never the store itself. + // + // Two things are wrong with locking the store, and they fail + // differently. The reader below used to read from the locked handle, so + // a caller that opened the store before another replaced it read the + // unlinked file and never saw the claim it had just waited for. That is + // the one the concurrency test reproduces. The second is not covered by + // any test here: once the store has been replaced, one caller can hold + // a lock on the unlinked inode while another holds a lock on the file + // that replaced it, so both are inside the critical section at once and + // exclusion has quietly stopped meaning anything. A sibling that is + // never renamed is what keeps the second case impossible. + // + // Locking the store meant locking an inode the write path then renamed + // over, which unlinks it. A second caller that opened the store before + // that rename held the old inode, waited on it, and was handed the lock + // once the first caller finished, at which point it read the unlinked + // file: the state from before the claim it was waiting for. It then + // re-issued the same round-1 commitment, and two shares over one nonce + // under different challenges recover the signer's key share, which is + // the outcome this guard exists to prevent. Serialising on a file that + // is never replaced is what makes the critical section mean anything. + let lock_path = self.path.with_extension("lock"); + let lock_file = { + let mut opts = OpenOptions::new(); + opts.create(true).write(true).truncate(false); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + opts.mode(0o600); + } + opts.open(&lock_path) + .context("Failed to open nonce store lock")? + }; + lock_file + .lock_exclusive() .context("Failed to acquire nonce store lock")?; + // Opened inside the critical section, so this always resolves to the + // file the previous holder left behind rather than one captured before + // they replaced it. let mut data = { let mut content = String::new(); - let mut reader = &file; - reader - .read_to_string(&mut content) - .context("Failed to read nonce store")?; + match std::fs::read_to_string(&self.path) { + Ok(c) => content = c, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => { + let _ = FileExt::unlock(&lock_file); + return Err(anyhow::Error::from(e).context("Failed to read nonce store")); + } + } if content.is_empty() { NonceStoreData { version: STORE_VERSION, @@ -122,7 +156,7 @@ impl NonceStore { self.data = data; - FileExt::unlock(&file).context("Failed to release nonce store lock")?; + FileExt::unlock(&lock_file).context("Failed to release nonce store lock")?; Ok(result) } @@ -436,4 +470,56 @@ mod tests { or the caller signs against a claim that may not survive a reboot" ); } + + /// Concurrent handles must not both hand out the same commitment. + /// + /// The sequential test above cannot reach this. It claims and then claims + /// again, so the second call opens the store after the first replaced it and + /// naturally sees the claim. The dangerous interleaving is the one where a + /// caller opens the store, waits on the lock while the holder replaces the + /// file, and is then handed a lock on the file that was replaced. + /// + /// Detection is probabilistic in the direction that is safe: on the fixed + /// code exactly one winner is guaranteed, so this cannot fail spuriously. + /// On the broken code it needs the threads to overlap, which repeating the + /// round makes very likely without making a green run a lie. + #[test] + fn concurrent_handles_hand_out_a_commitment_exactly_once() { + for round in 0..40 { + let dir = tempfile::tempdir().unwrap(); + // Establish the store so every thread races on replacing it rather + // than on creating it. + store(dir.path()) + .check_and_add_nonce("g", "seed", None) + .unwrap(); + + let path = dir.path().to_path_buf(); + let barrier = std::sync::Arc::new(std::sync::Barrier::new(8)); + let handles: Vec<_> = (0..8) + .map(|_| { + let path = path.clone(); + let barrier = std::sync::Arc::clone(&barrier); + std::thread::spawn(move || { + let mut s = NonceStore::open(&path).unwrap(); + barrier.wait(); + s.check_and_add_nonce("g", "contested", None) + .unwrap_or(false) + }) + }) + .collect(); + + let winners = handles + .into_iter() + .filter(|_| true) + .map(|h| h.join().unwrap()) + .filter(|granted| *granted) + .count(); + + assert_eq!( + winners, 1, + "round {round}: exactly one caller may be handed a commitment; \ + more than one means two signature shares under one nonce" + ); + } + } } From 3c2c87598a9e288508034b86fed2f784039c20ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kyle=20=F0=9F=90=86?= Date: Fri, 7 Aug 2026 15:45:43 -0400 Subject: [PATCH 2/2] Pin the lock-inode invariant the fix rests on --- keep-cli/src/signer/nonce_store.rs | 59 +++++++++++++++++++++++++++--- 1 file changed, 53 insertions(+), 6 deletions(-) diff --git a/keep-cli/src/signer/nonce_store.rs b/keep-cli/src/signer/nonce_store.rs index 1da74544..7eea200a 100644 --- a/keep-cli/src/signer/nonce_store.rs +++ b/keep-cli/src/signer/nonce_store.rs @@ -483,6 +483,11 @@ mod tests { /// code exactly one winner is guaranteed, so this cannot fail spuriously. /// On the broken code it needs the threads to overlap, which repeating the /// round makes very likely without making a green run a lie. + /// + /// It covers the stale-read half only. Reverting just the lock leaves this + /// green, because with the read taken by path there is no stale handle to + /// read from. The exclusion half is pinned separately, below, by asserting + /// the locked inode is the one thing the write path never replaces. #[test] fn concurrent_handles_hand_out_a_commitment_exactly_once() { for round in 0..40 { @@ -503,17 +508,18 @@ mod tests { let mut s = NonceStore::open(&path).unwrap(); barrier.wait(); s.check_and_add_nonce("g", "contested", None) - .unwrap_or(false) }) }) .collect(); - let winners = handles + // Every thread must succeed, not merely not-win. Mapping errors to + // "not granted" would let an implementation that fails under + // contention for seven of eight callers still show one winner. + let outcomes: Vec = handles .into_iter() - .filter(|_| true) - .map(|h| h.join().unwrap()) - .filter(|granted| *granted) - .count(); + .map(|h| h.join().unwrap().expect("a contended claim must not error")) + .collect(); + let winners = outcomes.iter().filter(|granted| **granted).count(); assert_eq!( winners, 1, @@ -522,4 +528,45 @@ mod tests { ); } } + + /// The locked file must be the one thing a claim does not replace. + /// + /// This is the invariant the whole fix rests on, and unlike the race above + /// it is directly observable: a claim replaces the store, so the store's + /// inode changes, and it must not touch the lock's. Locking the store made + /// those the same inode, so the lock was destroyed by the very operation it + /// was meant to be protecting. + /// + /// Deterministic and thread-free. It also fails if someone later deletes + /// the lock file between calls as a cleanup, which is the realistic way + /// this regresses. + #[cfg(unix)] + #[test] + fn a_claim_replaces_the_store_and_never_the_lock() { + use std::os::unix::fs::MetadataExt; + + let dir = tempfile::tempdir().unwrap(); + let mut s = store(dir.path()); + s.check_and_add_nonce("g", "first", None).unwrap(); + + let store_path = dir.path().join("nonce_store.json"); + let lock_path = dir.path().join("nonce_store.lock"); + let store_before = std::fs::metadata(&store_path).unwrap().ino(); + let lock_before = std::fs::metadata(&lock_path).unwrap().ino(); + + s.check_and_add_nonce("g", "second", None).unwrap(); + + assert_ne!( + store_before, + std::fs::metadata(&store_path).unwrap().ino(), + "a claim is written by replacing the store, so its inode must change" + ); + assert_eq!( + lock_before, + std::fs::metadata(&lock_path).unwrap().ino(), + "the locked file must survive the claim; if the write path replaces \ + it, callers end up holding locks on different inodes and exclusion \ + silently stops applying" + ); + } }