Skip to content

fix: secure local file transfer paths - #584

Merged
wchwawa merged 4 commits into
mainfrom
fix/local-file-transfer-20261026
Oct 26, 2026
Merged

wchwawa merged 4 commits into
mainfrom
fix/local-file-transfer-20261026

Conversation

@wchwawa

@wchwawa wchwawa commented Oct 26, 2026 •

Copy link
Copy Markdown
Member

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 refuses RENAME_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.md directs 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 nokv process 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:

  • A created file's record holds its relative directories, name, and inode identity instead of an open parent descriptor; rollback walks from the pinned root again and removes only a file whose identity matches.
  • When renameat2(RENAME_NOREPLACE) is refused with EINVAL, ENOSYS, or ENOTSUP, publication links the staged file under its final name, which fails with EEXIST when the name exists, and when linking is refused too it creates the final name with O_CREAT|O_EXCL under the pinned directory. Every strategy stays create-only.
  • Files are created with 0666 and directories with 0777 under the process umask, as before the hardening.
  • Path components are pinned with search-only descriptors (O_PATH on Linux, O_SEARCH on macOS); only the directories collect enumerates 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-*.tmp staging 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 -- --check and git diff --check: passed.
  • python3 scripts/workbench/workbench_contract_test.py: 8 passed.
  • Installed Python wheel: 35 adapter/API tests passed in Python 3.12.
  • On the final source change, 9 CLI transfer and 16 Python adapter Rust tests, targeted Clippy, formatting, and diff checks passed. The matched local primitive benchmark used the final CLI transfer source.
  • Review fixes (macOS, Rust 1.99.1): cargo test -p nokv --bin nokv transfer 13 passed; cargo test -p nokv-python --lib local_adapter 20 passed, run twice; cargo clippy -p nokv -p nokv-python --all-targets -- -D warnings clean; cargo fmt --all and git diff --check clean. 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 a 0311 ancestor is enough to collect from and materialize into.
  • Review fixes on Linux (rust:1.99.1-bookworm container): cargo test -p nokv --bin nokv transfer 12 passed and cargo test -p nokv-python --lib local_adapter 19 passed (the macOS alias test is macOS-only), with protobuf-compiler and python3-dev installed; the O_PATH directory pinning runs only there.
  • Live object-store qualification and end-to-end local-transfer throughput testing were not run; no production performance claim is made.

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
wchwawa marked this pull request as ready for review October 26, 2026 05:29
Signed-off-by: wchwawa <wch19961116@gmail.com>
@wchwawa
wchwawa merged commit a407472 into main Oct 26, 2026
10 checks passed
@wchwawa
wchwawa deleted the fix/local-file-transfer-20261026 branch October 26, 2026 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant