Repository navigation
fix(cron): harden usercron writeback temp file (O_EXCL, random name) #1577
Copy link
Copy link
Open
Labels
Description
Description
Scheduler writeback (update_usercron_job) writes to a fixed-name temp file next to the usercron file and renames it over the real file:
openab/crates/openab-core/src/cron.rs
Lines 954 to 959 in 20830d5
| // Atomic write: write to temp file then rename to avoid corruption on crash. | |
| let tmp = path.with_extension("toml.tmp"); | |
| std::fs::write(&tmp, doc.to_string())?; | |
| std::fs::rename(&tmp, path)?; | |
| Ok(()) | |
| } |
std::fs::write opens with O_CREAT|O_TRUNC, without O_EXCL or O_NOFOLLOW. If anyone other than OpenAB can write to the containing directory, they can get at the file through the predictable name:
attacker pre-creates cronjob.toml.tmp -> OpenAB truncates and writes into the attacker's inode
-> rename installs it; attacker still owns cronjob.toml
attacker makes cronjob.toml.tmp a symlink -> OpenAB writes the TOML through it to any file it can write
-> rename installs the symlink as cronjob.toml
The sticky bit does not stop pre-creation. #1563 removed the docs advice that suggested it, and now says the directory must not be writable by the agent. That is the only defence today. This issue covers the code-side hardening, so the writeback stays safe even when an operator gets the directory permissions wrong.
Steps to Reproduce
- Enable usercron and set
usercron_pathto a directory that a second user can write to, for example a group-writable directory. - As the second user, run
ln -s /path/to/victim-file <dir>/cronjob.toml.tmp, or create<dir>/cronjob.toml.tmpas a regular file. - Trigger a writeback, for example a job with
disable_on_success = truethat succeeds, or a thread_id writeback. - With the symlink,
/path/to/victim-filenow holds the usercron TOML, andcronjob.tomlis a symlink. With the pre-created file,cronjob.tomlis owned by the second user.
Expected Behavior
- The writeback creates its temp file with a random name in the same directory, opened with
O_CREAT|O_EXCL, which also refuses to follow a symlink at the final path component. Then it fsyncs and renames the temp file over the target.tempfile::NamedTempFile::new_in(dir)pluspersist(path)does this, andtempfileis already a regular dependency ofopenab-core. - An existing
cronjob.toml.tmpfile or symlink is never opened or followed. - Decide whether to keep the target's existing permission bits.
NamedTempFilecreates files as0600, while today the file gets0666 & umask. An operator who relies on another user readingcronjob.tomlwould notice the change. Copying the old mode onto the temp file before the rename keeps current behavior. - Regression tests (Unix): a pre-placed
cronjob.toml.tmpsymlink and a pre-placed regular file are both left untouched, and the writeback still succeeds.
Reactions are currently unavailable
Activity