kawa
c4a1775d7d
- Add per-account lockout + IP rate limiter on local sign-in (A07) - Emit CSP and security headers on every response (A05) - Run container as non-root `app`, /data 0700 (A05/A02) - Stop reflecting raw token-endpoint body into redirect URL (A09) - Handle missing refresh_token in connect callback without a 500 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
8.0 KiB
Security findings
Review date: 2026-06-11. Items 1–4 and 6 fixed the same day; #5 reviewed and accepted by design.
Second pass (OWASP Top 10), 2026-06-11: items 7–11 below found and fixed the same day.
1. [HIGH] ✅ FIXED — Open redirect in /connect/initiate leaked SharePoint session tokens
Was: returnUrl from the query string was stored verbatim in the OAuth flow state and used as the final redirect target after the code exchange — with token_key (a 2-minute, redeemable handle to the connected client's refresh token) appended. …?returnUrl=https://evil.com leaked that handle off-domain, enabling connection hijack.
Fix: returnUrl is now constrained to a site-relative path via the new string.ToLocalReturnUrl() helper before it is stored (Infrastructure/OAuth/OAuthEndpoints.cs). The helper rejects absolute and protocol-relative (//host, /\host) URLs.
2. [MEDIUM] ✅ FIXED — CSV formula injection in audit-log exports
Was: Both audit CSV exporters quoted per RFC 4180 but didn't neutralize a leading = + - @, so user-controlled fields (display/client/site names, details) could execute as formulas in Excel.
Fix: Both paths now use CsvSanitizer.Escape (which neutralizes formula prefixes): the /audit/export endpoint (Program.cs) and AuditService.ExportCsvAsync (Services/Audit/AuditService.cs). The local CsvEscape/Esc duplicates were removed.
3. [LOW-MEDIUM] ✅ FIXED — Spoofable X-Forwarded-Proto could yield a non-Secure auth cookie
Was: Forwarded headers are trusted from any source (proxy IP unknown inside the container network), and the prod auth cookie used SecurePolicy = SameAsRequest — so a spoofed X-Forwarded-Proto: http or a direct plaintext hit could emit a non-Secure cookie.
Fix: Prod auth cookie now uses CookieSecurePolicy.Always (Program.cs), so the Secure flag is set regardless of the (untrusted) forwarded scheme. The forwarded-headers trust config is unchanged (still required behind the proxy); the residual XFF concern is limited — client IP is not used for auth and is not recorded in the audit log.
4. [LOW] ✅ FIXED — Open redirect in /account/login/entra and /account/login/dev
Was: returnUrl was used directly as the post-auth redirect on the entra and dev sign-in paths; only local-login validated it (and even that check allowed protocol-relative //evil.com).
Fix: All three sign-in redirects now route returnUrl through ToLocalReturnUrl() (Program.cs), which also closes the protocol-relative gap in the old local-login check.
5. [LOW] ⚪ ACCEPTED (by design) — Authenticated users can download any export / report
Where: /export/download/{fileName} and /reports/download/{id} (Program.cs).
Decision: Not changed. The app has no per-user profile authorization anywhere — any tech may select and use any shared profile (see the "standard techs use profiles without sign-in" design), and GeneratedReport carries only a ProfileId, no owner. Reports are listed and shared per client/profile, not per user, so per-user download scoping would contradict the product model rather than fix a boundary. Both endpoints already require authentication and strip path traversal (Path.GetFileName). Revisit only if a per-tech profile ACL is introduced.
6. [LOW] ✅ FIXED — Admin pages gated only by a render-time role check
Was: /admin/users and /admin/audit used @attribute [Authorize] (any authenticated user) plus an in-markup if (Role != Admin) return;.
Fix: Added an Admin authorization policy (RequireClaim("app_role", "Admin")) in Program.cs and both pages now use [Authorize(Policy = "Admin")] (UserManagement.razor, AuditLogs.razor). A claim-value policy (not [Authorize(Roles=…)]) was used deliberately: the local/dev sign-in identities don't set a ClaimTypes.Role claim, so a Roles check would have silently denied local admins. The in-component checks were kept as defense-in-depth.
7. [MEDIUM] ✅ FIXED — No brute-force protection on local sign-in (OWASP A07)
Was: /account/local-login and UserService.ValidateLocalCredentialsAsync had no rate limiting and no account lockout, leaving local accounts (incl. the bootstrap admin) open to unlimited password guessing — relevant given the plain-HTTP/LAN bootstrap deployment mode.
Fix: Two layers. (1) A per-account lockout on AppUser (FailedLoginCount/LockoutEndUtc) — 5 consecutive failures → 15-minute lock, counter cleared on success (UserService.cs, AppUser.cs). A locked account is refused before the password is even checked, and failure stays generic (no enumeration). (2) An IP-partitioned fixed-window rate limiter ("login" policy, 10/min) on /account/local-login and /account/login/entra (Program.cs). The account lockout is the control that holds up when X-Forwarded-For is spoofed/rotated (forwarded headers are trusted from any source — see #3); the IP limiter is the coarse volumetric layer.
8. [MEDIUM] ✅ FIXED — Missing security response headers (OWASP A05)
Was: No Content-Security-Policy, X-Frame-Options, X-Content-Type-Options, or Referrer-Policy. A clickjacked page could drive the victim's live Blazor Server circuit, and there was no CSP backstop against injected script.
Fix: A middleware emits these on every response (Program.cs). CSP is tuned to the app: all scripts are external so script-src 'self' (no unsafe-*); style-src keeps 'unsafe-inline' for the login page's inline <style> and the components' style=""; img-src 'self' data: for base64 client logos / download links; connect-src 'self' for the SignalR WebSocket; frame-ancestors 'none' (+ X-Frame-Options: DENY) blocks clickjacking.
9. [LOW-MEDIUM] ✅ FIXED — Container ran as root; /data world-readable (OWASP A05 / A02)
Was: The Dockerfile had no USER directive, so the process ran as root and the /data volume (Data Protection keys, app-only certs, user store) had default permissions. The DP keys decrypt the auth cookie, the browser-stored SharePoint refresh tokens, and the on-disk certs — a large blast radius for anyone able to read the volume.
Fix: Final image now creates /data owned by the shipped non-root app user with mode 0700, then USER app (Dockerfile). Docker seeds a fresh named volume from that ownership/mode, so the running user can write it while other host users can't read the keys/certs at rest. (DP keys themselves remain unencrypted at rest — inherent to file-based keys in a container; the volume permissions are the mitigation.)
10. [LOW] ✅ FIXED — Raw token-endpoint response reflected into the redirect URL (OWASP A09)
Was: /connect/callback put the raw Azure token-endpoint JSON (and the provider's verbose error_description) into ?connect_error=…, landing in browser history and — behind the proxy — proxy access logs. Those bodies can carry correlation/trace ids and claim hints.
Fix: The token-endpoint failure body and the verbose error_description are now logged server-side via ILogger; the redirect carries only a generic notice (or the short, safe OAuth error code) (OAuthEndpoints.cs).
11. [LOW] ✅ FIXED — /connect/callback threw a 500 when no refresh_token was returned
Was: root.GetProperty("refresh_token").GetString()! threw KeyNotFoundException (→ 500 + stack trace in non-prod) when the tenant/app withheld offline_access.
Fix: Uses TryGetProperty; a missing/empty refresh token is logged and redirects with a friendly connect_error instead of throwing (OAuthEndpoints.cs).