Split out of the review of #112 (and the matching finding on #111), where both bots raised it and it was declined in both as out of scope for a path-validation fix. CodeRabbit asked for it to be filed as one issue covering the whole boundary rather than half of it per PR: #112 (comment)
The property that is missing
Vera decides whether a file may be read by path, and then opens it by path on a later statement. Between those two operations the path can be repointed. Every containment guarantee Vera makes today is therefore a guarantee about the state of the filesystem at validation time, not at read time.
This is not a defect introduced by #111 or #112. Both of those close a different hole (a hostile committed index, and symlinks planted before the walk) and neither claims to close this one; #112's body carries a "What this does not close" section saying so.
The precondition, stated honestly
An attacker needs concurrent write access to the victim's working tree while a query or an index run is in flight. Anyone with that also has .git/hooks/pre-commit, build.rs, the Makefile and .vscode/settings.json, so they already have code execution on the victim's next build or commit. Reading one file outside the root is strictly weaker than what they hold.
So this is a hardening issue, not an urgent one. It is filed because the boundary should be described accurately somewhere, and because a partial fix is worse than none.
Why it cannot be fixed in one place
discovery::read_source_lossy (crates/vera-core/src/discovery/mod.rs:190) is pub and path-taking, and it is the shared read for both halves:
| caller |
site |
| retrieval |
crates/vera-core/src/retrieval/references.rs:47 |
| retrieval |
crates/vera-core/src/retrieval/regex_search.rs:77 |
| retrieval |
crates/vera-core/src/retrieval/structural.rs:268 |
| retrieval |
crates/vera-core/src/retrieval/type_relations.rs:49 |
| indexing |
crates/vera-core/src/indexing/pipeline.rs:408 |
| indexing |
crates/vera-core/src/indexing/update.rs:371 |
| indexing |
crates/vera-core/src/indexing/freshness.rs:145 |
The indexing window is the wider one and it is upstream: discover_files_with_cancellation builds the whole Vec<DiscoveredFile> and returns, and content is re-opened by path in a later pass, so what actually gets stored is decided over a window spanning the rest of the walk (measured at 379 files / 60 ms on this repository) rather than the two syscalls inside the loop that the reviews pointed at.
Hardening retrieval alone leaves the indexer still storing content fetched through the unhardened path, and leaves read_source_lossy half handle-based and half path-based: the cost paid, the property not obtained.
What a real fix has to decide
- Descriptor-based traversal.
openat per component with O_NOFOLLOW, or cap-std. There is no std API for either, and vera-core carries no libc, rustix, cap-std, openat or nix dependency today.
- Windows.
x86_64-pc-windows-msvc is a release target (.github/workflows/release.yml:51) and has neither openat nor O_NOFOLLOW. The nearest equivalent, FILE_FLAG_OPEN_REPARSE_POINT, opens the reparse point and hands back reparse data rather than file content, so it is not a drop-in.
- In-tree symlinks. Blanket symlink rejection is a behaviour change: repositories containing legitimate symlinks inside the tree index and search fine today. The issue has to say whether those keep working, and by what rule.
- Descriptor budget. Holding a handle per discovered file across a walk runs into the per-process limit on large repositories, so the design has to say where handles are opened and closed.
- Signature churn.
read_source_lossy is pub. Changing it to take a handle is a breaking change for library consumers; adding a variant and delegating is the cheaper shape.
Not in scope
Acceptance
A test that wins the race deterministically rather than by timing: block the read at a known point, repoint the path, release, and assert the content that comes back is the in-root file or an error, never the out-of-root canary. A test that merely spawns a racing thread and hopes is the shape this repository has already had to fix twice.
Split out of the review of #112 (and the matching finding on #111), where both bots raised it and it was declined in both as out of scope for a path-validation fix. CodeRabbit asked for it to be filed as one issue covering the whole boundary rather than half of it per PR: #112 (comment)
The property that is missing
Vera decides whether a file may be read by path, and then opens it by path on a later statement. Between those two operations the path can be repointed. Every containment guarantee Vera makes today is therefore a guarantee about the state of the filesystem at validation time, not at read time.
This is not a defect introduced by #111 or #112. Both of those close a different hole (a hostile committed index, and symlinks planted before the walk) and neither claims to close this one; #112's body carries a "What this does not close" section saying so.
The precondition, stated honestly
An attacker needs concurrent write access to the victim's working tree while a query or an index run is in flight. Anyone with that also has
.git/hooks/pre-commit,build.rs, theMakefileand.vscode/settings.json, so they already have code execution on the victim's next build or commit. Reading one file outside the root is strictly weaker than what they hold.So this is a hardening issue, not an urgent one. It is filed because the boundary should be described accurately somewhere, and because a partial fix is worse than none.
Why it cannot be fixed in one place
discovery::read_source_lossy(crates/vera-core/src/discovery/mod.rs:190) ispuband path-taking, and it is the shared read for both halves:crates/vera-core/src/retrieval/references.rs:47crates/vera-core/src/retrieval/regex_search.rs:77crates/vera-core/src/retrieval/structural.rs:268crates/vera-core/src/retrieval/type_relations.rs:49crates/vera-core/src/indexing/pipeline.rs:408crates/vera-core/src/indexing/update.rs:371crates/vera-core/src/indexing/freshness.rs:145The indexing window is the wider one and it is upstream:
discover_files_with_cancellationbuilds the wholeVec<DiscoveredFile>and returns, and content is re-opened by path in a later pass, so what actually gets stored is decided over a window spanning the rest of the walk (measured at 379 files / 60 ms on this repository) rather than the two syscalls inside the loop that the reviews pointed at.Hardening retrieval alone leaves the indexer still storing content fetched through the unhardened path, and leaves
read_source_lossyhalf handle-based and half path-based: the cost paid, the property not obtained.What a real fix has to decide
openatper component withO_NOFOLLOW, orcap-std. There is no std API for either, andvera-corecarries nolibc,rustix,cap-std,openatornixdependency today.x86_64-pc-windows-msvcis a release target (.github/workflows/release.yml:51) and has neitheropenatnorO_NOFOLLOW. The nearest equivalent,FILE_FLAG_OPEN_REPARSE_POINT, opens the reparse point and hands back reparse data rather than file content, so it is not a drop-in.read_source_lossyispub. Changing it to take a handle is a breaking change for library consumers; adding a variant and delegating is the cheaper shape.Not in scope
Acceptance
A test that wins the race deterministically rather than by timing: block the read at a known point, repoint the path, release, and assert the content that comes back is the in-root file or an error, never the out-of-root canary. A test that merely spawns a racing thread and hopes is the shape this repository has already had to fix twice.