mathias/hyperguild
1
0
Fork 0
Code Issues 30 Pull Requests 3 Actions Packages Projects Releases Wiki Activity

brain_tunnel: path traversal lets an MCP token holder rewrite any file the pod can write #169

New Issue
Closed
opened 2026-10-04 20:24:01 +00:00 by mathias · 6 comments
mathias commented 2026-10-04 20:24:01 +00:00
Owner
Copy Link
Copy Source

Found 2026-10-04 while doing #157 slice 5. Verified by execution (TestWriteTunnelRefusesATargetThatTraversesOutOfTheBrain, red on main 57d0fb0).

The bug

brain_tunnel (internal/mcp/handlers.go) passes the caller's source and target to brain.WriteTunnel. That function checks only wingOf (wiki/<wing>/... with at least 4 segments), then calls filepath.Join(brainDir, path). Join cleans .., so wiki/b/x/../../../../<anything>.md resolves outside the brain. appendSeeAlso then reads that file and rewrites it with an appended ## See also link.

Effect

Anyone with the brain MCP token (agents; the public /mcp endpoint is bearer-authenticated) can append text to any existing file the pod user can write:

  • anything in the brain mount: raw/, sessions/, classification.yaml, other wings
  • any writable path outside it

The file must exist. There is no read-back, since the response is only {"status":"ok"}. The rewrite also turns symlinks into followed writes.

Fix

PR to follow:

  • validate a clean, local wiki/.../<slug>.md path
  • read and write through brainfs, with a new WriteRegular (Lstat + O_NOFOLLOW + os.SameFile on the handle)
  • don't rewrite the source when the target is refused

Still to check after the fix

  • Logs: whether past brain_tunnel calls with .. happened. Audit logs are in Loki (the audit middleware logs the tool name and args?).
  • Other handlers: a grep for filepath.Join(brainDir, <caller path>) found no other unvalidated caller path. IndexDoc gets brain_tunnel's raw paths, and #168 confines that read.

Created via git-mcp on behalf of @mathias

**Found 2026-10-04 while doing #157 slice 5. Verified by execution** (`TestWriteTunnelRefusesATargetThatTraversesOutOfTheBrain`, red on main 57d0fb0). ## The bug `brain_tunnel` (`internal/mcp/handlers.go`) passes the caller's `source` and `target` to `brain.WriteTunnel`. That function checks only `wingOf` (`wiki/<wing>/...` with at least 4 segments), then calls `filepath.Join(brainDir, path)`. `Join` cleans `..`, so `wiki/b/x/../../../../<anything>.md` resolves outside the brain. `appendSeeAlso` then reads that file and rewrites it with an appended `## See also` link. ## Effect Anyone with the brain MCP token (agents; the public `/mcp` endpoint is bearer-authenticated) can append text to **any existing file the pod user can write**: - anything in the brain mount: `raw/`, `sessions/`, `classification.yaml`, other wings - any writable path outside it The file must exist. There is no read-back, since the response is only `{"status":"ok"}`. The rewrite also turns symlinks into followed writes. ## Fix PR to follow: - validate a clean, local `wiki/.../<slug>.md` path - read and write through brainfs, with a new `WriteRegular` (Lstat + O_NOFOLLOW + os.SameFile on the handle) - don't rewrite the source when the target is refused ## Still to check after the fix - **Logs:** whether past `brain_tunnel` calls with `..` happened. Audit logs are in Loki (the audit middleware logs the tool name and args?). - **Other handlers:** a grep for `filepath.Join(brainDir, <caller path>)` found no other unvalidated caller path. `IndexDoc` gets `brain_tunnel`'s raw paths, and #168 confines that read. --- _Created via git-mcp on behalf of @mathias_
mathias added the risk/high label 2026-10-04 20:24:01 +00:00
mathias commented 2026-10-04 20:25:05 +00:00
Author
Owner
Copy Link
Copy Source

Log check, 2026-10-04. Loki query {namespace="supervisor"} |= "mcp tool call" over 30 days returns 83 lines in total, all from the recent audit logging:

Tool Calls
brain_write 32
brain_query 26
brain_get 9
brain_read 9
brain_edit 6
brain_context 1
session_log 1

There are zero brain_tunnel calls, and no logged line contains brain_tunnel at all. So nothing in the logged window exploited this. Calls before tool-call logging began can't be checked, because before that the server logged nothing per tool call. Fix PR: #170, which is in adversarial review.

Created via git-mcp on behalf of @mathias

