diff --git a/CHANGELOG.md b/CHANGELOG.md index f6b928237..7de359c01 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:** removal of a directory on Linux, the recovery of leftovers and the crash cases on Unix, `test-support` only (slice H1a-2, part 3). `remove_empty_dir` moves the directory through the same quarantine directory and checks the link count of the held directory afterward; macOS reports no usable signal for a removed directory that is still open (hosted probe: link count and `F_GETPATH` unchanged), so there an empty directory answers `Unsupported` before anything is moved; `remove_leftover_file` takes a leftover `.h1q-*` directory by its generated name only, authenticates the file in it again and removes it only if accepted. A simulated crash, an entry stranded by a blocked put-back and a foreign object left by a refused put-back are covered; the held directory is synced for what the unlink changed in it, like any removal; a non-empty directory swapped in the last window is `Disturbed`, not `NotEmpty`. The recovery also removes an empty directory left by a crashed directory removal (Linux; it holds nothing, so it is not authenticated), and syncs the parent when it removes the emptied quarantine directory, whatever the removal came to. No new `unsafe`, no new package. Issue #359. PR #1074. + - **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 a7a37536a..241d7a0ca 100644 --- a/crates/worldscript-secure-storage/src/pinned/removal.rs +++ b/crates/worldscript-secure-storage/src/pinned/removal.rs @@ -4,9 +4,11 @@ //! **Availability.** Everything in this module exists only with the test-only `test-support` feature, //! has no caller, and is not part of any production build. Reclamation of a real store is not admitted. //! -//! **Status of this slice.** The API and its isolation. Every call answers [`PinnedError::Unsupported`] -//! until the platform implementations land in the successor slices (Unix files; Unix directories and -//! leftovers; Windows), so nothing is removed and nothing claims a safety it does not have yet. +//! **Status of this slice.** The API and its isolation, and on Unix the removal of files and of the +//! leftovers of a crash, and on Linux the removal of directories. A call on Windows, and the removal of +//! a directory on any other system, answers [`PinnedError::Unsupported`] (see +//! [`PinnedDir::remove_empty_dir`]), so nothing is removed there and nothing claims a safety it does not +//! have yet. //! //! **The two designs the platform slices do not use.** Removing "the entry that was checked" with //! `unlinkat(parent, name)` after a check on the name is exposed to a swap of the last component: POSIX @@ -134,7 +136,11 @@ impl PinnedDir { } /// Removes the child directory `name` if it has no entries, and exactly the directory that was - /// opened. + /// opened. On Unix this is available on Linux only: the removal counts only if the open directory can + /// be asked afterwards whether it was the one removed, and no other Unix system answers that without + /// writing into the directory (macOS keeps the old link count and the old path of a removed directory + /// that is still open). There an empty directory answers [`PinnedError::Unsupported`] before anything is + /// moved; a link, a missing name and a directory with entries are told the same everywhere. pub fn remove_empty_dir(&self, name: impl AsRef) -> Result { self.remove_empty_dir_observed(name, &mut |_| Ok(())) } @@ -151,8 +157,11 @@ impl PinnedDir { /// Finishes what a crash or a refused put-back left in the quarantine directory `quarantine` (a /// `.h1q-*` entry of this directory): the file in it is authenticated again and, if `authenticate` - /// accepts it, removed like any other. A file that is not accepted stays; an empty quarantine - /// directory is removed. Unix only: Windows creates none (`Unsupported`). + /// accepts it, removed like any other. A file that is not accepted stays, and so does anything that + /// is neither a regular file nor an empty directory (a link, a directory with entries); an empty + /// directory that a crashed directory removal left holds nothing, needs no authentication and is + /// removed on Linux; an empty quarantine directory is removed. Unix only: Windows creates none + /// (`Unsupported`). pub fn remove_leftover_file( &self, quarantine: impl AsRef, diff --git a/crates/worldscript-secure-storage/src/pinned/tests/mod.rs b/crates/worldscript-secure-storage/src/pinned/tests/mod.rs index 7479f89d5..f77c34734 100644 --- a/crates/worldscript-secure-storage/src/pinned/tests/mod.rs +++ b/crates/worldscript-secure-storage/src/pinned/tests/mod.rs @@ -8,11 +8,15 @@ use std::sync::atomic::{AtomicU32, Ordering}; mod read; #[cfg(feature = "test-support")] mod remove; +#[cfg(all(feature = "test-support", target_os = "linux"))] +mod remove_linux_dir; #[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_dir; +#[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.rs b/crates/worldscript-secure-storage/src/pinned/tests/remove.rs index 03bfa29e7..539376f0a 100644 --- a/crates/worldscript-secure-storage/src/pinned/tests/remove.rs +++ b/crates/worldscript-secure-storage/src/pinned/tests/remove.rs @@ -147,6 +147,54 @@ fn removing_a_hard_linked_name_leaves_the_other_names() { ); } +#[cfg(unix)] +#[test] +fn an_empty_directory_is_removed_and_a_filled_one_or_a_link_is_not() { + let scratch = Scratch::new("dirs"); + let root = scratch.root(); + let aside = scratch.aside(); + fs::create_dir(root.join("empty")).unwrap(); + fs::create_dir(root.join("filled")).unwrap(); + fs::write(root.join("filled").join("inside"), b"inside").unwrap(); + fs::create_dir(aside.join("target")).unwrap(); + link_to_dir(&aside.join("target"), &root.join("link")); + let pinned = PinnedDir::open_root(&root).unwrap(); + // Only Linux can tell afterwards that the open directory was the one removed; elsewhere the removal + // of an empty directory is refused before anything is moved, and the other cases are the same. + let (empty_removal, left) = if cfg!(target_os = "linux") { + (removed(), vec!["filled".to_string(), "link".to_string()]) + } else { + ( + Err(PinnedError::Unsupported), + vec![ + "empty".to_string(), + "filled".to_string(), + "link".to_string(), + ], + ) + }; + assert_eq!( + ( + pinned.remove_empty_dir("empty"), + pinned.remove_empty_dir("filled"), + pinned.remove_empty_dir("link"), + pinned.remove_empty_dir("missing"), + fs::read(root.join("filled").join("inside")).unwrap(), + aside.join("target").is_dir(), + names(&pinned) + ), + ( + empty_removal, + Ok(RemovalOutcome::NotEmpty), + Err(PinnedError::Redirected), + Err(PinnedError::NotFound), + b"inside".to_vec(), + true, + left + ) + ); +} + #[cfg(windows)] #[test] fn every_removal_refuses_until_windows_implements_it_and_removes_nothing() { diff --git a/crates/worldscript-secure-storage/src/pinned/tests/remove_linux_dir.rs b/crates/worldscript-secure-storage/src/pinned/tests/remove_linux_dir.rs new file mode 100644 index 000000000..169766bb9 --- /dev/null +++ b/crates/worldscript-secure-storage/src/pinned/tests/remove_linux_dir.rs @@ -0,0 +1,154 @@ +//! Linux removal of a directory under the swaps the design exists for: another actor replaces the +//! directory, or the entry inside the quarantine directory, between the steps of the removal. Each case +//! forces the swap through the observer at a chosen stage and pins what happens to every object. Other +//! Unix systems answer `Unsupported` (see `remove_unix_dir`), because they cannot tell afterwards, without +//! writing into the directory, that the open one is the directory that was removed. + +use std::fs; +use std::os::unix::fs::PermissionsExt; +use std::panic::{catch_unwind, AssertUnwindSafe}; + +use super::remove::{names, removed}; +use super::remove_unix::{fixture, ino, on, quarantines, At}; +use crate::pinned::unix::removal::INSIDE; +use crate::pinned::{RemovalOutcome, QUARANTINE_PREFIX}; + +#[test] +fn a_directory_swapped_or_filled_after_the_check_is_put_back_and_not_removed() { + let f = fixture("dir-swap"); + fs::create_dir(f.root.join("d")).unwrap(); + fs::create_dir(f.root.join("e")).unwrap(); + let swapped = f.pinned.remove_empty_dir_observed( + "d", + &mut on(At::Checked, |_| { + fs::rename(f.root.join("d"), f.aside.join("original-d")).unwrap(); + fs::create_dir(f.root.join("d")).unwrap(); + fs::write(f.root.join("d").join("live"), b"live").unwrap(); + }), + ); + let filled = f.pinned.remove_empty_dir_observed( + "e", + &mut on(At::Checked, |_| { + fs::write(f.root.join("e").join("late"), b"late").unwrap() + }), + ); + assert_eq!( + ( + swapped, + fs::read(f.root.join("d").join("live")).unwrap(), + f.aside.join("original-d").is_dir(), + filled, + fs::read(f.root.join("e").join("late")).unwrap(), + names(&f.pinned) + ), + ( + Ok(RemovalOutcome::Swapped { restored: true }), + b"live".to_vec(), + true, + Ok(RemovalOutcome::NotEmpty), + b"late".to_vec(), + vec!["a".to_string(), "d".to_string(), "e".to_string()] + ) + ); +} + +#[test] +fn an_empty_directory_swapped_in_the_last_window_is_reported_as_disturbed() { + let f = fixture("window-empty-dir"); + fs::create_dir(f.root.join("d")).unwrap(); + let original = ino(&f.root.join("d")); + let outcome = f.pinned.remove_empty_dir_observed( + "d", + &mut on(At::Identified, |quarantined| { + let quarantined = quarantined.unwrap(); + fs::rename(quarantined, f.aside.join("original-d")).unwrap(); + fs::create_dir(quarantined).unwrap(); + }), + ); + assert_eq!( + ( + outcome, + f.root.join("d").exists(), + ino(&f.aside.join("original-d")) == original, + quarantines(&f.root).len() + ), + (Ok(RemovalOutcome::Disturbed), false, true, 0) + ); +} + +#[test] +fn a_non_empty_directory_swapped_in_the_last_window_is_not_reported_as_not_empty() { + // The refusal to remove a directory with entries is about the directory an actor put there, not about + // the authenticated one, which the actor moved away. What is there goes back under the name, and + // the outcome is tampering, not `NotEmpty`. + let f = fixture("window-filled-dir"); + fs::create_dir(f.root.join("d")).unwrap(); + let original = ino(&f.root.join("d")); + let outcome = f.pinned.remove_empty_dir_observed( + "d", + &mut on(At::Identified, |quarantined| { + let quarantined = quarantined.unwrap(); + fs::rename(quarantined, f.aside.join("original-d")).unwrap(); + fs::create_dir(quarantined).unwrap(); + fs::write(quarantined.join("late"), b"late").unwrap(); + }), + ); + assert_eq!( + ( + outcome, + fs::read(f.root.join("d").join("late")).ok(), + ino(&f.aside.join("original-d")) == original, + quarantines(&f.root).len() + ), + ( + Ok(RemovalOutcome::Disturbed), + Some(b"late".to_vec()), + true, + 0 + ) + ); +} + +#[test] +fn an_empty_directory_left_by_a_crashed_directory_removal_is_finished_by_the_leftover_recovery() { + let f = fixture("crash-dir"); + fs::create_dir(f.root.join("d")).unwrap(); + let crashed = catch_unwind(AssertUnwindSafe(|| { + let _ = f + .pinned + .remove_empty_dir_observed("d", &mut on(At::MovedAside, |_| panic!("simulated crash"))); + })); + assert!(crashed.is_err()); + let left = quarantines(&f.root); + let name = left[0].file_name().unwrap().to_owned(); + let before = (left[0].join(INSIDE).is_dir(), f.root.join("d").exists()); + // An empty directory holds nothing to authenticate: the callback is never asked. + let finished = f.pinned.remove_leftover_file(&name, |_| panic!("asked")); + assert_eq!( + ( + before, + finished, + names(&f.pinned), + quarantines(&f.root).len() + ), + ((true, false), removed(), vec!["a".to_string()], 0) + ); +} + +#[test] +fn a_directory_with_entries_left_in_a_quarantine_directory_is_never_removed_by_the_recovery() { + let f = fixture("leftover-filled-dir"); + let name = format!("{QUARANTINE_PREFIX}{}", "3".repeat(32)); + fs::create_dir(f.root.join(&name)).unwrap(); + fs::set_permissions(f.root.join(&name), fs::Permissions::from_mode(0o700)).unwrap(); + fs::create_dir(f.root.join(&name).join(INSIDE)).unwrap(); + fs::write(f.root.join(&name).join(INSIDE).join("inside"), b"inside").unwrap(); + let outcome = f.pinned.remove_leftover_file(&name, |_| Ok(true)); + assert_eq!( + ( + outcome, + fs::read(f.root.join(&name).join(INSIDE).join("inside")).unwrap() + ), + (Ok(RemovalOutcome::NotEmpty), b"inside".to_vec()) + ); +} 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 16bb21f68..f9a00299f 100644 --- a/crates/worldscript-secure-storage/src/pinned/tests/remove_unix.rs +++ b/crates/worldscript-secure-storage/src/pinned/tests/remove_unix.rs @@ -60,7 +60,7 @@ pub(super) fn at(stage: RemovalStage<'_>) -> (At, Option<&Path>) { } /// An observer that runs `action` at the stage `here` and otherwise does nothing. -fn on( +pub(super) fn on( here: At, mut action: impl FnMut(Option<&Path>), ) -> impl FnMut(RemovalStage<'_>) -> Result<(), PinnedError> { @@ -94,7 +94,7 @@ pub(super) fn swap_in_a_live_file(root: &Path, aside: &Path) { } /// The inode of the object at `path` (not following a link): who is who, whatever the content says. -fn ino(path: &Path) -> u64 { +pub(super) fn ino(path: &Path) -> u64 { fs::symlink_metadata(path).unwrap().ino() } diff --git a/crates/worldscript-secure-storage/src/pinned/tests/remove_unix_dir.rs b/crates/worldscript-secure-storage/src/pinned/tests/remove_unix_dir.rs new file mode 100644 index 000000000..ff4ec1c59 --- /dev/null +++ b/crates/worldscript-secure-storage/src/pinned/tests/remove_unix_dir.rs @@ -0,0 +1,293 @@ +//! Unix recovery of what a crash or a refused put-back left in a quarantine directory, under the swaps +//! the design exists for: another actor replaces the entry inside the quarantine directory between the +//! steps of the removal. Each case forces the swap through the observer at a chosen stage and pins what +//! happens to every object. The removal of a directory is in `remove_linux_dir`; where it is not +//! available it answers `Unsupported`, which is pinned here. + +use std::fs; +use std::panic::{catch_unwind, AssertUnwindSafe}; + +use super::remove::{holds, names, removed}; +use super::remove_unix::{ + at, denied, fixture, held_in_quarantine, on, quarantines, swap_in_a_live_file, At, Fixture, +}; +use crate::durable::DirectoryDurability::NotConfirmed; +use crate::pinned::unix::removal::{remove_leftover, sync_fault, INSIDE}; +use crate::pinned::{PinnedError, RemovalOutcome, QUARANTINE_PREFIX}; + +#[cfg(not(target_os = "linux"))] +#[test] +fn removing_a_directory_is_unsupported_where_the_system_cannot_tell_afterwards_and_moves_nothing() { + // The checks that need no such signal still hold: a directory with entries is `NotEmpty`, a link is + // refused, a missing name is not found; an empty directory is refused as unsupported before it is moved. + let f = fixture("dir-unsupported"); + fs::create_dir(f.root.join("empty")).unwrap(); + fs::create_dir(f.root.join("filled")).unwrap(); + fs::write(f.root.join("filled").join("inside"), b"inside").unwrap(); + assert_eq!( + ( + f.pinned.remove_empty_dir("empty"), + f.pinned.remove_empty_dir("filled"), + f.pinned.remove_empty_dir("missing"), + names(&f.pinned), + quarantines(&f.root).len() + ), + ( + Err(PinnedError::Unsupported), + Ok(RemovalOutcome::NotEmpty), + Err(PinnedError::NotFound), + vec!["a".to_string(), "empty".to_string(), "filled".to_string()], + 0 + ) + ); +} + +/// A removal that "crashes" (the observer panics) once the object is in the quarantine directory. +fn crash_after_the_move(f: &Fixture) { + let crashed = catch_unwind(AssertUnwindSafe(|| { + let _ = f.pinned.remove_authenticated_file_observed( + "a", + holds(b"alpha"), + &mut on(At::MovedAside, |_| panic!("simulated crash")), + ); + })); + assert!(crashed.is_err()); +} + +#[test] +fn a_crash_leaves_the_object_in_quarantine_and_a_leftover_removal_finishes_it_after_authenticating() +{ + let f = fixture("crash"); + crash_after_the_move(&f); + let left = quarantines(&f.root); + let name = left[0].file_name().unwrap().to_owned(); + let before = (names(&f.pinned).len(), held_in_quarantine(&f.root)); + let refused = f + .pinned + .remove_leftover_file(&name, holds(b"something else")); + let finished = f.pinned.remove_leftover_file(&name, holds(b"alpha")); + assert_eq!( + (before, refused, finished, names(&f.pinned)), + ( + (1, Some(b"alpha".to_vec())), + Ok(RemovalOutcome::Refused), + removed(), + Vec::::new() + ) + ); +} + +#[test] +fn a_leftover_whose_quarantine_directory_could_not_be_synced_is_not_confirmed() { + // The leftover removal changes the quarantine directory it unlinks in, like any removal, and says + // `NotConfirmed` if that directory could not be synced (modelled through the seam of the removal). + let f = fixture("leftover-sync"); + crash_after_the_move(&f); + let name = quarantines(&f.root)[0].file_name().unwrap().to_owned(); + let (outcome, syncs) = + sync_fault::during(Some(PinnedError::Io(std::io::ErrorKind::Other)), || { + f.pinned.remove_leftover_file(&name, holds(b"alpha")) + }); + assert_eq!( + (outcome, syncs, names(&f.pinned), quarantines(&f.root).len()), + ( + Ok(RemovalOutcome::Removed(NotConfirmed)), + 1, + Vec::::new(), + 0 + ) + ); +} + +#[test] +fn a_leftover_is_taken_by_its_generated_name_only_and_an_empty_one_is_removed() { + use std::os::unix::fs::PermissionsExt; + let f = fixture("leftover-names"); + let generated = format!("{QUARANTINE_PREFIX}{}", "0".repeat(32)); + for name in [generated.as_str(), ".h1q-empty", "not-a-quarantine"] { + fs::create_dir(f.root.join(name)).unwrap(); + fs::set_permissions(f.root.join(name), fs::Permissions::from_mode(0o700)).unwrap(); + } + let taken = |name: &str| f.pinned.remove_leftover_file(name, |_| Ok(true)); + // Right prefix, wrong digits: too short, too long, upper case, not hexadecimal. Refused by the name + // alone, before anything is opened. + let wrong_digits = [ + "0".repeat(31), + "0".repeat(33), + "A".repeat(32), + "G".repeat(32), + ] + .map(|digits| taken(&format!("{QUARANTINE_PREFIX}{digits}"))); + assert_eq!( + ( + taken("a"), + taken("not-a-quarantine"), + taken(".h1q-empty"), + wrong_digits, + taken(&generated), + names(&f.pinned) + ), + ( + Err(PinnedError::InvalidName), + Err(PinnedError::InvalidName), + Err(PinnedError::InvalidName), + [Err(PinnedError::InvalidName); 4], + Ok(RemovalOutcome::Vanished), + vec![ + ".h1q-empty".to_string(), + "a".to_string(), + "not-a-quarantine".to_string() + ] + ) + ); +} + +#[test] +fn a_foreign_object_left_in_quarantine_is_never_removed_by_a_leftover_removal() { + let f = fixture("leftover-foreign"); + let mut at_checked = on(At::Checked, |_| swap_in_a_live_file(&f.root, &f.aside)); + let mut at_moved = on(At::MovedAside, |_| { + fs::write(f.root.join("a"), b"retaken").unwrap() + }); + let first = f + .pinned + .remove_authenticated_file_observed("a", holds(b"alpha"), &mut |stage| { + at_checked(stage)?; + at_moved(stage) + }); + let name = quarantines(&f.root)[0].file_name().unwrap().to_owned(); + // The object in the quarantine directory is the live file, not an object of the sweep. + let refused = f.pinned.remove_leftover_file(&name, holds(b"alpha")); + assert_eq!( + (first, refused, held_in_quarantine(&f.root)), + ( + Ok(RemovalOutcome::Swapped { restored: false }), + Ok(RemovalOutcome::Refused), + Some(b"live".to_vec()) + ) + ); +} + +#[test] +fn a_stranded_entry_is_finished_by_the_leftover_removal_after_authenticating_it() { + // The put-back is blocked (the name is taken again) and the observer fails: the entry is proved to + // be in the quarantine directory, which is what `Stranded` says, and runs the same for root. + let f = fixture("stranded-recovery"); + let stranded = + f.pinned + .remove_authenticated_file_observed("a", holds(b"alpha"), &mut |stage| { + if at(stage).0 == At::MovedAside { + fs::write(f.root.join("a"), b"taken").unwrap(); + return Err(denied()); + } + Ok(()) + }); + let name = quarantines(&f.root)[0].file_name().unwrap().to_owned(); + let refused = f + .pinned + .remove_leftover_file(&name, holds(b"something else")); + let finished = f.pinned.remove_leftover_file(&name, holds(b"alpha")); + assert_eq!( + (stranded, refused, finished, names(&f.pinned)), + ( + Ok(RemovalOutcome::Stranded { cause: denied() }), + Ok(RemovalOutcome::Refused), + removed(), + vec!["a".to_string()] + ) + ); +} + +#[test] +fn a_link_left_in_a_quarantine_directory_is_refused_and_not_followed() { + use std::os::unix::fs::PermissionsExt; + let f = fixture("leftover-link"); + let name = format!("{QUARANTINE_PREFIX}{}", "1".repeat(32)); + fs::create_dir(f.root.join(&name)).unwrap(); + fs::set_permissions(f.root.join(&name), fs::Permissions::from_mode(0o700)).unwrap(); + fs::write(f.aside.join("target"), b"target").unwrap(); + std::os::unix::fs::symlink(f.aside.join("target"), f.root.join(&name).join("x")).unwrap(); + // Whatever the authentication would say, an entry that is not a regular file is never handed to it. + let outcome = f.pinned.remove_leftover_file(&name, |_| Ok(true)); + assert_eq!( + ( + outcome, + fs::read_link(f.root.join(&name).join("x")).ok(), + fs::read(f.aside.join("target")).unwrap() + ), + ( + Ok(RemovalOutcome::Refused), + Some(f.aside.join("target")), + b"target".to_vec() + ) + ); +} + +/// One leftover removal after a crash, with the entry taken away by an actor while it is authenticated +/// (or not), and what the authentication says: the outcome, whether the parent was about to be synced, +/// and how many quarantine directories are left. +fn leftover_run( + label: &str, + take_entry: bool, + verdict: Result, +) -> (Result, bool, usize) { + let f = fixture(label); + crash_after_the_move(&f); + let q = quarantines(&f.root)[0].clone(); + let name = q.file_name().unwrap().to_owned(); + let mut synced = false; + let outcome = remove_leftover( + &f.pinned, + &name, + |_file| { + if take_entry { + fs::remove_file(q.join(INSIDE)).unwrap(); + } + verdict + }, + &mut |stage| { + if at(stage).0 == At::BeforeSync { + synced = true; + } + Ok(()) + }, + ); + (outcome, synced, quarantines(&f.root).len()) +} + +#[test] +fn a_leftover_whose_empty_quarantine_directory_goes_with_the_discard_syncs_the_parent_whatever_the_result( +) { + // When the entry stays, nothing changed and nothing is synced. When an actor takes it away while it + // is looked at, the quarantine directory is empty and is removed, which changes the parent: the + // parent is synced, for a refusal and for an error, and the result is kept. + assert_eq!( + [ + leftover_run("kept", false, Ok(false)), + leftover_run("taken-refused", true, Ok(false)), + leftover_run("taken-error", true, Err(denied())), + ], + [ + (Ok(RemovalOutcome::Refused), false, 1), + (Ok(RemovalOutcome::Refused), true, 0), + (Err(denied()), true, 0), + ] + ); +} + +#[cfg(not(target_os = "linux"))] +#[test] +fn an_empty_directory_left_in_a_quarantine_directory_is_refused_where_its_removal_cannot_be_checked( +) { + use std::os::unix::fs::PermissionsExt; + let f = fixture("leftover-dir-unsupported"); + let name = format!("{QUARANTINE_PREFIX}{}", "2".repeat(32)); + fs::create_dir(f.root.join(&name)).unwrap(); + fs::set_permissions(f.root.join(&name), fs::Permissions::from_mode(0o700)).unwrap(); + fs::create_dir(f.root.join(&name).join(INSIDE)).unwrap(); + let outcome = f.pinned.remove_leftover_file(&name, |_| Ok(true)); + assert_eq!( + (outcome, f.root.join(&name).join(INSIDE).is_dir()), + (Ok(RemovalOutcome::Refused), true) + ); +} diff --git a/crates/worldscript-secure-storage/src/pinned/unix/removal.rs b/crates/worldscript-secure-storage/src/pinned/unix/removal.rs index 408ffac73..b5c2528bc 100644 --- a/crates/worldscript-secure-storage/src/pinned/unix/removal.rs +++ b/crates/worldscript-secure-storage/src/pinned/unix/removal.rs @@ -1,10 +1,10 @@ //! Unix removal, test-support only: move the entry into a quarantine directory, identify the open //! object there, and only then unlink it. See the parent `removal` module for the reasoning and for the -//! one window that no Unix primitive closes. This part removes files; directories and the leftover -//! recovery follow and refuse with `Unsupported` until then. +//! one window that no Unix primitive closes. use std::ffi::{OsStr, OsString}; use std::fs::File; +use std::os::unix::ffi::OsStrExt; use std::os::unix::fs::MetadataExt; use std::path::{Path, PathBuf}; @@ -27,6 +27,9 @@ use crate::random::{OsRandom, RandomSource}; /// The name an entry has inside the quarantine directory. pub(in crate::pinned) const INSIDE: &str = "x"; +/// How many hexadecimal digits follow the prefix in the name of a quarantine directory. +const SUFFIX_DIGITS: usize = 32; + /// A quarantine directory inside the pinned parent, held by descriptor. Whatever it holds is outside /// every name grammar of the store and is never removed by a sweep. struct Quarantine { @@ -52,6 +55,8 @@ enum Inside { /// How a removal ended before the parent directory was synced. enum Step { + /// Nothing changed (a leftover that was refused); the outcome is final and nothing is synced. + Unchanged(RemovalOutcome), /// The directory changed (a quarantine directory came and went, the name led nowhere, or tampering /// was found); the outcome is final. Changed(RemovalOutcome), @@ -68,6 +73,17 @@ enum Step { Stranded(PinnedError), } +/// Whether `name` is exactly what [`Quarantine::create`] produces. +fn is_generated_name(name: &OsStr) -> bool { + let bytes = name.as_bytes(); + let prefix = QUARANTINE_PREFIX.as_bytes(); + bytes.len() == prefix.len() + SUFFIX_DIGITS + && bytes.starts_with(prefix) + && bytes[prefix.len()..] + .iter() + .all(|digit| matches!(digit, b'0'..=b'9' | b'a'..=b'f')) +} + impl Quarantine { fn create(parent: &DirHandle) -> Result { let mut bits = [0u8; 16]; @@ -98,6 +114,19 @@ impl Quarantine { Ok(Quarantine { fd, name, identity }) } + /// An existing quarantine directory of `parent`, found by its generated name: a directory of this + /// process's user that no one else can enter. Its identity is the one seen now. + fn open_existing(parent: &DirHandle, name: &OsStr) -> Result { + if !is_generated_name(name) { + return Err(PinnedError::InvalidName); + } + let fd = Self::open_owned(parent, name)?; + if Self::mode_of(&fd)? & 0o077 != 0 { + return Err(PinnedError::Unsupported); + } + Self::held(fd, name.to_os_string()) + } + /// 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 { @@ -210,11 +239,11 @@ impl Quarantine { parent_path.join(&self.name).join(INSIDE) } - /// 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) { - remove_if_named(parent, &self.name, Some(self.identity)); + /// Removes this directory if it is empty; whether it was removed, which changed the parent. 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) -> bool { + remove_if_named(parent, &self.name, Some(self.identity)) } /// Whether the entry is inside, as far as a look at the name can prove it: it was looked at and is @@ -341,6 +370,113 @@ pub(in crate::pinned) fn remove_file( remove_moved(dir, target, observer) } +/// Removes the empty child directory `name`. Linux only: the removal is only counted if the open +/// directory can be asked afterwards whether it was the one that lost its name, and only Linux answers +/// that without writing into the directory (a removed directory that is still open reports a link count of +/// zero). On macOS the count stays at 2, `F_GETPATH` keeps answering with the old path, and only creating +/// an entry in it fails (`ENOENT`), which would write into a directory an actor may have moved away and +/// fails for lack of permission on a read-only one (hosted macOS probe). Elsewhere the call answers +/// `Unsupported` after the checks that need no such signal (a link, a missing name, a directory that has +/// entries) and before anything is moved. +pub(in crate::pinned) fn remove_dir( + dir: &PinnedDir, + name: &OsStr, + observer: Observer<'_>, +) -> Result { + let child = dir.open_dir(name)?; + match child.entries(0) { + Ok(_) => {} + Err(PinnedError::TooManyEntries) => return Ok(RemovalOutcome::NotEmpty), + Err(error) => return Err(error), + } + if !cfg!(target_os = "linux") { + return Err(PinnedError::Unsupported); + } + observer(RemovalStage::Checked)?; + let target = Target { + name, + expected: child.identity(), + kind: EntryKind::Directory, + }; + remove_moved(dir, target, observer) +} + +/// Removes the leftover file of the quarantine directory `quarantine` after authenticating it again. +pub(in crate::pinned) fn remove_leftover( + dir: &PinnedDir, + quarantine: &OsStr, + authenticate: impl FnOnce(&mut PinnedFile) -> Result, + observer: Observer<'_>, +) -> Result { + let held = Quarantine::open_existing(&dir.handle, quarantine)?; + let step = leftover_step(&held, dir, authenticate, observer); + // Removing the empty quarantine directory changes the parent whatever the removal came to (an actor + // can take the entry away while it is being looked at), so the parent is synced then too, and the + // result of the removal is kept. + let discarded = held.discard(&dir.handle); + match step { + Ok(Step::Unchanged(outcome)) if discarded => settle(dir, Step::Changed(outcome), observer), + Ok(step) => settle(dir, step, observer), + Err(error) => { + if discarded { + sync_after_failure(dir, observer); + } + Err(error) + } + } +} + +/// Looks at what a crash or a refused put-back left in `held`, authenticates it again and unlinks it +/// like any other removal: the quarantine directory is synced for what the unlink changed in it. +fn leftover_step( + held: &Quarantine, + dir: &PinnedDir, + authenticate: impl FnOnce(&mut PinnedFile) -> Result, + observer: Observer<'_>, +) -> Result { + let (file, identity) = match held.inspect(EntryKind::File)? { + // An empty quarantine directory is junk and goes with `discard`. + Inside::Absent => return Ok(Step::Changed(RemovalOutcome::Vanished)), + Inside::Foreign(_) => return leftover_directory(held, dir, observer), + Inside::Object(file, identity) => (file, identity), + }; + let mut pinned = PinnedFile { file, identity }; + if !authenticate(&mut pinned)? { + return Ok(Step::Unchanged(RemovalOutcome::Refused)); + } + observer(RemovalStage::Identified { + quarantined: &held.path_inside(&dir.path), + })?; + let outcome = unlink_verified(held, &pinned.file, EntryKind::File)?; + Ok(unlinked(outcome, held.sync())) +} + +/// A leftover that is not a file. An empty directory, which a crashed directory removal leaves, holds +/// nothing, so it needs no authentication and goes with the same unlink and the same check as the removal +/// of a directory (Linux only, see [`remove_dir`]). Anything else stays and is refused: a link, a FIFO, a +/// directory on a system that cannot check the removal, and a directory that has entries (`NotEmpty`). +fn leftover_directory( + held: &Quarantine, + dir: &PinnedDir, + observer: Observer<'_>, +) -> Result { + let Inside::Object(directory, _) = held.inspect(EntryKind::Directory)? else { + return Ok(Step::Unchanged(RemovalOutcome::Refused)); + }; + if !cfg!(target_os = "linux") { + return Ok(Step::Unchanged(RemovalOutcome::Refused)); + } + observer(RemovalStage::Identified { + quarantined: &held.path_inside(&dir.path), + })?; + Ok( + match unlink_verified(held, &directory, EntryKind::Directory)? { + RemovalOutcome::NotEmpty => Step::Unchanged(RemovalOutcome::NotEmpty), + outcome => unlinked(outcome, held.sync()), + }, + ) +} + /// Removes the entry of `target` from `dir` if, once moved aside, it is the object it must be. fn remove_moved( dir: &PinnedDir, @@ -424,14 +560,7 @@ impl Removal<'_> { quarantined: &inside, })?; match unlink_verified(&self.quarantine, object, self.target.kind)? { - // A directory that filled after the check goes back where it was. - RemovalOutcome::NotEmpty => { - self.restore()?; - Ok(Step::Moved( - RemovalOutcome::NotEmpty, - self.quarantine.sync(), - )) - } + RemovalOutcome::NotEmpty => self.filled(), 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 @@ -442,6 +571,23 @@ impl Removal<'_> { } } + /// The unlink was refused because the directory has entries. What is inside goes back where it was, + /// and is looked at under its name: it is the authenticated directory that filled after the check only + /// if it is that directory (the open one keeps its identity unique). If an actor swapped another + /// directory in, before it went back or while it did, the refusal was about that one and says nothing + /// about ours: the outcome is tampering, not `NotEmpty`. + fn filled(&self) -> Result { + self.restore()?; + let authenticated = + identity_at(&self.dir.handle, self.target.name) == Some(self.target.expected); + let held = self.quarantine.sync(); + Ok(if authenticated { + Step::Moved(RemovalOutcome::NotEmpty, held) + } else { + Step::Changed(RemovalOutcome::Disturbed) + }) + } + fn restore(&self) -> Result<(), PinnedError> { self.quarantine.restore(&self.dir.handle, self.target.name) } @@ -482,6 +628,7 @@ fn settle( observer: Observer<'_>, ) -> Result { match step { + Step::Unchanged(outcome) => Ok(outcome), Step::RolledBack(cause) => { sync_after_failure(dir, observer); Err(cause) @@ -507,25 +654,6 @@ fn settle( } } -/// Removing a directory follows in the next part. -pub(in crate::pinned) fn remove_dir( - _dir: &PinnedDir, - _name: &OsStr, - _observer: Observer<'_>, -) -> Result { - Err(PinnedError::Unsupported) -} - -/// Finishing a leftover quarantine directory follows in the next part. -pub(in crate::pinned) fn remove_leftover( - _dir: &PinnedDir, - _quarantine: &OsStr, - _authenticate: impl FnOnce(&mut PinnedFile) -> Result, - _observer: Observer<'_>, -) -> 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<'_>) { diff --git a/docs/native/r15/GATE4D-JOURNAL-DURABLE-EVIDENCE.md b/docs/native/r15/GATE4D-JOURNAL-DURABLE-EVIDENCE.md index b1cd3fb89..37a8a22db 100644 --- a/docs/native/r15/GATE4D-JOURNAL-DURABLE-EVIDENCE.md +++ b/docs/native/r15/GATE4D-JOURNAL-DURABLE-EVIDENCE.md @@ -1828,6 +1828,58 @@ dropping a failed sync of it, a `sync_all()?` inside the rename step, confirming 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, 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 +**Not claimed.** Directories and the leftover recovery are part 3 (below); 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 created by an unprivileged actor, which is argued, not tested. + +#### Part 3: removal of a directory on Linux, leftovers and crashes on Unix + +Status words: `H1A_2_UNIX_DIRECTORY_AND_LEFTOVERS=IMPLEMENTED_TEST_SUPPORT_ONLY` (directories: Linux only), `H1_PROVED=NO`. +**Directories are Linux only.** The removal counts only if the open directory can be asked afterwards whether it was the one that +lost its name. A hosted macOS probe of an open directory that was removed, one that was only moved and one that was untouched +showed: the link count stays 2 for all three; `F_GETPATH` keeps answering with the old path for the removed one; `openat("..")` and +`statat(".")` succeed for all three; only creating an entry in the removed one fails (`ENOENT`). The only signal is therefore a write +into a directory that, in the attack case, an actor moved away and that belongs to another path, and it would fail for lack of +permission on a read-only directory, so it is not used. Where the answer is not available an empty directory gives `Unsupported` +before anything is moved (`NotEmpty`, a link and a missing name are told the same everywhere); whether and how macOS +gets directory removal is left to the decision about the production use (H1b). `remove_empty_dir` opens the child +directory (no link followed), refuses one that has entries (`NotEmpty`) and moves it through the same quarantine directory: +identified by the open directory, then `unlinkat(AT_REMOVEDIR)`; a directory that fills after the check goes back +(`NotEmpty`), but only if what is inside is still the authenticated directory: if an actor swapped a non-empty directory in, +the refusal was about that one, it goes back under the name all the same and the outcome is `Disturbed`. The same window exists +and is detected the same way: the link count of the held directory must have dropped (`Disturbed` otherwise; for a directory +it falls to zero). An empty foreign directory that wins the race in the last window is removed instead of the authenticated +one, and the removal says so; it holds no data. + +**Leftovers and crashes.** A crash after the move, or an entry that could not be put back (`Swapped { restored: false }`, +`Stranded`), leaves a `.h1q-*` directory holding the entry as `x`. No sweep removes it by itself (a sweep skips the +`.h1q-` prefix); only an explicit call of the recovery below takes it. +`remove_leftover_file(name, authenticate)` takes it by its **generated name only** (`.h1q-` and exactly 32 lowercase +hexadecimal digits; any other name, including an unrelated private directory, is `InvalidName` and untouched), requires a +directory of the effective user without access for group or others, authenticates the file in it again and, only if +accepted, unlinks it with the same link-count check. A file that does not authenticate (a foreign object left by a refused +put-back) stays; an empty generated quarantine directory is removed. An empty directory that a crashed directory removal left as +`x` holds nothing, so it needs no authentication (the callback is not asked) and goes with the same unlink and the same check as +the removal of a directory (Linux; elsewhere it is refused); a directory with entries stays (`NotEmpty`). Removing the empty +quarantine directory changes the parent whatever the leftover removal came to (an actor can take the entry away while it is +looked at), so the parent is synced then too, for a refusal and for an error, and the result is kept. + +**Proof.** The directory cases run on Linux; on any other Unix system the same call is pinned to `Unsupported` with nothing moved. A directory swapped in the first window and one filled after the check are put back and not removed; an empty +directory that wins the last window gives `Disturbed` with the original directory intact, and so does a non-empty one (not +`NotEmpty`); a simulated crash (the observer +panics once the entry is in the quarantine directory) leaves the entry there under no name of the store, a leftover removal +with a failing authentication refuses and leaves it, and with the right one finishes it; names without the generated grammar +(`.h1q-empty`, too few or too many digits, upper case, non-hexadecimal digits, other directories, plain files) are refused +before anything is opened; a foreign object left in a quarantine directory by a refused put-back, and a link left there, are +never removed by it; an entry stranded by a blocked put-back (the name taken again, so it is proved to be in the quarantine +directory) is finished after the file authenticates again. The leftover removal syncs the quarantine directory it unlinks +in, like any removal, and says `NotConfirmed` if that fails (modelled through the same test-only seam). Mutation-checked: +syncing nothing in the leftover removal, not refusing a foreign entry, loosening the name grammar to any length, and +ignoring the authentication result, reporting `NotEmpty` for any directory, not looking at a directory again once it went back, +not recovering an empty directory leftover, and ignoring that the discard removed the quarantine directory each fail at least one +of these cases. An entry +stranded by a lost search permission is `Disturbed`, not `Stranded`: a failed look proves nothing (see part 2). + +**Not claimed.** Windows creates no quarantine directory (part 4); the Disturbed window is bounded and detected, not closed. +A foreign entry that is neither a file nor an empty directory (a link, a FIFO) stays in its `.h1q-*` directory: the recovery +refuses it (`Refused`) and no sweep touches it, so nothing removes it until a person decides.