Skip to content

fix(cron): harden usercron writeback temp file (O_EXCL, random name) #1577

Open

Description

@thepagent

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:

// 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.

Follow-up to #1562 / #1563.

Steps to Reproduce

  1. Enable usercron and set usercron_path to a directory that a second user can write to, for example a group-writable directory.
  2. As the second user, run ln -s /path/to/victim-file <dir>/cronjob.toml.tmp, or create <dir>/cronjob.toml.tmp as a regular file.
  3. Trigger a writeback, for example a job with disable_on_success = true that succeeds, or a thread_id writeback.
  4. With the symlink, /path/to/victim-file now holds the usercron TOML, and cronjob.toml is a symlink. With the pre-created file, cronjob.toml is 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) plus persist(path) does this, and tempfile is already a regular dependency of openab-core.
  • An existing cronjob.toml.tmp file or symlink is never opened or followed.
  • Decide whether to keep the target's existing permission bits. NamedTempFile creates files as 0600, while today the file gets 0666 & umask. An operator who relies on another user reading cronjob.toml would 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.tmp symlink and a pre-placed regular file are both left untouched, and the writeback still succeeds.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions