Skip to content

Revalidate cached stack leaves before directory reuse#2559

Merged
Sebastian Thiel (Byron) merged 8 commits into
mainfrom
fix/symlink-prefix-reuse-worktree-escape-ghsa-f89h-2fjh-2r9q
Apr 30, 2026
Merged

Revalidate cached stack leaves before directory reuse#2559
Sebastian Thiel (Byron) merged 8 commits into
mainfrom
fix/symlink-prefix-reuse-worktree-escape-ghsa-f89h-2fjh-2r9q

Conversation

@Byron

@Byron Sebastian Thiel (Byron) commented Apr 30, 2026

Copy link
Copy Markdown
Member

Tasks

  • refackiew Stack based solution
    • This makes it so that it re-validates previously pushed parents after popping. This makes it a little slower, but it's needed for correctness.
  • more robust gix_index::State::from_tree()
    • benchmarks
  • publish affected crates

gix-index regression

Index-reading is about 20% slower at least. But it has to be for now.

from_tree/flat 10k files
                        time:   [488.52 µs 490.57 µs 492.69 µs]
                        thrpt:  [20.297 Melem/s 20.384 Melem/s 20.470 Melem/s]
                 change:
                        time:   [+18.700% +19.444% +20.129%] (p = 0.00 < 0.05)
                        thrpt:  [−16.756% −16.279% −15.754%]
                        Performance has regressed.
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) high mild
from_tree/wide 100 x 100 files
                        time:   [402.83 µs 404.84 µs 406.94 µs]
                        thrpt:  [24.574 Melem/s 24.701 Melem/s 24.824 Melem/s]
                 change:
                        time:   [+10.719% +11.585% +12.502%] (p = 0.00 < 0.05)
                        thrpt:  [−11.112% −10.382% −9.6813%]
                        Performance has regressed.
Found 4 outliers among 100 measurements (4.00%)
  4 (4.00%) high mild
from_tree/sparse 10k directories
                        time:   [816.51 µs 819.70 µs 822.98 µs]
                        thrpt:  [12.151 Melem/s 12.200 Melem/s 12.247 Melem/s]
                 change:
                        time:   [+2.9523% +3.3909% +3.8470%] (p = 0.00 < 0.05)
                        thrpt:  [−3.7045% −3.2796% −2.8676%]
                        Performance has regressed.

This PR was created by Codex on behalf of Byron. Byron will review it before it is merged.

Advisory

Advisory URL: GHSA-f89h-2fjh-2r9q
GHSA ID: GHSA-f89h-2fjh-2r9q

Advisory summary

  • Severity: not yet assigned in the advisory metadata.
  • Affected package/range: not specified in the advisory metadata.
  • Patched versions: not specified in the advisory metadata.
  • CVE: not assigned.

This fixes a checkout path-stack validation gap without reproducing exploit details from the unpublished advisory. Cached leaf path components are now revalidated before being reused as parent directories, and stack state is restored correctly if delegate validation or directory setup fails.

Git baseline

Checked /Users/byron/dev/github.com/git/git at 94f057755b. Git uses lstat-based leading-path checks in symlinks.c and consults check_leading_path() from entry.c before removing entries.

Validation

  • cargo fmt --check
  • cargo test -p gix-fs stack
  • cargo test -p gix-worktree-tests
  • codex review --uncommitted: clean after follow-up fixes

Note: a later codex review --commit attempt was started, produced continuous exploratory output, and was terminated after running unreasonably long without a final verdict.

Copilot AI review requested due to automatic review settings April 30, 2026 01:26

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 63192ce900

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".

Comment thread gix-fs/src/stack.rs

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses a checkout path-stack validation gap by ensuring cached “leaf” components are revalidated when later reused as parent directories, and by improving stack state restoration on certain failure paths.

Changes:

  • Update gix_fs::Stack::make_relative_path_current() to re-run delegate validation when a cached leaf becomes a directory, and adjust delegate call ordering/state restoration.
  • Extend gix-fs stack tests to cover failure recovery around leaf→directory transitions and update expected delegate call counts.
  • Add gix-worktree regression tests covering symlink collisions when a previously cached leaf is later used as a directory (forced vs non-forced behavior).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
gix-worktree/tests/worktree/stack/create_directory.rs Adds regression tests for revalidating cached leaves when encountering symlink collisions during directory creation.
gix-fs/tests/fs/stack.rs Adds failure-mode tests for leaf→directory transitions and updates expected delegate call counts.
gix-fs/src/stack.rs Implements leaf revalidation via delegate calls and improves rollback behavior for some delegate failure paths.

Comment thread gix-fs/src/stack.rs Outdated
@Byron Sebastian Thiel (Byron) force-pushed the fix/symlink-prefix-reuse-worktree-escape-ghsa-f89h-2fjh-2r9q branch from 63192ce to 1d2f69e Compare April 30, 2026 02:05
@Byron Sebastian Thiel (Byron) marked this pull request as draft April 30, 2026 02:24
@Byron Sebastian Thiel (Byron) force-pushed the fix/symlink-prefix-reuse-worktree-escape-ghsa-f89h-2fjh-2r9q branch from 1d2f69e to 74e4b20 Compare April 30, 2026 08:27
Add a gix integration fixture that constructs a malformed tree with a
directory/file conflict on the same path:

