fix: shell-quote delivery hook command paths (F14) - #487
Merged
Conversation
…resolution delivery.sh spliced $project/$type into SessionStart/SessionEnd/Stop hook commands with a naive '$var' wrap; a project path containing a single quote broke the argument boundary and let the rest run as shell syntax on the next hook event. Add _agmsg_shq (proper '...' escaping with embedded quotes doubled via '\'') and use it for every hook argument. windows/dispatch.sh's kv_get() word-split whoami.sh's output and returned the first "key=value" match. Since the only attacker-choosable field (a registered agent name) always sorts first in that line, a name containing a space and a fake "teams=" token could shadow the real trailing team field and silently misdirect a caller's inbox/send/history to an attacker-controlled team. kv_get now keeps the last match, matching the rightmost-wins behavior check-inbox.sh's own parser already relies on.
co1's PR #487 review caught that this doesn't close the finding: whoami.sh's single-match line is "agent=<n> teams=<t> type=<ty> project=<p>", and project (like agent name) is attacker-influenceable and sorts LAST. A project path crafted to contain a trailing "teams=<x>" token defeats last-match the same way the original bug defeated first-match — the line's ambiguity isn't resolvable by picking a match order, since both ends can carry attacker content. A real fix needs whoami.sh to emit a delimiter-safe format and every consumer (check-inbox.sh, identities.sh, rename-team.sh, reset.sh, dispatch.sh) to be updated to match — an interface change, not a contained parsing fix. Reverting to keep this PR scoped to the F14 fix, which stands on its own.
fujibee
changed the title
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
1 participant
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
delivery.sh's SessionStart/SessionEnd/Stop hook commands spliced$project/$typeinto a naive'$var'-wrapped argument. A project directory path containing a single quote broke the argument boundary, letting the rest of the string run as unintended shell syntax on the next hook event. Added_agmsg_shq, a proper single-quote-escaping helper, and used it for every hook argument.Scope note: this PR originally also included a
dispatch.shkv_get()fix (F19). co1's review caught that the fix was insufficient —whoami.sh's line format hasprojectas its last field, and project paths are attacker-influenceable too, so a "keep the last match" strategy is defeated by a poisoned project path the same way the original bug was defeated by a poisoned agent name. A real fix needswhoami.shto emit a delimiter-safe format and every consumer updated to match (an interface change, not a contained parsing fix), so that piece was reverted out of this PR and reclassified.Test plan
bats tests/test_delivery.bats— all green, including two new tests exercising a project path with an embedded single quote inmonitorandturnmodes (verifying the resulting hook command round-trips through a real shell as exactly 3 arguments)