# MCIAS Security Audit Report

**Date:** 2026-03-14 (updated — penetration test round 4)
**Original audit date:** 2026-03-13
**Auditor role:** Penetration tester (code review + live instance probing)
**Scope:** Full codebase and running instance at mcias.metacircular.net:8443 — authentication flows, token lifecycle, cryptography, database layer, REST/gRPC/UI servers, authorization, headers, and operational security.
**Methodology:** Static code analysis, live HTTP probing, architectural review.

---

## Executive Summary

MCIAS has a strong security posture. All findings from the first three audit rounds (CRIT-01/CRIT-02, DEF-01 through DEF-10, and SEC-01 through SEC-12) have been remediated. The cryptographic foundations are sound, JWT validation is correct, SQL injection is not possible, XSS is prevented by Go's html/template auto-escaping, and CSRF protection is well-implemented.

A fourth-round penetration test (PEN-01 through PEN-07) against the live instance at `mcias.metacircular.net:8443` identified 7 new findings: 2 medium, 2 low, and 3 informational. **Unauthorized access was not achieved** — the system's defense-in-depth held. See the open findings table below for details.

---

## Open Findings (PEN-01 through PEN-07)

Identified during the fourth-round penetration test on 2026-03-14 against the live instance at `mcias.metacircular.net:8443` and the source code at the same commit.

| ID | Severity | Finding | Status |
|----|----------|---------|--------|
| PEN-01 | Medium | `extractBearerFromRequest` does not validate "Bearer " prefix | **Fixed** — uses `strings.SplitN` + `strings.EqualFold` prefix validation, matching middleware implementation |
| PEN-02 | Medium | Security headers missing from live instance responses | **Fixed** — redeployed; all headers confirmed present on live instance 2026-03-15 |
| PEN-03 | Low | CSP `unsafe-inline` on `/docs` Swagger UI endpoint | **Accepted** — self-hosting Swagger UI (1.7 MB) to enable nonces adds complexity disproportionate to the risk; inline script is static, no user-controlled input |
| PEN-04 | Info | OpenAPI spec publicly accessible without authentication | **Accepted** — intentional; public access required for agents and external developers |
| PEN-05 | Info | gRPC port 9443 publicly accessible | **Accepted** — intentional; required for server-to-server access by external systems |
| PEN-06 | Low | REST login increments lockout counter for missing TOTP code | **Fixed** — `RecordLoginFailure` removed from TOTP-missing branch; `TestTOTPMissingDoesNotIncrementLockout` added |
| PEN-07 | Info | Rate limiter is per-IP only, no per-account limiting | **Accepted** — per-account hard lockout (10 failures/15 min) already covers distributed brute-force; per-account rate limiting adds marginal benefit at this scale |

<details>
<summary>Finding descriptions (click to expand)</summary>

### PEN-01 — `extractBearerFromRequest` Does Not Validate "Bearer " Prefix (Medium)

**File:** `internal/server/server.go` (lines 1414–1425)

The server-level `extractBearerFromRequest` function extracts the token by slicing the `Authorization` header at offset 7 (`len("Bearer ")`) without first verifying that the header actually starts with `"Bearer "`. Any 8+ character `Authorization` value is accepted — e.g., `Authorization: XXXXXXXX` would extract `X` as the token string.

```go
// Current (vulnerable):
if len(auth) <= len(prefix) {
    return "", fmt.Errorf("malformed Authorization header")
}
return auth[len(prefix):], nil  // no prefix check
```

The middleware-level `extractBearerToken` in `internal/middleware/middleware.go` (lines 303–316) correctly uses `strings.SplitN` and `strings.EqualFold` to validate the prefix. The server-level function should be replaced with a call to the middleware version, or the same validation logic should be applied.

**Impact:** Low in practice because the extracted garbage is then passed to JWT validation which will reject it. However, it violates defense-in-depth: a future change to token validation could widen the attack surface, and the inconsistency between the two extraction functions is a maintenance hazard.

**Recommendation:** Replace `extractBearerFromRequest` with a call to `middleware.extractBearerToken` (after exporting it or moving the function), or replicate the prefix validation.

**Fix:** `extractBearerFromRequest` now uses `strings.SplitN` and `strings.EqualFold` to validate the `"Bearer"` prefix before extracting the token, matching the middleware implementation. Test `TestExtractBearerFromRequest` covers valid tokens, missing headers, non-Bearer schemes (Token, Basic), empty tokens, case-insensitive matching, and the previously-accepted garbage input.

