diff --git a/server/src/private.rs b/server/src/private.rs index 246bac3..4ea4c0e 100644 --- a/server/src/private.rs +++ b/server/src/private.rs @@ -46,13 +46,16 @@ pub fn write_file(path: &Path, contents: &[u8]) -> Result<()> { /// Opens `path` for writing, owner-readable only, truncating what is /// there. For a caller that streams rather than holding the whole body. pub fn create_file(path: &Path) -> Result { - std::fs::OpenOptions::new() - .write(true) - .create(true) - .truncate(true) - .mode(0o600) - .open(path) - .with_context(|| format!("write {}", path.display())) + restrict( + path, + std::fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .mode(0o600) + .open(path) + .with_context(|| format!("write {}", path.display()))?, + ) } /// Opens `path` for appending, owner-readable only, creating it if needed. @@ -61,12 +64,33 @@ pub fn create_file(path: &Path) -> Result { /// truncated by being opened, and the two differ by one flag that is easy /// to get wrong in a hurry. pub fn append_file(path: &Path) -> Result { - std::fs::OpenOptions::new() - .append(true) - .create(true) - .mode(0o600) - .open(path) - .with_context(|| format!("append to {}", path.display())) + restrict( + path, + std::fs::OpenOptions::new() + .append(true) + .create(true) + .mode(0o600) + .open(path) + .with_context(|| format!("append to {}", path.display()))?, + ) +} + +/// Narrows an already-open file to owner-only. +/// +/// `OpenOptions::mode` applies **only when the file is created**, so +/// opening one that already exists silently keeps whatever mode it had -- +/// which for an append helper is the normal case rather than the odd one, +/// and for a truncating one happens on every rewrite after the first. +/// Setting it through the handle rather than the path closes the window +/// where something could swap the path between the two. +/// +/// The same reasoning as [`create_dir`]'s second call, and it was missed +/// here first: a file made wrong by an older version, or by hand, would +/// otherwise stay wrong for as long as it is only ever appended to. +fn restrict(path: &Path, file: File) -> Result { + file.set_permissions(std::fs::Permissions::from_mode(0o600)) + .with_context(|| format!("restrict {}", path.display()))?; + Ok(file) } #[cfg(test)] @@ -95,6 +119,33 @@ mod tests { ); } + /// The case `OpenOptions::mode` cannot cover, because it applies only + /// at creation: a file that already exists, made wrong earlier, and + /// opened again. An append-only transcript hits this on every write + /// after the first. + #[test] + fn an_existing_file_made_wrong_is_narrowed_on_open() { + let dir = tempfile::tempdir().expect("tempdir"); + create_dir(dir.path()).expect("create_dir"); + + for (name, open) in [ + ("appended", append_file as fn(&Path) -> Result), + ("rewritten", create_file as fn(&Path) -> Result), + ] { + let path = dir.path().join(name); + std::fs::write(&path, b"made by an older version").expect("write"); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).expect("chmod"); + assert_eq!(mode_of(&path), 0o644, "precondition"); + + open(&path).expect("open"); + assert_eq!( + mode_of(&path), + 0o600, + "{name} kept a mode somebody else could read" + ); + } + } + #[test] fn files_are_owner_only_from_the_moment_they_exist() { let dir = tempfile::tempdir().expect("tempdir");