Harden auth, headers, and container per OWASP review
- 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>
This commit is contained in:
@@ -15,4 +15,17 @@ public class AppUser
|
||||
|
||||
public DateTimeOffset CreatedAt { get; set; } = DateTimeOffset.UtcNow;
|
||||
public DateTimeOffset? LastLogin { get; set; }
|
||||
|
||||
// ── Local-account brute-force lockout ───────────────────────────────────────
|
||||
// Consecutive failed password attempts and, once the threshold is hit, the UTC
|
||||
// instant the account unlocks again. Only meaningful for AuthProvider.Local.
|
||||
// A per-account counter (not just an IP rate limiter) is the control that holds
|
||||
// up here: forwarded headers are trusted from any source, so an attacker who can
|
||||
// rotate X-Forwarded-For would evade IP-based throttling but not this.
|
||||
|
||||
/// <summary>Consecutive failed local-login attempts since the last success.</summary>
|
||||
public int FailedLoginCount { get; set; }
|
||||
|
||||
/// <summary>UTC instant the account unlocks; null when not locked.</summary>
|
||||
public DateTimeOffset? LockoutEndUtc { get; set; }
|
||||
}
|
||||
|
||||
+10
@@ -25,6 +25,16 @@ FROM base AS final
|
||||
WORKDIR /app
|
||||
COPY --from=build /app/publish .
|
||||
|
||||
# Run as the non-root `app` user shipped in the aspnet image (UID 1654) instead of root.
|
||||
# /data holds the crown jewels (Data Protection keys, app-only certs, the user store), so
|
||||
# create it owned by `app` with 0700 before declaring the volume — Docker seeds a fresh
|
||||
# named volume from the image path's ownership/mode, so the running user can write it and
|
||||
# other host users can't read the keys/certs at rest.
|
||||
RUN mkdir -p /data \
|
||||
&& chown -R app:app /app /data \
|
||||
&& chmod 700 /data
|
||||
USER app
|
||||
|
||||
# Volume for persistent data (profiles, settings, templates, logs, exports)
|
||||
VOLUME ["/data"]
|
||||
|
||||
|
||||
@@ -72,12 +72,18 @@ public static class OAuthEndpoints
|
||||
string? error_description,
|
||||
IOAuthFlowCache flowCache,
|
||||
IOptions<ClientConnectOptions> opts,
|
||||
IHttpClientFactory httpClientFactory) =>
|
||||
IHttpClientFactory httpClientFactory,
|
||||
ILoggerFactory loggerFactory) =>
|
||||
{
|
||||
var log = loggerFactory.CreateLogger("SharepointToolbox.Web.OAuth.Connect");
|
||||
|
||||
if (!string.IsNullOrEmpty(error))
|
||||
{
|
||||
var errMsg = Uri.EscapeDataString(error_description ?? error);
|
||||
return Results.Redirect($"/?connect_error={errMsg}");
|
||||
// The provider's verbose error_description can carry correlation/trace ids and
|
||||
// lands in the URL bar + proxy access logs. Log it server-side; surface only the
|
||||
// short, safe OAuth error code (e.g. "access_denied") to the browser.
|
||||
log.LogWarning("Connect callback returned error {Error}: {Description}", error, error_description);
|
||||
return Results.Redirect($"/?connect_error={Uri.EscapeDataString(error)}");
|
||||
}
|
||||
|
||||
if (string.IsNullOrEmpty(code) || string.IsNullOrEmpty(state))
|
||||
@@ -107,14 +113,24 @@ public static class OAuthEndpoints
|
||||
|
||||
if (!resp.IsSuccessStatusCode)
|
||||
{
|
||||
var msg = Uri.EscapeDataString($"Token exchange failed: {json}");
|
||||
return Results.Redirect($"/?connect_error={msg}");
|
||||
// The raw token-endpoint body can contain trace ids / claim hints — keep it out of
|
||||
// the URL and the proxy logs. Record it server-side, redirect with a generic notice.
|
||||
log.LogWarning("Token exchange failed ({Status}): {Body}", resp.StatusCode, json);
|
||||
return Results.Redirect($"/?connect_error={Uri.EscapeDataString("Token exchange failed. Please try connecting again.")}");
|
||||
}
|
||||
|
||||
using var doc = JsonDocument.Parse(json);
|
||||
var root = doc.RootElement;
|
||||
var upn = ExtractUpnFromIdToken(root);
|
||||
var refreshToken = root.GetProperty("refresh_token").GetString()!;
|
||||
|
||||
// offline_access should yield a refresh_token; if the tenant/app withheld it the
|
||||
// session can't be persisted. Fail cleanly instead of throwing a 500 + stack trace.
|
||||
if (!root.TryGetProperty("refresh_token", out var refreshTokenEl) ||
|
||||
refreshTokenEl.GetString() is not { Length: > 0 } refreshToken)
|
||||
{
|
||||
log.LogWarning("Token response had no refresh_token for tenant {Tenant}.", flowState.TenantId);
|
||||
return Results.Redirect($"/?connect_error={Uri.EscapeDataString("Sign-in did not return a refresh token. Please try again.")}");
|
||||
}
|
||||
|
||||
var tokens = new SessionTokens
|
||||
{
|
||||
|
||||
+51
-2
@@ -6,6 +6,8 @@ using Microsoft.AspNetCore.Antiforgery;
|
||||
using Microsoft.AspNetCore.DataProtection;
|
||||
using Microsoft.AspNetCore.HttpOverrides;
|
||||
using Microsoft.AspNetCore.Identity;
|
||||
using Microsoft.AspNetCore.RateLimiting;
|
||||
using System.Threading.RateLimiting;
|
||||
using Microsoft.Extensions.Options;
|
||||
using Microsoft.IdentityModel.Protocols.OpenIdConnect;
|
||||
using Serilog;
|
||||
@@ -210,6 +212,25 @@ builder.Services.AddAuthorization(options =>
|
||||
options.AddPolicy("Admin", p => p.RequireClaim("app_role", nameof(UserRole.Admin)));
|
||||
});
|
||||
|
||||
// ── Rate limiting ───────────────────────────────────────────────────────────────
|
||||
// Volumetric defence on the sign-in endpoints, partitioned by client IP (RemoteIpAddress
|
||||
// reflects X-Forwarded-For — UseForwardedHeaders runs first). This is the coarse layer;
|
||||
// the per-account lockout in UserService is what holds up when XFF is spoofed/rotated,
|
||||
// since forwarded headers are trusted from any source behind the proxy.
|
||||
builder.Services.AddRateLimiter(options =>
|
||||
{
|
||||
options.RejectionStatusCode = StatusCodes.Status429TooManyRequests;
|
||||
options.AddPolicy("login", httpContext =>
|
||||
RateLimitPartition.GetFixedWindowLimiter(
|
||||
partitionKey: httpContext.Connection.RemoteIpAddress?.ToString() ?? "unknown",
|
||||
factory: _ => new FixedWindowRateLimiterOptions
|
||||
{
|
||||
PermitLimit = 10,
|
||||
Window = TimeSpan.FromMinutes(1),
|
||||
QueueLimit = 0,
|
||||
}));
|
||||
});
|
||||
|
||||
// ── Memory cache (used by OAuth flow cache) ───────────────────────────────────
|
||||
builder.Services.AddMemoryCache();
|
||||
builder.Services.AddHttpClient("oauth");
|
||||
@@ -362,6 +383,33 @@ if (publicBaseUri is not null)
|
||||
}
|
||||
}
|
||||
|
||||
// ── Security response headers ───────────────────────────────────────────────────
|
||||
// Defence-in-depth on every response. CSP is tuned to this app: all scripts are external
|
||||
// (_framework/blazor.web.js, js/app.js) so script-src can stay 'self' with no unsafe-*;
|
||||
// the login page and the Blazor components use inline <style>/style="" so style-src needs
|
||||
// 'unsafe-inline'. img-src allows data: for the base64 client logos and CSV download links.
|
||||
// connect-src 'self' covers the Blazor Server WebSocket. frame-ancestors blocks clickjacking
|
||||
// of the live circuit (X-Frame-Options for legacy agents).
|
||||
app.Use(async (context, next) =>
|
||||
{
|
||||
var headers = context.Response.Headers;
|
||||
headers["Content-Security-Policy"] =
|
||||
"default-src 'self'; " +
|
||||
"script-src 'self'; " +
|
||||
"style-src 'self' 'unsafe-inline'; " +
|
||||
"img-src 'self' data:; " +
|
||||
"font-src 'self' data:; " +
|
||||
"connect-src 'self'; " +
|
||||
"base-uri 'self'; " +
|
||||
"object-src 'none'; " +
|
||||
"form-action 'self'; " +
|
||||
"frame-ancestors 'none'";
|
||||
headers["X-Frame-Options"] = "DENY";
|
||||
headers["X-Content-Type-Options"] = "nosniff";
|
||||
headers["Referrer-Policy"] = "no-referrer";
|
||||
await next();
|
||||
});
|
||||
|
||||
if (!app.Environment.IsDevelopment())
|
||||
{
|
||||
app.UseExceptionHandler("/Error", createScopeForErrors: true);
|
||||
@@ -374,6 +422,7 @@ app.UseStatusCodePagesWithReExecute("/not-found");
|
||||
app.MapStaticAssets();
|
||||
app.UseAuthentication();
|
||||
app.UseAuthorization();
|
||||
app.UseRateLimiter();
|
||||
app.UseAntiforgery();
|
||||
|
||||
// ── Login / Logout endpoints ──────────────────────────────────────────────────
|
||||
@@ -425,7 +474,7 @@ else
|
||||
RedirectUri = returnUrl.ToLocalReturnUrl()
|
||||
};
|
||||
await ctx.ChallengeAsync(OpenIdConnectDefaults.AuthenticationScheme, props);
|
||||
});
|
||||
}).RequireRateLimiting("login");
|
||||
}
|
||||
|
||||
// Local password sign-in — available in every environment.
|
||||
@@ -456,7 +505,7 @@ app.MapPost("/account/local-login", async (HttpContext ctx, IAntiforgery antifor
|
||||
|
||||
await ctx.SignInAsync(CookieAuthenticationDefaults.AuthenticationScheme, principal);
|
||||
return Results.Redirect(safeReturn);
|
||||
});
|
||||
}).RequireRateLimiting("login");
|
||||
|
||||
app.MapGet("/account/logout", async (HttpContext ctx) =>
|
||||
{
|
||||
|
||||
@@ -2,6 +2,8 @@
|
||||
|
||||
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
|
||||
@@ -49,3 +51,43 @@ Review date: 2026-06-11. Items 1–4 and 6 fixed the same day; #5 reviewed and a
|
||||
**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](Program.cs) and both pages now use `[Authorize(Policy = "Admin")]` ([UserManagement.razor](Components/Pages/Admin/UserManagement.razor), [AuditLogs.razor](Components/Pages/Admin/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](Services/Auth/UserService.cs), [AppUser.cs](Core/Models/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](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](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](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](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](Infrastructure/OAuth/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](Infrastructure/OAuth/OAuthEndpoints.cs)).
|
||||
|
||||
@@ -109,21 +109,58 @@ public class UserService : IUserService
|
||||
return user;
|
||||
}
|
||||
|
||||
// Brute-force lockout: after this many consecutive failures the account is locked
|
||||
// for the window below. Tuned to stop guessing while staying usable for a fat-fingered
|
||||
// admin. The counter resets on any successful sign-in.
|
||||
private const int LockoutThreshold = 5;
|
||||
private static readonly TimeSpan LockoutWindow = TimeSpan.FromMinutes(15);
|
||||
|
||||
public async Task<AppUser?> ValidateLocalCredentialsAsync(string email, string password)
|
||||
{
|
||||
var user = await _repo.FindByEmailAsync(email);
|
||||
if (user is null || user.Provider != AuthProvider.Local || string.IsNullOrEmpty(user.PasswordHash))
|
||||
return null;
|
||||
|
||||
var now = DateTimeOffset.UtcNow;
|
||||
|
||||
// Account currently locked → refuse without even checking the password, so a locked
|
||||
// account can't be probed. Returning null (generic failure) avoids account enumeration.
|
||||
if (user.LockoutEndUtc is { } until && until > now)
|
||||
{
|
||||
_logger.LogWarning("Local login blocked: account {Email} is locked until {Until:o}.", user.Email, until);
|
||||
return null;
|
||||
}
|
||||
|
||||
// Lock window elapsed → clear it before re-evaluating.
|
||||
if (user.LockoutEndUtc is not null)
|
||||
{
|
||||
user.LockoutEndUtc = null;
|
||||
user.FailedLoginCount = 0;
|
||||
}
|
||||
|
||||
var result = _hasher.VerifyHashedPassword(user, user.PasswordHash, password);
|
||||
if (result == PasswordVerificationResult.Failed)
|
||||
{
|
||||
user.FailedLoginCount++;
|
||||
if (user.FailedLoginCount >= LockoutThreshold)
|
||||
{
|
||||
user.LockoutEndUtc = now + LockoutWindow;
|
||||
_logger.LogWarning(
|
||||
"Local login: account {Email} locked for {Minutes} min after {Count} failed attempts.",
|
||||
user.Email, LockoutWindow.TotalMinutes, user.FailedLoginCount);
|
||||
}
|
||||
await _repo.UpsertAsync(user);
|
||||
return null;
|
||||
}
|
||||
|
||||
// Transparently upgrade the hash if the algorithm parameters changed
|
||||
if (result == PasswordVerificationResult.SuccessRehashNeeded)
|
||||
user.PasswordHash = _hasher.HashPassword(user, password);
|
||||
|
||||
user.LastLogin = DateTimeOffset.UtcNow;
|
||||
// Success → clear the failure trail.
|
||||
user.FailedLoginCount = 0;
|
||||
user.LockoutEndUtc = null;
|
||||
user.LastLogin = now;
|
||||
await _repo.UpsertAsync(user);
|
||||
return user;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user