---

### PEN-02 — Security Headers Missing from Live Instance Responses (Medium)

**Live probe:** `https://mcias.metacircular.net:8443/login`

The live instance's `/login` response did not include the security headers (`X-Content-Type-Options`, `Strict-Transport-Security`, `Cache-Control`, `Permissions-Policy`) that the source code's `globalSecurityHeaders` and UI `securityHeaders` middleware should be applying (SEC-04 and SEC-10 fixes).

This is likely a code/deployment discrepancy — the deployed binary may predate the SEC-04/SEC-10 fixes, or the middleware may not be wired into the route chain correctly for all paths.

**Impact:** Without HSTS, browsers will not enforce HTTPS-only access. Without `X-Content-Type-Options: nosniff`, MIME-type sniffing attacks are possible. Without `Cache-Control: no-store`, authenticated responses may be cached by proxies or browsers.

**Recommendation:** Redeploy the current source to the live instance and verify headers with `curl -I`.

**Fix:** Redeployed 2026-03-15. Live probe confirms all headers present on `/login`, `/v1/health`, and `/`:
`cache-control: no-store`, `content-security-policy`, `permissions-policy`, `referrer-policy`, `strict-transport-security: max-age=63072000; includeSubDomains`, `x-content-type-options: nosniff`, `x-frame-options: DENY`.

---

### PEN-03 — CSP `unsafe-inline` on `/docs` Swagger UI Endpoint (Low)

**File:** `internal/server/server.go` (lines 1450–1452)

The `docsSecurityHeaders` wrapper sets a Content-Security-Policy that includes `script-src 'self' 'unsafe-inline'` and `style-src 'self' 'unsafe-inline'`. This is required by Swagger UI's rendering approach, but it weakens CSP protection on the docs endpoint.

**Impact:** If an attacker can inject content into the Swagger UI page (e.g., via a reflected parameter in the OpenAPI spec URL), inline scripts would execute. The blast radius is limited to the `/docs` path, which requires no authentication (see PEN-04).

**Recommendation:** Consider serving Swagger UI from a separate subdomain or using CSP nonces instead of `unsafe-inline`. Alternatively, accept the risk given the limited scope.

---

### PEN-04 — OpenAPI Spec Publicly Accessible Without Authentication (Informational)

**Live probe:** `GET /openapi.yaml` returns the full API specification without authentication.

The OpenAPI spec reveals all API endpoints, request/response schemas, authentication flows, and error codes. While security-through-obscurity is not a defense, exposing the full API surface to unauthenticated users provides a roadmap for attackers.

**Recommendation:** Consider requiring authentication for `/openapi.yaml` and `/docs`, or accept the risk if the API surface is intended to be public.

---

### PEN-05 — gRPC Port 9443 Publicly Accessible (Informational)

**Live probe:** Port 9443 accepts TLS connections and serves gRPC.

The gRPC interface is accessible from the public internet. While it requires authentication for all RPCs, exposing it increases the attack surface (gRPC-specific vulnerabilities, protocol-level attacks).

**Recommendation:** If gRPC is only used for server-to-server communication, restrict access at the firewall/network level. If it must be public, ensure gRPC-specific rate limiting and monitoring are in place (SEC-06 fix applies here).

---

### PEN-06 — REST Login Increments Lockout Counter for Missing TOTP Code (Low)

**File:** `internal/server/server.go` (lines 271–277)

When a TOTP-enrolled account submits a login request without a TOTP code, the REST handler calls `s.db.RecordLoginFailure(acct.ID)` before returning the `"TOTP code required"` error. This increments the lockout counter even though the user has not actually failed authentication — they simply omitted the TOTP field.

The gRPC handler was fixed for this exact issue in DEF-08, but the REST handler was not updated to match.

```go
// Current (REST — increments lockout for missing TOTP):
if acct.TOTPRequired {
    if req.TOTPCode == "" {
        s.writeAudit(r, model.EventLoginFail, &acct.ID, nil, `{"reason":"totp_missing"}`)
        _ = s.db.RecordLoginFailure(acct.ID)  // should not increment
        middleware.WriteError(w, http.StatusUnauthorized, "TOTP code required", "totp_required")
        return
    }
```

