Эх сурвалжийг харах

Annotate security audit with post-fix statuses

Every SA finding now records what shipped, what was scoped out by the human's RFC decisions, and the top follow-ups (SA-5 default-deny guards, logout revocation, FormDataParser limits, login rate limiting).
saddle-Clanker-Saddle 4 долоо хоног өмнө
parent
commit
c72fd6eb81

+ 93 - 13
docs/security-audit.md

@@ -98,7 +98,17 @@ frame the client held is still cryptographically valid:
 with a copy of the frame (localStorage, a proxy log, a backup) re-enters as
 with a copy of the frame (localStorage, a proxy log, a backup) re-enters as
 that user until the signing key changes.
 that user until the signing key changes.
 
 
-**Fix direction:** add a server-side validity epoch: include a monotonically
+**Status: PARTIALLY ADDRESSED (approved scope: password changes only).**
+`M0002` adds `spry_users.sessions_valid_after`; `UserService.set_password`
+bumps it (self- and admin-forced changes); `SpryAuth.revalidate` fails
+closed on frames whose signed `as_at` predates the epoch, and
+`ChangePasswordAction` re-mints the slot in the same response so the
+changing user stays logged in while every other device is revoked.
+**Logout still does not revoke** — the human explicitly accepted this
+residual risk for now; revisit by bumping the epoch in `LogoutAction` (one
+line, but it then logs out every device).
+
+**Fix direction (original):** add a server-side validity epoch: include a monotonically
 increasing `user`-scoped version (or a per-slot generation counter) in the
 increasing `user`-scoped version (or a per-slot generation counter) in the
 signed snapshot, and have `revalidate`/`restore_slot` compare it against a
 signed snapshot, and have `revalidate`/`restore_slot` compare it against a
 `users` row (e.g. `sessions_valid_after` timestamp bumped on logout and on
 `users` row (e.g. `sessions_valid_after` timestamp bumped on logout and on
@@ -151,7 +161,15 @@ routing bug is fixed (or on the SHALLOW/EXACT match paths, which use
 with a traversal regression test (`/assets/../../etc/passwd`, URL-encoded
 with a traversal regression test (`/assets/../../etc/passwd`, URL-encoded
 `%2e%2e%2f`, symlink escape).
 `%2e%2e%2f`, symlink escape).
 
 
-**Fix direction:** canonicalise with
+**Status: ADDRESSED.** The `**` wildcard remainder now comes from the
+request's segments (deep static routes work again), and the guard rejects
+any relative path containing `..`, resolves both root and target
+symlink-free, and requires `resolved == root || resolved.has_prefix(root +
+"/")`. Regression tests cover `../../../etc/passwd`, `%2e%2e%2f`, symlink
+escape, sibling-directory and `/www` vs `/www2` boundaries (38-check Astralis
+suite).
+
+**Fix direction (original):** canonicalise with
 `File.resolve_symbolic_link_async` (or `realpath`), then require
 `File.resolve_symbolic_link_async` (or `realpath`), then require
 `resolved_path == resolved_server_path ||
 `resolved_path == resolved_server_path ||
 resolved_path.has_prefix(resolved_server_path + "/")`. Reject any request
 resolved_path.has_prefix(resolved_server_path + "/")`. Reject any request
@@ -187,7 +205,15 @@ Statum/src/ChannelSubscriptionEndpoint.vala:30 (`read_all`)
 POSTs OOMs the process; slow-loric attacks also hold buffered memory for the
 POSTs OOMs the process; slow-loric attacks also hold buffered memory for the
 connection lifetime.
 connection lifetime.
 
 
-**Fix direction:** pass `MHD_OPTION_CONNECTION_LIMIT` and
+**Status: ADDRESSED (core limits).** The MHD daemon starts with
+CONNECTION_LIMIT (256), PER_IP_CONNECTION_LIMIT (32) and
+CONNECTION_MEMORY_LIMIT (128 KiB), all configurable via the `server`
+section; a configurable `max_request_body` (default 10 MiB, 0 disables)
+rejects with 413 on declared Content-Length and defensively while streaming
+in `ServerInput.write`. The FormDataParser whole-body multipart copy and
+O(n·m) boundary scan remain — noted as follow-up work.
+
+**Fix direction (original):** pass `MHD_OPTION_CONNECTION_LIMIT` and
 `MHD_OPTION_PER_IP_CONNECTION_LIMIT` in `Daemon.start`; enforce a configurable
 `MHD_OPTION_PER_IP_CONNECTION_LIMIT` in `Daemon.start`; enforce a configurable
 `max_request_body` (reject >413 immediately, ideally as soon as
 `max_request_body` (reject >413 immediately, ideally as soon as
 `Content-Length` is known, and defensively while streaming in
 `Content-Length` is known, and defensively while streaming in
@@ -217,7 +243,12 @@ slot indefinitely (compounding SA-1: a logged-out session frame can be
 re-cached forever). Also a slow memory leak: frames replayed on a schedule
 re-cached forever). Also a slow memory leak: frames replayed on a schedule
 keep their slots pinned.
 keep their slots pinned.
 
 
-**Fix direction:** in `restore_slot`, parse `invalid_after` from the
+**Status: ADDRESSED.** `restore_slot` now rejects frames past
+`invalid_after` (new `StateError.FRAME_EXPIRED`, surfaced as an `invalidate`
+directive by `SlotPostEndpoint`) and keeps the snapshot's `as_at` as
+`last_touched`, so restored slots still expire via the cleanup sweep.
+
+**Fix direction (original):** in `restore_slot`, parse `invalid_after` from the
 snapshot and reject (with `invalidate`) frames past it; do not reset
 snapshot and reject (with `invalidate`) frames past it; do not reset
 `last_touched` to `now` on restore — keep the snapshot's `as_at`.
 `last_touched` to `now` on restore — keep the snapshot's `as_at`.
 
 
@@ -248,7 +279,14 @@ the signed snapshot; the authoritative checks are `require_login` /
    bars, but any *decision* made on `current()` rather than the guards is a
    bars, but any *decision* made on `current()` rather than the guards is a
    vulnerability. The doc comments say this, but nothing enforces it.
    vulnerability. The doc comments say this, but nothing enforces it.
 
 
-**Fix direction:** add an opt-out (not opt-in) authorisation hook:
+**Status: NOT ADDRESSED — top follow-up.** SA-5 was not included in the
+approved RFC scope (the endpoint-level guard hooks of SA-6 shipped instead).
+All shipped Spry actions do call the guards (verified per file), and
+disabled/deleted accounts are caught by `revalidate` on every guarded
+request, but nothing yet *forces* a guard on a new action — the framework
+default remains allow.
+
+**Fix direction (original):** add an opt-out (not opt-in) authorisation hook:
 `StatumConfigurator` gains a `default_guard` invoked by
 `StatumConfigurator` gains a `default_guard` invoked by
 `StatumAction.handle_request` before `handle()`, or an
 `StatumAction.handle_request` before `handle()`, or an
 `[RequiresPermission]`-style registration-time declaration that
 `[RequiresPermission]`-style registration-time declaration that
@@ -280,7 +318,13 @@ session-agnostic data leak is reachable unauthenticated; SSE also gives an
 uncosted resource to anonymous clients. The framework cannot know what is
 uncosted resource to anonymous clients. The framework cannot know what is
 sensitive, so it must let apps gate these endpoints.
 sensitive, so it must let apps gate these endpoints.
 
 
-**Fix direction:** provide guard hooks on both endpoints (e.g. an
+**Status: ADDRESSED (hooks provided, default open).**
+`StatumConfigurator.guard_entrypoint/guard_channel/guard_channel_subscriptions`
+install an `AuthorizationPolicy` consulted by the three endpoints — return
+null to allow, an `HttpResult` (401/redirect) to deny. Spry guards are the
+intended implementations; documented in Statum's getting-started.
+
+**Fix direction (original):** provide guard hooks on both endpoints (e.g. an
 overridable `authorize(http_context)` returning a 401/redirect result),
 overridable `authorize(http_context)` returning a 401/redirect result),
 documented as "put your auth guard here"; consider binding channel ids to
 documented as "put your auth guard here"; consider binding channel ids to
 the held auth slot so a channel leak cannot be used cross-session.
 the held auth slot so a channel leak cannot be used cross-session.
@@ -410,7 +454,11 @@ exploits the mismatch — the signer string is informational — but the API
 invites trust in a field it does not validate, and future multi-key support
 invites trust in a field it does not validate, and future multi-key support
 would silently build on it.
 would silently build on it.
 
 
-**Fix direction:** `verify_frame` should decode `frame.signer` (it is
+**Status: ADDRESSED (single-signer contract).** `verify_frame` asserts
+`frame.signer == signer_id` and fails closed on mismatch; `verify_with_key`
+is documented as the explicit path for a second signer during rotation.
+
+**Fix direction (original):** `verify_frame` should decode `frame.signer` (it is
 `Base64(public_key)`) and use `verify_with_key` with it, falling back to the
 `Base64(public_key)`) and use `verify_with_key` with it, falling back to the
 local key only when the signer is unknown — or simply assert
 local key only when the signer is unknown — or simply assert
 `frame.signer == signer_id` and document the single-signer contract.
 `frame.signer == signer_id` and document the single-signer contract.
@@ -443,7 +491,11 @@ Residual surfaces:
 3. **`get_user_by_username` timing** on unknown vs known username is
 3. **`get_user_by_username` timing** on unknown vs known username is
    equalised only by the dummy hash — correct as written.
    equalised only by the dummy hash — correct as written.
 
 
-**Fix direction:** decide and document the enumeration stance for
+**Status: NOT ADDRESSED (explicit human decision).** The human chose to keep
+distinct username/email registration messages for usability; login already
+answers generically. Revisit only if a threat model change demands it.
+
+**Fix direction (original):** decide and document the enumeration stance for
 registration; optionally rate-limit register by IP (SA-8).
 registration; optionally rate-limit register by IP (SA-8).
 
 
 ---
 ---
@@ -460,7 +512,12 @@ The tool prints all three secret keys to stdout for pasting into
 the file at 0600, but direct use (as the Statum warnings instruct) leaks to
 the file at 0600, but direct use (as the Statum warnings instruct) leaks to
 whatever captured the output.
 whatever captured the output.
 
 
-**Fix direction:** add `--out <file>` (write 0600 directly) and print only
+**Status: ADDRESSED.** `statum-genkeys --out <file>` writes the full config
+at mode 0600 (creating parent dirs); without `--out`, stdout carries only
+the public halves in the same JSON shape plus a pointer to `--out`. `spry
+keys` was adapted to spawn `--out` against a 0600 tempfile.
+
+**Fix direction (original):** add `--out <file>` (write 0600 directly) and print only
 public halves to the terminal; or print secrets only with an explicit
 public halves to the terminal; or print secrets only with an explicit
 `--print-secrets` acknowledgement.
 `--print-secrets` acknowledgement.
 
 
@@ -481,7 +538,13 @@ SSE sends `no-cache`, but HTML pages and static files carry no defaults.
 Given the client renders server data via `innerHTML` paths (stm-html /
 Given the client renders server data via `innerHTML` paths (stm-html /
 stm-markdown), a CSP would materially shrink the XSS blast radius.
 stm-markdown), a CSP would materially shrink the XSS blast radius.
 
 
-**Fix direction:** a pipeline component in Astralis that appends a
+**Status: ADDRESSED.** `SecurityHeadersComponent` appends
+`X-Content-Type-Options: nosniff`, `X-Frame-Options: SAMEORIGIN`,
+`Referrer-Policy: strict-origin-when-cross-origin` and a CSP template by
+default; endpoint-set headers win; everything is overridable/disableable
+via the `securityHeaders` config section, and HSTS is strictly opt-in.
+
+**Fix direction (original):** a pipeline component in Astralis that appends a
 configurable default header set (`nosniff`, `frame-ancestors 'self'`,
 configurable default header set (`nosniff`, `frame-ancestors 'self'`,
 `referrer-policy strict-origin-when-cross-origin`, and a CSP template apps
 `referrer-policy strict-origin-when-cross-origin`, and a CSP template apps
 extend); HSTS only behind explicit opt-in (it bricks local HTTP dev
 extend); HSTS only behind explicit opt-in (it bricks local HTTP dev
@@ -506,7 +569,11 @@ the root of SA-1's severity: the auth slot is a bearer token with no
 revocation and no subject binding beyond `user_id` inside the encrypted
 revocation and no subject binding beyond `user_id` inside the encrypted
 half.
 half.
 
 
-**Fix direction:** document the bearer-token model explicitly; for the auth
+**Status: ADDRESSED for the auth slot (see SA-1's epoch); bearer model
+documented.** `sessions_valid_after` binds the auth slot to server-side
+validity; the general bearer model of other slot types stands as designed.
+
+**Fix direction (original):** document the bearer-token model explicitly; for the auth
 slot specifically, bind it (inside the encrypted private half, which the
 slot specifically, bind it (inside the encrypted private half, which the
 server controls) to a server-generated session id that logout and password
 server controls) to a server-generated session id that logout and password
 change invalidate — this is the same mechanism SA-1 needs, and one
 change invalidate — this is the same mechanism SA-1 needs, and one
@@ -528,7 +595,14 @@ connections or total subscriptions, and the keep-alive loop wakes every
 main loop). Combined with SA-3's missing `MHD_OPTION_CONNECTION_LIMIT`,
 main loop). Combined with SA-3's missing `MHD_OPTION_CONNECTION_LIMIT`,
 an anonymous client can open thousands of channel streams.
 an anonymous client can open thousands of channel streams.
 
 
-**Fix direction:** cap concurrent SSE connections per IP and globally;
+**Status: ADDRESSED (Statum side).** 256 global / 16 per-IP concurrent
+channel connections (released on disconnect, swept when closed) and 64
+subscriptions per channel, over-cap answers 503; connection caps also run
+before the SSE result is created via `ChannelGatewayEndpoint`. The
+Astralis `SseEndpoint` 100 ms keep-alive poll remains as-is (cheap; the
+connection caps bound the total wakeups now).
+
+**Fix direction (original):** cap concurrent SSE connections per IP and globally;
 bound subscriptions per channel; lengthen the poll interval or drive close
 bound subscriptions per channel; lengthen the poll interval or drive close
 notification from the disconnect signal rather than polling `is_closed`.
 notification from the disconnect signal rather than polling `is_closed`.
 
 
@@ -611,7 +685,13 @@ only as *values* (`stm-text` → `textContent`, `stm-html` → innerHTML,
 `stm-markdown` → the CommonMark renderer), so no injection path exists in
 `stm-markdown` → the CommonMark renderer), so no injection path exists in
 the shipped stack. This is a sharp edge to document and defend.
 the shipped stack. This is a sharp edge to document and defend.
 
 
-**Fix direction:** document "attribute expressions are trusted template
+**Status: DOCUMENTED (accepted for now).** Documented in Statum's
+getting-started "Server-side Security" section as a page-author contract:
+expressions are trusted authoring; never bind user data into expression
+*source*. No shipped component does. A whitelist expression parser remains
+possible future work.
+
+**Fix direction (original):** document "attribute expressions are trusted template
 authoring, never bind them to user data"; consider a small expression
 authoring, never bind them to user data"; consider a small expression
 whitelist parser (paths, comparisons, ternaries) to make misuse impossible.
 whitelist parser (paths, comparisons, ternaries) to make misuse impossible.