fix(unlock): reject key names carrying shell metacharacters (RCE) - #28
Merged
Merged
Conversation
`unlock` derived the key name from a directory name under `.git-crypt/keys/` — attacker-controlled repository content — and passed it unvalidated to `git config filter.<name>.smudge`. git runs filter commands through a shell, so a crafted directory name executed arbitrary code on every collaborator who unlocked the repository. The attack needs no special access: a `.gpg` file encrypted to the victim's *public* key decrypts normally, at which point `configure_filters` writes the poisoned command and the `git checkout` that follows fires it. Validate at the choke point. `validate_existing_key_name` applies the existing `[a-zA-Z0-9_-]` rule to names read back from disk or repository content, accepting only the additional reserved name `default` (the on-disk name of the default key). `configure_filters` and `deconfigure_filters` now reject before writing anything, so no future caller can reintroduce the bug. `unlock` skips invalid key directories with a warning rather than aborting, so one poisoned directory cannot deny access to legitimate keys beside it. The name is escaped for display so it cannot smuggle terminal control sequences. `lock --all` gets the same guard. Also adds Dependabot config (cargo + github-actions) and a `cargo audit` CI job — neither existed, so no dependency scanning was configured at all. Tests: 117 (was 112). The GPG integration test reproduces the full attack end to end and fails on the unfixed binary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TYMP8eH52L6rNdjbukfuyZ
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.
Summary
unlockderived the key name from a directory name under.git-crypt/keys/— attacker-controlled repository content — and passed it unvalidated togit config filter.<name>.smudge. git runs filter commands through a shell, so a crafted directory name executed arbitrary code on every collaborator who unlocked the repository.The attack needs no special access. A
.gpgfile encrypted to the victim's public GPG key decrypts normally, at which pointconfigure_filters()writes the poisoned command and thegit checkoutinforce_checkout_files()fires it immediately — no further interaction.Reproduced against the unfixed binary:
The fix
src/key/key_file.rs—validate_existing_key_name()applies the existing[a-zA-Z0-9_-]rule to names read back from disk or repository content, accepting only the extra reserved namedefault(the on-disk name of the default key, which the strict validator rejects for new keys).src/git/config.rs—configure_filters/deconfigure_filtersvalidate before writing anything. This is the choke point: every path to a shell-executed filter command goes through it, so a future caller can't reintroduce the bug.src/commands/unlock.rs— the hole itself. Invalid key directories are skipped with a warning rather than aborting, so one poisoned directory can't deny access to legitimate keys beside it. The name is printed viaescape_default(), which also closes terminal-escape injection on this path.src/commands/lock.rs— same guard onlock --all.Tests
117, up from 112.
defaultaccepted; 12 metacharacter/traversal/empty variants rejected; both config functions reject before touching git..gitattributesroutes a file through the matching filter, victim runsunlock). Asserts no payload artifact, no$(in git config, and that unlock fails loudly. It fails on the unfixed binary.The payload uses
${IFS}rather than a space deliberately — it must be simultaneously a legal path component and a legal.gitattributesattribute value.Also included
No dependency scanning was configured at all: no
dependabot.yml(so no version-update PRs) and no audit step in CI. Adds both — weekly cargo + github-actions updates with minor/patch grouped and majors separate, plus acargo auditjob (cached binary, weekly cron). It fails on vulnerabilities but not on RustSec informational advisories, which would otherwise make CI red today over two unsoundness warnings that don't affect gitveil.Verification
cargo fmt --check,cargo clippy --all-targets -- -D warnings, 117/117 tests,cargo audit— all clean locally.🤖 Generated with Claude Code
https://claude.ai/code/session_01TYMP8eH52L6rNdjbukfuyZ