**Impact:** An attacker who knows a username with TOTP enabled can lock the account by sending 10 login requests with a valid password but no TOTP code. The password must be correct (wrong passwords also increment the counter), but this lowers the bar from "must guess TOTP" to "must omit TOTP." More practically, legitimate users with buggy clients that forget the TOTP field could self-lock.

**Recommendation:** Remove the `RecordLoginFailure` call from the TOTP-missing branch, matching the gRPC handler's behavior after the DEF-08 fix.

**Fix:** `RecordLoginFailure` removed from the TOTP-missing branch in `internal/server/server.go`. The branch now matches the gRPC handler exactly, including the rationale comment. `TestTOTPMissingDoesNotIncrementLockout` verifies the fix: it fully enrolls TOTP via the HTTP API, sets `LockoutThreshold=1`, issues a TOTP-missing login, and asserts the account is not locked.

---

### PEN-07 — Rate Limiter Is Per-IP Only, No Per-Account Limiting (Informational)

The rate limiter uses a per-IP token bucket. An attacker with access to multiple IP addresses (botnet, cloud instances, rotating proxies) can distribute brute-force attempts across IPs to bypass the per-IP limit.

The account lockout mechanism (10 failures in 15 minutes) provides a secondary defense, but it is a blunt instrument — it locks out the legitimate user as well.

**Recommendation:** Consider adding per-account rate limiting as a complement to per-IP limiting. This would cap login attempts per username regardless of source IP, without affecting other users. The account lockout already partially serves this role, but a softer rate limit (e.g., 1 req/s per username) would slow distributed attacks without locking out the user.

</details>

---

## Remediated Findings (SEC-01 through SEC-12)

All findings from the SEC audit round have been remediated. The original descriptions are preserved below for reference.

| ID | Severity | Finding | Status |
|----|----------|---------|--------|
| SEC-01 | Medium | TOTP enrollment did not require password re-authentication | **Fixed** — both REST and gRPC now require current password, with lockout counter on failure |
| SEC-02 | Medium | Account lockout response leaked account existence | **Fixed** — locked accounts now return same 401 `"invalid credentials"` as wrong password, with dummy Argon2 for timing uniformity |
| SEC-03 | Medium | Token renewal had no proximity or re-auth check | **Fixed** — renewal requires token to have consumed ≥50% of its lifetime |
| SEC-04 | Low-Med | REST API responses lacked security headers | **Fixed** — `globalSecurityHeaders` middleware applies `X-Content-Type-Options`, HSTS, and `Cache-Control: no-store` to all routes |
| SEC-05 | Low | No request body size limit on REST API | **Fixed** — `decodeJSON` wraps body with `http.MaxBytesReader` (1 MiB); max password length enforced |
| SEC-06 | Low | gRPC rate limiter ignored TrustedProxy | **Fixed** — `grpcClientIP` extracts real client IP via metadata when peer matches trusted proxy |
| SEC-07 | Low | Static file directory listing enabled | **Fixed** — `noDirListing` wrapper returns 404 for directory requests |
| SEC-08 | Low | System token issuance was not atomic | **Fixed** — `IssueSystemToken` wraps revoke+track in a single SQLite transaction |
| SEC-09 | Info | Navigation bar exposed admin UI structure to non-admin users | **Fixed** — nav links conditionally rendered with `{{if .IsAdmin}}` |
| SEC-10 | Info | No `Permissions-Policy` header | **Fixed** — `Permissions-Policy: camera=(), microphone=(), geolocation=(), payment=()` added |
| SEC-11 | Info | Audit log details used `fmt.Sprintf` instead of `json.Marshal` | **Fixed** — `audit.JSON` and `audit.JSONWithRoles` helpers use `json.Marshal` |
| SEC-12 | Info | Default token expiry was 30 days | **Fixed** — default reduced to 7 days (168h); renewal proximity check (SEC-03) further limits exposure |

<details>
<summary>Original finding descriptions (click to expand)</summary>

### SEC-01 — TOTP Enrollment Does Not Require Password Re-authentication (Medium)

**Files:** `internal/server/server.go`, `internal/grpcserver/auth.go`

`POST /v1/auth/totp/enroll` and the gRPC `EnrollTOTP` RPC originally required only a valid JWT — no password confirmation. If an attacker stole a session token, they could enroll TOTP on the victim's account.

**Fix:** Both endpoints now require the current password, with lockout counter incremented on failure.

