Repository navigation
fix: keep agent-supplied text out of the web app's code, and refuse ids no herdr pane could have - #92
Merged
Merged
Conversation
A row's handlers were written into its markup as openTerminal('<pane_id>'),
and an id is the agent side's: it comes from herdr pane list, over SSH for
every remote. Escaping the quote for HTML does not save that sink, because the
browser decodes the attribute before it parses the handler, so the quote is
live again by then and the rest of the id runs as JavaScript in the page that
holds the relay token. With the token an attacker has the relay's whole write
surface against every pane.
The ids now travel as data only and bindRowHandlers wires click, Enter and
Space from the dataset. The timeline also interpolated project and agent raw,
which needed no click at all: project is basename(cwd), so a cloned directory
picks it.
tests/test_web_injection.py drives the real message path in a browser and
asserts on a payload that breaks out of the quote rather than one carrying a
tag -- an 'is there a tag' assertion passes against a working exfiltration
payload, which contains no <.
Second half of the same hole, and the half that also covers the native clients, which do no escaping of their own. pane_id, workspace_id and tab_id were copied verbatim from herdr pane list, from the unauthenticated UDP socket on 127.0.0.1:8376, from the HTTP push query and from the WebSocket, then cached and broadcast. Every ingest path now checks the charset herdr's own ids use before the record is cached or broadcast. A record whose own id is malformed is dropped; a grouping id is blanked instead, so the pane stays answerable and only loses its chip. Nothing is rewritten: a sanitised id would name a different pane. re.fullmatch, not '...$': Python's $ also matches before a trailing newline, and a newline in an id is an unterminated string literal in a client.
Deploying herdr-remote with
|
| Latest commit: |
e50221f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://947302ac.herdr-remote.pages.dev |
| Branch Preview URL: | https://security-ghsa-cxfh-cwv7-wvwm.herdr-remote.pages.dev |
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.
Fixes GHSA-cxfh-cwv7-wvwm, reported by @sparkyfen.
Agent-supplied ids reached a JavaScript sink in the page that holds the relay token, and the token is the relay's whole write surface (
send_text,send_keys) against every pane.Reproduced on 21e292b before the fix
Driving
handleMessagein a browser the wayws.onmessagedoes:pane_idlanded inside the JS string of an inlineonclick. The existingescapeAttrturns the quote into', and the browser HTML-decodes the attribute before parsing the handler, so the quote is live by then: a real click read the token out oflocalStorage.projectandagentwere interpolated raw into the timeline, so a directory named<img src=x onerror=…>ran as the panel drew, with no click.projectisbasename(cwd), so a cloned repo or an unpacked archive picks it.The display fields on the cards were already escaped; these two sinks were not.
web
Row handlers are gone from the markup. Ids travel as
data-pane-id/data-ws-key/data-tab-keyonly, andbindRowHandlerswires click, Enter and Space from the dataset after render — extending the post-render pass the page already ran for long-press. Timeline fields and the session name/hostname are escaped.Preferred over per-sink JS-string escaping: with no handler in the markup there is no JS string for an id to sit in, so there is no escaping to get right at the next sink.
relay
ID_PATTERN/valid_id(re.fullmatch, never…$, which also matches before a trailing newline) gate every ingest path before anything is cached or broadcast:pane listfor local and everyHERDR_REMOTEShost, the workspace/tab hierarchy, theevent_pushconsumer, HTTP?d=, WebSocketagent_event, and the unauthenticated UDP socket on 127.0.0.1:8376.A record whose own id is malformed is dropped; a grouping id is blanked instead, so the pane stays answerable and only loses its chip. Nothing is rewritten — a sanitised id would name a different pane. Refusals log once per distinct value, not once per poll tick.
This half also covers the macOS and Windows clients, which do no escaping of their own.
Tests
tests/test_web_injection.py(7, browser) andRelayIdAllowlistTests/RelayIdAllowlistEventQueueTests(11) assert on a payload that breaks out of the quote, not on the absence of<img: a tag-absence assertion passes against a working exfiltration payload, which contains no<. The trailing-newline case is pinned, so swappingfullmatchforre.match(…+'$')fails.Mutation-checked: with the web fix stashed 4 of the 7 browser tests fail (the 3 that pass are the activation-parity ones); with the relay fix stashed the relay suite fails 8 and errors 18.
Local: 632 python tests OK, 234 browser tests OK,
tests/run.sh24 passed 0 failed.