From 8f8c98830f1fa7034fb2df2cdb1d549d0f60e7e4 Mon Sep 17 00:00:00 2001 From: qnbs <155236708+qnbs@users.noreply.github.com> Date: Thu, 15 Oct 2026 21:09:56 +0200 Subject: [PATCH 1/3] feat(core): harden the Unix file removal against a same-user actor in the quarantine directory (#445) Clean successor of #1072 (head ae96312f, 15 of 15 commits, budget SATURATED): the reviewed net state of that PR plus the two findings that were still open on it. test-support only, no caller, no new unsafe, no new package. Carried from #1072: the parent is synced after a rollback, a stranded entry, a put-back and a vanished name; the quarantine directory an entry moved through is synced too, and a sync of it that fails is an error after a put-back, a kept entry or an entry an actor removed again and makes a removal NotConfirmed, never another outcome; the rename alone decides `restored`; the quarantine directory is removed by its name only after an identity check, also when its setup fails; an entry is reported as retained only if the recorded name still leads to its directory, otherwise Disturbed; a quarantine directory that cannot be opened is refused. Fixed on top of it: - A foreign entry (link, directory, FIFO, inaccessible file) carries the identity it was seen with (a look that does not follow a link), as an opened object does, so an actor that replaces it after the inspection while the put-back is blocked gets Disturbed instead of Swapped { restored: false }. An identity that cannot be taken is a failed inspection and not a retained entry, so an unobservable state earns no unproven success. The test replaces the entry at the sync of the quarantine directory, the one point between the inspection and the check that a test can reach, and a control leaves the same entry untouched (it is still kept). - The changelog no longer says the name-based cleanup removes "only if" the name leads to the directory that was created: the check narrows the race and does not close it, as the code and the evidence document already said. Mutation-checked, each caught: passing no identity for a foreign entry, plus the mutations recorded on #1072 for the sync, the identity comparison and the vanished-entry paths. The failing-sync cases use a small cfg(test) seam that models the error; no crash is simulated. Issue #359. The PR number in the changelog entry is to be set to the number of this PR when it is opened. --- CHANGELOG.md | 2 + .../src/pinned/removal.rs | 26 +- .../src/pinned/tests/mod.rs | 4 + .../src/pinned/tests/remove_unix.rs | 263 ++++++++++++-- .../src/pinned/tests/remove_unix_cleanup.rs | 65 ++++ .../src/pinned/tests/remove_unix_sync.rs | 265 +++++++++++++++ .../src/pinned/unix/removal.rs | 320 ++++++++++++++++-- .../r15/GATE4D-JOURNAL-DURABLE-EVIDENCE.md | 58 +++- 8 files changed, 924 insertions(+), 79 deletions(-) create mode 100644 crates/worldscript-secure-storage/src/pinned/tests/remove_unix_cleanup.rs create mode 100644 crates/worldscript-secure-storage/src/pinned/tests/remove_unix_sync.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a72b160b..b79e049ff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +- **R-15 Gate 4:** hardening of the Unix file removal against a same-user actor in the quarantine directory, `test-support` only (slice H1a-2, part 2b of 5). The parent is synced after a rollback, a stranded entry, a put-back and a vanished name, and the quarantine directory an entry moved through is synced too (a sync of it that fails is an error after a put-back, a kept entry or an entry an actor removed again, and makes a removal `NotConfirmed`, never another outcome); the quarantine directory, also when its setup fails, is removed by its name only after a check that the name still leads to the directory that was created (the check narrows the race and does not close it, since `rmdir` takes a name: a lost race removes at most an empty directory); an entry, a foreign one included, is reported as retained only if the recorded name still leads to its directory and the entry in it is the one that was seen, otherwise `Disturbed` (also a look at a name, so it detects after the fact and does not prevent); a quarantine directory that cannot be opened is refused. Tests force each case, including the quarantine renamed away and the put-back blocked. No new `unsafe`, no new package. Issue #359. PR #1072. + - **R-15 Gate 4:** removal of a file on Unix, `test-support` only (slice H1a-2, part 2 of 4). `PinnedDir::remove_authenticated_file` moves the entry into a quarantine directory with `RENAME_NOREPLACE`, identifies the open object there and only then unlinks it; anything else is put back, a failure after the move rolls back or reports `Stranded`, and the parent is synced. The one window no Unix primitive closes (a same-user actor in the quarantine directory after the last identification) is forced in a test and reported as `Disturbed`, detected after the fact and not prevented. The rustix `process` feature is added for `geteuid`; no new package, no new `unsafe`. Directories, leftovers and Windows follow. Issue #359. PR #1069. - **R-15 Gate 4:** the API and the isolation of the bound removal, `test-support` only (slice H1a-2, part 1 of 4). `PinnedDir::remove_authenticated_file`, `remove_empty_dir` and `remove_leftover_file` with `RemovalOutcome`, `RemovalStage` and `Observer` exist only with the test-only feature, have no caller and, in this part, answer `Unsupported` and remove nothing; the platform implementations follow in three stacked parts. A scan of the sources and a compile of a dependent with and without the feature keep the primitives out of every other build. `PinnedDir::sync` (no deletion) is added. The contract states what no Unix primitive can do (unlink "this inode": a same-user actor in the quarantine directory can win one race, which is detected after the fact as `Disturbed`, not prevented) and leaves the decision about that residual to the maintainer before any sweep uses it. No new `unsafe`, no new dependency feature, no new package. Issue #359. PR #1068. diff --git a/crates/worldscript-secure-storage/src/pinned/removal.rs b/crates/worldscript-secure-storage/src/pinned/removal.rs index 14299abaf..b5d9f9273 100644 --- a/crates/worldscript-secure-storage/src/pinned/removal.rs +++ b/crates/worldscript-secure-storage/src/pinned/removal.rs @@ -51,27 +51,37 @@ pub const QUARANTINE_PREFIX: &str = ".h1q-"; pub enum RemovalOutcome { /// The authenticated object lost this name (on Unix as far as the check after the unlink can tell, /// see [`Disturbed`](Self::Disturbed)); the durability of the directory entry change as far as the - /// platform can confirm it. + /// platform can confirm it. On Unix that covers the directory and the quarantine directory the + /// object was unlinked from: if the latter could not be synced the removal is `NotConfirmed`. Removed(DirectoryDurability), /// The authentication said no; nothing was changed. Refused, - /// The name no longer led to an object (someone else removed it); nothing was removed. + /// The name no longer led to an object (someone else removed it); nothing was removed. If the entry + /// had been moved aside and was gone from there, the quarantine directory was synced as for + /// [`Swapped`](Self::Swapped), and a sync that fails is an error. Vanished, /// The directory still has entries; nothing was removed. NotEmpty, /// Something other than the authenticated object was bound to the name. It was not removed: it is /// back under its name (`restored`) or, if the name was taken meanwhile, kept in the quarantine - /// directory (`.h1q-*`, never removed by a sweep). + /// directory (`.h1q-*`, never removed by a sweep). Reported only after the directory and the + /// quarantine directory the entry moved through were synced. A sync that fails is an error and not + /// another outcome: the rename decided `restored`, and the entry is back or aside whatever the sync + /// did. Swapped { restored: bool }, - /// Unix only: after the unlink the authenticated object had not lost a name, so the name was bound - /// to another object when it was unlinked (see the window in the module documentation). That - /// object is gone and nothing can be undone: the caller must treat the directory as tampered with, - /// and never count this as a success. + /// Unix only: the quarantine directory was tampered with. Either the authenticated object had not + /// lost a name after the unlink, so the name was bound to another object when it was unlinked (see + /// the window in the module documentation), and that object is gone; or the entry could not be put + /// back and is not where the outcome would have said: an actor renamed the quarantine directory + /// away, removed the entry, or replaced it with another object, so the authenticated object cannot + /// be found by the name that was recorded. Nothing can be undone: the caller must treat the + /// directory as tampered with, and never count this as a success. Disturbed, /// Unix only: an error happened after the entry was moved into the quarantine directory and it /// could not be put back (`cause` is the error that started it). The entry is in the quarantine /// directory (`.h1q-*`, as `x`), found with a listing; [`PinnedDir::remove_leftover_file`] takes it - /// from there after authenticating it again. Nothing was removed. + /// from there after authenticating it again. Nothing was removed. This reports a failure by its + /// cause, so both directories are synced best effort and a crash may undo where the entry is. Stranded { cause: PinnedError }, } diff --git a/crates/worldscript-secure-storage/src/pinned/tests/mod.rs b/crates/worldscript-secure-storage/src/pinned/tests/mod.rs index 950f4ed6c..7479f89d5 100644 --- a/crates/worldscript-secure-storage/src/pinned/tests/mod.rs +++ b/crates/worldscript-secure-storage/src/pinned/tests/mod.rs @@ -10,6 +10,10 @@ mod read; mod remove; #[cfg(all(feature = "test-support", unix))] mod remove_unix; +#[cfg(all(feature = "test-support", unix))] +mod remove_unix_cleanup; +#[cfg(all(feature = "test-support", unix))] +mod remove_unix_sync; #[cfg(windows)] mod windows_redirect; diff --git a/crates/worldscript-secure-storage/src/pinned/tests/remove_unix.rs b/crates/worldscript-secure-storage/src/pinned/tests/remove_unix.rs index baffda71e..addae62e5 100644 --- a/crates/worldscript-secure-storage/src/pinned/tests/remove_unix.rs +++ b/crates/worldscript-secure-storage/src/pinned/tests/remove_unix.rs @@ -14,19 +14,19 @@ use rustix::fs::{unlinkat, AtFlags}; use super::remove::{holds, names, removed}; use super::Scratch; use crate::durable::DirectoryDurability; -use crate::pinned::unix::removal::{make_usable, INSIDE}; +use crate::pinned::unix::removal::{both_confirmed, make_usable, INSIDE}; use crate::pinned::{PinnedDir, PinnedError, RemovalOutcome, RemovalStage, QUARANTINE_PREFIX}; /// A pinned directory holding `a` = "alpha", with the paths a swap needs. -struct Fixture { +pub(super) struct Fixture { /// Keeps the directories until the test ends. _scratch: Scratch, - root: PathBuf, - aside: PathBuf, - pinned: PinnedDir, + pub(super) root: PathBuf, + pub(super) aside: PathBuf, + pub(super) pinned: PinnedDir, } -fn fixture(label: &str) -> Fixture { +pub(super) fn fixture(label: &str) -> Fixture { let scratch = Scratch::new(label); let root = scratch.root(); let aside = scratch.aside(); @@ -42,7 +42,7 @@ fn fixture(label: &str) -> Fixture { /// The stages of a removal, without their payload. #[derive(Clone, Copy, PartialEq)] -enum At { +pub(super) enum At { Checked, MovedAside, Identified, @@ -50,7 +50,7 @@ enum At { } /// The stage, and the path inside the quarantine directory once there is one. -fn at(stage: RemovalStage<'_>) -> (At, Option<&Path>) { +pub(super) fn at(stage: RemovalStage<'_>) -> (At, Option<&Path>) { match stage { RemovalStage::Checked => (At::Checked, None), RemovalStage::MovedAside { quarantined } => (At::MovedAside, Some(quarantined)), @@ -83,12 +83,12 @@ fn failing_at(here: At) -> impl FnMut(RemovalStage<'_>) -> Result<(), PinnedErro } } -fn denied() -> PinnedError { +pub(super) fn denied() -> PinnedError { PinnedError::Io(std::io::ErrorKind::PermissionDenied) } /// The last-component swap: the original is renamed out of the way and a live file takes its name. -fn swap_in_a_live_file(root: &Path, aside: &Path) { +pub(super) fn swap_in_a_live_file(root: &Path, aside: &Path) { fs::rename(root.join("a"), aside.join("original")).unwrap(); fs::write(root.join("a"), b"live").unwrap(); } @@ -99,7 +99,7 @@ fn ino(path: &Path) -> u64 { } /// The quarantine directories a removal left in `root`. -fn quarantines(root: &Path) -> Vec { +pub(super) fn quarantines(root: &Path) -> Vec { let mut found: Vec = fs::read_dir(root) .unwrap() .map(|entry| entry.unwrap().path()) @@ -115,7 +115,7 @@ fn quarantines(root: &Path) -> Vec { } /// The content of the entry left in the only quarantine directory of `root`. -fn held_in_quarantine(root: &Path) -> Option> { +pub(super) fn held_in_quarantine(root: &Path) -> Option> { let all = quarantines(root); assert!(all.len() <= 1, "{all:?}"); fs::read(all.first()?.join(INSIDE)).ok() @@ -388,24 +388,31 @@ fn a_name_taken_after_the_move_keeps_the_new_file_and_a_vanished_name_is_reporte }), ); let gone = fixture("vanish"); - let vanished = gone.pinned.remove_authenticated_file_observed( - "a", - holds(b"alpha"), - &mut on(At::Checked, |_| { - fs::remove_file(gone.root.join("a")).unwrap() - }), - ); + let mut synced = false; + let vanished = + gone.pinned + .remove_authenticated_file_observed("a", holds(b"alpha"), &mut |stage| { + match at(stage) { + (At::Checked, _) => fs::remove_file(gone.root.join("a")).unwrap(), + // The quarantine directory came and went, which changed the parent. + (At::BeforeSync, _) => synced = true, + _ => {} + } + Ok(()) + }); assert_eq!( ( after, fs::read(taken.root.join("a")).unwrap(), vanished, + synced, names(&gone.pinned) ), ( removed(), b"new".to_vec(), Ok(RemovalOutcome::Vanished), + true, Vec::::new() ) ); @@ -464,24 +471,35 @@ fn the_quarantine_directory_is_private_usable_under_any_umask_and_gone_afterward } #[test] -fn a_failure_at_any_stage_leaves_the_directory_as_it_was_found() { +fn a_failure_at_any_stage_leaves_the_directory_as_it_was_found_and_syncs_it_after_a_move() { let outcomes: Vec<_> = [At::Checked, At::MovedAside, At::Identified] .into_iter() .map(|here| { let f = fixture("rollback"); - let outcome = f.pinned.remove_authenticated_file_observed( - "a", - holds(b"alpha"), - &mut failing_at(here), - ); + let synced = std::cell::Cell::new(false); + let outcome = + f.pinned + .remove_authenticated_file_observed("a", holds(b"alpha"), &mut |stage| { + if at(stage).0 == At::BeforeSync { + synced.set(true); + return Ok(()); + } + failing_at(here)(stage) + }); ( outcome, fs::read(f.root.join("a")).unwrap(), quarantines(&f.root).len(), + synced.get(), ) }) .collect(); - assert_eq!(outcomes, vec![(Err(denied()), b"alpha".to_vec(), 0); 3]); + let rolled_back = |synced| (Err(denied()), b"alpha".to_vec(), 0, synced); + // Nothing moved before the check, so there is nothing to sync; after the move the directory changed. + assert_eq!( + outcomes, + vec![rolled_back(false), rolled_back(true), rolled_back(true)] + ); } #[test] @@ -533,9 +551,9 @@ fn lock_the_quarantine(quarantined: Option<&Path>) { } #[test] -fn an_error_after_the_move_that_cannot_be_rolled_back_is_reported_as_stranded() { +fn a_search_permission_lost_after_the_move_is_reported_as_stranded() { if rustix::process::geteuid().is_root() { - // Root is not kept out by permissions; the case is about the permission failure itself. + // Root is not kept out by permissions; the same logic is covered without them by the next test. eprintln!("skipped: permissions do not apply to root"); return; } @@ -560,3 +578,192 @@ fn an_error_after_the_move_that_cannot_be_rolled_back_is_reported_as_stranded() ) ); } + +#[test] +fn an_error_after_the_move_whose_put_back_is_blocked_is_reported_as_stranded_whoever_runs_it() { + let f = fixture("stranded-injected"); + let original = ino(&f.root.join("a")); + let mut synced = false; + let outcome = f + .pinned + .remove_authenticated_file_observed("a", holds(b"alpha"), &mut |stage| { + match at(stage) { + // The name is taken again and the observer fails: the entry cannot go back. + (At::MovedAside, _) => { + fs::write(f.root.join("a"), b"taken").unwrap(); + return Err(denied()); + } + (At::BeforeSync, _) => synced = true, + _ => {} + } + Ok(()) + }); + let left = quarantines(&f.root); + assert_eq!( + ( + outcome, + fs::read(f.root.join("a")).unwrap(), + left.len(), + ino(&left[0].join(INSIDE)) == original, + synced + ), + ( + Ok(RemovalOutcome::Stranded { cause: denied() }), + b"taken".to_vec(), + 1, + true, + true + ) + ); +} + +#[test] +fn a_quarantine_name_that_leads_somewhere_else_at_the_end_is_not_removed() { + // The held directory is renamed away and an empty directory of the actor takes its name: the + // removal goes on in the directory it holds and leaves the actor's directory alone. + let f = fixture("discard"); + let outcome = f.pinned.remove_authenticated_file_observed( + "a", + holds(b"alpha"), + &mut on(At::MovedAside, |quarantined| { + let directory = quarantined.unwrap().parent().unwrap(); + fs::rename(directory, f.aside.join("held")).unwrap(); + fs::create_dir(directory).unwrap(); + }), + ); + let held = fs::read_dir(f.aside.join("held")).unwrap().count(); + assert_eq!( + (outcome, held, quarantines(&f.root).len()), + (removed(), 0, 1) + ); +} + +#[test] +fn a_quarantine_directory_the_owner_cannot_open_is_refused_fail_closed() { + if rustix::process::geteuid().is_root() { + eprintln!("skipped: permissions do not apply to root"); + return; + } + // A umask that takes the owner's read access away leaves a directory that cannot be opened: the + // removal is refused and nothing of the store is touched. + let f = fixture("owner-masked"); + let name = format!("{QUARANTINE_PREFIX}{}", "b".repeat(32)); + fs::create_dir(f.root.join(&name)).unwrap(); + fs::set_permissions(f.root.join(&name), fs::Permissions::from_mode(0o000)).unwrap(); + let refused = make_usable(&f.pinned.handle, name.as_ref()); + fs::set_permissions(f.root.join(&name), fs::Permissions::from_mode(0o700)).unwrap(); + assert_eq!( + refused, + Err(PinnedError::Io(std::io::ErrorKind::PermissionDenied)) + ); +} + +/// What the actor does to the quarantine directory once the entry is inside, while the put-back is +/// blocked by taking the name again and the observer fails. +type Tamper = (&'static str, fn(&Path, &Path, &Path)); + +#[test] +fn an_entry_that_is_not_where_the_outcome_would_say_is_reported_as_disturbed() { + let tampers: [Tamper; 3] = [ + // The held directory is renamed away: the recorded name leads somewhere else. + ("renamed-away", |directory, _inside, aside| { + fs::rename(directory, aside.join("held")).unwrap(); + }), + // The entry itself is removed: the directory is still named, but holds nothing. + ("entry-removed", |_directory, inside, _aside| { + fs::remove_file(inside).unwrap(); + }), + // The entry is moved away and another object takes its place: something is there, not the + // authenticated object. + ("entry-replaced", |_directory, inside, aside| { + fs::rename(inside, aside.join("authenticated")).unwrap(); + fs::write(inside, b"foreign").unwrap(); + }), + ]; + let outcomes: Vec<_> = tampers + .into_iter() + .map(|(label, tamper)| { + let f = fixture(label); + let outcome = + f.pinned + .remove_authenticated_file_observed("a", holds(b"alpha"), &mut |stage| { + if let (At::MovedAside, Some(inside)) = at(stage) { + tamper(inside.parent().unwrap(), inside, &f.aside); + fs::write(f.root.join("a"), b"taken").unwrap(); + return Err(denied()); + } + Ok(()) + }); + (label, outcome, fs::read(f.root.join("a")).unwrap()) + }) + .collect(); + let disturbed = |label| (label, Ok(RemovalOutcome::Disturbed), b"taken".to_vec()); + assert_eq!( + outcomes, + vec![ + disturbed("renamed-away"), + disturbed("entry-removed"), + disturbed("entry-replaced") + ] + ); +} + +#[test] +fn a_quarantine_directory_an_actor_put_an_entry_into_survives_and_the_removal_still_completes() { + // The directory cannot be removed while it holds the actor's entry; the authenticated file is gone + // all the same, and the actor's entry is untouched. + let f = fixture("occupied-quarantine"); + let outcome = f.pinned.remove_authenticated_file_observed( + "a", + holds(b"alpha"), + &mut on(At::Identified, |inside| { + let directory = inside.unwrap().parent().unwrap(); + fs::write(directory.join("other"), b"foreign").unwrap(); + }), + ); + let left = quarantines(&f.root); + assert_eq!( + (outcome, f.root.join("a").exists(), left.len()), + (removed(), false, 1) + ); + assert_eq!(fs::read(left[0].join("other")).unwrap(), b"foreign"); + assert!(!left[0].join(INSIDE).exists()); +} + +#[test] +fn a_removal_is_confirmed_only_if_both_directories_were_synced() { + use DirectoryDurability::{Confirmed, NotConfirmed}; + let all = [ + (Confirmed, Confirmed), + (Confirmed, NotConfirmed), + (NotConfirmed, Confirmed), + (NotConfirmed, NotConfirmed), + ]; + assert_eq!( + all.map(|(parent, held)| both_confirmed(parent, held)), + [Confirmed, NotConfirmed, NotConfirmed, NotConfirmed] + ); +} + +#[test] +fn a_quarantine_renamed_away_is_still_removed_through_the_held_directory_and_the_entry_is_gone() { + // The removal goes on in the directory it holds even if its name leads elsewhere; the entry is + // unlinked there, and that directory is synced as well as the parent. + let f = fixture("renamed-held"); + let outcome = f.pinned.remove_authenticated_file_observed( + "a", + holds(b"alpha"), + &mut on(At::MovedAside, |inside| { + let directory = inside.unwrap().parent().unwrap(); + fs::rename(directory, f.aside.join("held")).unwrap(); + }), + ); + assert_eq!( + ( + outcome, + f.aside.join("held").join(INSIDE).exists(), + f.root.join("a").exists() + ), + (removed(), false, false) + ); +} diff --git a/crates/worldscript-secure-storage/src/pinned/tests/remove_unix_cleanup.rs b/crates/worldscript-secure-storage/src/pinned/tests/remove_unix_cleanup.rs new file mode 100644 index 000000000..fa4825678 --- /dev/null +++ b/crates/worldscript-secure-storage/src/pinned/tests/remove_unix_cleanup.rs @@ -0,0 +1,65 @@ +//! The cleanup of a quarantine directory by its name removes the directory that was made, and only when +//! it is empty. A name that leads somewhere else by then (an actor renamed the directory away and put +//! one of its own at the name) is left alone, and so is everything when no identity was taken. The check +//! narrows the race and does not close it (`rmdir` takes a name); these cases pin what it does decide. + +use std::fs; +use std::os::unix::fs::MetadataExt; +use std::path::Path; + +use super::remove_unix::fixture; +use crate::pinned::unix::removal::remove_if_named; +use crate::pinned::ObjectIdentity; + +fn identity(path: &Path) -> ObjectIdentity { + let metadata = fs::symlink_metadata(path).unwrap(); + ObjectIdentity::new(metadata.dev(), u128::from(metadata.ino())) +} + +/// What is left after `remove_if_named` was asked about the directory `q` of a fixture whose `setup` +/// ran: whether it reported a removal, whether the name still leads to a directory, and whether the +/// directory that was made is still there (renamed away). +fn cleanup( + label: &str, + setup: impl FnOnce(&Path, &Path), + expected: impl FnOnce(&Path) -> Option, +) -> (bool, bool, bool) { + let f = fixture(label); + let q = f.root.join("q"); + fs::create_dir(&q).unwrap(); + let wanted = expected(&q); + setup(&q, &f.aside); + let removed = remove_if_named(&f.pinned.handle, "q".as_ref(), wanted); + (removed, q.is_dir(), f.aside.join("made").is_dir()) +} + +#[test] +fn only_the_directory_that_was_made_is_removed_and_only_when_empty() { + let same = cleanup("same", |_, _| {}, |q| Some(identity(q))); + // The directory was renamed away and an empty one of the actor's took its name. + let replaced = cleanup( + "replaced", + |q, aside| { + fs::rename(q, aside.join("made")).unwrap(); + fs::create_dir(q).unwrap(); + }, + |q| Some(identity(q)), + ); + // The directory is not empty: `rmdir` refuses, and nothing of it is touched. + let occupied = cleanup( + "occupied", + |q, _| fs::write(q.join("entry"), b"x").unwrap(), + |q| Some(identity(q)), + ); + // No identity was taken: nothing is removed, however plausible the name looks. + let unknown = cleanup("unknown", |_, _| {}, |_| None); + assert_eq!( + (same, replaced, occupied, unknown), + ( + (true, false, false), + (false, true, true), + (false, true, false), + (false, true, false) + ) + ); +} diff --git a/crates/worldscript-secure-storage/src/pinned/tests/remove_unix_sync.rs b/crates/worldscript-secure-storage/src/pinned/tests/remove_unix_sync.rs new file mode 100644 index 000000000..23d0d64e5 --- /dev/null +++ b/crates/worldscript-secure-storage/src/pinned/tests/remove_unix_sync.rs @@ -0,0 +1,265 @@ +//! The quarantine directory is synced whenever an entry moved through it, and a sync that fails is +//! reported where the outcome promises something and nowhere else. An `fsync` error on a directory +//! cannot be provoked portably, so the cases with a failing sync fail it through the seam of the removal +//! ([`sync_fault`]): that models the failure. The cases without a failure run the real sync, and the +//! seam counts how often it was asked for. + +use std::cell::Cell; +use std::fs; +use std::io::ErrorKind; + +use super::remove::holds; +use super::remove_unix::{ + at, denied, fixture, held_in_quarantine, quarantines, swap_in_a_live_file, At, Fixture, +}; +use crate::durable::DirectoryDurability::{Confirmed, NotConfirmed}; +use crate::pinned::unix::removal::sync_fault; +use crate::pinned::{PinnedError, RemovalOutcome, RemovalStage}; + +/// What the actor and the observer do during one removal of `a` ("alpha"). +#[derive(Clone, Copy, Debug, PartialEq)] +enum Scene { + /// Nothing: the file is authenticated and removed. + Removal, + /// A live file takes the name after the check: it is moved aside, found to be foreign and put back. + PutBack, + /// As `PutBack`, and the name is taken again while the live file is aside: it stays aside. + KeptAside, + /// As `KeptAside`, and the quarantine directory is renamed away: the entry is not where the outcome + /// would say. + Tampered, + /// A directory (not the kind of entry that was authenticated) takes the name after the check and + /// the name is taken again while it is aside, so it stays aside, untouched: the control for the next. + ForeignKept, + /// As `ForeignKept`, and an actor replaces the entry after it was inspected and before it is looked + /// at again: the entry is not where the outcome would say. + ForeignReplaced, + /// The observer fails once the entry is identified: it is put back and the error returned. + RolledBack, + /// The observer fails while the entry is aside and the name is taken again: it cannot go back. + Stranded, + /// The name is gone after the check: nothing is moved. + Vanished, + /// An actor removes the entry once it is aside, before it is looked at. + GoneBeforeInspection, + /// An actor removes the entry once it is identified, before it is unlinked. + GoneBeforeUnlink, + /// The observer fails at the check, before anything is moved. + FailedEarly, +} + +fn sync_failure() -> PinnedError { + PinnedError::Io(ErrorKind::Other) +} + +/// The doing of `scene` at one stage of the removal. +fn act( + scene: Scene, + f: &Fixture, + parent_synced: &Cell, + stage: RemovalStage<'_>, +) -> Result<(), PinnedError> { + let (now, inside) = at(stage); + let name = f.root.join("a"); + match (scene, now) { + (_, At::BeforeSync) => parent_synced.set(true), + (Scene::PutBack | Scene::KeptAside | Scene::Tampered, At::Checked) => { + swap_in_a_live_file(&f.root, &f.aside); + } + (Scene::Vanished, At::Checked) => fs::remove_file(name).unwrap(), + (Scene::GoneBeforeInspection, At::MovedAside) + | (Scene::GoneBeforeUnlink, At::Identified) => { + fs::remove_file(inside.unwrap()).unwrap(); + } + (Scene::FailedEarly, At::Checked) | (Scene::RolledBack, At::Identified) => { + return Err(denied()); + } + (Scene::ForeignKept | Scene::ForeignReplaced, At::Checked) => { + fs::rename(&name, f.aside.join("original")).unwrap(); + fs::create_dir(&name).unwrap(); + } + (Scene::ForeignKept, At::MovedAside) => fs::write(&name, b"retaken").unwrap(), + (Scene::ForeignReplaced, At::MovedAside) => { + fs::write(&name, b"retaken").unwrap(); + sync_fault::replace_at_next_sync(inside.unwrap().to_path_buf()); + } + (Scene::KeptAside, At::MovedAside) => fs::write(name, b"retaken").unwrap(), + (Scene::Tampered, At::MovedAside) => { + fs::rename(inside.unwrap().parent().unwrap(), f.aside.join("held")).unwrap(); + fs::write(name, b"retaken").unwrap(); + } + (Scene::Stranded, At::MovedAside) => { + fs::write(name, b"taken").unwrap(); + return Err(denied()); + } + _ => {} + } + Ok(()) +} + +/// What one removal did and left. +#[derive(Clone, Debug, PartialEq)] +struct Report { + outcome: Result, + /// How often the quarantine directory was synced. + syncs: usize, + /// Whether the removal got as far as syncing the parent. + parent_synced: bool, + /// What the name holds afterwards. + name: Option>, + /// The entry kept in the quarantine directory that is left, if there is one. + kept: Option>, + /// How many quarantine directories are left. + left: usize, +} + +fn run(scene: Scene, failure: Option) -> Report { + let f = fixture("sync"); + let parent_synced = Cell::new(false); + let (outcome, syncs) = sync_fault::during(failure, || { + f.pinned + .remove_authenticated_file_observed("a", holds(b"alpha"), &mut |stage| { + act(scene, &f, &parent_synced, stage) + }) + }); + Report { + outcome, + syncs, + parent_synced: parent_synced.get(), + name: fs::read(f.root.join("a")).ok(), + kept: held_in_quarantine(&f.root), + left: quarantines(&f.root).len(), + } +} + +type Expected = (Scene, Result, usize, bool); + +#[test] +fn every_move_through_the_quarantine_directory_ends_with_one_sync_of_it() { + // A removal, a put-back, a kept entry, a rollback, a stranded entry and an entry an actor removed + // again each changed the quarantine directory, and the sync of the parent does not cover that. + // Nothing moved, nothing to sync there. + let expected: [Expected; 12] = [ + ( + Scene::Removal, + Ok(RemovalOutcome::Removed(Confirmed)), + 1, + true, + ), + ( + Scene::PutBack, + Ok(RemovalOutcome::Swapped { restored: true }), + 1, + true, + ), + ( + Scene::KeptAside, + Ok(RemovalOutcome::Swapped { restored: false }), + 1, + true, + ), + (Scene::Tampered, Ok(RemovalOutcome::Disturbed), 1, true), + ( + Scene::ForeignKept, + Ok(RemovalOutcome::Swapped { restored: false }), + 1, + true, + ), + ( + Scene::ForeignReplaced, + Ok(RemovalOutcome::Disturbed), + 1, + true, + ), + (Scene::RolledBack, Err(denied()), 1, true), + ( + Scene::Stranded, + Ok(RemovalOutcome::Stranded { cause: denied() }), + 1, + true, + ), + (Scene::Vanished, Ok(RemovalOutcome::Vanished), 0, true), + // The entry was moved here, so this directory changed, although an actor removed the entry again. + ( + Scene::GoneBeforeInspection, + Ok(RemovalOutcome::Vanished), + 1, + true, + ), + ( + Scene::GoneBeforeUnlink, + Ok(RemovalOutcome::Vanished), + 1, + true, + ), + (Scene::FailedEarly, Err(denied()), 0, false), + ]; + let actual = expected.map(|(scene, ..)| { + let report = run(scene, None); + (scene, report.outcome, report.syncs, report.parent_synced) + }); + assert_eq!(actual, expected); +} + +#[test] +fn a_failed_sync_of_the_quarantine_directory_is_an_error_after_a_put_back_a_kept_entry_and_a_vanished_one( +) { + // The rename decided where the entry is, whatever the sync did: it is back, aside or gone, and the + // parent was synced. Only the result differs, and it is the sync error, not `Disturbed` or `Stranded`. + for scene in [ + Scene::PutBack, + Scene::KeptAside, + Scene::GoneBeforeInspection, + Scene::GoneBeforeUnlink, + ] { + let clean = run(scene, None); + let failed = run(scene, Some(sync_failure())); + let (name, kept, left) = match scene { + Scene::PutBack => (Some(b"live".to_vec()), None, 0), + Scene::KeptAside => (Some(b"retaken".to_vec()), Some(b"live".to_vec()), 1), + _ => (None, None, 0), + }; + assert_eq!( + (failed.name.clone(), failed.kept.clone(), failed.left), + (name, kept, left), + "{scene:?}" + ); + assert_eq!( + failed, + Report { + outcome: Err(sync_failure()), + ..clean + }, + "{scene:?}" + ); + } +} + +#[test] +fn a_failed_sync_changes_no_outcome_that_reports_a_failure_or_tampering() { + // `RolledBack` and `Stranded` report a failure by its cause (the sync is best effort and does not + // replace it); `Disturbed` is tampering and wins; and where nothing moved there is nothing to sync. + for scene in [ + Scene::RolledBack, + Scene::Stranded, + Scene::Tampered, + Scene::ForeignReplaced, + Scene::Vanished, + Scene::FailedEarly, + ] { + assert_eq!( + run(scene, Some(sync_failure())), + run(scene, None), + "{scene:?}" + ); + } +} + +#[test] +fn a_removal_whose_quarantine_directory_could_not_be_synced_is_not_confirmed() { + let failed = run(Scene::Removal, Some(sync_failure())); + assert_eq!( + (failed.outcome, failed.syncs, failed.name, failed.left), + (Ok(RemovalOutcome::Removed(NotConfirmed)), 1, None, 0) + ); +} diff --git a/crates/worldscript-secure-storage/src/pinned/unix/removal.rs b/crates/worldscript-secure-storage/src/pinned/unix/removal.rs index 88333ca43..fda0cdbd1 100644 --- a/crates/worldscript-secure-storage/src/pinned/unix/removal.rs +++ b/crates/worldscript-secure-storage/src/pinned/unix/removal.rs @@ -14,7 +14,9 @@ use rustix::fs::{ use rustix::io::Errno; use rustix::process::geteuid; -use super::{dir_flags, file_flags, io_error, kind_of, stat_child, widen, DirHandle}; +use super::{ + dir_flags, file_flags, identity_from, io_error, kind_of, stat_child, widen, DirHandle, +}; use crate::durable::{DirectoryDurability, EntryKind}; use crate::pinned::{ ObjectIdentity, Observer, PinnedDir, PinnedError, PinnedFile, RemovalOutcome, RemovalStage, @@ -30,27 +32,38 @@ pub(in crate::pinned) const INSIDE: &str = "x"; struct Quarantine { fd: File, name: OsString, + /// The directory that was created, to tell it from whatever the name leads to later. + identity: ObjectIdentity, } /// What is bound to the quarantine name right now. enum Inside { Absent, /// Not the kind that was asked for (a link, a directory instead of a file, a FIFO), or not openable - /// by this process although the authenticated object was. - Foreign, + /// by this process although the authenticated object was. It carries what the name led to when this + /// was seen (a look that does not follow a link), so that the entry can be told from a replacement + /// later, as for an opened object. + Foreign(ObjectIdentity), /// Opened, with the identity of the open object. Object(File, ObjectIdentity), } /// How a removal ended before the parent directory was synced. enum Step { - /// Nothing in the directory changed. - Unchanged(RemovalOutcome), - /// The directory changed (an entry was moved and put back, or a quarantine directory came and - /// went); the outcome is final. + /// The directory changed (a quarantine directory came and went, the name led nowhere, or tampering + /// was found); the outcome is final. Changed(RemovalOutcome), - /// The object lost its name. - Unlinked, + /// The entry was moved aside and then put back, kept aside or found gone, so the quarantine directory + /// changed as well as the parent; the outcome is final, and `held` is how syncing the quarantine + /// directory went. A sync that failed does not change what the outcome says about where the entry is, + /// and is reported after the parent was synced. + Moved(RemovalOutcome, Result<(), PinnedError>), + /// The object lost its name; whether the quarantine directory it was unlinked from was synced. + Unlinked(DirectoryDurability), + /// The entry was moved aside and put back; this is why the removal stopped. + RolledBack(PinnedError), + /// The entry could not be put back and stays in the quarantine directory. + Stranded(PinnedError), } impl Quarantine { @@ -62,15 +75,27 @@ impl Quarantine { let suffix: String = bits.iter().map(|byte| format!("{byte:02x}")).collect(); let name = OsString::from(format!("{QUARANTINE_PREFIX}{suffix}")); mkdirat(parent, &name, Mode::RWXU).map_err(io_error)?; - match Self::made_usable(parent, &name) { - Ok(fd) => Ok(Quarantine { fd, name }), + // `mkdirat` returns no descriptor, so what the name leads to right now stands for the directory + // that was made; a setup that fails removes the name again only if it still leads to that. + let made = identity_at(parent, &name); + match Self::made_usable(parent, &name).and_then(|fd| Self::held(fd, name.clone())) { + Ok(held) => Ok(held), Err(error) => { - let _ = unlinkat(parent, &name, AtFlags::REMOVEDIR); + if remove_if_named(parent, &name, made) { + // The directory came and went: sync the parent best effort, so the error does not + // leave an unsynced quarantine directory behind. + let _ = parent.sync_all(); + } Err(error) } } } + fn held(fd: File, name: OsString) -> Result { + let identity = identity_from(&fstat(&fd).map_err(io_error)?)?; + Ok(Quarantine { fd, name, identity }) + } + /// The directory just created, with the owner's access set explicitly (a restrictive umask would /// otherwise leave a directory that nothing can be moved into) and checked. fn made_usable(parent: &DirHandle, name: &OsStr) -> Result { @@ -118,7 +143,7 @@ impl Quarantine { let inside = OsStr::new(INSIDE); match stat_child(&self.fd, inside)? { None => return Ok(Inside::Absent), - Some(stat) if kind_of(&stat) != kind => return Ok(Inside::Foreign), + Some(stat) if kind_of(&stat) != kind => return self.foreign(), Some(_) => {} } let flags = if kind == EntryKind::Directory { @@ -132,7 +157,7 @@ impl Quarantine { // Swapped between the look and the open, or not openable: not the object that was opened // before, which this process could open. Err(Errno::LOOP | Errno::NOTDIR | Errno::ISDIR | Errno::NXIO | Errno::ACCESS) => { - return Ok(Inside::Foreign) + return self.foreign() } Err(errno) => return Err(io_error(errno)), }; @@ -143,26 +168,93 @@ impl Quarantine { metadata.is_file() }; if !right_kind { - return Ok(Inside::Foreign); + return self.foreign(); } let identity = ObjectIdentity::new(metadata.dev(), u128::from(metadata.ino())); Ok(Inside::Object(object, identity)) } + /// An entry that is not the kind asked for, with the identity the name leads to now (a look that does + /// not follow a link). An entry whose identity cannot be taken could not be told from a replacement + /// later and would count as retained whatever it is, so a look that fails is an error like any other + /// failed inspection (the entry is put back or stays aside as a failure), and a name that leads + /// nowhere by now is `Absent`. + fn foreign(&self) -> Result { + Ok(match stat_child(&self.fd, OsStr::new(INSIDE))? { + None => Inside::Absent, + Some(stat) => Inside::Foreign(identity_from(&stat)?), + }) + } + /// Moves the entry back under `name`; an error if that was not possible (the name is taken, or - /// anything else prevents it), in which case the entry stays here. + /// anything else prevents it), in which case the entry stays here. Only the rename: the sync of this + /// directory is [`sync`](Self::sync), kept apart so that a sync that fails cannot be taken for a + /// put-back that did not happen. fn restore(&self, parent: &DirHandle, name: &OsStr) -> Result<(), PinnedError> { renameat_with(&self.fd, INSIDE, parent, name, RenameFlags::NOREPLACE).map_err(io_error) } + /// Syncs this directory. Every move into or out of it changes it, and the sync of the parent does + /// not cover that: without it a crash can undo the removal of `x` after the parent shows the name + /// again (or leave `x` missing after the parent shows the name gone). + fn sync(&self) -> Result<(), PinnedError> { + if let Some(error) = forced_sync_failure() { + return Err(error); + } + self.fd.sync_all().map_err(PinnedError::from) + } + fn path_inside(&self, parent_path: &Path) -> PathBuf { parent_path.join(&self.name).join(INSIDE) } - /// Removes this directory if it is empty. A directory that still holds an entry is left. + /// Removes this directory if it is empty. A directory that still holds an entry is left, and so is + /// whatever the name leads to if that is no longer the directory that was created (see + /// [`remove_if_named`]). fn discard(&self, parent: &DirHandle) { - let _ = unlinkat(parent, &self.name, AtFlags::REMOVEDIR); + remove_if_named(parent, &self.name, Some(self.identity)); } + + /// Whether the entry may still be inside: `false` only if it is known to be gone, or to be another + /// object than `expected` (an actor removed or replaced it); an error while looking (a permission + /// lost) proves neither. Without an expectation any object counts. + fn holds(&self, expected: Option) -> bool { + match stat_child(&self.fd, OsStr::new(INSIDE)) { + Ok(None) => false, + Ok(Some(stat)) => match expected { + None => true, + Some(wanted) => identity_from(&stat).is_ok_and(|found| found == wanted), + }, + Err(_) => true, + } + } + + /// Whether the name still leads to the directory that was created. + fn is_named(&self, parent: &DirHandle) -> bool { + identity_at(parent, &self.name) == Some(self.identity) + } +} + +/// The identity of what `name` of `parent` leads to right now, without following a link; `None` if it +/// leads nowhere or cannot be looked at. +fn identity_at(parent: &DirHandle, name: &OsStr) -> Option { + stat_child(parent, name) + .ok() + .flatten() + .and_then(|stat| identity_from(&stat).ok()) +} + +/// Removes the directory `name` of `parent` if it is empty and still leads to `expected`; whether it was +/// removed. Without an identity nothing is removed. The check narrows the race and does not close it +/// (`rmdir` takes a name, not a descriptor): what a lost race can remove is at most an empty directory. +pub(in crate::pinned) fn remove_if_named( + parent: &DirHandle, + name: &OsStr, + expected: Option, +) -> bool { + expected.is_some() + && identity_at(parent, name) == expected + && unlinkat(parent, name, AtFlags::REMOVEDIR).is_ok() } /// Makes the quarantine directory `name` of `parent` usable the way [`Quarantine::create`] does: the @@ -172,6 +264,18 @@ pub(in crate::pinned) fn make_usable(parent: &DirHandle, name: &OsStr) -> Result Quarantine::made_usable(parent, name).map(|_| ()) } +/// The durability of a removal that touched two directories: confirmed only if both were synced. +pub(in crate::pinned) fn both_confirmed( + first: DirectoryDurability, + second: DirectoryDurability, +) -> DirectoryDurability { + if first == DirectoryDurability::Confirmed && second == DirectoryDurability::Confirmed { + DirectoryDurability::Confirmed + } else { + DirectoryDurability::NotConfirmed + } +} + /// Unlinks the entry that was identified in the quarantine directory and asks the open object whether /// it was the one that lost a name: its link count must have dropped. That is a check after the fact /// and a heuristic, not a proof of identity (see the residual in the parent module): an actor that also @@ -260,14 +364,25 @@ impl Removal<'_> { /// the move puts the entry back first; if that is impossible the removal says so /// ([`RemovalOutcome::Stranded`]) instead of reporting an error that hides where the entry is. fn detach(&self, observer: Observer<'_>) -> Result { - if !self.quarantine.adopt(&self.dir.handle, self.target.name)? { - return Ok(Step::Unchanged(RemovalOutcome::Vanished)); + match self.quarantine.adopt(&self.dir.handle, self.target.name) { + // Nothing was moved, but the quarantine directory came and went: the parent changed. + Err(cause) => return Ok(Step::RolledBack(cause)), + Ok(false) => return Ok(Step::Changed(RemovalOutcome::Vanished)), + Ok(true) => {} } match self.judge(observer) { - Err(cause) => match self.restore() { - Ok(()) => Err(cause), - Err(_) => Ok(Step::Unchanged(RemovalOutcome::Stranded { cause })), - }, + Err(cause) => { + let put_back = self.restore(); + // The failure that is reported is `cause`, and a sync that fails does not replace it. + let _ = self.quarantine.sync(); + match put_back { + Ok(()) => Ok(Step::RolledBack(cause)), + Err(_) if self.retained(Some(self.target.expected)) => { + Ok(Step::Stranded(cause)) + } + Err(_) => Ok(Step::Changed(RemovalOutcome::Disturbed)), + } + } step => step, } } @@ -278,17 +393,34 @@ impl Removal<'_> { quarantined: &inside, })?; match self.quarantine.inspect(self.target.kind)? { - Inside::Absent => Ok(Step::Changed(RemovalOutcome::Vanished)), + // An actor removed the entry after it was moved here: that moved it into this directory, and + // out again, without the sync of the parent covering either. + Inside::Absent => Ok(Step::Moved( + RemovalOutcome::Vanished, + self.quarantine.sync(), + )), Inside::Object(object, found) if found == self.target.expected => { self.conclude(&object, observer) } - Inside::Object(..) | Inside::Foreign => { - let restored = self.restore().is_ok(); - Ok(Step::Changed(RemovalOutcome::Swapped { restored })) - } + Inside::Object(_, found) => self.swapped(Some(found)), + Inside::Foreign(seen) => self.swapped(Some(seen)), } } + /// What arrived is not the authenticated object: it goes back under its name, or stays aside if + /// that is impossible (and is then still `arrived`, the identity it was seen with if it was opened). + /// Either way the quarantine directory changed (the entry left it, or arrived in it), and it is synced + /// after the rename was tried: the rename alone decides `restored`, so a sync that fails is neither a + /// put-back that did not happen nor a reason to call the entry tampered with. + fn swapped(&self, arrived: Option) -> Result { + let restored = self.restore().is_ok(); + let held = self.quarantine.sync(); + if !restored && !self.retained(arrived) { + return Ok(Step::Changed(RemovalOutcome::Disturbed)); + } + Ok(Step::Moved(RemovalOutcome::Swapped { restored }, held)) + } + /// The last window: the entry in the quarantine directory is the authenticated object. fn conclude(&self, object: &File, observer: Observer<'_>) -> Result { let inside = self.quarantine.path_inside(&self.dir.path); @@ -299,42 +431,81 @@ impl Removal<'_> { // A directory that filled after the check goes back where it was. RemovalOutcome::NotEmpty => { self.restore()?; - Ok(Step::Changed(RemovalOutcome::NotEmpty)) + Ok(Step::Moved( + RemovalOutcome::NotEmpty, + self.quarantine.sync(), + )) + } + outcome => { + // The unlink changed the directory that is held, and that directory can outlive the + // removal (an actor renamed it away, or put an entry into it so that it cannot be + // removed). The parent sync does not cover it, so it is synced here, and a sync that + // fails leaves the removal unconfirmed. + Ok(unlinked(outcome, self.quarantine.sync())) } - outcome => Ok(unlinked(outcome)), } } fn restore(&self) -> Result<(), PinnedError> { self.quarantine.restore(&self.dir.handle, self.target.name) } + + /// The entry stays in the quarantine directory: the outcome may only say where the entry is if the + /// recorded name still leads to the directory that holds it and the entry in it is still `expected` + /// (or cannot be looked at). If an actor renamed that directory away, or removed or replaced the + /// entry, the entry cannot be found where the outcome would say, and that is reported as tampering. + /// Syncing that directory, which changed as well as the parent, is up to the caller. + fn retained(&self, expected: Option) -> bool { + self.quarantine.is_named(&self.dir.handle) && self.quarantine.holds(expected) + } } -fn unlinked(outcome: RemovalOutcome) -> Step { +/// What the unlink came to, with how syncing the quarantine directory it was made in went: a removal is +/// confirmed only if that sync worked, and an entry that was gone by the time of the unlink is reported +/// like one that was gone before it. Tampering (`Disturbed`) outranks a failed sync. +fn unlinked(outcome: RemovalOutcome, held: Result<(), PinnedError>) -> Step { match outcome { - RemovalOutcome::Removed(_) => Step::Unlinked, + RemovalOutcome::Removed(_) => Step::Unlinked(match held { + Ok(()) => DirectoryDurability::Confirmed, + Err(_) => DirectoryDurability::NotConfirmed, + }), + RemovalOutcome::Vanished => Step::Moved(outcome, held), other => Step::Changed(other), } } /// Syncs the parent directory if anything in it changed. A sync that fails is an error although the /// entry is gone or back: a crash may undo that, and the caller repeats [`PinnedDir::sync`] (a rerun of -/// the removal finds nothing to do and would not sync). +/// the removal finds nothing to do and would not sync). The same holds for the quarantine directory of an +/// entry that was put back or kept aside; it is reported after the parent was synced, whether or not the +/// directory is removed afterwards, since a sync that failed proves nothing about what reached the disk. fn settle( dir: &PinnedDir, step: Step, observer: Observer<'_>, ) -> Result { match step { - Step::Unchanged(outcome) => Ok(outcome), + Step::RolledBack(cause) => { + sync_after_failure(dir, observer); + Err(cause) + } + Step::Stranded(cause) => { + sync_after_failure(dir, observer); + Ok(RemovalOutcome::Stranded { cause }) + } Step::Changed(outcome) => { observer(RemovalStage::BeforeSync)?; dir.sync()?; Ok(outcome) } - Step::Unlinked => { + Step::Moved(outcome, held) => { + observer(RemovalStage::BeforeSync)?; + dir.sync()?; + held.map(|()| outcome) + } + Step::Unlinked(held) => { observer(RemovalStage::BeforeSync)?; - Ok(RemovalOutcome::Removed(dir.sync()?)) + Ok(RemovalOutcome::Removed(both_confirmed(dir.sync()?, held))) } } } @@ -357,3 +528,78 @@ pub(in crate::pinned) fn remove_leftover( ) -> Result { Err(PinnedError::Unsupported) } + +/// After a failure that moved an entry aside and back (or left it aside) the directory has changed too: +/// the sync is attempted, and its own failure does not replace the failure that is being reported. +fn sync_after_failure(dir: &PinnedDir, observer: Observer<'_>) { + let _ = observer(RemovalStage::BeforeSync); + let _ = dir.sync(); +} + +/// The failure a test forced on the sync of a quarantine directory, if any. An `fsync` error on a +/// directory cannot be provoked portably, so the sync asks here first; a build without tests never does. +#[cfg(test)] +fn forced_sync_failure() -> Option { + sync_fault::asked() +} + +#[cfg(not(test))] +fn forced_sync_failure() -> Option { + None +} + +/// The seam behind [`forced_sync_failure`]: a test runs a removal with the sync of the quarantine +/// directory failing (or not), and learns how often that sync was asked for. +#[cfg(test)] +pub(in crate::pinned) mod sync_fault { + use std::cell::Cell; + use std::fs; + use std::path::PathBuf; + + use crate::pinned::PinnedError; + + thread_local! { + static FAILURE: Cell> = const { Cell::new(None) }; + static ASKED: Cell = const { Cell::new(0) }; + static REPLACE: Cell> = const { Cell::new(None) }; + } + + /// Disarms the seam when the removal under test ends, also by a panic. + struct Disarm; + + impl Drop for Disarm { + fn drop(&mut self) { + FAILURE.set(None); + REPLACE.set(None); + } + } + + /// Makes the next sync of a quarantine directory replace the entry at `entry` by another object + /// first, the way an actor could after the entry was inspected and before it is looked at again: + /// no stage of the observer sits between the two. + pub(in crate::pinned) fn replace_at_next_sync(entry: PathBuf) { + REPLACE.set(Some(entry)); + } + + /// Runs `body` with every sync of a quarantine directory failing with `failure` (none: succeeding), + /// and returns what it returned with the number of syncs that were asked for. + pub(in crate::pinned) fn during( + failure: Option, + body: impl FnOnce() -> T, + ) -> (T, usize) { + FAILURE.set(failure); + ASKED.set(0); + let _disarm = Disarm; + let result = body(); + (result, ASKED.get()) + } + + pub(super) fn asked() -> Option { + ASKED.set(ASKED.get() + 1); + if let Some(entry) = REPLACE.take() { + fs::rename(&entry, entry.with_extension("moved")).unwrap(); + fs::write(&entry, b"impostor").unwrap(); + } + FAILURE.get() + } +} diff --git a/docs/native/r15/GATE4D-JOURNAL-DURABLE-EVIDENCE.md b/docs/native/r15/GATE4D-JOURNAL-DURABLE-EVIDENCE.md index c20255dc8..2a1064897 100644 --- a/docs/native/r15/GATE4D-JOURNAL-DURABLE-EVIDENCE.md +++ b/docs/native/r15/GATE4D-JOURNAL-DURABLE-EVIDENCE.md @@ -1751,9 +1751,39 @@ quarantine directory; anything else (a live file, a link, a directory, a file th never unlinked. A file system without `RENAME_NOREPLACE` answers `Unsupported` before anything is touched (not exercised on the hosted runners). **Every failure after the move is handled at one point:** the entry is put back and the error is returned, or, if it cannot be put back, the outcome is `Stranded { cause }` (the entry is in the quarantine directory as `x`, -nothing was removed) and not an error that hides where the entry is. The parent is synced whenever the directory changed; a -sync that fails is an error although the entry is gone or back, and the caller repeats `PinnedDir::sync` (a rerun of the -removal finds nothing to do and would not sync). +nothing was removed) and not an error that hides where the entry is. The parent is synced whenever the directory changed: after +a removal and after a put-back as a step of the removal (a sync that fails is an error although the entry is gone or back, and the +caller repeats `PinnedDir::sync`; a rerun of the removal finds nothing to do and would not sync), and after a rolled-back or +stranded failure best-effort (the `BeforeSync` stage is reached; a failing sync does not replace the failure that is reported). **The +quarantine directory changes with every move, and the sync of the parent does not cover it:** the rename that moves an entry in +and the rename that puts it back each change it as well as the parent, so it is synced after the rename was tried. For a put-back +and a kept entry (`Swapped`) a sync that fails is an error reported after the parent was synced, like a failing parent sync, and +not another outcome: the rename alone decides `restored`, so the entry is back or aside whatever the sync did, and the failure is +neither a put-back that did not happen (a `sync_all()?` inside the rename step would make it one, and report `Disturbed`) nor +`Stranded`. It is reported whether or not the directory is removed afterwards, since a failed sync proves nothing about what +reached the disk. For a rollback and a stranded entry the sync is best effort and does not replace the failure that is reported, +and tampering (`Disturbed`) outranks a failed sync. An entry that an actor removed again after it was moved aside (found gone at the +inspection, or at the unlink) is reported as `Vanished` and treated like a put-back: the move changed the directory, so it is +synced, and a sync that fails is the error. Only where the name led nowhere, so that nothing moved, nothing is synced in it. The +quarantine directory is removed by its name only if the name still leads to the directory that was created, so a directory an +actor put at that name is left alone (what could be removed is at most an empty directory); this holds for the cleanup of a setup +that fails as well, where the identity is taken right after `mkdirat` and nothing is removed without one. That the directory behind +the name is the one `mkdirat` made cannot be proved (`mkdirat` returns no +descriptor); it is opened by name and its owner and mode are checked, and since the quarantine directory is not exclusive anyway, an +actor that wins that earlier race gains nothing the window below does not already give it. An entry that stays in the quarantine +directory (`restored: false`, `Stranded`) is only reported as being there if the recorded name still leads to that directory and +the entry in it is still the object the +outcome names (the authenticated object, or for a swapped-in object the identity it was seen with, also one that is not the kind +asked for, by a look that does not follow a link, where a look that fails is a failed inspection and not a retained entry; an +entry that cannot be looked at when it is checked again counts as there, since an error proves neither); if an actor renamed the directory away, removed the entry or replaced it with +another object the outcome is `Disturbed` (tampered, the entry cannot be found where the outcome would say). The check is itself +a look at a name and does not close the same-user race. The quarantine directory the unlink happened in is synced as well as the +parent before `Removed` is reported, whether it is removed afterwards, survives because an actor put an entry into it, or was +renamed away; if that sync fails the removal is `Removed(NotConfirmed)`. The identity check before the +name-based `rmdir` of the quarantine directory narrows the race and does not close it: a lost race removes at most an empty +directory. A failure to move the entry at all (`Unsupported`, an error from +`renameat2`) also syncs the parent, since the quarantine directory came and went. A umask that takes the owner's read access +away leaves a quarantine directory that cannot be opened: the removal is refused (`PermissionDenied`), fail closed. **The window, stated.** The quarantine directory is not exclusive: another process of the same user can list the parent, find it and enter it. The design holds when it is manipulated at every point before the last identification (tests below). After the @@ -1772,10 +1802,26 @@ directory before the identification; the last window forced with a foreign file directory (goes back) and a hard link (object alive under the other name); a name taken after the move, a vanished name, renaming the root during the removal; the quarantine directory is mode 0700 and gone afterwards, and a directory made with mode 0500 is made usable; an injected failure at each stage leaves the directory as it was found; a failed sync is reported -for a removal and for a put-back and repeated by `sync`; a quarantine directory that loses its search permission after the -move gives `Stranded` with the entry still the authenticated inode. Mutation-checked: skipping the identity comparison fails +for a removal and for a put-back and repeated by `sync`; the quarantine directory is synced once after a removal, a put-back, a +kept entry, a rollback, a stranded entry and an entry an actor removed again, and not when nothing moved, and a sync of it that +fails is an error for the put-back, the kept entry and the entry that was gone (each where the move left it), leaves a rollback, a +stranded entry, tampering and the outcomes before the move +as they are, and makes a removal `NotConfirmed` (an `fsync` error on a directory cannot be provoked portably, so these cases fail +the sync through a test-only seam, which models the failure; the cases without a failure run the real sync); a foreign entry (a +directory) that an actor replaces after the inspection, while the put-back is blocked, gives `Disturbed`, and the same entry left +untouched is kept (`Swapped { restored: false }`; the replacement runs at the sync of the quarantine directory, the one point +between the inspection and the check that a test can reach); a quarantine +directory that loses its search permission after the +move gives `Stranded` with the entry still the authenticated inode, and the same case without permissions (an observer that +fails while the name is taken again, so it runs the same for root); a quarantine directory renamed away and replaced by an empty +directory of the actor is not removed, and the removal of a directory by its name is exercised on its own (the directory that was +made goes, one renamed away and replaced, a non-empty one and a name without an identity stay; the setup failure that reaches +it in `create` cannot be provoked without changing the process umask and is not forced). Mutation-checked: skipping the identity comparison fails five of these cases, skipping the link-count verification fails the forced last window, and removing the rollback of a -failed inspection fails the `Stranded` case. +failed inspection fails the `Stranded` case; leaving out the sync of the quarantine directory after a put-back or after a rollback, +dropping a failed sync of it, a `sync_all()?` inside the rename step, confirming a removal despite a failed sync, skipping the +sync for an entry found gone at the inspection, dropping it for one gone at the unlink and passing no identity for a foreign +entry each fail at least one of the sync cases. **Not claimed.** Directories and the leftover recovery are part 3 (they answer `Unsupported` here); Windows is part 4; the Disturbed window is bounded and detected, not closed; a device node swapped in is refused before it is opened and cannot be From 9933665970c6bb9931d5319a996f53e62446f502 Mon Sep 17 00:00:00 2001 From: qnbs <155236708+qnbs@users.noreply.github.com> Date: Thu, 15 Oct 2026 23:53:52 +0200 Subject: [PATCH 2/3] docs(changelog): set the PR number of the Unix quarantine hardening entry (#445) --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b79e049ff..4bb7a19e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -- **R-15 Gate 4:** hardening of the Unix file removal against a same-user actor in the quarantine directory, `test-support` only (slice H1a-2, part 2b of 5). The parent is synced after a rollback, a stranded entry, a put-back and a vanished name, and the quarantine directory an entry moved through is synced too (a sync of it that fails is an error after a put-back, a kept entry or an entry an actor removed again, and makes a removal `NotConfirmed`, never another outcome); the quarantine directory, also when its setup fails, is removed by its name only after a check that the name still leads to the directory that was created (the check narrows the race and does not close it, since `rmdir` takes a name: a lost race removes at most an empty directory); an entry, a foreign one included, is reported as retained only if the recorded name still leads to its directory and the entry in it is the one that was seen, otherwise `Disturbed` (also a look at a name, so it detects after the fact and does not prevent); a quarantine directory that cannot be opened is refused. Tests force each case, including the quarantine renamed away and the put-back blocked. No new `unsafe`, no new package. Issue #359. PR #1072. +- **R-15 Gate 4:** hardening of the Unix file removal against a same-user actor in the quarantine directory, `test-support` only (slice H1a-2, part 2b of 5). The parent is synced after a rollback, a stranded entry, a put-back and a vanished name, and the quarantine directory an entry moved through is synced too (a sync of it that fails is an error after a put-back, a kept entry or an entry an actor removed again, and makes a removal `NotConfirmed`, never another outcome); the quarantine directory, also when its setup fails, is removed by its name only after a check that the name still leads to the directory that was created (the check narrows the race and does not close it, since `rmdir` takes a name: a lost race removes at most an empty directory); an entry, a foreign one included, is reported as retained only if the recorded name still leads to its directory and the entry in it is the one that was seen, otherwise `Disturbed` (also a look at a name, so it detects after the fact and does not prevent); a quarantine directory that cannot be opened is refused. Tests force each case, including the quarantine renamed away and the put-back blocked. No new `unsafe`, no new package. Issue #359. PR #1073. - **R-15 Gate 4:** removal of a file on Unix, `test-support` only (slice H1a-2, part 2 of 4). `PinnedDir::remove_authenticated_file` moves the entry into a quarantine directory with `RENAME_NOREPLACE`, identifies the open object there and only then unlinks it; anything else is put back, a failure after the move rolls back or reports `Stranded`, and the parent is synced. The one window no Unix primitive closes (a same-user actor in the quarantine directory after the last identification) is forced in a test and reported as `Disturbed`, detected after the fact and not prevented. The rustix `process` feature is added for `geteuid`; no new package, no new `unsafe`. Directories, leftovers and Windows follow. Issue #359. PR #1069. From 1602d79e1307a7d7d5a34797a24b6819ac372865 Mon Sep 17 00:00:00 2001 From: qnbs <155236708+qnbs@users.noreply.github.com> Date: Fri, 16 Oct 2026 00:26:47 +0200 Subject: [PATCH 3/3] fix(core): treat a failed look at a retained entry as no proof, state the limit of a foreign identity (#445) Two findings on 8f8c9883, both valid. A look at the entry that failed (a search permission lost after the move) was counted as "the entry is there", so a removal whose put-back was blocked reported Stranded, and a kept foreign entry reported Swapped (restored false), without having seen the entry (Codex P2). An outcome that says where an entry is needs a proof, and a failed look proves nothing: it is now Disturbed. The identity that is compared is no longer optional (the "any object counts" path had no caller left), so there is no lenient path to fall into. The Stranded case that is proved, with the name taken again, is unchanged and runs for root as well; the permission case now expects Disturbed and still checks that the entry is in fact the authenticated inode, which is why this is conservative and not a claim that the directory was tampered with. The identity of a foreign entry is a stat identity without a held handle (Codex P2): a link, a FIFO or a device node is not opened, because opening has effects, and no handle can be taken for every kind without them. A file system may give the identity of a removed object to its replacement, which the check cannot tell apart. This is stated where the outcome is described (the variant, the Disturbed contract, the evidence document and the changelog) rather than covered by a mechanism that would hold for Linux only; it is part of the same-user residual that stays owner-gated before any production use. Mutation-checked: counting a failed look as present fails the permission case. test-support only, no caller, no new unsafe, no new package. Issue #359. --- CHANGELOG.md | 2 +- .../src/pinned/removal.rs | 8 ++- .../src/pinned/tests/remove_unix.rs | 19 +++---- .../src/pinned/unix/removal.rs | 55 +++++++++---------- .../r15/GATE4D-JOURNAL-DURABLE-EVIDENCE.md | 17 ++++-- 5 files changed, 52 insertions(+), 49 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4bb7a19e7..f6b928237 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -- **R-15 Gate 4:** hardening of the Unix file removal against a same-user actor in the quarantine directory, `test-support` only (slice H1a-2, part 2b of 5). The parent is synced after a rollback, a stranded entry, a put-back and a vanished name, and the quarantine directory an entry moved through is synced too (a sync of it that fails is an error after a put-back, a kept entry or an entry an actor removed again, and makes a removal `NotConfirmed`, never another outcome); the quarantine directory, also when its setup fails, is removed by its name only after a check that the name still leads to the directory that was created (the check narrows the race and does not close it, since `rmdir` takes a name: a lost race removes at most an empty directory); an entry, a foreign one included, is reported as retained only if the recorded name still leads to its directory and the entry in it is the one that was seen, otherwise `Disturbed` (also a look at a name, so it detects after the fact and does not prevent); a quarantine directory that cannot be opened is refused. Tests force each case, including the quarantine renamed away and the put-back blocked. No new `unsafe`, no new package. Issue #359. PR #1073. +- **R-15 Gate 4:** hardening of the Unix file removal against a same-user actor in the quarantine directory, `test-support` only (slice H1a-2, part 2b of 5). The parent is synced after a rollback, a stranded entry, a put-back and a vanished name, and the quarantine directory an entry moved through is synced too (a sync of it that fails is an error after a put-back, a kept entry or an entry an actor removed again, and makes a removal `NotConfirmed`, never another outcome); the quarantine directory, also when its setup fails, is removed by its name only after a check that the name still leads to the directory that was created (the check narrows the race and does not close it, since `rmdir` takes a name: a lost race removes at most an empty directory); an entry, a foreign one included, is reported as retained only if the recorded name still leads to its directory and the entry in it was looked at and is the one that was seen, otherwise `Disturbed` (a look at a name, so it detects after the fact and does not prevent; a foreign entry is not opened, so nothing holds its identity unique and a reused identity is not noticed); a quarantine directory that cannot be opened is refused. Tests force each case, including the quarantine renamed away and the put-back blocked. No new `unsafe`, no new package. Issue #359. PR #1073. - **R-15 Gate 4:** removal of a file on Unix, `test-support` only (slice H1a-2, part 2 of 4). `PinnedDir::remove_authenticated_file` moves the entry into a quarantine directory with `RENAME_NOREPLACE`, identifies the open object there and only then unlinks it; anything else is put back, a failure after the move rolls back or reports `Stranded`, and the parent is synced. The one window no Unix primitive closes (a same-user actor in the quarantine directory after the last identification) is forced in a test and reported as `Disturbed`, detected after the fact and not prevented. The rustix `process` feature is added for `geteuid`; no new package, no new `unsafe`. Directories, leftovers and Windows follow. Issue #359. PR #1069. diff --git a/crates/worldscript-secure-storage/src/pinned/removal.rs b/crates/worldscript-secure-storage/src/pinned/removal.rs index b5d9f9273..a7a37536a 100644 --- a/crates/worldscript-secure-storage/src/pinned/removal.rs +++ b/crates/worldscript-secure-storage/src/pinned/removal.rs @@ -74,12 +74,14 @@ pub enum RemovalOutcome { /// the window in the module documentation), and that object is gone; or the entry could not be put /// back and is not where the outcome would have said: an actor renamed the quarantine directory /// away, removed the entry, or replaced it with another object, so the authenticated object cannot - /// be found by the name that was recorded. Nothing can be undone: the caller must treat the - /// directory as tampered with, and never count this as a success. + /// be found by the name that was recorded; or a look at the entry failed (a permission lost), so it + /// cannot be shown to be where the outcome would say, whether or not it is. Nothing can be undone: + /// the caller must treat the directory as tampered with, and never count this as a success. Disturbed, /// Unix only: an error happened after the entry was moved into the quarantine directory and it /// could not be put back (`cause` is the error that started it). The entry is in the quarantine - /// directory (`.h1q-*`, as `x`), found with a listing; [`PinnedDir::remove_leftover_file`] takes it + /// directory (`.h1q-*`, as `x`; it was looked at there, a look that fails gives + /// [`Disturbed`](Self::Disturbed)), found with a listing; [`PinnedDir::remove_leftover_file`] takes it /// from there after authenticating it again. Nothing was removed. This reports a failure by its /// cause, so both directories are synced best effort and a crash may undo where the entry is. Stranded { cause: PinnedError }, diff --git a/crates/worldscript-secure-storage/src/pinned/tests/remove_unix.rs b/crates/worldscript-secure-storage/src/pinned/tests/remove_unix.rs index addae62e5..16bb21f68 100644 --- a/crates/worldscript-secure-storage/src/pinned/tests/remove_unix.rs +++ b/crates/worldscript-secure-storage/src/pinned/tests/remove_unix.rs @@ -551,13 +551,15 @@ fn lock_the_quarantine(quarantined: Option<&Path>) { } #[test] -fn a_search_permission_lost_after_the_move_is_reported_as_stranded() { +fn a_search_permission_lost_after_the_move_is_reported_as_disturbed_because_nothing_can_be_proved() +{ if rustix::process::geteuid().is_root() { - // Root is not kept out by permissions; the same logic is covered without them by the next test. + // Root is not kept out by permissions; the proved case (`Stranded`) is covered without them by + // the next test. eprintln!("skipped: permissions do not apply to root"); return; } - let f = fixture("stranded"); + let f = fixture("unprovable"); let original = ino(&f.root.join("a")); let outcome = f.pinned.remove_authenticated_file_observed( "a", @@ -565,17 +567,14 @@ fn a_search_permission_lost_after_the_move_is_reported_as_stranded() { &mut on(At::MovedAside, lock_the_quarantine), ); let left = quarantines(&f.root); - // Opened up again, the entry is where the outcome said it is, and is the authenticated object. + // Opened up again, the entry is in fact still there and is the authenticated object. The removal + // could not look at it and so could not prove that, and an outcome that says where an entry is needs + // a proof: it is `Disturbed`, conservative, and not `Stranded`. fs::set_permissions(&left[0], fs::Permissions::from_mode(0o700)).unwrap(); let same_object = ino(&left[0].join(INSIDE)) == original; assert_eq!( (outcome, f.root.join("a").exists(), left.len(), same_object), - ( - Ok(RemovalOutcome::Stranded { cause: denied() }), - false, - 1, - true - ) + (Ok(RemovalOutcome::Disturbed), false, 1, true) ); } diff --git a/crates/worldscript-secure-storage/src/pinned/unix/removal.rs b/crates/worldscript-secure-storage/src/pinned/unix/removal.rs index fda0cdbd1..408ffac73 100644 --- a/crates/worldscript-secure-storage/src/pinned/unix/removal.rs +++ b/crates/worldscript-secure-storage/src/pinned/unix/removal.rs @@ -41,8 +41,10 @@ enum Inside { Absent, /// Not the kind that was asked for (a link, a directory instead of a file, a FIFO), or not openable /// by this process although the authenticated object was. It carries what the name led to when this - /// was seen (a look that does not follow a link), so that the entry can be told from a replacement - /// later, as for an opened object. + /// was seen (a look that does not follow a link), so that a replacement can be noticed later. That is + /// weaker than for an opened object: a foreign entry is not opened (opening a link, a FIFO or a device + /// node has effects), so no handle keeps its identity unique, and a file system may give the identity + /// of an object that was removed to its replacement, which this look cannot tell apart. Foreign(ObjectIdentity), /// Opened, with the identity of the open object. Object(File, ObjectIdentity), @@ -215,18 +217,14 @@ impl Quarantine { remove_if_named(parent, &self.name, Some(self.identity)); } - /// Whether the entry may still be inside: `false` only if it is known to be gone, or to be another - /// object than `expected` (an actor removed or replaced it); an error while looking (a permission - /// lost) proves neither. Without an expectation any object counts. - fn holds(&self, expected: Option) -> bool { - match stat_child(&self.fd, OsStr::new(INSIDE)) { - Ok(None) => false, - Ok(Some(stat)) => match expected { - None => true, - Some(wanted) => identity_from(&stat).is_ok_and(|found| found == wanted), - }, - Err(_) => true, - } + /// Whether the entry is inside, as far as a look at the name can prove it: it was looked at and is + /// `expected`. Gone, another object, and a look that fails (a permission lost) all give `false`: an + /// outcome that says where the entry is needs a proof, and a failed look proves nothing. + fn holds(&self, expected: ObjectIdentity) -> bool { + matches!( + stat_child(&self.fd, OsStr::new(INSIDE)), + Ok(Some(stat)) if identity_from(&stat).is_ok_and(|found| found == expected) + ) } /// Whether the name still leads to the directory that was created. @@ -377,9 +375,7 @@ impl Removal<'_> { let _ = self.quarantine.sync(); match put_back { Ok(()) => Ok(Step::RolledBack(cause)), - Err(_) if self.retained(Some(self.target.expected)) => { - Ok(Step::Stranded(cause)) - } + Err(_) if self.retained(self.target.expected) => Ok(Step::Stranded(cause)), Err(_) => Ok(Step::Changed(RemovalOutcome::Disturbed)), } } @@ -402,17 +398,17 @@ impl Removal<'_> { Inside::Object(object, found) if found == self.target.expected => { self.conclude(&object, observer) } - Inside::Object(_, found) => self.swapped(Some(found)), - Inside::Foreign(seen) => self.swapped(Some(seen)), + Inside::Object(_, found) => self.swapped(found), + Inside::Foreign(seen) => self.swapped(seen), } } /// What arrived is not the authenticated object: it goes back under its name, or stays aside if - /// that is impossible (and is then still `arrived`, the identity it was seen with if it was opened). - /// Either way the quarantine directory changed (the entry left it, or arrived in it), and it is synced - /// after the rename was tried: the rename alone decides `restored`, so a sync that fails is neither a - /// put-back that did not happen nor a reason to call the entry tampered with. - fn swapped(&self, arrived: Option) -> Result { + /// that is impossible (and is then still `arrived`, the identity it was seen with). Either way the + /// quarantine directory changed (the entry left it, or arrived in it), and it is synced after the + /// rename was tried: the rename alone decides `restored`, so a sync that fails is neither a put-back + /// that did not happen nor a reason to call the entry tampered with. + fn swapped(&self, arrived: ObjectIdentity) -> Result { let restored = self.restore().is_ok(); let held = self.quarantine.sync(); if !restored && !self.retained(arrived) { @@ -451,11 +447,12 @@ impl Removal<'_> { } /// The entry stays in the quarantine directory: the outcome may only say where the entry is if the - /// recorded name still leads to the directory that holds it and the entry in it is still `expected` - /// (or cannot be looked at). If an actor renamed that directory away, or removed or replaced the - /// entry, the entry cannot be found where the outcome would say, and that is reported as tampering. - /// Syncing that directory, which changed as well as the parent, is up to the caller. - fn retained(&self, expected: Option) -> bool { + /// recorded name still leads to the directory that holds it and the entry in it was looked at and is + /// still `expected`. If an actor renamed that directory away, or removed or replaced the entry, or + /// the look fails, the entry cannot be shown where the outcome would say, and that is reported as + /// tampering (it is not known to be absent, but nothing can be proved). Syncing that directory, + /// which changed as well as the parent, is up to the caller. + fn retained(&self, expected: ObjectIdentity) -> bool { self.quarantine.is_named(&self.dir.handle) && self.quarantine.holds(expected) } } diff --git a/docs/native/r15/GATE4D-JOURNAL-DURABLE-EVIDENCE.md b/docs/native/r15/GATE4D-JOURNAL-DURABLE-EVIDENCE.md index 2a1064897..b1cd3fb89 100644 --- a/docs/native/r15/GATE4D-JOURNAL-DURABLE-EVIDENCE.md +++ b/docs/native/r15/GATE4D-JOURNAL-DURABLE-EVIDENCE.md @@ -1774,9 +1774,13 @@ actor that wins that earlier race gains nothing the window below does not alread directory (`restored: false`, `Stranded`) is only reported as being there if the recorded name still leads to that directory and the entry in it is still the object the outcome names (the authenticated object, or for a swapped-in object the identity it was seen with, also one that is not the kind -asked for, by a look that does not follow a link, where a look that fails is a failed inspection and not a retained entry; an -entry that cannot be looked at when it is checked again counts as there, since an error proves neither); if an actor renamed the directory away, removed the entry or replaced it with -another object the outcome is `Disturbed` (tampered, the entry cannot be found where the outcome would say). The check is itself +asked for, by a look that does not follow a link, where a look that fails is a failed inspection and not a retained entry). An +entry that cannot be looked at when it is checked again is not reported as there either: a failed look proves nothing, and an outcome +that says where the entry is needs a proof. If an actor renamed the directory away, removed the entry or replaced it with +another object, or the look fails, the outcome is `Disturbed` (tampered, or at least the entry cannot be shown where the outcome +would say). The identity of a foreign entry is weaker than that of an opened object: it is not opened (opening a link, a FIFO or a +device node has effects, and no handle can be taken for every kind without them), so nothing holds its identity unique and a file +system may give the identity of a removed object to its replacement, which the check cannot tell apart. The check is itself a look at a name and does not close the same-user race. The quarantine directory the unlink happened in is synced as well as the parent before `Removed` is reported, whether it is removed afterwards, survives because an actor put an entry into it, or was renamed away; if that sync fails the removal is `Removed(NotConfirmed)`. The identity check before the @@ -1812,8 +1816,9 @@ directory) that an actor replaces after the inspection, while the put-back is bl untouched is kept (`Swapped { restored: false }`; the replacement runs at the sync of the quarantine directory, the one point between the inspection and the check that a test can reach); a quarantine directory that loses its search permission after the -move gives `Stranded` with the entry still the authenticated inode, and the same case without permissions (an observer that -fails while the name is taken again, so it runs the same for root); a quarantine directory renamed away and replaced by an empty +move gives `Disturbed` although the entry is in fact still the authenticated inode (the removal could not look at it and cannot +prove it; skipped for root), and the proved case, `Stranded`, without permissions (an observer that fails while the name is taken +again, so it runs the same for root); a quarantine directory renamed away and replaced by an empty directory of the actor is not removed, and the removal of a directory by its name is exercised on its own (the directory that was made goes, one renamed away and replaced, a non-empty one and a name without an identity stay; the setup failure that reaches it in `create` cannot be provoked without changing the process umask and is not forced). Mutation-checked: skipping the identity comparison fails @@ -1821,7 +1826,7 @@ five of these cases, skipping the link-count verification fails the forced last failed inspection fails the `Stranded` case; leaving out the sync of the quarantine directory after a put-back or after a rollback, dropping a failed sync of it, a `sync_all()?` inside the rename step, confirming a removal despite a failed sync, skipping the sync for an entry found gone at the inspection, dropping it for one gone at the unlink and passing no identity for a foreign -entry each fail at least one of the sync cases. +entry each fail at least one of the sync cases, and counting a failed look as present fails the permission case. **Not claimed.** Directories and the leftover recovery are part 3 (they answer `Unsupported` here); Windows is part 4; the Disturbed window is bounded and detected, not closed; a device node swapped in is refused before it is opened and cannot be