---

### SEC-02 — Account Lockout Response Leaks Account Existence (Medium)

Locked accounts originally returned HTTP 429 / gRPC `ResourceExhausted` with `"account temporarily locked"`, distinguishable from the HTTP 401 `"invalid credentials"` returned for wrong passwords.

**Fix:** All login paths now return the same `"invalid credentials"` response for locked accounts, with dummy Argon2 to maintain timing uniformity.

---

### SEC-03 — Token Renewal Has No Proximity or Re-auth Check (Medium)

`POST /v1/auth/renew` originally accepted any valid token regardless of remaining lifetime.

**Fix:** Renewal now requires the token to have consumed ≥50% of its lifetime before it can be renewed.

---

### SEC-04 — REST API Responses Lack Security Headers (Low-Medium)

API endpoints originally returned only `Content-Type` — no `Cache-Control`, `X-Content-Type-Options`, or HSTS.

**Fix:** `globalSecurityHeaders` middleware applies these headers to all routes (API and UI).

---

### SEC-05 — No Request Body Size Limit on REST API Endpoints (Low)

`decodeJSON` originally read from `r.Body` without any size limit.

**Fix:** `http.MaxBytesReader` with 1 MiB limit added to `decodeJSON`. Maximum password length also enforced.

---

### SEC-06 — gRPC Rate Limiter Ignores TrustedProxy (Low)

The gRPC rate limiter originally used `peer.FromContext` directly, always getting the proxy IP behind a reverse proxy.

**Fix:** `grpcClientIP` now reads from gRPC metadata headers when the peer matches the trusted proxy.

---

### SEC-07 — Static File Directory Listing Enabled (Low)

`http.FileServerFS` served directory listings by default.

**Fix:** `noDirListing` wrapper returns 404 for directory requests.

---

### SEC-08 — System Token Issuance Is Not Atomic (Low)

`handleTokenIssue` originally performed three sequential non-transactional operations.

**Fix:** `IssueSystemToken` wraps all operations in a single SQLite transaction.

---

### SEC-09 — Navigation Bar Exposes Admin UI Structure to Non-Admin Users (Informational)

Nav links were rendered for all authenticated users.

**Fix:** Admin nav links wrapped in `{{if .IsAdmin}}` conditional.

---

### SEC-10 — No `Permissions-Policy` Header (Informational)

The security headers middleware did not include `Permissions-Policy`.

**Fix:** `Permissions-Policy: camera=(), microphone=(), geolocation=(), payment=()` added.

---

### SEC-11 — Audit Log Details Use `fmt.Sprintf` Instead of `json.Marshal` (Informational)

Audit details were constructed with `fmt.Sprintf` and `%q`, which is fragile for JSON.

**Fix:** `audit.JSON` and `audit.JSONWithRoles` helpers use `json.Marshal`.

---

### SEC-12 — Default Token Expiry Is 30 Days (Informational / Configuration)

Default expiry was 720h (30 days).

**Fix:** Reduced to 168h (7 days). Combined with SEC-03's renewal proximity check, exposure window is significantly reduced.

</details>

---

## Previously Remediated Findings (CRIT/DEF series)

The following findings from the initial audit (2026-03-12) were confirmed fixed in the 2026-03-13 audit:

| ID | Finding | Status |
|----|---------|--------|
| CRIT-01 | TOTP replay attack — no counter tracking | **Fixed** — `CheckAndUpdateTOTPCounter` with atomic SQL, migration 000007 |
| CRIT-02 | gRPC `EnrollTOTP` called `SetTOTP` instead of `StorePendingTOTP` | **Fixed** — now calls `StorePendingTOTP` |
| DEF-01 | No rate limiting on UI login | **Fixed** — `loginRateLimit` applied to `POST /login` |
| DEF-02 | `pendingLogins` map had no expiry cleanup | **Fixed** — `cleanupPendingLogins` goroutine runs every 5 minutes |
| DEF-03 | Rate limiter ignored `X-Forwarded-For` | **Fixed** — `ClientIP()` respects `TrustedProxy` config |
| DEF-04 | Missing `nbf` claim on tokens | **Fixed** — `NotBefore: jwt.NewNumericDate(now)` added |
| DEF-05 | No max token expiry ceiling | **Fixed** — upper bounds enforced in config validation |
| DEF-06 | Incorrect case-sensitivity comment | **Fixed** — comment corrected |
| DEF-07 | SQLite `synchronous=NORMAL` | **Fixed** — changed to `PRAGMA synchronous=FULL` |
| DEF-08 | gRPC counted TOTP-missing as failure | **Fixed** — no longer increments lockout counter |
| DEF-09 | Security headers missing on docs endpoints | **Fixed** — `docsSecurityHeaders` wrapper added |
| DEF-10 | Role strings not validated | **Fixed** — `model.ValidateRole()` with compile-time allowlist |

