fix: verify the state directory before use, keep fstab copies at the original mode [P1]
1. The state directory was created too late and never verified. ensure_state_dir only ran when the ownership record was written, well after assert_swap_path_is_safe had already required every parent of swap.path to exist. On a clean host that meant a perfectly legal /var/lib/awg2-harness/swapfile was rejected because the directory this package is supposed to create did not exist yet. Worse, when the directory did exist the check only confirmed it was a directory. A pre-existing /var/lib/awg2-harness that was group- or world-writable defeated the whole point of restricting swap.path to root-owned roots: another user could still swap the swapfile out from under a root-owned fallocate or mkswap. ensure_state_dir now runs first in ensure_swap and asserts what the restriction actually depends on: the parent is a real directory and not a symlink, the state directory itself is not a symlink, it is owned by uid 0, and it is not writable by group or others. A mode that is merely narrower than 0700 is tightened rather than rejected; anything group- or world-writable is a hard error, because the safety argument for the whole allow-list rests on it. The units also declare StateDirectory=awg2-harness with mode 0700 and RuntimeDirectory=awg2-harness, so in the common case systemd creates and owns both and the binary only has to verify. The runtime checks stay regardless: the CLI subcommands run outside the service lifecycle. 2. The fstab copies could be more permissive than the file they mirror. The backup and pristine copies were written at a hardcoded 0644 with a comment claiming they must not be wider than the original. On a host where /etc/fstab is 0600 they were, and the rollback then handed that wider mode to the live file too. FstabTransaction now captures the live mode when it opens and uses it for the backup, the pristine copy, the candidate and the rollback. One test added, unix-only since it asserts a mode: a 0600 fstab produces 0600 copies and stays 0600 after the commit. cargo test --workspace: 180 + 4 passed. clippy -D warnings clean. Unit files not verified here - no systemd on this host; tools/build.sh runs systemd-analyze verify on the build machine.
This commit is contained in:
@@ -174,7 +174,10 @@ pub fn ensure_swap(cfg: &Config) -> anyhow::Result<()> {
|
||||
return remove_managed_swap_state();
|
||||
}
|
||||
|
||||
// 1. Preflight: nothing is modified here.
|
||||
// 1. Preflight. The state directory is created and verified first: on a clean host
|
||||
// /var/lib/awg2-harness does not exist yet, and the path check below requires every
|
||||
// parent to be present.
|
||||
ensure_state_dir()?;
|
||||
let desired = DesiredSwap::from_config(cfg)?;
|
||||
assert_swap_path_is_safe(&desired.path)?;
|
||||
let active_before = read_active_swaps()?;
|
||||
@@ -453,16 +456,80 @@ fn record_ownership(path: &Path) -> anyhow::Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn ensure_state_dir() -> anyhow::Result<()> {
|
||||
/// Creates the state directory if needed and asserts it is safe to keep state in.
|
||||
///
|
||||
/// Run before `swap.path` is validated, for two reasons. On a clean host
|
||||
/// `/var/lib/awg2-harness` does not exist yet, so a perfectly legal
|
||||
/// `/var/lib/awg2-harness/swapfile` would be rejected by the parent-chain check for a
|
||||
/// directory this package is supposed to create. And an existing directory has to be
|
||||
/// verified, not just assumed: if it is group- or world-writable, the whole point of
|
||||
/// restricting `swap.path` to root-owned roots is gone, because another user could swap
|
||||
/// the swapfile out from under a root-owned operation.
|
||||
pub fn ensure_state_dir() -> anyhow::Result<()> {
|
||||
let dir = Path::new(STATE_DIR);
|
||||
|
||||
// The parent must exist and must not be a symlink, or the directory we create could
|
||||
// land somewhere else entirely.
|
||||
if let Some(parent) = dir.parent() {
|
||||
match fs::symlink_metadata(parent) {
|
||||
Ok(md) if md.file_type().is_symlink() => {
|
||||
anyhow::bail!("{} is a symlink; refusing to use it", parent.display())
|
||||
}
|
||||
Ok(md) if !md.file_type().is_dir() => {
|
||||
anyhow::bail!("{} is not a directory", parent.display())
|
||||
}
|
||||
Ok(_) => {}
|
||||
Err(e) => anyhow::bail!("failed to stat {}: {e}", parent.display()),
|
||||
}
|
||||
}
|
||||
|
||||
match fs::symlink_metadata(dir) {
|
||||
Ok(md) if md.file_type().is_dir() => return Ok(()),
|
||||
Ok(_) => anyhow::bail!("{STATE_DIR} exists and is not a directory"),
|
||||
Ok(md) if md.file_type().is_symlink() => {
|
||||
anyhow::bail!("{STATE_DIR} is a symlink; refusing to use it")
|
||||
}
|
||||
Ok(md) if !md.file_type().is_dir() => {
|
||||
anyhow::bail!("{STATE_DIR} exists and is not a directory")
|
||||
}
|
||||
Ok(md) => return assert_state_dir_is_private(dir, &md),
|
||||
Err(e) if e.kind() == std::io::ErrorKind::NotFound => {}
|
||||
Err(e) => anyhow::bail!("failed to stat {STATE_DIR}: {e}"),
|
||||
}
|
||||
fs::create_dir_all(dir).map_err(|e| anyhow::anyhow!("failed to create {STATE_DIR}: {e}"))?;
|
||||
set_dir_mode(dir)
|
||||
|
||||
fs::create_dir(dir).map_err(|e| anyhow::anyhow!("failed to create {STATE_DIR}: {e}"))?;
|
||||
set_dir_mode(dir)?;
|
||||
let md = fs::symlink_metadata(dir)
|
||||
.map_err(|e| anyhow::anyhow!("failed to stat {STATE_DIR} after create: {e}"))?;
|
||||
assert_state_dir_is_private(dir, &md)
|
||||
}
|
||||
|
||||
/// Refuses a state directory anyone but root can write into.
|
||||
#[cfg(unix)]
|
||||
fn assert_state_dir_is_private(dir: &Path, md: &fs::Metadata) -> anyhow::Result<()> {
|
||||
use std::os::unix::fs::MetadataExt;
|
||||
|
||||
if md.uid() != 0 {
|
||||
anyhow::bail!(
|
||||
"{} must be owned by root, got uid {}",
|
||||
dir.display(),
|
||||
md.uid()
|
||||
);
|
||||
}
|
||||
let mode = md.permissions().mode() & 0o777;
|
||||
if mode & 0o022 != 0 {
|
||||
anyhow::bail!(
|
||||
"{} is writable by group or others (mode {mode:04o}); refusing to keep state there",
|
||||
dir.display()
|
||||
);
|
||||
}
|
||||
if mode != 0o700 {
|
||||
set_dir_mode(dir)?;
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(not(unix))]
|
||||
fn assert_state_dir_is_private(_dir: &Path, _md: &fs::Metadata) -> anyhow::Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
|
||||
Reference in New Issue
Block a user