diff --git a/swarmctl/src/main.rs b/swarmctl/src/main.rs index c4a9c914..2d46a83d 100644 --- a/swarmctl/src/main.rs +++ b/swarmctl/src/main.rs @@ -35,8 +35,8 @@ mod schedule; mod users; use std::fs::{self, File, Permissions}; -use std::io::Write as _; -use std::os::unix::fs::{MetadataExt as _, PermissionsExt as _}; +use std::io::{self, Write as _}; +use std::os::unix::fs::{MetadataExt as _, OpenOptionsExt as _, PermissionsExt as _}; use std::path::{Path, PathBuf}; use std::process::Command; @@ -1003,7 +1003,6 @@ fn write_atomic(path: &Path, contents: &str) -> Result<()> { let name = path .file_name() .with_context(|| format!("{} has no file name", path.display()))?; - let tmp = dir.join(format!(".{}.swarmctl.tmp", name.to_string_lossy())); let existing = fs::metadata(path).ok(); // 0600 only when the file does not exist yet: this content is @@ -1013,21 +1012,25 @@ fn write_atomic(path: &Path, contents: &str) -> Result<()> { .as_ref() .map_or(0o600, |meta| meta.permissions().mode() & 0o7777); - let mut file = File::create(&tmp).with_context(|| format!("creating {}", tmp.display()))?; - file.write_all(contents.as_bytes()) - .with_context(|| format!("writing {}", tmp.display()))?; - file.sync_all() - .with_context(|| format!("flushing {}", tmp.display()))?; - drop(file); - - fs::set_permissions(&tmp, Permissions::from_mode(mode)) - .with_context(|| format!("setting mode on {}", tmp.display()))?; - if let Some(meta) = existing.as_ref() { - std::os::unix::fs::chown(&tmp, Some(meta.uid()), Some(meta.gid())) - .with_context(|| format!("setting owner on {}", tmp.display()))?; + let (mut file, tmp) = create_temp(dir, &name.to_string_lossy())?; + let written = (|| -> Result<()> { + file.write_all(contents.as_bytes()) + .with_context(|| format!("writing {}", tmp.display()))?; + // Owner before mode: chown(2) clears setuid/setgid bits. + if let Some(meta) = existing.as_ref() { + std::os::unix::fs::fchown(&file, Some(meta.uid()), Some(meta.gid())) + .with_context(|| format!("setting owner on {}", tmp.display()))?; + } + file.set_permissions(Permissions::from_mode(mode)) + .with_context(|| format!("setting mode on {}", tmp.display()))?; + file.sync_all() + .with_context(|| format!("flushing {}", tmp.display()))?; + fs::rename(&tmp, path).with_context(|| format!("renaming {} into place", tmp.display())) + })(); + if written.is_err() { + let _ = fs::remove_file(&tmp); } - - fs::rename(&tmp, path).with_context(|| format!("renaming {} into place", tmp.display()))?; + written?; // The rename itself is metadata: without this the file can survive a // crash while the directory entry pointing at it does not. @@ -1037,6 +1040,41 @@ fn write_atomic(path: &Path, contents: &str) -> Result<()> { Ok(()) } +/// Creates `...swarmctl.tmp` in `dir`, mode 0600. +/// +/// `dir` can be writable by a less-privileged uid (authelia's state +/// directory), so the name must be unguessable and the open must not +/// follow a link planted there: see [`open_new_nofollow`]. +fn create_temp(dir: &Path, name: &str) -> Result<(File, PathBuf)> { + for _ in 0..8 { + let mut raw = [0u8; 8]; + getrandom::fill(&mut raw) + .map_err(|e| anyhow::anyhow!("drawing randomness from the kernel: {e}"))?; + let suffix = format!("{:016x}", u64::from_le_bytes(raw)); + let tmp = dir.join(format!(".{name}.{suffix}.swarmctl.tmp")); + match open_new_nofollow(&tmp) { + Ok(file) => return Ok((file, tmp)), + Err(e) if e.kind() == io::ErrorKind::AlreadyExists => {} + Err(e) => return Err(e).with_context(|| format!("creating {}", tmp.display())), + } + } + bail!("no unused temp name for {name} in {}", dir.display()) +} + +/// Opens `tmp` for writing, mode 0600, failing if anything exists there. +/// +/// `O_EXCL` fails the open on any existing entry, a symlink included, and +/// `O_NOFOLLOW` fails it on a symlink in the final component regardless; +/// neither truncates or touches the link's target. +fn open_new_nofollow(tmp: &Path) -> io::Result { + fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .custom_flags(rustix::fs::OFlags::NOFOLLOW.bits().cast_signed()) + .open(tmp) +} + #[cfg(test)] mod tests { use super::*; @@ -1406,6 +1444,108 @@ mod tests { fs::remove_dir_all(&dir).ok(); } + fn tmp_entries(dir: &Path) -> Vec { + let mut names: Vec = fs::read_dir(dir) + .expect("read dir") + .map(|e| e.expect("entry").file_name().to_string_lossy().into_owned()) + .filter(|n| n.ends_with(".swarmctl.tmp")) + .collect(); + names.sort(); + names + } + + #[test] + fn write_atomic_ignores_a_symlink_planted_at_the_old_fixed_temp_name() { + let dir = std::env::temp_dir().join(format!("swarmctl-plant-{}", std::process::id())); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(&dir).expect("temp dir"); + let path = dir.join("users.yml"); + fs::write(&path, "users: {}\n").expect("seed"); + fs::set_permissions(&path, Permissions::from_mode(0o640)).expect("chmod"); + + let sentinel = dir.join("sentinel"); + fs::write(&sentinel, "keep me\n").expect("sentinel"); + fs::set_permissions(&sentinel, Permissions::from_mode(0o644)).expect("chmod"); + let planted = dir.join(".users.yml.swarmctl.tmp"); + std::os::unix::fs::symlink(&sentinel, &planted).expect("plant"); + + write_atomic(&path, "users:\n mara:\n").expect("rewrite"); + + assert_eq!(fs::read_to_string(&sentinel).expect("read"), "keep me\n"); + let mode = fs::metadata(&sentinel).expect("stat").permissions().mode() & 0o7777; + assert_eq!(mode, 0o644, "the link's target must keep its mode"); + assert!( + fs::symlink_metadata(&planted) + .expect("stat link") + .is_symlink(), + "the planted link is not ours to remove or rename" + ); + assert_eq!( + fs::read_to_string(&path).expect("read"), + "users:\n mara:\n" + ); + let mode = fs::metadata(&path).expect("stat").permissions().mode() & 0o7777; + assert_eq!(mode, 0o640); + + fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn the_temp_open_refuses_a_planted_link_and_leaves_its_target_alone() { + let dir = std::env::temp_dir().join(format!("swarmctl-excl-{}", std::process::id())); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(&dir).expect("temp dir"); + + let sentinel = dir.join("sentinel"); + fs::write(&sentinel, "keep me\n").expect("sentinel"); + let to_file = dir.join("to-file"); + std::os::unix::fs::symlink(&sentinel, &to_file).expect("plant"); + let dangling = dir.join("dangling"); + std::os::unix::fs::symlink(dir.join("absent"), &dangling).expect("plant"); + + for planted in [&to_file, &dangling] { + let err = open_new_nofollow(planted).expect_err("must refuse a link"); + assert_eq!(err.kind(), io::ErrorKind::AlreadyExists); + } + assert_eq!(fs::read_to_string(&sentinel).expect("read"), "keep me\n"); + assert!( + !dir.join("absent").exists(), + "a dangling link is not created through" + ); + + // Control: the same call on a free name succeeds, so the refusals + // above are the link's doing and not a broken open. + let mut fresh = open_new_nofollow(&dir.join("fresh")).expect("free name opens"); + fresh.write_all(b"ok").expect("write"); + let mode = fs::metadata(dir.join("fresh")) + .expect("stat") + .permissions() + .mode() + & 0o7777; + assert_eq!(mode, 0o600); + + fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn write_atomic_removes_its_temp_file_when_the_rename_fails() { + let dir = std::env::temp_dir().join(format!("swarmctl-cleanup-{}", std::process::id())); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(&dir).expect("temp dir"); + // A non-empty directory at the target makes rename(2) fail. + let path = dir.join("users.yml"); + fs::create_dir_all(path.join("inner")).expect("blocker"); + + write_atomic(&path, "x\n").expect_err("rename over a directory fails"); + assert_eq!(tmp_entries(&dir), Vec::::new()); + + // Control: the helper does see a stray temp file. + fs::write(dir.join(".stray.swarmctl.tmp"), "").expect("stray"); + assert_eq!(tmp_entries(&dir), [".stray.swarmctl.tmp"]); + + fs::remove_dir_all(&dir).ok(); + } + /// Replaces `load_store_refuses_to_take_over_a_populated_users_file`, /// which pinned the guard this change removes. ///