Narrow a file this server rewrites, not only one it creates

`OpenOptions::mode` applies to a file the call creates and to nothing else,
so rewriting a file that already existed kept whatever permissions it had.
Three functions above, `create_dir` has carried a comment about exactly
this hazard since it was written -- the file path never got the same
treatment.

This is not hypothetical here. `certs.rs` reissues the TLS leaf and rewrites
its **private key on every start**, so a key that ever existed
world-readable would have stayed that way for the rest of its life, with
every subsequent start looking like it was setting the mode. The config's
temp file is the other one: normally fresh, but a leftover from a crashed
save would be reused with its old mode and then renamed over the real
config, which holds the enrolled token hashes.

Set through the open handle rather than the path, deliberately:
`set_permissions` on a path re-resolves it, so between the open and the
chmod something could put a different file -- or a symlink to one -- where
this was, and the mode would land there instead. A handle cannot be
redirected.

Three tests, and the first was checked against the bug rather than only
against the fix: with the new line commented out it fails with "rewriting
left it at 644".

Found by dev-updater's session, which had taken this module for a shared
crate and read it as a unit. I had spotted the same line being wrong in
their new `append_file` and missed that `create_file` -- the one I wrote --
had it too.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017xn8nHw1tw1R6PtiY1eEtw
This commit is contained in:
irisandClaude Opus 5 committed 2026-08-28 13:24:53 -04:00
1 parent 9f54a80ca4
commit d0b6b66a44
1 file changed
+72 -4
+72 -4
View File
@@ -27,16 +27,35 @@ pub fn create_dir(dir: &Path) -> Result<()> {
.with_context(|| format!("restrict {}", dir.display()))
}
/// Opens `path` for writing, truncating it, owner-readable only from the
/// moment it exists rather than by a `chmod` afterwards.
/// Opens `path` for writing, truncating it, owner-readable only.
///
/// `mode` covers the file this call creates; [`restrict`] covers the one
/// that was already there. Both are needed, and the second is the one that
/// is easy to miss: the leaf certificate's private key is rewritten on
/// every start, so a key that ever existed with loose permissions would
/// keep them for the rest of its life.
pub fn create_file(path: &Path) -> Result<File> {
std::fs::OpenOptions::new()
let file = std::fs::OpenOptions::new()
.write(true)
.create(true)
.truncate(true)
.mode(0o600)
.open(path)
.with_context(|| format!("write {}", path.display()))
.with_context(|| format!("write {}", path.display()))?;
restrict(&file, path)?;
Ok(file)
}
/// Narrows an already-open file to owner-only.
///
/// Through the handle rather than the path, deliberately: `set_permissions`
/// on a path re-resolves it, so between opening and chmod-ing something
/// could put a different file -- or a symlink to one -- where this one was,
/// and the mode would land on that instead. The handle cannot be
/// redirected.
fn restrict(file: &File, path: &Path) -> Result<()> {
file.set_permissions(std::fs::Permissions::from_mode(0o600))
.with_context(|| format!("restrict {}", path.display()))
}
/// Writes `contents` to `path`, owner-readable only.
@@ -46,3 +65,52 @@ pub fn write_file(path: &Path, contents: &[u8]) -> Result<()> {
.write_all(contents)
.with_context(|| format!("write {}", path.display()))
}
#[cfg(test)]
mod tests {
use super::*;
#[test]
/// The case `mode` alone does not cover: a file that already exists
/// keeps whatever permissions it had, because `mode` applies only to a
/// file this call creates. `create_dir` three functions up has carried
/// a comment about exactly this since it was written; the file path
/// did not, until dev-updater's session read the module as a unit and
/// noticed the same line was wrong twice.
fn rewriting_a_file_narrows_permissions_it_did_not_set() {
let dir = tempfile::tempdir().expect("tempdir");
let path = dir.path().join("secret");
// As an older version, or a hand-edit, might have left it.
std::fs::write(&path, b"old").expect("plant");
std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).expect("loosen");
write_file(&path, b"new").expect("rewrite");
let mode = std::fs::metadata(&path).expect("stat").permissions().mode() & 0o777;
assert_eq!(mode, 0o600, "rewriting left it at {mode:o}");
}
#[test]
fn a_new_file_is_owner_only_from_the_start() {
let dir = tempfile::tempdir().expect("tempdir");
let path = dir.path().join("fresh");
write_file(&path, b"x").expect("write");
let mode = std::fs::metadata(&path).expect("stat").permissions().mode() & 0o777;
assert_eq!(mode, 0o600);
}
#[test]
fn an_existing_directory_is_narrowed_too() {
let dir = tempfile::tempdir().expect("tempdir");
let nested = dir.path().join("state");
std::fs::create_dir(&nested).expect("create");
std::fs::set_permissions(&nested, std::fs::Permissions::from_mode(0o755)).expect("loosen");
create_dir(&nested).expect("recreate");
let mode = std::fs::metadata(&nested)
.expect("stat")
.permissions()
.mode()
& 0o777;
assert_eq!(mode, 0o700, "left at {mode:o}");
}
}