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)]