diff --git a/docs/implementer-log.md b/docs/implementer-log.md index 5815b8f..fc93bb2 100644 --- a/docs/implementer-log.md +++ b/docs/implementer-log.md @@ -555,7 +555,7 @@ the milestone held. | # | Severity | Owner | Finding | Fix | |---|---|---|---|---| | 1 | nit | implementer (06, 07) | `as` casts on values already bounded a few lines up: five in `http.rs` (chunk sizes, `MAX_BODY`), several in `handshake.rs`'s base64. None can lose data; tip I6 prefers `try_from`. | Fixed by the design model at the owner's request, with a third the review missed in `sha1.rs` | -| 2 | low | spec, reference and implementer (04) | A secret file is checked with `symlink_metadata` and then opened by path, so someone who can write to its directory could swap it for a symbolic link in between. The reference had the same (tip T5). Opening once with `O_NOFOLLOW` and checking the open file closes it. | A follow-up task, if the owner wants it | +| 2 | low | spec, reference and implementer (04) | A secret file is checked with `symlink_metadata` and then opened by path, so someone who can write to its directory could swap it for a symbolic link in between. The reference had the same (tip T5). Opening once with `O_NOFOLLOW` and checking the open file closes it. | Task 16 | | 3 | nit | implementer (06) | The HTTP reader is more lenient than the reference: two spaces in the status line, and a bare `\n` ending a chunk line, are accepted. Every hostile case is still refused before any allocation. | Fixed by the design model at the owner's request: single spaces in the status line, CRLF on every chunk line (`tests/http_strict.rs`, red before) | | 4 | nit | plan (11) | The skeleton's `Pending.resume` was read only in a `todo!()` whose comment did not mention it; the implementer wrote `resume: true` directly, then removed the unused field and reported it (tip T26). | None needed | diff --git a/docs/plans/M4a/16-gatewayd-secret-file-race.md b/docs/plans/M4a/16-gatewayd-secret-file-race.md new file mode 100644 index 0000000..4713b0e --- /dev/null +++ b/docs/plans/M4a/16-gatewayd-secret-file-race.md @@ -0,0 +1,113 @@ +# 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>`. + +`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: + +```rust +/// 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>, String> { + // 1. `path.is_absolute()`, else " is not an absolute path". + // 2. `let named = std::fs::symlink_metadata(path)`, an Err(e) is "cannot read : "; + // `named.file_type().is_symlink()` is " is a symbolic link"; + // `!named.file_type().is_file()` is " is not a regular file". + // 3. `between();` + // 4. `let mut file = std::fs::File::open(path)`, an Err(e) is "cannot read : "; + // `let opened = file.metadata()`, the same error text. + // 5. `if (opened.dev(), opened.ino()) != (named.dev(), named.ino())`: + // " 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"; " is not owned by the user gatewayd runs as"; + // " has mode ; 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 : ". 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: + +```rust + 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. diff --git a/docs/plans/M4a/README.md b/docs/plans/M4a/README.md index e9cdc73..1f45ebc 100644 --- a/docs/plans/M4a/README.md +++ b/docs/plans/M4a/README.md @@ -62,8 +62,9 @@ checked to compile against its task's tests and fail them. | 13 | `13-gatewayd-deliver.md` | turns on `loop.sock`, answers in the thread | `deliver.rs`, `support/fake_loop.rs` | reference; 11 mutations, 10 caught; the one left accepts an event frame with another id, which `loopd` never sends | | 14 | `14-gatewayd-serve.md` | the serve loop: routing, typing, catch-up, reconnecting | `serve.rs`, `serve_restart.rs`, `support/fake_mm.rs`, `support/gateway.rs` | reference; 14 mutations, all caught once four tests were added; 8 runs clean | | 15 | `15-gatewayd-main.md` | `gatewayd serve --config ` | `main.rs` | reference; run against the owner's server, token never printed | +| 16 | `16-gatewayd-secret-file-race.md` | review finding 2: a secret file is read from the file that was checked | `secrets_race.rs` | reference in the working tree, then removed; 2 mutations, both caught; gate at the end state | -At the end of task 15: about 762 tests (650 before task 01). +At the end of task 15: about 762 tests (650 before task 01). The design model's review fixes add 2; at the end of task 16: about 769. ## Changes during the run @@ -100,6 +101,9 @@ At the end of task 15: about 762 tests (650 before task 01). signature. Tasks 14 and 15 now say to stop and quote a comment that does not fit. The attempt is saved in `.state/runs/M4a/14-second-attempt*`. Resume from task 14. +- 2026-09-24, after the review: the design model fixed findings 1 and 3 (commit `0081f24`), and + wrote task 16 for finding 2. Run it with `tools/run-plan.sh docs/plans/M4a 16`. + ## Running it ```sh diff --git a/docs/plans/M4a/files/crates/gatewayd/tests/secrets_race.rs b/docs/plans/M4a/files/crates/gatewayd/tests/secrets_race.rs new file mode 100644 index 0000000..219864a --- /dev/null +++ b/docs/plans/M4a/files/crates/gatewayd/tests/secrets_race.rs @@ -0,0 +1,80 @@ +//! A secret file is read from the file that was checked, never from whatever the path names a +//! moment later (M4a review, finding 2). `read_checked` runs `between` after checking the path and +//! before opening it; each test swaps something there. Do not edit. + +#[path = "support/tmp.rs"] +mod tmp; + +use std::os::unix::fs::PermissionsExt; +use std::path::{Path, PathBuf}; + +use gatewayd::secrets::read_checked; +use tmp::TempDir; + +const TOKEN: &str = "the-real-token"; +const OTHER: &str = "a-file-the-owner-never-chose"; + +fn owner_file(dir: &TempDir, name: &str, text: &str) -> PathBuf { + let path = dir.write(name, text); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).unwrap(); + path +} + +fn refused(path: &Path, between: &dyn Fn(), word: &str) { + let why = read_checked(path, between).expect_err(word); + assert!(why.contains(word), "{word}: {why}"); + assert!(why.contains(&path.display().to_string()), "{why}"); + assert!(!why.contains(TOKEN) && !why.contains(OTHER), "never a value: {why}"); +} + +#[test] +fn an_untouched_file_is_read() { + let dir = TempDir::new("race-ok"); + let path = owner_file(&dir, "token", &format!("{TOKEN}\n")); + let bytes = read_checked(&path, &|| {}).unwrap(); + assert_eq!(bytes.as_slice(), format!("{TOKEN}\n").as_bytes()); +} + +#[test] +fn a_file_swapped_for_a_link_is_refused() { + let dir = TempDir::new("race-link"); + let path = owner_file(&dir, "token", TOKEN); + let other = owner_file(&dir, "other", OTHER); + let swap = || { + std::fs::remove_file(&path).unwrap(); + std::os::unix::fs::symlink(&other, &path).unwrap(); + }; + refused(&path, &swap, "changed while it was read"); +} + +#[test] +fn a_file_swapped_for_another_file_is_refused() { + let dir = TempDir::new("race-rename"); + let path = owner_file(&dir, "token", TOKEN); + let other = owner_file(&dir, "other", OTHER); + let swap = || std::fs::rename(&other, &path).unwrap(); + refused(&path, &swap, "changed while it was read"); +} + +#[test] +fn the_checks_hold_for_the_file_that_is_read() { + let dir = TempDir::new("race-mode"); + let path = owner_file(&dir, "token", TOKEN); + let widen = || { + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).unwrap(); + }; + refused(&path, &widen, "has mode 644"); +} + +#[test] +fn the_path_checks_still_come_first() { + let dir = TempDir::new("race-first"); + let other = owner_file(&dir, "other", OTHER); + let link = dir.path().join("link"); + std::os::unix::fs::symlink(&other, &link).unwrap(); + refused(&link, &|| panic!("never reached for a link"), "is a symbolic link"); + refused(dir.path(), &|| panic!("never reached for a directory"), "is not a regular file"); + let relative = Path::new("relative/token"); + let why = read_checked(relative, &|| panic!("never reached")).unwrap_err(); + assert!(why.contains("is not an absolute path"), "{why}"); +}