fix(security): prefix allow-rules were bypassable by command chaining - #15
Merged
Conversation
Re-audit of Phase 1. Three real defects that the first pass missed, two of
them exploitable, plus two coverage gaps in my own tests.
## Prefix allow-rules could be bypassed (pre-existing, HIGH)
`split_compound_command` treated `&&`, `||`, `;` and `|` as separators but not
newline or a bare `&`. Both separate commands in sh. So with a rule a
security-conscious user would plausibly write:
"permissions": { "allow": ["Bash(prefix:git )"] }
this ran with no prompt:
git status
rm -rf /
The whole string still starts with `git `, so the prefix matched and both
commands executed. Same via `git status & rm -rf /`.
That defeats the entire point of prefix rules — they exist so a user can
authorise a narrow set of commands, and the check was authorising anything
chained after the first one.
## PowerShell was worse, and that part was mine (HIGH)
The previous PR added `PowerShell` to SENSITIVE_TOOLS and taught `rule_matches`
about its `command` field — but compound checking was dispatched only for
`Bash`, so PowerShell prefix rules got no splitting at all. Under a `Get-` rule,
`Get-Process; Remove-Item -Recurse -Force C:\` was auto-allowed.
Adding prefix rules without the splitting that makes them safe was a mistake in
the fix, not in the original code.
`check_compound_command` is now generic over the tool, and the dispatch
predicate `is_command_tool` lives in permissions rather than inline at the call
site so it can be asserted against SENSITIVE_TOOLS — a gated command tool that
is not compound-checked is exactly this bug returning.
## Bounding output did not bound memory
`stream_tx` is an UnboundedSender and `emit_line` forwarded every line before
the cap check, so the previous "output is bounded" fix bounded the captured
buffer only. A runaway command still queued a clone of every line. Forwarding
now stops at the cap while the pipe keeps draining (the child must still be able
to exit).
## PowerShell had drifted from Bash on two already-fixed classes
- `Command::output()` reads both pipes to EOF with no cap — the same OOM
already fixed for Bash.
- Nothing killed the child on timeout: dropping an `output()` future does not
kill the process without `kill_on_drop`, so a timed-out command and anything
it spawned kept running.
Both fixed by reusing Bash's `ProcessGroupGuard` and a shared bounded reader
rather than duplicating the logic a third time.
## Coverage gaps in my own tests
Verifying the fixes surfaced two:
- Reverting the run.rs dispatch left the PowerShell test green, because it
called the function directly and never exercised the wiring. Hence
`is_command_tool` as a testable predicate.
- Nothing measured the channel, so the unbounded-forwarding bug was invisible
to the existing bounds tests. Added a test that counts what reaches the UI.
QA: 588 tests, 0 failures, 30 binaries. Clippy clean under the CI gate. Release
19.06 MB. Zero panics in production code across all five Phase 1 files. Each fix
verified by reintroducing its bug — removing the separators fails 3 tests,
restoring unbounded forwarding fails the channel test, and narrowing
`is_command_tool` fails the drift test.
Co-Authored-By: Arch Linux <noreply@archlinux.org>
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.
Re-audit of Phase 1. Three real defects the first pass missed — two exploitable — plus two coverage gaps in my own tests.
Prefix allow-rules could be bypassed (pre-existing, HIGH)
split_compound_commandhandled&&,||,;and|but not newline or a bare&. Both separate commands in sh. So with a rule a security-conscious user would plausibly write:this ran with no prompt:
The whole string still starts with
git, so the prefix matched and both commands executed. Same viagit status & rm -rf /.That defeats the entire point of prefix rules: they exist so a user can authorise a narrow set of commands, and the check was authorising whatever got chained after the first one.
PowerShell was worse — and that part was mine (HIGH)
PR #12 added
PowerShelltoSENSITIVE_TOOLSand taughtrule_matchesabout itscommandfield, but compound checking was dispatched only forBash. So PowerShell prefix rules got no splitting at all — under aGet-rule,Get-Process; Remove-Item -Recurse -Force C:\was auto-allowed.Adding prefix rules without the splitting that makes them safe was a mistake in the fix, not in the original code.
check_compound_commandis now generic over the tool, and the dispatch predicateis_command_toollives inpermissionsrather than inline at the call site so it can be asserted againstSENSITIVE_TOOLS— a gated command tool that isn't compound-checked is exactly this bug returning.Bounding output did not bound memory
stream_txis anUnboundedSenderandemit_lineforwarded every line before the cap check, so PR #12's "output is bounded" fix bounded the captured buffer only. A runaway command still queued a clone of every line. Forwarding now stops at the cap while the pipe keeps draining — the child must still be able to exit.PowerShell had drifted from Bash on two already-fixed classes
Command::output()reads both pipes to EOF with no cap — the same OOM already fixed for Bash.output()future doesn't kill the process withoutkill_on_drop, so a timed-out command and anything it spawned kept running.Both fixed by reusing Bash's
ProcessGroupGuardand a shared bounded reader rather than duplicating the logic a third time.Coverage gaps in my own tests
Verifying the fixes surfaced two, which is the point of reintroducing every bug:
is_command_toolas a testable predicate.QA
588 tests, 0 failures, 30 binaries. Clippy clean under the CI gate. Release 19.06 MB. Zero panics in production code across all five Phase 1 files.
Each fix verified by reintroducing its bug: removing the separators fails 3 tests, restoring unbounded forwarding fails the channel test, narrowing
is_command_toolfails the drift test.