Bound every slog line against client-chosen text (closes #176)
All checks were successful
check / check (push) Successful in 2m52s
All checks were successful
check / check (push) Successful in 2m52s
MaxBodySize logged r.URL.Path untruncated at WARN, and routes.go registers it ahead of RequireAuth, so an unauthenticated POST /source/<8 KB>/edit with an oversize declared Content-Length wrote attacker-chosen text of attacker-chosen length into the operator's log, for the cost of a request with no body. The 2,560-byte per-line budget #146 established did not reach it: that budget lives in the access-log field capping and this is a separate slog call. The capping mechanism moves out of internal/middleware into internal/logfield so there is one budget and one implementation rather than a second ad-hoc truncation. Truncate and EncodedBytes are unchanged; the access log now spends logfield.MaxBytes where it spent maxLogFieldBytes. The sweep the issue asked for found five more call sites of the same shape, all reachable unauthenticated, all now capped: the CSRF 403 (also registered ahead of RequireAuth), the rate limiters' 429 (the per-entrypoint receiver limiter is unauthenticated), RequireAuth's own DEBUG line, the unknown-entrypoint DEBUG line on the receiver, and the failed-login DEBUG lines. DEBUG being off by default is not a bound: an operator turning it on to diagnose a flood must not thereby hand the flood an unbounded write. Every other slog call in the tree was read and judged; the PR body lists all of them, including the ones left alone and why. MaxBodySize stays ahead of RequireAuth. An oversize body should be refused before the request buys a cookie decrypt and a session load, and rejecting first is what keeps an unauthenticated flood from choosing how much session work the process does. The ordering and what it costs are now written at the registration, on maxFormBodySize. MaxAccessLogLineBytes is restated as the ceiling on every slog line carrying a client-supplied value, not just the access log's: each of these lines carries strictly fewer client-supplied fields than the access log does, so none can be wider. That is asserted per line under both handlers rather than argued. Two writers are called out as NOT covered, so the figure is not read as more than it is: the log delivery target, which exists to emit the whole event and is deliberate, and GORM's default logger, which prints the interpolated SQL to stdout on a record-not-found and is unbounded on the receiver and login lookups. That second one is a real defect this audit turned up and is filed separately as #178, not fixed here. Tests drive 8 KB of client-chosen text at all six sites, through both handlers internal/logger can install and through each character they escape — including a bare C0 control, which costs six bytes on the line against the one it cost to send and is the case a raw-byte budget breaks on first. Each holds the encoded line to the ceiling, holds the whole flood's output to what that ceiling allows, and asserts the markers at the far end of the input are absent, so a value that merely happened to be short cannot pass. internal/logfield gains a test that measures the per-rune charge against what the handlers really emit over roughly 3,000 code points on each, so an undercharged rune fails a test instead of quietly falsifying the ceiling. Verified by mutation: reverting the MaxBodySize cap alone fails 12 subtests with a 16,583-byte line against the 2,560 ceiling; reverting the other five fails 70; budgeting raw bytes instead of encoded ones fails 23 across three packages.
This commit is contained in:
70
README.md
70
README.md
@@ -991,15 +991,16 @@ costs; log volume it caps rather than eliminates. A path that names no
|
||||
entrypoint is recorded by the handler at `DEBUG`, and the aggregate
|
||||
limiter logs its own rejections at `DEBUG` and without the path, so
|
||||
neither appears at all under the default level. The per-entrypoint
|
||||
limiter is the loud one: it still logs every rejection at `WARN` with
|
||||
the request path, which on this route is attacker-controlled text. A
|
||||
client hammering a single invented path is served `RECEIVER_RATE_LIMIT`
|
||||
limiter is the loud one: it logs every rejection at `WARN` with the
|
||||
request path, which on this route is attacker-controlled text. A client
|
||||
hammering a single invented path is served `RECEIVER_RATE_LIMIT`
|
||||
requests and has the rest of its aggregate budget rejected there, so
|
||||
the aggregate limit is what bounds those `WARN` lines — to under ten
|
||||
times `RECEIVER_RATE_LIMIT` per minute per client IP, 1080 at the
|
||||
defaults, where before it there was no bound at all. The access log is
|
||||
bounded by neither limit: every request is recorded once at `INFO`,
|
||||
served or rejected alike.
|
||||
the aggregate limit is what bounds the _number_ of those `WARN` lines —
|
||||
to under ten times `RECEIVER_RATE_LIMIT` per minute per client IP, 1080
|
||||
at the defaults, where before it there was no bound at all. Their
|
||||
_width_ is bounded by the field budgets below, the same ones the access
|
||||
log spends. The access log is bounded by neither limit: every request
|
||||
is recorded once at `INFO`, served or rejected alike.
|
||||
|
||||
What the access log does bound is the _content_ of those lines. A 3xx
|
||||
or 4xx response logs the chi route pattern — `/webhook/{uuid}`,
|
||||
@@ -1063,6 +1064,59 @@ that the rate is not bounded by the limits above on every route:
|
||||
`/.well-known/healthcheck` and `/s/*` sit behind no limiter, so there
|
||||
the multiplier is whatever the deployment will serve.
|
||||
|
||||
**The same ceiling covers every other line the service writes through
|
||||
`slog`.** The access log is not the only line a client can put its own
|
||||
text into, and a budget that held for one line and not the others would
|
||||
be worse than no stated budget at all. Every other `slog` call that
|
||||
reaches a client-chosen value spends the same per-field budget through
|
||||
`internal/logfield`, and each carries strictly fewer client-supplied
|
||||
fields than the access log does, so none of them can be wider than it:
|
||||
|
||||
| Log line | Level | Client-chosen value | Reachable unauthenticated |
|
||||
| ------------------------------------------ | ------- | ------------------- | ----------------------------------------- |
|
||||
| `request body exceeds limit` (413) | `WARN` | path, method | yes — `MaxBodySize` precedes `RequireAuth` |
|
||||
| `csrf: token validation failed` (403) | `WARN` | path, method | yes — `CSRF` precedes `RequireAuth` |
|
||||
| `... rate limit exceeded` (429) | `WARN` | path | yes, on the receiver |
|
||||
| `auth middleware: unauthenticated request` | `DEBUG` | path, method | yes, by definition |
|
||||
| `entrypoint not found` | `DEBUG` | entrypoint UUID | yes, on the receiver |
|
||||
| `user not found` / `invalid password` | `DEBUG` | username | yes, on the login form |
|
||||
|
||||
`DEBUG` being off by default is not a bound. An operator turning it on
|
||||
to diagnose a flood must not thereby hand the flood an unbounded write,
|
||||
so those lines are capped too.
|
||||
|
||||
`internal/middleware/logbound_test.go` and
|
||||
`internal/handlers/logbound_test.go` drive 8 KB of client-chosen text at
|
||||
each of these, through both handlers and through every character the
|
||||
handlers escape, and hold each line to the 2,560-byte ceiling — and the
|
||||
whole flood's output to what that ceiling allows, which is the property
|
||||
an operator actually cares about.
|
||||
`internal/logfield/logfield_test.go` measures the per-rune charge
|
||||
against what the handlers really emit, over roughly 3,000 code points on
|
||||
each, so an undercharged rune fails a test rather than quietly
|
||||
falsifying the ceiling.
|
||||
|
||||
Two writers that ceiling does **not** cover, stated here so the figure
|
||||
is not read as more than it is:
|
||||
|
||||
- The **`log` delivery target**, which writes the whole inbound event —
|
||||
headers and body — to the log. This one is deliberate: capping it
|
||||
would defeat the target, since emitting the payload is the delivery.
|
||||
It costs nothing unless an authenticated operator creates a target of
|
||||
that type on a specific webhook, and each line it writes is bounded
|
||||
per event by the 1 MB receiver body cap. Adding one is a decision to
|
||||
spend log volume on that webhook's payloads.
|
||||
- **GORM's default logger**, which prints the fully interpolated SQL to
|
||||
stdout on every record-not-found — including the client-chosen path
|
||||
on `/webhook/{uuid}` and the submitted username on the login form.
|
||||
This one is not deliberate and not yet fixed; it does not go through
|
||||
`internal/logger` at all, so no level the operator sets and no budget
|
||||
above applies to it. Tracked at
|
||||
<https://git.eeqj.de/sneak/webhooker/issues/178>. Until it is fixed,
|
||||
an unauthenticated flood can still write text of its own choosing and
|
||||
its own length to the operator's stdout, and the ceiling above
|
||||
describes only the `slog` half of the picture.
|
||||
|
||||
Every limiter here — receiver, login, and password change — identifies
|
||||
the client the same way, through one shared key function: the
|
||||
connection's own address, unless the peer is listed in
|
||||
|
||||
Reference in New Issue
Block a user