Security audit: the authorization surface has already failed once, and nothing systematic looks at it

[hanthor]

This is not hypothetical. In one afternoon, reading code that had passed review and a green CI, two authorization defects were found:

• Check membership before answering a sliding-sync subscription #257 — a sliding-sync room_subscriptions entry was never membership-checked. Any registered account could name any room ID and be sent its name and full timeline.
• Check who is asking before handing over a room #258 — nine more read handlers with the same defect: /rooms/{id}/state, /state/{type}/{key}, /messages, /event/{eventId}, /context/{eventId}, three /relations/ spellings, /threads. Four of them bound Authenticated(_identity) and discarded it at the binding site. Confirmed by probe: HTTP 200 with private message bodies to a user in no rooms at all.

The federation surface was stricter than the client surface the whole time — federation_room_origin refuses a remote server with no member in the room. A peer could not read what a local stranger could.

Both were found by a human reading, not by a test, a lint, or a review. That is the thing to fix: the finding rate is currently "however much someone happens to look".

What an audit should cover

Authorization, systematically. Every handler, against a matrix of caller states: stranger, invited, joined, left, banned, server admin, appservice, appservice ghost. The property is not "does it 403" but "what does a caller in state X learn". #258's test is a route table walked by two tests; the same shape should cover every route, and the table should be generated from the router rather than hand-maintained, so a new route is in it by default.

History visibility properly. #258 implements joined-or-world_readable and is knowingly stricter than the spec: a user who has left a room can no longer read the history they are entitled to. That is a deliberate, recorded deviation and it needs finishing — bounding each read at the caller's departure point.

Rate limiting and resource exhaustion. [ratelimit] exists and every test disables it. What is actually limited, and what is not? Unbounded per-user growth was found and capped in one place this week (#264, delayed events) by reading the issue that predicted it — the same question has not been asked of filters, account data, aliases, media, device lists, or one-time keys.

Media. serve_media and thumbnail_media take _identity and discard it: any authenticated user can fetch any MXC by ID. That matches Synapse's authenticated-media model, where possession of the URI is the capability — but it composes badly with a room-read hole, since an attacker who can read a private room's state can harvest the URIs. Worth an explicit decision rather than an inherited one.

Write authorization, not just reads. create_alias lets any authenticated user attach an alias to any existing room — it checks rooms.exists and not membership. may_publish argues the opposite policy for directory visibility three hundred lines away. One of the two is wrong.

The signing and federation trust boundary. Key rotation, server_keys caching and expiry, what happens when a peer presents a key we have not seen, and whether a signature failure is distinguishable from a network failure to the caller.

Secrets in logs and errors. MatrixError::internal deliberately hides detail from the response and logs it — check nothing else does the reverse.

Shape

Not a one-off document. The valuable output is:

1. A findings list with severity and a reproduction for each, filed as individual issues.
2. Tests, in the ordinary suite, that fail on the unfixed behaviour — the thing Check who is asking before handing over a room #258 added and the reason it will not silently regress.
3. A generated route-coverage table, so "which endpoints have an authorization test" is a number that CI can hold at 100% rather than a question nobody asks.

An external audit is worth having eventually. It is worth much more after the above, because otherwise it spends its budget finding what a route table would have.

Related

• Static analysis: nothing looks at this code except clippy and a human #266 — static analysis, including a custom rule for exactly the Check who is asking before handing over a room #258 defect shape
• Fuzz the parsers: every byte a stranger controls reaches one #267 — fuzzing the parsers a stranger's bytes reach
• Check who is asking before handing over a room #258 — the fix, and the deviation it knowingly leaves open

[hanthor]

Progress on the audit's concrete items:

Item	State
Write authorization: create_alias vs may_publish	#291 — one predicate, may_advertise, for both; membership or server admin. The doc comment's old argument that aliases were different is recorded as reversed rather than deleted.
Generated route-coverage table	#293 — all 35 room-scoped client routes (40 rows), each Refused or Exempt with a written reason; a test reads src/routes.rs and fails on any route missing from or stale in the table.
Found by that walk	POST /receipt/… and POST /read_markers accepted a stranger's receipt into a private room, fanned out to its members. Fixed in Rooms::set_receipt (#293), pinned by a named receipts test.
Media: serve_media discards identity	Decision drafted as ADR 0003 (the URI is the capability; 128-bit random IDs; room-scoped authorization is the control) with two pinning tests. PR next.
Signing / federation trust boundary	Partly covered by #286 (origin grammar gate) and #288 (address ranges, open). Key rotation and expiry not yet audited.
Rate limiting inventory, history visibility for left users, secrets in logs	Not started.

Also found while gating, not an authorization item: #292, an intermittent shutdown deadlock in fjall's Drop reached through the delayed-events fire loop holding the last store reference.

---

Generated by Claude Code