Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
21 changes: 15 additions & 6 deletions crates/worldscript-secure-storage/src/pinned/removal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<OsStr>) -> Result<RemovalOutcome, PinnedError> {
self.remove_empty_dir_observed(name, &mut |_| Ok(()))
}
Expand All @@ -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<OsStr>,
Expand Down
4 changes: 4 additions & 0 deletions crates/worldscript-secure-storage/src/pinned/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
48 changes: 48 additions & 0 deletions crates/worldscript-secure-storage/src/pinned/tests/remove.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
@@ -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())
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down Expand Up @@ -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()
}

Expand Down
Loading
Loading