prod/AWG2_harness
0
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 2 Wiki Activity

fix(fs): refuse to write through symlinks, make fstab validation mandatory [P0/P1]

Browse Source 
Three defects from the follow-up audit, all in code that runs as root.

1. The fstab backup was an arbitrary-file-overwrite primitive.

   write_file_synced used fs::File::create, which follows symlinks. A symlink planted at
   /etc/fstab.awg2-harness.bak would be followed and its target truncated and overwritten
   with the contents of /etc/fstab. Same class of bug as the sidecar write in swap.rs,
   which the next commit removes entirely.

   New module atomic.rs owns the write path for every file this package installs under
   /etc and /var/lib: stat the destination with symlink_metadata, refuse anything that is
   not a regular file or absent, create a sibling temp with create_new so a symlink at the
   temp path cannot be followed either, write, sync_all, set the mode, rename, flush the
   directory. fstab.rs and managed_file.rs both use it, which also removes the two
   near-identical write_atomic implementations they each carried.

2. Directory fsync failures were swallowed.

   Both modules document crash-durable writes, then ignored the result of syncing the
   parent directory - the step that makes the rename survive a crash. sync_parent_dir now
   returns Result and callers propagate it.

3. findmnt validation was optional in practice.

   verify_candidate had `Err(_) => Ok(())`, so a host without util-linux silently skipped
   the only structural check on a file that decides whether the machine boots. The package
   already depends on util-linux, so this is now a hard error, and findmnt joins the
   required host tools when swap.enabled is true - failing preflight rather than at rewrite
   time.

   To keep the transaction testable off Linux, commit takes the validator through an
   injected fn pointer; production always passes the real findmnt check. That seam also
   made it possible to test the rejection path directly, which was previously unreachable
   in tests.

Also in this commit, since they are the same class of defect:

- fstab rollback used fs::copy straight onto /etc/fstab, which can leave it truncated if
  it fails midway. It now writes the original - held in memory since begin, so the
  recovery path does not depend on re-reading the backup - through the same atomic
  no-follow path as a forward write.
- the pristine .original copy is checked to be a regular file when it already exists; a
  symlink or directory there means the recovery copy cannot be trusted, so the commit
  refuses instead of proceeding.
- ManagedFile decided ownership with `first.contains(marker)`, so a file whose first line
  read "# NOT Managed by awg2-harness sysctl_tuning" was treated as ours and became
  eligible for overwrite and removal. Now an exact match on the trimmed first line.
- systemctl daemon-reload after a successful fstab commit, since /etc/fstab feeds
  systemd's generators and not only mount. Best-effort with a warning: the change is
  already committed and verified, and failing the operation over a missed notification
  would be worse than a stale generator state the next boot resolves.

Thirteen tests added: symlinked destination refused with the victim asserted intact,
symlink at the temp path not followed, directory destination refused, mode applied, stale
temp cleared, rejected candidate leaves the target untouched, non-regular pristine copy
refused, symlinked backup path refused with the victim intact, exact marker matching
against three near-miss first lines, and trailing whitespace tolerated on our own marker.