**Log check, 2026-10-04.** Loki query `{namespace="supervisor"} |= "mcp tool call"` over 30 days returns 83 lines in total, all from the recent audit logging: | Tool | Calls | |---|---| | brain_write | 32 | | brain_query | 26 | | brain_get | 9 | | brain_read | 9 | | brain_edit | 6 | | brain_context | 1 | | session_log | 1 | There are **zero `brain_tunnel` calls**, and no logged line contains `brain_tunnel` at all. So nothing in the logged window exploited this. **Calls before tool-call logging began can't be checked**, because before that the server logged nothing per tool call. Fix PR: #170, which is in adversarial review. --- _Created via git-mcp on behalf of @mathias_
mathias commented 2026-10-04 20:38:18 +00:00
Author
Owner
Copy Link
Copy Source

Correction: the line "a grep found no other unvalidated caller path" was wrong. The PR #170 review checked it by execution and found two more handlers with the same class of bug:

  • brain_update: the caller's path was checked only lexically against the brain root. Any brain file could be superseded (including classification.yaml, raw/ and sessions/), and a final symlink was written through.
  • session_log: session_id was not validated, so a caller could create or append a .jsonl file outside the sessions dir.

Both are fixed in PR #170 (round 2, head below), which this issue now covers. brain_edit, brain_promote/brain_reject, brain_ingest and BuildWingIndex already validate their paths; the reviewer ran the same probes against them and they refused.

The audit-log check on the earlier comment covers these too. In the logged window: session_log 1 call, which was a normal session id from this session's own tooling, and brain_update 0 calls.

Created via git-mcp on behalf of @mathias

**Correction:** the line "a grep found no other unvalidated caller path" was wrong. The PR #170 review checked it by execution and found two more handlers with the same class of bug: - **brain_update:** the caller's path was checked only lexically against the brain root. Any brain file could be superseded (including `classification.yaml`, `raw/` and `sessions/`), and a final symlink was written through. - **session_log:** `session_id` was not validated, so a caller could create or append a `.jsonl` file outside the sessions dir. Both are fixed in PR #170 (round 2, head below), which this issue now covers. `brain_edit`, `brain_promote`/`brain_reject`, `brain_ingest` and BuildWingIndex already validate their paths; the reviewer ran the same probes against them and they refused. The audit-log check on the earlier comment covers these too. In the logged window: **session_log 1 call**, which was a normal session id from this session's own tooling, and **brain_update 0 calls**. --- _Created via git-mcp on behalf of @mathias_
mathias commented 2026-10-04 20:38:31 +00:00
Author
Owner
Copy Link
Copy Source

Precision on the session_log call: the log line (2026-10-03 16:30, principal=local-cli) doesn't record the session_id, so I can't say from the log alone what it was. The only sessions file modified that day is sessions/bb979e40-bbaf-56fa-8d31-f0bf8f3a740c.jsonl, a plain UUID name, which matches that call. Nothing suggests a traversal, but that is an inference, not proof. Logging session_id (sanitised) in the tool-call line would make it checkable.

Created via git-mcp on behalf of @mathias

**Precision on the session_log call:** the log line (2026-10-03 16:30, principal=local-cli) doesn't record the `session_id`, so I can't say from the log alone what it was. The only sessions file modified that day is `sessions/bb979e40-bbaf-56fa-8d31-f0bf8f3a740c.jsonl`, a plain UUID name, which matches that call. Nothing suggests a traversal, but that is an inference, not proof. Logging `session_id` (sanitised) in the tool-call line would make it checkable. --- _Created via git-mcp on behalf of @mathias_
mathias commented 2026-10-04 20:44:15 +00:00
Author
Owner
Copy Link
Copy Source

Gatekeeper verdict on PR #170, round 2 (87fa28e): BLOCKED. Full review is on the PR (comments 15038 for round 1, 15040 for round 2).

The brain_tunnel, brain_update and session_log fixes hold under execution through the MCP handlers. Two guards in session.Append have no test that fails when they are removed: the ban on / in session ids, and the O_NONBLOCK that is its only FIFO guard. Both need a test that goes red.

Residuals to record explicitly:

  • Writes (brain_update, brain_tunnel, session_log) follow a static intermediate in-brain directory symlink. #167 is a read-side race, so it does not cover this.
  • DetectTunnels blocks on a FIFO under wiki/, which hangs brain_update and brain_write. This was already true on main.
**Gatekeeper verdict on PR #170, round 2 (87fa28e): BLOCKED.** Full review is on the PR (comments 15038 for round 1, 15040 for round 2). The brain_tunnel, brain_update and session_log fixes hold under execution through the MCP handlers. Two guards in `session.Append` have no test that fails when they are removed: the ban on `/` in session ids, and the `O_NONBLOCK` that is its only FIFO guard. Both need a test that goes red. Residuals to record explicitly: - Writes (brain_update, brain_tunnel, session_log) follow a static intermediate in-brain directory symlink. #167 is a read-side race, so it does not cover this. - DetectTunnels blocks on a FIFO under wiki/, which hangs brain_update and brain_write. This was already true on main.
mathias commented 2026-10-05 05:09:14 +00:00
Author
Owner
Copy Link
Copy Source

