audit: TOCTOU on filesystem paths — 13 sites (corrosion_mitigation::fs_path_toctou)

[zancas]

Tracking 13 instances of CLAUDE.md checklist item #1 ("TOCTOU on filesystem paths") found in tree on branch dev. Test scaffold for the class will land at zcash_local_net/tests/corrosion_mitigation/fs_path_toctou.rs (this subclass is corrosion_mitigation::fs_path_toctou).

The pattern, from CLAUDE.md:

Operating on the same &Path across multiple syscalls lets an attacker swap a symlink between the check and the use, redirecting privileged operations to unintended targets.

Two of the four highest-risk sites have a deterministic exploit (no race-window required): Path::exists() follows symlinks, so a pre-placed dangling or valid symlink fools the check, and the subsequent cp -r (which re-resolves the path on its own) writes to or reads from the symlink's target.

Tier 1 — externally-attacker-influenceable trust boundary

#	Site	Pattern	Exploit
A1	zcash_local_net/src/utils/executable_finder.rs:26 → caller pick_command	path.exists() then Command::new(path) (exec)	Symlink-swap of the binary at $TEST_BINARIES_DIR/<name> between check and execve → arbitrary code execution. Mitigation: drop the redundant .exists() and let Command::new(...).spawn() fail with ENOENT. The .exists() check also creates the silent-fallback bug observed in test failures on dev (zainod missing → fall back to PATH → spawn fails with confusing error).
A2	zcash_local_net/src/validator.rs:366 (Validator::cache_chain default impl), used at :374	assert!(!chain_cache.exists()) then cp -r self.data_dir() chain_cache	Deterministic. A dangling symlink at chain_cache reports exists() == false (since exists() follows symlinks and the target is missing), the assertion passes, and cp -r DST follows the symlink to write the data dir contents wherever the attacker pointed.
A3	zcash_local_net/src/validator/zebrad.rs:599 (Zebrad::load_chain), used at :604	assert!(state_dir.exists()) then cp -r state_dir validator_data_dir	Deterministic. A valid symlink at chain_cache/state pointing to an attacker-controlled directory passes exists() and cp -r SRC reads the attacker's files into the validator data dir.
A4	zcash_local_net/src/validator/zcashd.rs:425 (Zcashd::load_chain), used at :429	Same as A3 with chain_cache/regtest	Same as A3.

Tier 2 — tempdir-scoped, lower real-world risk but pattern hits

The log filenames stdout.log, stderr.log, lwd.log are constructed by joining a tempfile::TempDir (mode 0700) with a constant filename. Each call site re-resolves the same logical path string and re-issues a syscall:

#	Creator	Later syscall(s) on same logical path
B7	zcash_local_net/src/logs.rs:21–22 (File::create stdout.log)	launch.rs:34 File::open, launch.rs:193 fs::read_to_string, logs.rs:13 File::open (via print_log → print_stdout/print_stderr), tests/integration.rs:436 fs::read_to_string, tests/integration.rs:892 fs::read_to_string
B8	zcash_local_net/src/logs.rs:26–27 (File::create stderr.log)	launch.rs:38 File::open, launch.rs:195 fs::read_to_string, logs.rs:13 (via print_stderr), tests/integration.rs:436 fs::read_to_string
B9	zcash_local_net/src/indexer/lightwalletd.rs:113–114 (File::create lwd.log)	launch.rs:43 File::open (when this is the additional_log_path), launch.rs:14 fs::read_to_string (via snapshot_additional_log at launch.rs:64, 104, 196)

Mitigation: keep the File handle returned by File::create and read through the FD on subsequent calls (or use *at syscalls relative to an open dir handle on the tempdir).

Tier 3 — write-then-read across function boundaries (config files)

#	Writer	Same-path re-read
C10	config.rs:33 write_zcashd_config	config.rs:443, 462 fs::read_to_string (tests)
C11	config.rs:132 write_zebrad_config	(no in-process re-read; consumed by zebrad child)
C12	config.rs:318 write_zainod_config	config.rs:490 fs::read_to_string (test)
C13	config.rs:361 write_lightwalletd_config	config.rs:526 fs::read_to_string (test)

Each writer itself does only one syscall per path; the pattern emerges across function boundaries when callers re-read by path. Tempdir-scoped, low risk.

Tier 4 — redundant-check smell (test-only)

#	Site	Why it's a smell
A5	zcash_local_net/tests/testutils.rs:39–40	if !chain_cache_dir.exists() { fs::create_dir_all(...) } — create_dir_all is itself idempotent. The exists() guard is redundant and introduces the TOCTOU pattern unnecessarily. Drop the guard.
A6	zcash_local_net/tests/testutils.rs:53–54	Same as A5, second occurrence in generate_zcashd_chain_cache.

