fix(security): P0 remediation wave — SSRF guards, one-time-use, server-side passkey ceremony, lockout concurrency#159
Merged
Conversation
…ere missing First remediation wave from the code-verified July security review. Every fix here reuses a pattern this repo had already solved once and simply hadn't wired up at the second site — no new security machinery is invented. SSRF guard, now shared The CIMD metadata fetcher's transport guard (DNS resolved and validated before connect, closing the rebinding window; redirects off; tight timeouts) moves out of the CIMD namespace to Modgud.Infrastructure/Http as SsrfIpGuard + SsrfSafeHttpHandlerFactory, with a purpose label so a refusal is diagnosable from the log. The IP range table is unchanged; its tests moved with it. Newly protected, both previously a bare HttpClient: - SAML IdP metadata fetch (SamlSetup) - dynamic OIDC discovery + backchannel (DynamicOidcSchemeManager) Both URLs are realm-admin supplied, and a realm admin is a lower-trust tier than the platform operator, so "an admin configured it" is not a reason to skip the guard. One-time use, actually enforced EmailOtpChallenge and PasskeyCeremony consumed via Delete. Marten does NOT version-check deletes, so two concurrent redemptions of the same code or ceremony_id both passed and each completed a login / minted a token — the single-use guarantee existed only in a comment. Both documents now carry a ConsumedAt marker written through a version-checked Store (the pattern MagicLinkChallenge already used), so exactly one racer wins. The native urn:cocoar:magic grant queried by user + token hash and checked only expiry, never IsConsumed. Because the web flow marks links consumed rather than deleting them, a link already used in the browser stayed redeemable through the native channel. Now gated. Follow-on fix found while writing the tests: since a consumed challenge now survives, the issue-path rate limit would have throttled the next request and locked a user out of email OTP right after a successful login. Consumed challenges are exempt, and the re-issue path mutates the loaded row instead of storing a fresh instance (a fresh instance carries no version and would be rejected now that the document is version-checked). SAML ACS body cap The anonymous ACS endpoint does XML parsing + signature validation per request and inherited Kestrel's 30 MB default. Capped at 512 KB, matching the tightening AssetsEndpoints already applies. Tests Four regression tests: concurrent consume of a passkey ceremony and of an email OTP each let exactly one racer win; a consumed OTP is rejected while re-issue still works; and a magic link consumed in the web flow is rejected by the native grant. That last one was negative-controlled — with the gate removed it mints tokens successfully, so it pins the actual vulnerability rather than asserting current behaviour. The passkey ceremony test helper now asserts the security property (still redeemable?) instead of the storage mechanism (row deleted?). Gated locally: build clean, unit 1395/1395, integration 521/521. Deferred deliberately: the SAML ACS rate limit. Its policy enum is per-realm configurable and would pull in DTOs, manifest export, settings patch and frontend — out of scope for a "wire up the existing guard" change, and P3.
The web login ceremony shipped the full AssertionOptions to the browser as plain Base64 in the Modgud.Passkey.Challenge cookie — no signature, no encryption — and the login endpoint parsed it back as trusted input. A client could therefore rewrite the ceremony options or re-present an old challenge together with an old assertion, and the cookie was never actually cleared because Delete was called without the Path the cookie was set with. The native /connect/passkey/begin flow already solved this with a server-side PasskeyCeremony record; the code comment even noted that the two flows differed only in "challenge transport". They no longer differ: - login-options persists a PasskeyCeremony (realm-scoped, so ClientId is null) and the cookie carries nothing but its opaque id. Tampering with the id merely fails to resolve. - login loads the ceremony, rejects it when expired or already consumed, and consumes it BEFORE verifying via a version-checked Store of ConsumedAt — the same single-use mechanism wave 1 gave this document, so a captured ceremony cannot be replayed and two concurrent logins cannot both redeem one challenge. - The RP ID is pinned at begin-time and reused at redeem, so an admin editing PrimaryDomain mid-ceremony cannot cause a begin/redeem drift (matching the native flow's rationale). - Cookie name and path are constants now, shared by Append and Delete, so the delete actually matches. - Expired ceremonies are cleaned opportunistically on the same traffic that creates them, as the native begin endpoint does. Test: the cookie must parse as a bare Guid, the authoritative options must live in the server-side record, and a cookie in the OLD client-held format (Base64 of attacker-chosen options) must not authenticate anything. Gated locally: build clean, unit 1395/1395, integration 522/522. One run showed the known load-dependent UserView catch-up flake in an unrelated class's fixture setup (documented in the Critter-Stack blocker notes); it passed in isolation and the suite was green on a re-run. Not included: the account-lockout concurrency fix (P0-4). An atomic increment alone does nothing there because UpdateAsync writes AccessFailedCount back from the in-memory user after every AccessFailedAsync, and the reset-detection that emits a security event keys off that same write-back. Untangling the counter from that round-trip is a focused change to the Identity store contract and needs its own pass with genuinely concurrent tests.
The five-attempt lockout was bypassable by sending the guesses in parallel instead of sequentially. IncrementAccessFailedCountAsync only did an in-memory `user.AccessFailedCount++`, and EventSourcedUserStore.UpdateAsync -- which ASP.NET Identity calls after EVERY AccessFailedAsync -- wrote the whole UserSecurityData document back from a load-time snapshot. Concurrent failed logins therefore all read N and all wrote N+1, so a burst registered as roughly one failure and the threshold never tripped. Fixing the increment alone would not have helped: the whole-document Store was the second half of the race. So the lockout fields are now owned end to end by the IUserLockoutStore methods: - IncrementAccessFailedCountAsync does a server-side jsonb increment (the existing "Audit #24" pattern from the email-OTP attempt counter) and reads the authoritative value back, because UserManager compares that return value against MaxFailedAccessAttempts. - ResetAccessFailedCountAsync and SetLockoutEndDateAsync write through atomic patches. - GetAccessFailedCountAsync and GetLockoutEndDateAsync re-fetch authoritatively, the same idiom as GetSecurityStampAsync. This also closes a smaller gap: IsLockedOutAsync read a mirror that could not see a lockout set by a concurrent request. - UpdateAsync patches only the fields it actually owns and no longer touches AccessFailedCount or LockoutEnd. That incidentally protects the grace-period fields from the same clobbering. The lock/unlock and resolved-failure-streak audit events were stale-vs-fresh diffs inside UpdateAsync that only worked because of the whole-document Store. They move to the paths that actually change those values, unchanged in shape. Tests: three integration tests driving the real login endpoint, so each attempt gets its own DI scope and Marten session. Verified as real coverage by reverting the store and confirming both concurrency tests fail without the fix. Unit 1395/1395, integration 525/525. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Remediation wave from a security review of
develop. Four issues, bundled becausethey share a theme: for most of them the correct pattern already existed in this
repo and simply was not wired up at the second site.
What changed
Shared SSRF guard (was CIMD-only).
CimdIpGuard+CimdHttpMessageHandlerFactorywere DNS-rebinding-safe, redirect-free and timeout-bounded, but only the CIMD metadata
fetch used them. SAML metadata fetching used a bare
HttpClient(auto-redirects, notimeout, unbounded body) and the dynamic OIDC backchannel did
??= new HttpClient().Both are admin-reachable, and a realm admin is a lower-trust tier than the platform
operator, so this was a real boundary crossing. Extracted to
Modgud.Infrastructure/Http/asSsrfIpGuard+SsrfSafeHttpHandlerFactoryandapplied to all three call sites.
One-time credentials are actually consumed once. The native
urn:cocoar:magicgrant queried by user + token hash and checked only null/expired — never
IsConsumed—so a magic link already redeemed in the browser stayed redeemable through the native
channel. Separately,
EmailOtpChallengeandPasskeyCeremonyconsumed viaDelete,which Marten does not version-check, so two concurrent redemptions could both win.
Both now carry a
ConsumedAtmarker written through a version-checkedStore, matchingwhat the web magic-link flow already did.
Web passkey ceremony moved server-side. The full
AssertionOptionswas written intoa cookie as plain Base64 — no Data Protection, no signature — and read back as trusted.
The cookie now carries only an opaque ceremony id; the ceremony is consumed before
verification and the RP-ID is pinned at begin. Also fixes a cookie set with
Path=/api/account/passkeybut deleted without the path, so it was never actuallyremoved.
Account lockout is concurrency-safe. The five-attempt lockout was bypassable by
sending the guesses in parallel.
IncrementAccessFailedCountAsyncdid an in-memory++, andUpdateAsync— which ASP.NET Identity calls after everyAccessFailedAsync— wrote the whole security document back from a load-time snapshot, so concurrent
failures all read N and wrote N+1. Fixing the increment alone would not have helped;
the whole-document
Storewas the second half of the race. The lockout fields are nowowned end to end by the
IUserLockoutStoremethods via atomic jsonb patches, andUpdateAsyncpatches only what it owns.IsLockedOutAsyncalso reads authoritativelynow, so it can see a lockout set by a concurrent request.
Notes for review
inside
UpdateAsyncthat only worked because of the whole-documentStore. They moveto the paths that actually change those values; event shape is unchanged.
AccessFailedCount/LockoutEndout ofUpdateAsync's write set incidentallyprotects the grace-period fields (
SecureSetupDueAt,GracePeriodDaysOverride,TwoFactorExempt) from the same clobbering.leaves the row in place instead of deleting it, an "is there a row?" check would have
blocked the next code request after a successful login. Covered by a test.
Testing
Unit 1395/1395, integration 525/525.
The new security tests were verified as real coverage, not just a snapshot of current
behaviour: each was re-run with its fix reverted and confirmed to fail. Without the
lockout fix both concurrency tests fail; without the consume gate the spent magic link
successfully mints tokens.
Deliberately not in this PR
direction decision (KMS/HSM vs certificate envelope vs process-level master key)
because it determines the operational and recovery story, not just the code.
pulls in DTOs, manifest export, settings patch and frontend; it does not fit a
"wire up an existing guard" bundle.
🤖 Generated with Claude Code