diff --git a/docs/changes/2026-07-08-adversarial-input-audit.md b/docs/changes/2026-07-08-adversarial-input-audit.md new file mode 100644 index 0000000..99080d5 --- /dev/null +++ b/docs/changes/2026-07-08-adversarial-input-audit.md @@ -0,0 +1,129 @@ +# Adversarial input-validation audit — every dangerous sink fails closed + +- **Type:** negative-path / input-validation audit (no production code change) +- **Date:** 2026-07-08 +- **Area:** `internal/api`, `internal/naming`, `internal/submit`, `internal/rcon` +- **Task:** a different question from the #82 / round-2 mutation audits. Those asked + *do the TESTS catch a gate regression?* This asks *does the CODE reject hostile + INPUT, or does bad data PASS?* — feed the real endpoints malformed, boundary, and + hostile bodies (故意加错误数据) and confirm they fail closed (4xx) rather than letting + the garbage reach a sink. + +## Method — sink-first, not fuzz-everything + +The low-hanging garbage (oversized body, unknown field, wrong content-type) is already +caught by the universal body guards, so a blanket "fuzz all ~80 handlers" would burn +effort where the answer is known. The real "can bad data PASS?" risk lives at the +**sinks** — the few places a request string is concatenated into an RCON command, used +as a K8s object name, joined into a filesystem/archive path, put in a SQL query, or +accepted as an enum/quantity **without a validator in front**. So the audit traces each +dangerous sink class from its handler entry to the sink, and for the crown-jewel class +(text → RCON) **mutation-verifies** the guard is non-vacuously pinned: loosen the guard +in source, run the package tests, confirm the specifically-named negative test reddens +(`--- FAIL: `), then revert. Oracle: WSL Fedora-44, go1.26.4. + +## The universal belt (caught before any sink) + +`decodeJSON` (`internal/api/util.go`) wraps every body in +`http.MaxBytesReader(w, r.Body, 1<<20)` (1 MiB cap), sets `DisallowUnknownFields()`, and +rejects trailing data after the first JSON value — all → `400 bad_request`. +`requireJSONContentType` returns `415 unsupported_media_type` on credential writes +(a CSRF belt). So oversized, unknown-field, multi-document, and wrong-type bodies never +reach a handler body at all. + +## The dangerous sinks — each traced fail-closed + +### 1. Text → RCON (the lead). Two vectors, both fenced. + +**(a) Structured commands** — `handlers_access.go` (LuckPerms permission/group, +whitelist/ban/kick). Every operand is validated against an anchored allow-list charset +*before* it is concatenated: `mcNameRe = ^[A-Za-z0-9_]{1,16}$` (player), +`lpNodeRe = ^[A-Za-z0-9_.*-]{1,64}$` (node), `lpCtxRe = ^[A-Za-z0-9_-]{1,48}$` +(world/group). No space, separator, or control character can appear in a validated +operand, and **there is no free-text field anywhere** — a ban/kick deliberately carries +no reason string (that would be the one splice vector). Go's `$` is `\z` (absolute end, +not `\Z`), so even a single trailing `\n` is rejected. +*Mutation-verified:* loosening `mcNameRe` to admit a space +(`^[A-Za-z0-9_ ]{1,16}$`) reddens +`TestAccessInjectionRejected/{whitelist,ban,kick,permission}_player_space` — the guard +is real, not vacuous. + +**(b) Free-text passthrough** — `handlers_console.go` `handleCommand`, POST +`/servers/{name}/command`. This is the *one deliberate* free-text → RCON vector, and it +is **owner/admin-gated** (403 for a stranger, 404 for an unknown server). Its input +fence: trim + strip a single leading `/`, reject empty, cap at 1024 bytes, and +`strings.IndexFunc(command, func(c rune) bool { return c < 0x20 }) >= 0 → 400` — every +C0 control (incl. `\n`) is rejected so one request cannot splice a second command. +*Mutation-verified:* disabling the scan (`c < 0x20` → `c < 0x00`) reddens +`TestConsoleCommand/control_character_(newline)_->_400,_no_RCON_call`, whose input is +literally `{"command":"say hi\nop attacker"}` and whose assertion is 400 **and** +`console.calls == 0`. (Severity note: because this vector is owner-gated by design, the +scan is an audit-integrity measure — one request = one command — not a privilege +boundary; a splice on your *own* server escalates nothing, since the owner may already +run any RCON command. The fence exists regardless.) + +### 2. Break-glass / internal-face (the newest code — scrutinised specifically) + +This surface runs under a "service-token-authed / local-root, inputs trusted" posture, +the classic place a field-level guard gets skipped. Traced end to end: + +- **Server name** — every internal handler (`handleReady`, `handleJoinEvent`, + `handleInternalWake`, `handleInternalClaim`, `handleInternalMenuStatus`) validates the + path name with `naming.ValidateServerName` before use. +- **`mc_uuid`** (join/wake/claim bodies) — checked non-empty, then flows *only* to + DB-parameterized calls (`RecordJoin`, `UserByMCUUID`, `UUIDInAllowlist`). The + "allowlist" is a **DB table**, not a live RCON `whitelist add` — there is no + `mc_uuid` → RCON path. +- **Break-glass "OP-create"** (`performAddOperator` → `provisionOperator` → + `InsertOperator(ctx, id, username, email)`) is a **parameterized DB INSERT** creating a + *panel staff account*, **not** a Minecraft `op` RCON command. The hypothesised + name → RCON `op` sink was checked and **does not exist** in this shape; the username is + `TrimSpace`d and reaches only `$N`-parameterized SQL, from a local-root caller. +- **`os_user`** (internal backup attribution) — `TrimSpace`d, sets only the audit actor + (a DB row); shown non-vacuous in the round-2 backup/restore audit (`b7b4a3b`). + +### 3. SQL injection — dismissed. + +`pgrepo.go` uses uniform `$1/$2/$3` parameterization throughout +(`QueryRowContext`/`ExecContext(ctx, q, args…)`); no request string is `Sprintf`'d into a +query. + +### 4. Path traversal (submit) — dismissed. + +`internal/submit` validates the submission id (rejects `..`, path separators, uppercase, +space, empty), and the on-disk blob name is a **fixed** constant (`contextBlobName`) — no +attacker-supplied filename is ever joined. The hostile-id matrix +`{"../evil","sub/../../etc","SUB-UPPER","has space","","a/b"}` is test-pinned in both the +local and S3 backends. The one free-form field a submission carries (`DisplayName`) is +charset-constrained by `displayNameRE` and rejects control chars +(`submit_test.go` "control chars" case). + +### 5. K8s object names — validated at every cluster write. + +`ValidateServerName` / `ValidateSystemServerName` (`^[a-z0-9-]{3,32}$`, no +leading/trailing dash, reserved-name set) and `ValidateHostname` +(`dnsLabelRE`, single label under the configured root domain) gate every create/patch. +`cluster.go` documents the invariant: "there is no free-form YAML path — every field is a +typed, validated value," so no raw CRD field can be smuggled through a create/patch body. + +### 6. Numeric / enum — fail-closed. + +`resolveResources` routes **every** quantity (memory, `resources.cpu/memory`, requests) +through `parsePositiveQuantity`, which rejects `q.Sign() <= 0` (negative *and* zero) with +a field-named 400, plus a request>limit guard; `parseStorageSize` carries the same guard. +Enums are closed sets: `parseAutostartPolicy` (ownerOnly/public/allowlist), the +access-action switch, and the image `ImageAdmitted` allow-list (no free image string). + +## Verdict + +**Bad data does not pass.** Every dangerous sink is fail-closed — including the +break-glass / internal-face surface, where the hypothesised `mc_uuid`/OP-create → RCON +paths were traced and found not to exist (parameterized DB, not RCON). Both text → RCON +vectors — structured (`handlers_access`) and free-text (`handleCommand`) — are +mutation-pinned by their named negative tests. No gap was found and no production code +changed; the honest result of "故意加错误数据" is that the input surface rejects it. + +Scope is deliberately bounded to the dangerous sinks and the newest (break-glass) code, +not an exhaustive fuzz of all ~80 handlers — the claim proven is "every place user input +reaches a dangerous sink validates before the sink," by trace plus two mutations, not +"every handler was fuzzed." diff --git a/docs/changes/INDEX.md b/docs/changes/INDEX.md index 950d4e8..0561338 100644 --- a/docs/changes/INDEX.md +++ b/docs/changes/INDEX.md @@ -25,7 +25,7 @@ not yet committed. | Change | Detail doc | Status | |---|---|---| -| _None._ | | | +| Adversarial input-validation audit — sink-first negative-path trace; two text→RCON guards mutation-pinned; no gap, no production code change | [adversarial-input-audit](2026-07-08-adversarial-input-audit.md) | `[uncommitted]` (GPG pinentry timeout) — intended: `docs(changes): record the adversarial input-validation audit (sink-first negative-path)` | ## Detail docs @@ -252,3 +252,4 @@ primary record. | 729bd7b | 2026-07-07 | docs(changes): index the mutation audit and two lagging ledger rows | | c67a4d3 | 2026-07-08 | docs(changes): close §B4 with the S3 archive backend deferred by design | | 85b8a92 | 2026-07-08 | test(api): pin restore's owner gate against a superseded former owner | +| b7b4a3b | 2026-07-08 | docs(changes): record the round-2 backup/restore mutation audit and index the owner-gate test |