Kyle Isom
2a85d4bf2b
- Mark SEC-01 through SEC-12 as fixed with fix descriptions - Update executive summary to reflect full remediation - Move original finding descriptions to collapsible section - Replace remediation priority table with status section Security: documentation-only change, no code modifications Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
195 lines
10 KiB
Markdown
195 lines
10 KiB
Markdown
# MCIAS Security Audit Report
|
|
|
|
**Date:** 2026-03-14 (updated — all findings remediated)
|
|
**Original audit date:** 2026-03-13
|
|
**Auditor role:** Penetration tester (code review + live instance probing)
|
|
**Scope:** Full codebase and running instance at localhost: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 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.
|
|
|
|
**All findings from this audit have been remediated.** See the remediation table below for details.
|
|
|
|
---
|
|
|
|
## Remediated Findings (SEC-01 through SEC-12)
|
|
|
|
All findings from this audit 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 |
|
|
|
|
---
|
|
|
|
## Remediation Status
|
|
|
|
**All findings remediated.** No open items remain. Next audit should focus on:
|
|
- Any new features added since 2026-03-14
|
|
- Dependency updates and CVE review
|
|
- Live penetration testing of remediated endpoints
|