- `a` as a symlink to `.git/hooks`
- `a` as a tree containing `post-checkout -> ../../payload`
- `payload` as an executable file

The tree is created with `git hash-object --literally` so the duplicate
root entries are preserved in the fixture repository.

Add a clone checkout regression test that clones this repository and
asserts that checkout does not write `.git/hooks/post-checkout` through the
attacker-controlled `a` symlink prefix. The test also checks that the
payload itself is checked out and remains executable.

This currently reproduces the vulnerability: delayed symlink checkout
creates `a` first, then reuses the cached `a` prefix when processing
`a/post-checkout`, allowing the final symlink creation to resolve through
`a` into `.git/hooks/post-checkout`.

The fixture is Unix-only at the test level because the exploit depends on
Unix symlink behavior.

Co-authored-by: Sebastian Thiel <sebastian.thiel@icloud.com>
@Byron Sebastian Thiel (Byron) force-pushed the fix/symlink-prefix-reuse-worktree-escape-ghsa-f89h-2fjh-2r9q branch from 74e4b20 to 83628bf Compare April 30, 2026 08:27
GHSA-f89h-2fjh-2r9q reports that checkout could process a later path beneath
a previously cached leaf without replaying the delegate validation and
directory-creation path for that reused component.

The generic gix_fs::Stack now promotes cached leaves through
delegate.push(false, ...) before entering directory state, so worktree checkout
re-runs collision handling and path validation before using the component as
a parent. The new error handling restores the prior leaf or directory state
if delegate setup or a child push fails, keeping reusable stack state balanced
after recoverable errors.

Regression coverage exercises both overwrite modes in gix-worktree
and the gix-fs delegate callback/error-state transitions. Git baseline
checked in /Users/byron/dev/github.com/git/git at 94f057755b: symlinks.c
check_leading_path() uses lstat-based leading-path checks, and entry.c
unlink_entry() consults that path check before removing entries.
@Byron Sebastian Thiel (Byron) force-pushed the fix/symlink-prefix-reuse-worktree-escape-ghsa-f89h-2fjh-2r9q branch from 6c41c25 to cedcb63 Compare April 30, 2026 08:52
Add a Criterion benchmark for `gix_index::State::from_tree()` to track the
cost of constructing an index from in-memory tree objects.

The benchmark covers the same broad input shapes used by the existing
`to_tree` benchmark:

- a flat tree with 10k files
- a wide/deep tree with 100 directories and 100 files each
- a sparse-directory-like tree with 10k tree entries

The benchmark builds each input once with `gix_object::tree::Editor`, stores
serialized tree objects in an in-memory object database, and then measures
only `State::from_tree()` over those prebuilt trees. Throughput is reported
in logical entries, so regressions from extra sorting, conflict handling, or
tree traversal changes are easier to compare across shapes.

This is especially useful while `from_tree()` performs post-collection
normalization for file/directory conflicts. The desired long-term state is
to preserve the same normalized index invariant while moving closer to
Git’s insertion-time conflict handling, avoiding unnecessary whole-index
work when traversal already produces entries in order.

Co-authored-by: Sebastian Thiel <sebastian.thiel@icloud.com>
@Byron Sebastian Thiel (Byron) force-pushed the fix/symlink-prefix-reuse-worktree-escape-ghsa-f89h-2fjh-2r9q branch from cedcb63 to f7bae0f Compare April 30, 2026 09:16
@Byron Sebastian Thiel (Byron) marked this pull request as ready for review April 30, 2026 09:16
Copilot AI review requested due to automatic review settings April 30, 2026 09:16

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f7bae0f2c5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".

Comment thread gix-index/src/init.rs Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 13 changed files in this pull request and generated 5 comments.

Comment thread gix/tests/fixtures/make_symlink_prefix_reuse_advisory.sh
Comment thread gix-index/tests/fixtures/make_symlink_prefix_reuse_advisory.sh
Comment thread gix-index/tests/fixtures/make_symlink_prefix_reuse_advisory.sh Outdated
Comment thread gix/tests/gix/clone.rs
Comment thread gix-index/src/init.rs Outdated
Codex (codex) and others added 2 commits April 30, 2026 17:52
This is currently not easily exploitable as anything that interacts
with the worktree goes through a symlink check, which catches this issue
naturally.

The plan is to get closer to how Git keeps the sort order, which detects
such issues more naturally.

Co-authored-by: Sebastian Thiel <sebastian.thiel@icloud.com>
@Byron Sebastian Thiel (Byron) force-pushed the fix/symlink-prefix-reuse-worktree-escape-ghsa-f89h-2fjh-2r9q branch from f7bae0f to 2facc6f Compare April 30, 2026 09:52
Copilot AI review requested due to automatic review settings April 30, 2026 10:00

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 14 changed files in this pull request and generated 4 comments.

Comment thread gix/tests/gix/clone.rs
Comment thread etc/plan/performance.md
Comment thread etc/plan/performance.md
Comment thread gix-index/tests/index/init.rs
@Byron Sebastian Thiel (Byron) merged commit 3af9b4a into main Apr 30, 2026
29 checks passed
@Byron

Copy link
Copy Markdown
Member Author

Bypassed check of 'changelog' commit because it just changed some version numbers and a changelog.

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.

3 participants