itsybitsy/AUDIT.md

118 lines
11 KiB
Markdown
Raw Normal View History

# Security and robustness audit — 2026-10-04
Audit of itsybitsy at commit `c29364d8` ("serve gemini behind a tls terminator"), covering integration behaviour, adversarial input and load. Three defects were found and fixed in `49a63503`; the remaining items are recorded below rather than fixed, because each is a design decision rather than a bug.
The headline result is the first finding: a single content file could abort the whole process, taking every virtual host with it, in a way the existing panic handler could not catch.
## Method
A release binary built with `--features "wml figlet hyphenation"` was run against a purpose-built content tree with two sites and seven listeners — HTTP (three: negotiating, single-format, and one with `max_connections = 5`), Spartan, Gemini, Nex and Gopher. Requests were sent as raw bytes over real sockets, so each protocol's own parser was exercised rather than a client library's idea of it.
The tree deliberately contained things a well-behaved tree would not: a canary file outside both roots, symlinks pointing to that file, to `/etc/passwd` and to `/tmp`, a hidden page, files whose names carry CR LF and tab bytes, documents nested thousands of levels deep, an 8 MiB single line, a 200 000-row table, a 2.5 GB Markdown file, include cycles, a 40-deep include chain, and a 2²⁴ diamond include fan-out.
138 checks ran in four phases:
| Phase | Checks | Covers |
| --- | --- | --- |
| Integration | 59 | All five protocols end to end, negotiation, virtual hosting, every output format, card sub-documents, live reload |
| Containment | 16 | 27 traversal encodings × 5 protocols, dotfiles, symlink escapes, cross-site isolation |
| Protocol abuse | 23 | Response splitting, request smuggling, header flooding, `Host` abuse, request caps, upload abuse, slow clients |
| Exhaustion | 25 | Nesting depth, oversized documents, include bombs, include and art containment |
| Load | 15 | Concurrency, throughput, the connection cap, memory growth |
Everything of lasting value is now in the repository's own suite as 13 regression tests. The ad-hoc harness was not kept.
## Findings
### 1. Deep nesting aborted the process — critical, fixed
A served document containing 10 000 nested block quotes killed the server outright:
```
thread '<unknown>' has overflowed its stack
fatal runtime error: stack overflow, aborting
[exited with code 134]
```
Severity comes from three things together. A stack overflow raises SIGABRT rather than unwinding, so the `catch_unwind` at the handler boundary — which exists precisely so one bad document cannot take the process down — could not contain it. One process serves every configured site, so the failure is not scoped to the site whose content caused it. And nothing restarts the process on its own.
The Markdown parser was not at fault: pulldown-cmark handled 100 000 levels without trouble, being iterative. The recursion was ours, in three separate walks over the parsed document — the directive pass, each renderer's block walk, and the derived `Drop` for `Block`. The depth at which it died therefore depended on available stack, not on any limit in the code: a debug test thread died at 1 000 levels where the release server survived to 10 000.
**Fix.** A single cap at the parse boundary (`MAX_NESTING = 100` in `core/src/parse.rs`), checked where the nesting depth is already explicit as the builder's open-container stacks. Rewriting three recursive walks would have been the alternative; capping once means none of them can see a tree deep enough to matter, including any walk added later. An over-deep document is refused whole — HTTP 500, Gemini 40, Spartan 5 — rather than truncated at the limit, which would serve a page missing most of its content with nothing to indicate it.
The limit is two orders of magnitude above real prose. Ten levels of nesting is already unusual.
A secondary result worth recording, because it stops someone adding a limit that is not needed: inline nesting cannot run away. 150 consecutive `*` produce 75 levels of emphasis and 150 consecutive `[` produce one, because pulldown-cmark pairs delimiters and forbids links from nesting at all. Only block containers are unbounded. A test pins this.
### 2. A filename could inject a response header — moderate, fixed
```
GET /ev%0d%0aX-Injected:%20yes.md
HTTP/1.1 301 Moved Permanently
Location: /ev
X-Injected: yes <- injected
Connection: close
```
`url_for` interpolated the resolved filename into a URL with no encoding, and that URL is emitted as a redirect target. Spartan (`3 /ev\r\nX-Injected: yes`) and Gemini (`31 ...`) terminate their status lines the same way and split identically — one defect, three protocols.
Exploiting it requires a file named `ev\r\nX-Injected: yes.md`, which is legal on Linux. Under the current threat model the content tree is author-controlled, which is what keeps this moderate rather than critical; it becomes remotely reachable the moment any part of a tree accepts contributions from someone who is not the operator.
**Fix.** `url_for` now percent-encodes everything outside RFC 3986's unreserved set, leaving `/` as the separator. `clean_path` already decodes on the way in, so URLs round-trip, and a test asserts that. This also repaired a quieter bug that had not been noticed: filenames containing a space, `?`, `#`, `%` or non-ASCII characters previously produced URLs that were wrong or truncated. `/my%20notes`, `/a%23b` and `/caf%C3%A9` now resolve.
### 3. Large files were read before being rejected — low, fixed
`preprocess::expand` called `fs::read_to_string` on each file, allocating it in full, and only then applied the 8 MiB expansion cap. A 2.5 GB Markdown file in the tree therefore cost 2.5 GB of transient allocation per request, despite no more than 8 MiB of it ever being usable. With the default `max_connections = 256`, concurrent requests multiply that.
**Fix.** The file's length is checked before it is opened, and the read itself goes through a capped reader so the bound holds even if the file grew since the check.
## Results after the fixes
Containment held everywhere. 27 traversal encodings — double-encoded, overlong UTF-8, backslash, NUL byte, `..;/`, absolute paths — were tried against all five protocols, 135 attempts, and none returned the canary or `/etc/passwd`. Symlinks out of the root were refused on the resolved path, not the requested one. Neither site could read the other's tree, and the server configuration file, which sits outside both roots, was unreachable by every spelling tried.
No request shape broke a handler. Absolute-form and authority-form targets, missing and bogus HTTP versions, bare LF line endings, NUL bytes, invalid UTF-8, a 5 000-byte method, `Transfer-Encoding` with `Content-Length`, and a pipelined second request all produced exactly one response per connection and left the server serving. A `Host` carrying CR LF, NUL, 5 000 bytes or non-ASCII injected nothing. Every protocol enforced its request cap: Gemini 1026 bytes, Spartan 4096, Nex 2048, Gopher 512. A Spartan upload claiming 10 GB was refused in under three seconds rather than drained.
Include bombs were all bounded: the cycle, the self-reference, the 40-deep chain and the 2²⁴ diamond fan-out each produced an error and a live server, the fan-out caught by the byte cap that a per-stack cycle check cannot see.
Load behaved well:
| Measurement | Result |
| --- | --- |
| 32 concurrent clients, 3 200 requests | 8 464 req/s, 0 errors, p50 3.5 ms, p99 7.7 ms |
| 64 concurrent clients | 8 009 req/s, 0 errors |
| Mixed load across all five protocols | 9 248 req/s, 0 errors |
| 6 400 requests, leak check | RSS flat at 122 660 KiB throughout |
| 8 concurrent readers of a 4 MiB page | RSS delta 0 KiB — one shared cached copy |
| Connection cap of 5, 10 excess connections | 10/10 closed immediately; 13 threads while held, 8 when idle |
## Outstanding
**The render cache is unbounded.** Measured from a cold start: 105 pages totalling 16 588 KiB of Markdown, rendered into five formats, grew RSS from 3 876 KiB to 106 092 KiB — a cache cost of **6.2× the source size**. Memory is flat under repeated requests, so this is growth by distinct page visited rather than a leak, and at capsule scale it is fine. 6.2× is the multiplier to size `cache_max_bytes` with when LRU eviction lands.
**An oversized header produces a TCP reset instead of the 400.** The server writes the status and closes while the client is still sending, so the client may never read the response. nginx behaves similarly. Fixing it means draining a bounded amount before closing, as the Spartan listener already does for uploads.
**`ListenerSpec.width` is accepted and ignored.** A per-listener width cannot work until the render cache is keyed on `(format, width)` rather than format alone, so this is not a one-line change. Either delete the key or key the cache.
**No graceful shutdown.** SIGTERM terminates mid-response. Queued for the operations milestone along with SIGHUP reload.
## What this audit did not cover
The Gemini listener expects TLS to be terminated in front of it, and **that leg was never tested** — no `stunnel`, `ghostunnel` or `openssl` was available on the audit machine and no network to fetch one. Only the plaintext protocol behind the terminator was exercised. The `stunnel` configuration in the README is written from its documentation, not from a run.
Also out of scope: no coverage-guided fuzzing of the Markdown parser or the protocol readers, which is the right tool for the input-handling code and would likely find more than hand-written cases do. All load testing was over loopback on one machine, so the figures measure the server rather than any network. No audit of the dependency tree for known advisories. The threat model throughout assumes the content tree is author-controlled; findings 1 and 2 both become materially more serious if that stops being true, and that is the assumption most worth revisiting.
## Reproducing
The three findings are pinned by tests in the repository:
- `core/src/parse.rs` — `nesting_past_the_cap_is_refused_rather_than_overflowing_the_stack`, `deeply_repeated_inline_markers_are_bounded_by_the_parser_itself`
- `core/src/path.rs` — `url_for_escapes_bytes_that_would_end_a_response_header`, `an_encoded_url_still_resolves_back_to_the_same_path`
- `core/src/preprocess.rs` — `an_oversized_file_is_refused_without_being_read_into_memory`
- `bin/tests/listeners.rs` — `a_document_nested_past_the_cap_is_an_error_and_the_server_survives`, `an_injected_redirect_target_is_escaped_on_every_protocol`
```bash
devbox run check # 320 tests
devbox run -- cargo test --workspace --features "wml figlet hyphenation" # 375 tests
```