cargo test --workspace: 171 + 4 passed. clippy -D warnings clean.
This commit is contained in:
Crimson
2026-08-07 04:04:01 +05:00
parent 6643328567
commit c2cf194a68
5 changed files with 469 additions and 143 deletions
Download Patch File Download Diff File Expand all files Collapse all files
traffic_core/src/managed_file.rs
+42 -73
View File
@@ -12,14 +12,12 @@
//! - a file whose first line does not carry our marker is never written or removed;
//! - writes are atomic: temp sibling, `sync_all`, rename, directory flush.
use std::{
fs,
io::{ErrorKind, Write},
path::{Path, PathBuf},
};
use std::{fs, io::ErrorKind, path::PathBuf};
#[cfg(unix)]
use std::os::unix::fs::PermissionsExt;
use crate::atomic;
/// Debian default for files under `/etc/sysctl.d` and `/etc/modules-load.d`.
const MANAGED_MODE: u32 = 0o644;
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum Ownership {
@@ -54,12 +52,17 @@ impl ManagedFile {
}
}
/// Ownership is decided by an **exact** first line, not a substring.
///
/// `contains` accepted a file whose first line merely embedded the marker, so
/// `# NOT Managed by awg2-harness sysctl_tuning` counted as ours and became eligible
/// for overwrite and removal.
pub fn ownership(&self) -> anyhow::Result<Ownership> {
let Some(content) = self.read()? else {
return Ok(Ownership::Absent);
};
let first = content.lines().next().unwrap_or_default();
if first.contains(&self.marker) {
if first.trim_end() == self.marker.trim_end() {
Ok(Ownership::Managed)
} else {
Ok(Ownership::Foreign)
@@ -86,7 +89,7 @@ impl ManagedFile {
}
Ownership::Absent => {}
}
write_atomic(&self.path, content)?;
atomic::write_atomic_no_follow(&self.path, content, MANAGED_MODE)?;
Ok(true)
}
@@ -109,73 +112,10 @@ impl ManagedFile {
}
}
fn write_atomic(path: &Path, content: &str) -> anyhow::Result<()> {
let mut temp_name = path
.file_name()
.ok_or_else(|| anyhow::anyhow!("invalid managed file path: {}", path.display()))?
.to_os_string();
temp_name.push(".awg2-harness.tmp");
let temp = path.with_file_name(temp_name);
match fs::remove_file(&temp) {
Ok(()) => {}
Err(e) if e.kind() == ErrorKind::NotFound => {}
Err(e) => {
return Err(anyhow::anyhow!(
"failed to clear stale {}: {e}",
temp.display()
));
}
}
let mut f = fs::OpenOptions::new()
.create_new(true)
.write(true)
.open(&temp)
.map_err(|e| anyhow::anyhow!("failed to create {}: {e}", temp.display()))?;
let write_result = f
.write_all(content.as_bytes())
.and_then(|()| f.sync_all())
.map_err(|e| anyhow::anyhow!("failed to write {}: {e}", temp.display()));
drop(f);
if let Err(e) = write_result {
let _ = fs::remove_file(&temp);
return Err(e);
}
#[cfg(unix)]
if let Err(e) = fs::set_permissions(&temp, fs::Permissions::from_mode(0o644)) {
let _ = fs::remove_file(&temp);
return Err(anyhow::anyhow!(
"failed to set permissions on {}: {e}",
temp.display()
));
}
fs::rename(&temp, path).map_err(|e| {
let _ = fs::remove_file(&temp);
anyhow::anyhow!("failed to install {}: {e}", path.display())
})?;
sync_parent_dir(path);
Ok(())
}
#[cfg(unix)]
fn sync_parent_dir(target: &Path) {
if let Some(dir) = target.parent()
&& let Ok(f) = fs::File::open(dir)
{
let _ = f.sync_all();
}
}
#[cfg(not(unix))]
fn sync_parent_dir(_target: &Path) {}
#[cfg(test)]
mod tests {
use super::*;
use std::path::Path;
const MARKER: &str = "# Managed by awg2-harness test";
@@ -270,4 +210,33 @@ mod tests {
assert!(f.write(&format!("{MARKER}\nkey = 1\n")).expect("write"));
assert!(!dir.join("conf.awg2-harness.tmp").exists());
}
/// `contains` used to accept any first line that embedded the marker, so a file
/// explicitly declaring it is *not* managed became eligible for overwrite.
#[test]
fn marker_must_match_the_whole_first_line() {
let dir = scratch("marker_exact");
let f = managed(&dir);
for first_line in [
format!("# NOT {}", MARKER.trim_start_matches("# ")),
format!("{MARKER} but actually hand written"),
format!("prefix {MARKER}"),
] {
fs::write(dir.join("conf"), format!("{first_line}\nkey = 1\n")).expect("seed");
assert_eq!(
f.ownership().expect("ownership"),
Ownership::Foreign,
"must not claim ownership of first line {first_line:?}"
);
}
}
/// Trailing whitespace on the marker line is not a reason to disown our own file.
#[test]
fn marker_comparison_ignores_trailing_whitespace() {
let dir = scratch("marker_trailing");
let f = managed(&dir);
fs::write(dir.join("conf"), format!("{MARKER} \nkey = 1\n")).expect("seed");
assert_eq!(f.ownership().expect("ownership"), Ownership::Managed);
}
}