Repository navigation
fix: secure local file transfer paths - #584
Merged
Merged
Conversation
Signed-off-by: wchwawa <wch19961116@gmail.com>
Signed-off-by: wchwawa <wch19961116@gmail.com>
A materialization no longer keeps one open directory descriptor per file it created: the record of a created file holds only its relative directories, name, and inode identity, and rollback walks from the pinned root again, so a workbench of any size materializes under the default descriptor limit. When a file system refuses RENAME_NOREPLACE, as the Linux NFS client does for every rename flag, publication falls back to linking the staged file under its final name and then to an exclusive create under the pinned directory, both still create-only. Files and directories are created with 0666 and 0777 under the process umask again, as before the hardening, instead of 0600 and 0700. Path components are pinned with search-only descriptors (O_PATH on Linux, O_SEARCH on macOS), so an ancestor that grants search but not read no longer fails collect or materialize; only the directories collect lists need read permission. Signed-off-by: wchwawa <wch19961116@gmail.com>
wchwawa
marked this pull request as ready for review
October 26, 2026 05:29
Signed-off-by: wchwawa <wch19961116@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TLDR
Secure local
collect,materialize, and Python file adapters against path replacement while preserving their Workbench behavior, and keep them portable: no descriptor per materialized file, a create-only fallback where the file system refusesRENAME_NOREPLACE, umask-relative modes, and search-only ancestors.Problem
A private security follow-up identified a race between checking a local path and opening or creating it. No public issue was opened because
SECURITY.mddirects vulnerability details to private reporting.Consequences
A changed local path could make a command read a different file or place materialized bytes outside its intended scratch directory. A local principal with write access to any ancestor of the path, such as a sandboxed job writing the output directory a harness collects, could make the privileged
nokvprocess read a file the principal cannot read, or create one in a directory the principal cannot write.Example
Previously, a parent path could change after validation. Now the operation keeps open directory handles, reads the file it checked, and rejects a destination whose parent identity changes. A materialization of 300 files holds no descriptor per created file, and a destination on an NFS mount, where every rename flag is refused, still publishes without overwriting.
Solution
Open each local component without following symlinks, require regular source files, and publish staged output with create-only atomic rename. Verify source and destination identities and clean up through the held directory handle on failure.
Review fixes on top of that:
renameat2(RENAME_NOREPLACE)is refused withEINVAL,ENOSYS, orENOTSUP, publication links the staged file under its final name, which fails withEEXISTwhen the name exists, and when linking is refused too it creates the final name withO_CREAT|O_EXCLunder the pinned directory. Every strategy stays create-only.0666and directories with0777under the process umask, as before the hardening.O_PATHon Linux,O_SEARCHon macOS); only the directoriescollectenumerates are opened for reading.Gain
Symlink-based path replacement fails closed. The 18-tool Workbench schema and remote publication rules are unchanged. Materializing a medium workbench no longer exhausts the default descriptor limit, NFS and FUSE destinations keep working, materialized files keep the permissions callers had before, and an ancestor with search-only permission no longer blocks a transfer.
Tradeoffs
Caller-created symlinked parents require a physical path; macOS root-owned system aliases remain supported. Hard-linked replacements are not detected: a regular file that is a hard link to another file passes the regular-file and identity checks, so the claim is symlink-based replacement, not every replacement. Materialized output remains disposable scratch: its file is synced and published atomically, without a directory durability promise, and an interrupted run can leave a
.nokv-materialize-*.tmpstaging file behind. The path walk is implemented once in the CLI and once in the Python adapter with slightly different..handling; consolidating them is a follow-up. Matched 4 MiB primitive benchmarks measured macOS collect at 1.498 ms before and 1.477 ms after, and materialize at 10.327 ms before and 10.838 ms after. Linux/arm64 Docker on a macOS bind mount measured collect at 2.598 ms before and 2.862 ms after, and materialize at 4.426 ms before and 4.948 ms after. The latter is a diagnostic environment, not deployment throughput qualification. The review fixes were not re-benchmarked.Validation
cargo test --workspace --locked: passed on macOS, including local path race and rollback tests.cargo clippy --workspace --all-targets --locked -- -D warnings: passed locally and with pinned Rust 1.99.1 on Linux.cargo fmt --all -- --checkandgit diff --check: passed.python3 scripts/workbench/workbench_contract_test.py: 8 passed.cargo test -p nokv --bin nokv transfer13 passed;cargo test -p nokv-python --lib local_adapter20 passed, run twice;cargo clippy -p nokv -p nokv-python --all-targets -- -D warningsclean;cargo fmt --allandgit diff --checkclean. New tests: 300 created files add no open descriptor, the link and exclusive-create fallbacks stay create-only and drop the staging name, created files and directories match a control file's umask-relative mode, and a0311ancestor is enough to collect from and materialize into.cargo test -p nokv --bin nokv transfer12 passed andcargo test -p nokv-python --lib local_adapter19 passed (the macOS alias test is macOS-only), with protobuf-compiler and python3-dev installed; theO_PATHdirectory pinning runs only there.