From 1571706d215ea163ad9230bd2ad486f732f9a9d9 Mon Sep 17 00:00:00 2001 From: Johan Lundberg Date: Fri, 5 Jun 2026 13:53:10 +0200 Subject: [PATCH] fix(security): reject WebAuthn signature-counter rollback Sign counters were stored but never checked, so a cloned authenticator or a replayed assertion with an equal/lower counter was accepted. Reject the authentication when the presented counter does not exceed the stored one, while allowing counter-less/synced passkeys that always report 0. Refs: porchlight-3cr Co-Authored-By: Claude Opus 4.8 (1M context) --- src/porchlight/authn/routes.py | 18 +++++++++++++++++- tests/test_authn/test_sign_count.py | 22 ++++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 tests/test_authn/test_sign_count.py diff --git a/src/porchlight/authn/routes.py b/src/porchlight/authn/routes.py index 606b315..47f58d4 100644 --- a/src/porchlight/authn/routes.py +++ b/src/porchlight/authn/routes.py @@ -23,6 +23,19 @@ def _login_redirect_target(request: Request) -> str: return "/manage/credentials" +def _is_sign_count_rollback(stored: int, presented: int) -> bool: + """Detect a WebAuthn signature-counter rollback (cloned authenticator/replay). + + Authenticators that don't maintain a counter (incl. synced passkeys) report + 0; when both stored and presented are 0 the counter is meaningless and no + rollback can be inferred. Otherwise the presented counter must strictly + exceed the stored one. + """ + if stored == 0 and presented == 0: + return False + return presented <= stored + + def _establish_authenticated_session(request: Request, user: User) -> None: """Reset the session before recording the authenticated identity. @@ -156,7 +169,7 @@ async def login_webauthn_begin(request: Request) -> Response: @router.post("/login/webauthn/complete") @limiter.limit("10/minute") -async def login_webauthn_complete(request: Request) -> Response: +async def login_webauthn_complete(request: Request) -> Response: # noqa: PLR0911 webauthn_service = request.app.state.webauthn_service user_repo = request.app.state.user_repo cred_repo = request.app.state.credential_repo @@ -194,6 +207,9 @@ async def login_webauthn_complete(request: Request) -> Response: stored = await cred_repo.get_webauthn_by_credential_id(matched_credential_id) if stored is not None: + if _is_sign_count_rollback(stored.sign_count, new_counter): + # Counter went backwards — likely a cloned authenticator or replay. + return JSONResponse({"error": "Authentication failed"}, status_code=400) stored.sign_count = new_counter await cred_repo.update_webauthn(stored) diff --git a/tests/test_authn/test_sign_count.py b/tests/test_authn/test_sign_count.py new file mode 100644 index 0000000..5c8a65d --- /dev/null +++ b/tests/test_authn/test_sign_count.py @@ -0,0 +1,22 @@ +from porchlight.authn.routes import _is_sign_count_rollback + + +def test_increasing_counter_is_not_rollback() -> None: + assert _is_sign_count_rollback(stored=5, presented=6) is False + + +def test_equal_counter_is_rollback() -> None: + assert _is_sign_count_rollback(stored=5, presented=5) is True + + +def test_lower_counter_is_rollback() -> None: + assert _is_sign_count_rollback(stored=5, presented=3) is True + + +def test_both_zero_sync_passkey_is_allowed() -> None: + # Sync passkeys (and counter-less authenticators) always report 0. + assert _is_sign_count_rollback(stored=0, presented=0) is False + + +def test_first_increment_from_zero_is_allowed() -> None: + assert _is_sign_count_rollback(stored=0, presented=1) is False