From fa44ffbb246f44c2ee590cad66ff07854e00ad69 Mon Sep 17 00:00:00 2001 From: Crimson Date: Fri, 7 Aug 2026 04:44:35 +0500 Subject: [PATCH] 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. --- awg2-harness.service | 7 +++ awg2-reboot-scheduler.service | 4 ++ traffic_core/src/fstab.rs | 90 ++++++++++++++++++++++++++++------- traffic_core/src/swap.rs | 79 +++++++++++++++++++++++++++--- 4 files changed, 156 insertions(+), 24 deletions(-) diff --git a/awg2-harness.service b/awg2-harness.service index b5847f4..0dc7299 100644 --- a/awg2-harness.service +++ b/awg2-harness.service @@ -18,6 +18,13 @@ User=root # sequence, where it shares one error contract, one lock and one log stream. Running it # twice per start only duplicated the work and split the failure path across two units. ExecStart=/usr/sbin/awg2-harness --config /etc/awg2-harness/config.toml daemon +# systemd creates and owns these, so the common case needs no directory handling in the +# binary. The runtime checks stay regardless: the CLI subcommands run outside this unit. +RuntimeDirectory=awg2-harness +RuntimeDirectoryMode=0755 +RuntimeDirectoryPreserve=yes +StateDirectory=awg2-harness +StateDirectoryMode=0700 # on-failure, not always: a clean exit is deliberate and must not be restarted. Restart=on-failure # Stepped backoff 10 -> 20 -> 40 -> 80 -> 160s. A tight restart loop against a dependency diff --git a/awg2-reboot-scheduler.service b/awg2-reboot-scheduler.service index d3a23a3..b1d8a79 100644 --- a/awg2-reboot-scheduler.service +++ b/awg2-reboot-scheduler.service @@ -9,6 +9,10 @@ StartLimitBurst=10 Type=exec User=root ExecStart=/usr/sbin/awg2-reboot-scheduler --config /etc/awg2-harness/config.toml +# Shared with awg2-harness.service; the plan file lives here. +RuntimeDirectory=awg2-harness +RuntimeDirectoryMode=0755 +RuntimeDirectoryPreserve=yes # The scheduler exits 0 in two normal cases: reboot.enabled = false, and after systemd has # accepted the reboot request. Neither is a failure, so on-failure is required here - # Restart=always would respawn it in a loop on a host with reboots disabled. diff --git a/traffic_core/src/fstab.rs b/traffic_core/src/fstab.rs index 0c8fc75..7702879 100644 --- a/traffic_core/src/fstab.rs +++ b/traffic_core/src/fstab.rs @@ -21,9 +21,22 @@ use std::{ use crate::atomic; pub const FSTAB_PATH: &str = "/etc/fstab"; -/// Matches the Debian default for `/etc/fstab`; used for the backup and pristine copies, -/// which must not be more permissive than the file they mirror. -const FSTAB_MODE: u32 = 0o644; +/// Used only when the live file's mode cannot be read, which on Linux means it is about +/// to fail for other reasons anyway. +const FSTAB_FALLBACK_MODE: u32 = 0o644; + +#[cfg(unix)] +fn mode_of(path: &Path) -> anyhow::Result { + use std::os::unix::fs::PermissionsExt; + let md = fs::metadata(path) + .map_err(|e| anyhow::anyhow!("failed to stat {}: {e}", path.display()))?; + Ok(md.permissions().mode() & 0o777) +} + +#[cfg(not(unix))] +fn mode_of(_path: &Path) -> anyhow::Result { + Ok(FSTAB_FALLBACK_MODE) +} /// Rolling backup: the state immediately before the most recent rewrite. const BACKUP_SUFFIX: &str = ".awg2-harness.bak"; @@ -72,14 +85,23 @@ type CandidateVerifier = fn(&Path) -> anyhow::Result<()>; pub struct FstabTransaction { target: PathBuf, original: String, + /// Mode of the live file when the transaction opened. + /// + /// The copies and the replacement inherit it instead of a hardcoded 0644. On a host + /// where `/etc/fstab` is 0600, writing the backups at 0644 would publish the mount + /// table more widely than the administrator chose - and the rollback would then hand + /// that wider mode to the live file too. + original_mode: u32, } impl FstabTransaction { pub fn begin(target: &Path) -> anyhow::Result { let original = read_required(target)?; + let original_mode = mode_of(target)?; Ok(Self { target: target.to_path_buf(), original, + original_mode, }) } @@ -162,7 +184,7 @@ impl FstabTransaction { fn write_backups(&self) -> anyhow::Result<()> { let backup = sibling_path(&self.target, BACKUP_SUFFIX)?; - atomic::write_atomic_no_follow(&backup, &self.original, FSTAB_MODE) + atomic::write_atomic_no_follow(&backup, &self.original, self.original_mode) .map_err(|e| anyhow::anyhow!("failed to write backup {}: {e}", backup.display()))?; let original = sibling_path(&self.target, ORIGINAL_SUFFIX)?; @@ -185,7 +207,7 @@ impl FstabTransaction { )); } } - atomic::write_atomic_no_follow(&original, &self.original, FSTAB_MODE).map_err(|e| { + atomic::write_atomic_no_follow(&original, &self.original, self.original_mode).map_err(|e| { anyhow::anyhow!("failed to write pristine copy {}: {e}", original.display()) }) } @@ -215,7 +237,7 @@ impl FstabTransaction { .map_err(|e| anyhow::anyhow!("failed to flush {}: {e}", temp.display()))?; drop(f); - copy_permissions(&self.target, temp)?; + set_mode(temp, self.original_mode)?; Ok(()) } @@ -225,12 +247,14 @@ impl FstabTransaction { /// re-reading the backup would add a failure mode to the recovery path, and a plain /// `fs::copy` onto `/etc/fstab` could leave it truncated if it failed midway. fn restore_original(&self) -> anyhow::Result<()> { - atomic::write_atomic_no_follow(&self.target, &self.original, FSTAB_MODE).map_err(|e| { - anyhow::anyhow!( - "rollback failed: could not restore {}: {e}", - self.target.display() - ) - }) + atomic::write_atomic_no_follow(&self.target, &self.original, self.original_mode).map_err( + |e| { + anyhow::anyhow!( + "rollback failed: could not restore {}: {e}", + self.target.display() + ) + }, + ) } } @@ -289,15 +313,14 @@ fn reload_systemd_generators() { } #[cfg(unix)] -fn copy_permissions(from: &Path, to: &Path) -> anyhow::Result<()> { - let md = fs::metadata(from) - .map_err(|e| anyhow::anyhow!("failed to stat {}: {e}", from.display()))?; - fs::set_permissions(to, md.permissions()) - .map_err(|e| anyhow::anyhow!("failed to set permissions on {}: {e}", to.display())) +fn set_mode(path: &Path, mode: u32) -> anyhow::Result<()> { + use std::os::unix::fs::PermissionsExt; + fs::set_permissions(path, fs::Permissions::from_mode(mode)) + .map_err(|e| anyhow::anyhow!("failed to chmod {mode:o} {}: {e}", path.display())) } #[cfg(not(unix))] -fn copy_permissions(_from: &Path, _to: &Path) -> anyhow::Result<()> { +fn set_mode(_path: &Path, _mode: u32) -> anyhow::Result<()> { Ok(()) } @@ -511,6 +534,37 @@ mod tests { assert_eq!(fs::read_to_string(&target).expect("read"), SAMPLE); } + /// The backup and the pristine copy inherit the live file's mode. Writing them at a + /// hardcoded 0644 would publish a 0600 mount table more widely than the administrator + /// chose, and the rollback would then hand that wider mode to the live file. + #[cfg(unix)] + #[test] + fn copies_inherit_the_original_mode() { + use std::os::unix::fs::PermissionsExt; + + let dir = scratch("mode_inherit"); + let target = seed(&dir, SAMPLE); + fs::set_permissions(&target, fs::Permissions::from_mode(0o600)).expect("chmod"); + + let tx = FstabTransaction::begin(&target).expect("begin"); + tx.commit_with( + &format!("{SAMPLE}/swapfile none swap sw,pri=1 0 0\n"), + accept_any, + ) + .expect("commit"); + + for suffix in [BACKUP_SUFFIX, ORIGINAL_SUFFIX] { + let mode = fs::metadata(dir.join(format!("fstab{suffix}"))) + .expect("stat") + .permissions() + .mode() + & 0o777; + assert_eq!(mode, 0o600, "copy {suffix} must not widen the mode"); + } + let live = fs::metadata(&target).expect("stat").permissions().mode() & 0o777; + assert_eq!(live, 0o600); + } + #[test] fn escapes_fstab_field_separators() { assert_eq!(escape_field("/var/my swap"), "/var/my\\040swap"); diff --git a/traffic_core/src/swap.rs b/traffic_core/src/swap.rs index c040c56..35b702a 100644 --- a/traffic_core/src/swap.rs +++ b/traffic_core/src/swap.rs @@ -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)]