boxmaker/docs/plans/M4a/16-gatewayd-secret-file-race.md

M4a task 16: read a secret file from the file that was checked

Branch: m4a (run git switch m4a; git status --short must be empty, otherwise stop) Commit subject: gatewayd: read a secret file from the file that was checked

Goal

M4a review, finding 2. load checks a secret file by its path (check_file: not a symbolic link, a regular file, owned by us, mode 0600 or 0400), then reads it by the path again (std::fs::read(path)). Between the two, someone who can write to the directory could replace the file with a link to another file, or with another file, and gatewayd would read that instead.

The fix, without any new dependency: check the path, open it, then check the opened file, and read from that same handle.

1. Check the path, as now: absolute; symlink_metadata (it does not follow links); not a link; a regular file. Keep that metadata as named.
2. Call between() (see below).
3. Open the path (std::fs::File::open).
4. Take the open file's metadata (file.metadata(), as opened). If its device and inode are not the ones in named, the path changed between the check and the open: refuse. Then the owner and mode checks, on opened.
5. Read the whole file from that handle into a Zeroizing<Vec<u8>>.

between exists only for the tests: it runs after step 1 and before step 3, and each test swaps the file there. load passes &|| {}.

Files

• Copy: crates/gatewayd/tests/secrets_race.rs
• Modify: crates/gatewayd/src/secrets.rs, docs/implementer-log.md

Before anything else

Your first two actions are steps 1 and 2 below: copy the test, and see it fail. Everything you need is in this file; read only crates/gatewayd/src/secrets.rs besides it.

The changes, exactly

1. Replace fn check_file with pub fn read_checked. Delete check_file and write in its place:

/// The bytes of an owner-only regular file, read from the very file that was checked. `between`
/// runs after the path is checked and before it is opened: `load` passes `&|| {}`; tests use it
/// to swap the file.
pub fn read_checked(path: &Path, between: &dyn Fn()) -> Result<Zeroizing<Vec<u8>>, String> {
    // 1. `path.is_absolute()`, else "<path> is not an absolute path".
    // 2. `let named = std::fs::symlink_metadata(path)`, an Err(e) is "cannot read <path>: <e>";
    //    `named.file_type().is_symlink()` is "<path> is a symbolic link";
    //    `!named.file_type().is_file()` is "<path> is not a regular file".
    // 3. `between();`
    // 4. `let mut file = std::fs::File::open(path)`, an Err(e) is "cannot read <path>: <e>";
    //    `let opened = file.metadata()`, the same error text.
    // 5. `if (opened.dev(), opened.ino()) != (named.dev(), named.ino())`:
    //    "<path> changed while it was read".
    // 6. The owner and mode checks from the old `check_file`, word for word, on `opened` (not on
    //    `named`): the uid of "/proc/self"; "<path> is not owned by the user gatewayd runs as";
    //    "<path> has mode <mode:03o>; only the owner may read it (0600 or 0400)".
    // 7. `let mut bytes = Zeroizing::new(Vec::new());` then `file.read_to_end(&mut bytes)`, an
    //    Err(e) is "cannot read <path>: <e>". Ok(bytes).
    todo!()
}

Every message is built with path.display(), as in the old check_file. dev, ino, uid and mode come from std::os::unix::fs::MetadataExt, which the file already imports. Add use std::io::Read; for read_to_end.

2. In load, the SecretSource::File(path) branch. Replace the check_file call and the std::fs::read(path) after it with:

            let mut bytes = match read_checked(path, &|| {}) {
                Ok(bytes) => bytes,
                Err(why) => {
                    return Err(SecretError {
                        name: name.to_string(),
                        why,
                    });
                }
            };

and the value(bytes) call in that branch becomes value(std::mem::take(&mut *bytes)), which moves the bytes out and leaves nothing unwiped behind. The warning and the other two branches do not change.

Steps

• 1. Copy. git switch m4a, then cp docs/plans/M4a/files/crates/gatewayd/tests/secrets_race.rs crates/gatewayd/tests/
• 2. See it fail. cargo test -p gatewayd --test secrets_race. Expected: it does not compile (read_checked does not exist).
• 3. Make the two changes. Write read_checked step by step as its comment says, cargo check -p gatewayd after it, then change load. Run cargo fmt --all.
• 4. See it pass. cargo test -p gatewayd --test secrets_race --test secrets --test main. Expected: 5, 8 and 5 passed.
• 5. Check. grep -n "check_file\|std::fs::read(" crates/gatewayd/src/secrets.rs prints only the credential branch's std::fs::read(&path).
• 6. Run the gate. make gate. Expected last line: gate: ok, with about 769 tests.
• 7. Log and commit. git add crates/gatewayd docs/implementer-log.md Cargo.lock && git commit

This is the last follow-up task of M4a. Stop after the commit.

Done when

• The three suites pass; make gate prints gate: ok.

Stop and report if

• A comment above the todo!() does not fit, or contradicts a test: quote it.
• A test seems to need unsafe, libc or any new dependency.