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) <noreply@anthropic.com>
This commit is contained in:
parent
f03d509eb4
commit
1571706d21
2 changed files with 39 additions and 1 deletions
|
|
@ -23,6 +23,19 @@ def _login_redirect_target(request: Request) -> str:
|
||||||
return "/manage/credentials"
|
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:
|
def _establish_authenticated_session(request: Request, user: User) -> None:
|
||||||
"""Reset the session before recording the authenticated identity.
|
"""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")
|
@router.post("/login/webauthn/complete")
|
||||||
@limiter.limit("10/minute")
|
@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
|
webauthn_service = request.app.state.webauthn_service
|
||||||
user_repo = request.app.state.user_repo
|
user_repo = request.app.state.user_repo
|
||||||
cred_repo = request.app.state.credential_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)
|
stored = await cred_repo.get_webauthn_by_credential_id(matched_credential_id)
|
||||||
if stored is not None:
|
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
|
stored.sign_count = new_counter
|
||||||
await cred_repo.update_webauthn(stored)
|
await cred_repo.update_webauthn(stored)
|
||||||
|
|
||||||
|
|
|
||||||
22
tests/test_authn/test_sign_count.py
Normal file
22
tests/test_authn/test_sign_count.py
Normal file
|
|
@ -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
|
||||||
Loading…
Add table
Add a link
Reference in a new issue