Gatekeeper verdict on PR #170, round 3 (6325c2b): PASS. The full review is on the PR. This is risk/high, so merging is Mathias's call. Nothing was merged.

  • Round-2 blockers: both have tests now, and each flip went red. Allowing / in the session id breaks the new escape test, and dropping O_NONBLOCK breaks the new FIFO test.
  • Rebase resolution: WriteRegular and its test were only moved, with byte-identical bodies, and nothing from #168 was dropped.
  • Probes: 192 probes through the brain_tunnel, brain_update and session_log handlers, judged by filesystem snapshot, caused 0 changes. The same harness on main 77e32de recorded 52 escapes.
  • Gates: CI run 9283 is green. CI=1 task check exited 0, and -race -count=10 passed. The PR also passes on top of the current main, b6ef116.
  • Residuals: filed as #173 and #174. Neither got worse in the rebase.
**Gatekeeper verdict on PR #170, round 3 (6325c2b): PASS.** The full review is on the PR. This is `risk/high`, so **merging is Mathias's call**. Nothing was merged. - **Round-2 blockers:** both have tests now, and each flip went red. Allowing `/` in the session id breaks the new escape test, and dropping `O_NONBLOCK` breaks the new FIFO test. - **Rebase resolution:** `WriteRegular` and its test were only moved, with byte-identical bodies, and nothing from #168 was dropped. - **Probes:** 192 probes through the brain_tunnel, brain_update and session_log handlers, judged by filesystem snapshot, caused 0 changes. The same harness on main 77e32de recorded 52 escapes. - **Gates:** CI run 9283 is green. `CI=1 task check` exited 0, and `-race -count=10` passed. The PR also passes on top of the current main, b6ef116. - **Residuals:** filed as #173 and #174. Neither got worse in the rebase.
mathias commented 2026-10-05 18:51:03 +00:00
Author
Owner
Copy Link
Copy Source

Closed by #170 (febab3e), verified live, 2026-10-05. The pod runs the merge image; the graph backfill indexed 1593 notes with no errors.

  • Live brain_tunnel with a .. target is refused: "not a clean wiki note path".
  • Live session_log with a path-segment id is refused: "session id is not a plain name".
  • brain_update: I did not run a live probe (the session's tool-safety classifier blocked it). Its refusal of non-note paths is covered by TestUpdateNoteRefusesPathsOutsideKnowledgeAndWikiNotes, the race test, and the reviewer's end-to-end handler probes in rounds 2–3.

Follow-ups: #173 (static intermediate in-brain dir redirect on writes) and #174 (DetectTunnels FIFO hang, PR coming next).

Created via git-mcp on behalf of @mathias

**Closed by #170 (febab3e), verified live, 2026-10-05.** The pod runs the merge image; the graph backfill indexed 1593 notes with no errors. - **Live brain_tunnel** with a `..` target is refused: "not a clean wiki note path". - **Live session_log** with a path-segment id is refused: "session id is not a plain name". - **brain_update:** I did not run a live probe (the session's tool-safety classifier blocked it). Its refusal of non-note paths is covered by `TestUpdateNoteRefusesPathsOutsideKnowledgeAndWikiNotes`, the race test, and the reviewer's end-to-end handler probes in rounds 2–3. Follow-ups: #173 (static intermediate in-brain dir redirect on writes) and #174 (DetectTunnels FIFO hang, PR coming next). --- _Created via git-mcp on behalf of @mathias_
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
agent/ready
Meets the agent-ready contract; authorizes unattended execution
 blocked/client
Waiting on the client
 blocked/supplier
Waiting on a supplier
 blocked/upstream
Waiting on an upstream project or vendor
 risk/high
Irreversible, security-relevant, or control plane
 risk/low
Reversible, local, test-covered
 risk/medium
Touches shared state, deploys or data
 size/l
Too big - split it
 size/m
Under two days
 size/s
Under half a day
 size/xs
Under an hour
 status:dispatch
status:dispatched
status:review
status:reviewing
type/chore
Maintenance, no behaviour change
 type/decision
Human decision required; never agent/ready
 type/docs
Documentation only
 type/feat
New capability
 type/fix
Corrects broken behaviour
 type/ops
Infrastructure, deploy, runtime
 type/research
Investigate; output is a finding, not code
 type/test
Test coverage only
No labels risk/high
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
renovate-bot mathias
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mathias/hyperguild#169