---

## Positive Findings (Preserved)

These implementation details are exemplary and should be maintained:

| Area | Detail |
|------|--------|
| JWT alg confusion | `ValidateToken` enforces `alg=EdDSA` in the key function before signature verification |
| Constant-time operations | `crypto/subtle.ConstantTimeCompare` for password hashes, CSRF tokens; all three TOTP windows evaluated without early exit |
| Timing uniformity | Dummy Argon2 via `sync.Once` for unknown/inactive users on all login paths |
| Token revocation | Fail-closed: untracked tokens are rejected, not silently accepted |
| Token renewal atomicity | `RenewToken` wraps revoke+track in a single SQLite transaction |
| TOTP replay prevention | Counter-based replay detection with atomic SQL UPDATE/WHERE |
| TOTP nonce design | 128-bit single-use server-side nonce; password never retransmitted in step 2 |
| CSRF protection | HMAC-SHA256 double-submit cookie, domain-separated key derivation, SameSite=Strict, constant-time validation |
| Credential exclusion | `json:"-"` on all credential fields; password hash never in API responses |
| Security headers (UI) | CSP (no unsafe-inline), X-Content-Type-Options, X-Frame-Options DENY, HSTS 2yr, Referrer-Policy no-referrer |
| Cookie hardening | HttpOnly + Secure + SameSite=Strict on session cookie |
| Account lockout | 10-attempt rolling window, checked before Argon2, with timing-safe dummy hash |
| Argon2id parameters | Config validator enforces OWASP 2023 minimums; rejects weakening |
| SQL injection | Zero string concatenation — all queries parameterized |
| Input validation | Username regex + length, password min length, account type enum, role allowlist, JSON strict decoder |
| Audit logging | Append-only, no delete path, credentials never logged, actor/target/IP captured |
| Master key hygiene | Env var cleared after read, key zeroed on shutdown, AES-256-GCM at rest |
| TLS | MinVersion TLS 1.2, X25519 preferred, no plaintext listener, read/write/idle timeouts set |

---

## Penetration Test — Attacks That Failed (2026-03-14)

The following attacks were attempted against the live instance and failed, confirming the effectiveness of existing defenses:

| Attack | Result |
|--------|--------|
| JWT `alg:none` bypass | Rejected — `ValidateToken` enforces `alg=EdDSA` |
| JWT `alg:HS256` key-confusion | Rejected — only EdDSA accepted |
| Forged JWT with random Ed25519 key | Rejected — signature verification failed |
| Username enumeration via timing | Not possible — ~355ms for both existing and non-existing users (dummy Argon2 working) |
| Username enumeration via error messages | Not possible — identical `"invalid credentials"` for all failure modes |
| Account lockout enumeration | Not possible — locked accounts return same response as wrong password (SEC-02 fix confirmed) |
| SQL injection via login fields | Not possible — parameterized queries throughout |
| JSON body bomb (oversized payload) | Rejected — `http.MaxBytesReader` returns 413 (SEC-05 fix confirmed) |
| Unknown JSON fields | Rejected — `DisallowUnknownFields` active on decoder |
| Rate limit bypass | Working correctly — 429 after burst exhaustion, `Retry-After` header present |
| Admin endpoint access without auth | Properly returns 401 |
| Directory traversal on static files | Not possible — `noDirListing` wrapper returns 404 (SEC-07 fix confirmed) |
| Public key endpoint | Returns Ed25519 PKIX key (expected; public by design) |

---

## Remediation Status

**CRIT/DEF/SEC series:** All 24 findings remediated. No open items.

**PEN series (2026-03-14):** All 7 findings resolved — 4 fixed, 3 accepted by design. Unauthorized access was not achieved. No open items remain.

Next audit should focus on:
- Any new features added since 2026-03-15
- Dependency updates and CVE review
- Re-evaluate PEN-03 if Swagger UI self-hosting becomes desirable