Mitigations summary (per CLAUDE.md #1)

• For "must-not-exist" sites (A2, A5, A6): use OpenOptions::create_new(true).open(p) (atomically refuses pre-existing or symlinked targets), and for directories DirBuilder::new().recursive(false).create(p).
• For "must-exist" sites (A3, A4): replace Path::exists() with fs::symlink_metadata(p) (which does not follow symlinks) and reject if it's a symlink, or use *at syscalls relative to an opened directory FD.
• For cp -r subprocess invocations: the subprocess re-resolves the path on its own and is the actual TOCTOU vector. Fixing the in-process assert! does not stop the subprocess from following a symlink. Real fix is to drop cp -r for a Rust-level recursive copy that opens the source as a directory FD once and walks via *at syscalls (or via walkdir + per-file FDs).
• For A1: drop the redundant path.exists() and let Command::new(...).spawn() fail with ENOENT. The trust boundary is $TEST_BINARIES_DIR (operator-set), so this is a defense-in-depth fix — the operator owning that dir is presumed by the test harness anyway. Same change also fixes the silent-fallback-to-PATH bug observed today (zainod missing → cryptic spawn failure deep in the test).

What I did not find

• No fs::canonicalize, read_link, symlink_metadata, set_permissions, chmod, rename, hard_link, or read_dir calls anywhere in tree.
• No is_dir / is_file / is_symlink / metadata / try_exists predicates.
• regtest-launcher/ does no filesystem I/O at all.
• No OpenOptions::create_new(true) (which would be the recommended atomic-create pattern) — currently nothing in tree uses it.

Plan

Audit-test scaffold at zcash_local_net/tests/corrosion_mitigation/fs_path_toctou.rs. Tests are written so they FAIL while the pattern is in tree; will be landed alongside the fixes in a single PR so they go green by merge. Top sites (A1–A4) get behavioural exploit tests; lower-tier sites (B/C, A5/A6) are addressed when their fixes ship.

[zancas]

Status update — Tier-1 (deterministic-exploit) sites closed as of #257.

• A1 (pick_path redundant .exists()): dropped the pre-check. Lookup and exec are now bound by a single syscall (Command::new(path).spawn()). Side effect: when TEST_BINARIES_DIR is set but the binary is missing, the spawn now fails with ENOENT at the resolved path instead of disappearing into a silent PATH fallback — also resolves the four zainod-missing integration-test failures that prompted this audit.
• A2 (Validator::cache_chain default impl): adopts utils::safe_copy::safe_copy_into_new. Return type changed from Future<Output = std::process::Output> to Future<Output = io::Result<()>> so rejection surfaces as Err.
• A3 (Zebrad::load_chain) and A4 (Zcashd::load_chain): both adopt utils::safe_copy::safe_copy_into_existing. The leaf is opened with O_NOFOLLOW; symlinked chain_cache/state or chain_cache/regtest is rejected at the kernel boundary. Trait return type changed from PathBuf to io::Result<PathBuf>; non-test callers in Zebrad::launch_once / Zcashd::launch_once keep panic-on-failure semantics via inline .expect().

New helper module zcash_local_net::utils::safe_copy exposes open_dir_no_symlinks, safe_copy_into_new, and safe_copy_into_existing. All three route through *at syscalls relative to FDs we hold open, so paths are not re-resolved by the kernel between steps — closing both the deterministic exploit and the race window. (Adds nix = "0.29" with the fs and dir features as a regular dependency.)

Audit tests at unit_tests::corrosion_mitigation::fs_path_toctou::* now pass for all four Tier-1 sites:

• utils::executable_finder::unit_tests::corrosion_mitigation::fs_path_toctou::pick_path_does_not_pre_validate_with_exists
• validator::unit_tests::corrosion_mitigation::fs_path_toctou::cache_chain_refuses_chain_cache_with_symlinked_parent
• validator::zebrad::unit_tests::corrosion_mitigation::fs_path_toctou::load_chain_refuses_symlinked_state_dir
• validator::zcashd::unit_tests::corrosion_mitigation::fs_path_toctou::load_chain_refuses_symlinked_regtest_dir

Plus 8 unit tests at utils::safe_copy::tests covering the helper's primitives.

Still open in this issue:

• B7/B8/B9 — log-file paths (stdout.log/stderr.log/lwd.log) re-resolved across multiple call sites in tempfile::TempDirs. Race-window only; no deterministic exploit. Mitigation: keep the File returned by File::create and read through the FD on subsequent calls (or use *at syscalls relative to an open dir handle on the tempdir).
• C10–C13 — config-file paths re-resolved across function boundaries (each write_*_config writes once; tests re-read by path). Same race-window-only category.
• A5/A6 — redundant .exists() guards in tests/testutils.rs:39-40 and :53-54. Test-only smell; trivial fix (drop the guard, create_dir_all is idempotent) — could land alongside the next testutils cleanup.