From c91194c2838d226fe56b9d6511e9e86c14cb0821 Mon Sep 17 00:00:00 2001 From: timmy Date: Wed, 12 Aug 2026 10:26:51 +0000 Subject: [PATCH] fix: detect atomic passkey counter anomalies (Closes #641) --- README.md | 7 ++- frontend/security-center.js | 4 ++ src/main.py | 24 ++++++++- src/passkey_store.py | 21 ++++++-- tests/test_dashboard_auth.py | 96 +++++++++++++++++++++++++++++++++++ tests/test_passkey_store.py | 41 +++++++++++++++ tests/test_security_center.py | 39 ++++++++++++++ 7 files changed, 225 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 7b2d7b6..443bca9 100644 --- a/README.md +++ b/README.md @@ -271,7 +271,12 @@ session linked to the credential is revoked atomically; unrelated credentials an sessions remain valid. Removing the current device's passkey keeps its current session active, so the sheet warns that the recovery token will be required after sign-out. Remotely revoking an enrolled device also deletes its passkey; signing out -normally keeps the passkey available for the next sign-in. +normally keeps the passkey available for the next sign-in. Passkey assertion counters +advance with an atomic compare-and-swap across workers: stale or equal nonzero counters +fail before a session or authorization grant is issued, while counterless authenticators +remain compatible. The bounded **Passkey counter anomaly** Security activity entry names +only the device label and attempted action; repeated alerts should prompt removal and +re-enrollment of that passkey. High-impact actions—merging a pull request, closing an assigned issue, revoking a remote device, or signing out every device—require a passkey assertion or the diff --git a/frontend/security-center.js b/frontend/security-center.js index 2a91e3d..c29776d 100644 --- a/frontend/security-center.js +++ b/frontend/security-center.js @@ -128,6 +128,7 @@ } const labels = { sign_in: 'Signed in', sign_out: 'Signed out', passkey_enrolled: 'Passkey enrolled', + passkey_counter_anomaly: 'Passkey counter anomaly', device_revoked: 'Device access revoked', all_sessions_revoked: 'All device access revoked', issue_closed: 'Issue closed', pull_merged: 'Pull request merged', pull_review_approved: 'Pull request approved', @@ -141,6 +142,9 @@ const details = root.document.createElement('span'); details.className = 'small muted'; const context = [event.device_label, event.method, event.target, + event.kind === 'passkey_counter_anomaly' + ? 'Remove and re-enroll this passkey if the alert repeats' + : null, event.status === 'pending' ? 'Outcome confirmation pending' : null] .filter(value => typeof value === 'string' && value).join(' · '); details.textContent = `${new Date(event.created_at * 1000).toLocaleString()}${context ? ' · ' + context : ''}`; diff --git a/src/main.py b/src/main.py index 60dc9f3..f9d88e9 100644 --- a/src/main.py +++ b/src/main.py @@ -1595,9 +1595,19 @@ async def verify_passkey_authorization( stored=stored, ) updated = await asyncio.to_thread( - store.update_counter, stored.credential_id, verified.new_sign_count + store.advance_counter, + stored.credential_id, + expected=stored.sign_count, + new=verified.new_sign_count, ) if not updated: + await asyncio.to_thread( + _security_event_store().record, + "passkey_counter_anomaly", + method="passkey", + device_label=stored.device_label, + target=f"{payload.action}:{payload.target}", + ) raise ValueError("stale passkey counter") grant = await dashboard_auth.issue_step_up( request.state.dashboard_session, @@ -1686,9 +1696,19 @@ async def verify_passkey_authentication( stored=stored, ) updated = await asyncio.to_thread( - store.update_counter, stored.credential_id, verified.new_sign_count + store.advance_counter, + stored.credential_id, + expected=stored.sign_count, + new=verified.new_sign_count, ) if not updated: + await asyncio.to_thread( + _security_event_store().record, + "passkey_counter_anomaly", + method="passkey", + device_label=stored.device_label, + target="sign_in:dashboard", + ) raise ValueError("stale passkey counter") await dashboard_auth.revoke_managed_session(stored.management_id) signed, session = await asyncio.to_thread( diff --git a/src/passkey_store.py b/src/passkey_store.py index 6225606..33730a6 100644 --- a/src/passkey_store.py +++ b/src/passkey_store.py @@ -225,13 +225,26 @@ class PasskeyStore: raise SessionStoreError("Passkey registry is temporarily unavailable") from exc return StoredPasskey(*row) if row else None - def update_counter(self, credential_id: bytes, new_sign_count: int) -> bool: + def advance_counter( + self, credential_id: bytes, *, expected: int, new: int + ) -> bool: + if expected == 0 and new == 0: + comparison = "sign_count = 0" + elif new > expected: + comparison = "sign_count = ?" + else: + return False try: with self._connect() as connection: + parameters = ( + (new, credential_id) + if expected == 0 and new == 0 + else (new, credential_id, expected) + ) cursor = connection.execute( - "UPDATE passkey_credentials SET sign_count = ? " - "WHERE credential_id = ? AND sign_count <= ?", - (new_sign_count, credential_id, new_sign_count), + f"UPDATE passkey_credentials SET sign_count = ? " + f"WHERE credential_id = ? AND {comparison}", + parameters, ) return cursor.rowcount == 1 except (OSError, sqlite3.Error) as exc: diff --git a/tests/test_dashboard_auth.py b/tests/test_dashboard_auth.py index cdd567a..f425632 100644 --- a/tests/test_dashboard_auth.py +++ b/tests/test_dashboard_auth.py @@ -170,6 +170,102 @@ async def test_enrolled_passkey_can_sign_in_without_the_operator_token( assert "correct horse battery staple" not in signed_in.text +@pytest.mark.anyio +async def test_stale_nonzero_passkey_counter_denies_sign_in_and_records_anomaly( + access_control, monkeypatch +): + class StaleAuthentication: + new_sign_count = 4 + + monkeypatch.setattr( + main.passkeys, + "verify_authentication", + lambda **_kwargs: StaleAuthentication(), + ) + await asyncio.to_thread( + main._passkey_store().register, + credential_id=b"phone-credential", + public_key=b"phone-public-key", + sign_count=4, + device_label="Phone", + management_id="phone-management-id", + ) + transport = httpx.ASGITransport(app=main.app) + async with httpx.AsyncClient(transport=transport, base_url="https://test") as client: + options = await client.post("/api/v1/passkeys/authentication/options") + denied = await client.post( + "/api/v1/passkeys/authentication/verify", + json={ + "challenge": options.json()["challenge"], + "credential": {"id": "cGhvbmUtY3JlZGVudGlhbA"}, + "device_label": "Phone", + "action": "sign_in", + "target": "dashboard", + }, + ) + + events = main._security_event_store().list(limit=10).events + assert denied.status_code == 401 + assert "stackchain_session=" not in denied.headers.get("set-cookie", "") + assert [event.kind for event in events] == ["passkey_counter_anomaly"] + assert events[0].device_label == "Phone" + assert events[0].target == "sign_in:dashboard" + + +@pytest.mark.anyio +async def test_stale_nonzero_passkey_counter_denies_fresh_authorization( + access_control, monkeypatch +): + class StaleAuthentication: + new_sign_count = 4 + + monkeypatch.setattr( + main.passkeys, + "verify_authentication", + lambda **_kwargs: StaleAuthentication(), + ) + await asyncio.to_thread( + main._passkey_store().register, + credential_id=b"phone-credential", + public_key=b"phone-public-key", + sign_count=4, + device_label="Phone", + management_id="phone-management-id", + ) + transport = httpx.ASGITransport(app=main.app) + async with httpx.AsyncClient(transport=transport, base_url="https://test") as client: + await client.post( + "/api/v1/session", + json={"access_token": "correct horse battery staple", "device_label": "Phone"}, + ) + headers = { + "Origin": "https://test", + "X-CSRF-Token": client.cookies["stackchain_csrf"], + } + options = await client.post( + "/api/v1/passkeys/authorization/options", + json={"action": "merge_pull", "target": "stackchain/dashboard#42"}, + headers=headers, + ) + denied = await client.post( + "/api/v1/passkeys/authorization/verify", + json={ + "challenge": options.json()["challenge"], + "credential": {"id": "cGhvbmUtY3JlZGVudGlhbA"}, + "action": "merge_pull", + "target": "stackchain/dashboard#42", + }, + headers=headers, + ) + + events = main._security_event_store().list(limit=10).events + assert options.status_code == 200 + assert denied.status_code == 401 + assert "grant" not in denied.json() + assert events[0].kind == "passkey_counter_anomaly" + assert events[0].target == "merge_pull:stackchain/dashboard#42" + + @pytest.mark.anyio async def test_passkey_options_are_source_limited_before_challenge_generation( access_control, monkeypatch diff --git a/tests/test_passkey_store.py b/tests/test_passkey_store.py index 45a1c4c..31625a5 100644 --- a/tests/test_passkey_store.py +++ b/tests/test_passkey_store.py @@ -104,3 +104,44 @@ def test_existing_challenge_registry_is_migrated_without_losing_live_challenges( ).fetchall() assert "source_hash" in columns assert sorted(rows) == sorted([("existing",), (store._digest(b"new"),)]) + + +def test_passkey_counter_advancement_is_atomic_across_store_instances(tmp_path): + database = tmp_path / "passkeys.sqlite3" + first = PasskeyStore(database, clock=lambda: 1_000.0) + second = PasskeyStore(database, clock=lambda: 1_000.0) + first.register( + credential_id=b"phone-credential", + public_key=b"phone-public-key", + sign_count=4, + device_label="Phone", + management_id="phone-management-id", + ) + + assert first.advance_counter(b"phone-credential", expected=4, new=5) is True + assert second.advance_counter(b"phone-credential", expected=4, new=5) is False + assert first.get(b"phone-credential").sign_count == 5 + + +def test_passkey_counter_rejects_non_advancing_values_but_supports_counterless_devices( + tmp_path, +): + store = PasskeyStore(tmp_path / "passkeys.sqlite3", clock=lambda: 1_000.0) + store.register( + credential_id=b"phone-credential", + public_key=b"phone-public-key", + sign_count=4, + device_label="Phone", + management_id="phone-management-id", + ) + store.register( + credential_id=b"counterless-credential", + public_key=b"counterless-public-key", + sign_count=0, + device_label="Security key", + management_id="counterless-management-id", + ) + + assert store.advance_counter(b"phone-credential", expected=4, new=4) is False + assert store.advance_counter(b"phone-credential", expected=4, new=3) is False + assert store.advance_counter(b"counterless-credential", expected=0, new=0) is True diff --git a/tests/test_security_center.py b/tests/test_security_center.py index 84417d7..22bbe03 100644 --- a/tests/test_security_center.py +++ b/tests/test_security_center.py @@ -47,3 +47,42 @@ const boundary={{ "hidden": False, "focused": True, } + + +def test_counter_anomaly_activity_gives_passkey_remediation_guidance(): + harness = f""" +const attachSecurityCenter=require({json.dumps(str(SECURITY_CENTER))}); +const element=()=>({{ + hidden:true, disabled:false, textContent:'', children:[], className:'', + addEventListener(){{}}, focus(){{}}, replaceChildren(){{this.children=[];}}, + append(...values){{this.children.push(...values);}}, +}}); +const ids={{ + 'active-devices':element(), 'active-devices-sheet':element(), + 'active-devices-list':element(), 'active-devices-status':element(), + 'enrolled-passkeys-list':element(), 'enrolled-passkeys-status':element(), + 'enroll-passkey':element(), 'security-activity-list':element(), + 'security-activity-status':element(), 'load-more-security-activity':element(), + 'close-active-devices':element(), +}}; +const root={{document:{{getElementById:id=>ids[id]||null,createElement:()=>element()}}}}; +const boundary={{ + listActiveDevices:async()=>[], listPasskeys:async()=>[], + listSecurityEvents:async()=>({{events:[{{ + kind:'passkey_counter_anomaly', device_label:'Phone', method:'passkey', + target:'sign_in:dashboard', status:'completed', created_at:1, + }}],authentication_alerts:[],next_cursor:null}}), +}}; +(async()=>{{ + await attachSecurityCenter({{root,boundary}}).open(); + const row=ids['security-activity-list'].children[0]; + console.log(JSON.stringify(row.children.map(child=>child.textContent))); +}})().catch(error=>{{console.error(error);process.exit(1);}}); +""" + completed = subprocess.run( + ["node", "-e", harness], check=True, capture_output=True, text=True + ) + + title, detail = json.loads(completed.stdout) + assert title == "Passkey counter anomaly" + assert "Remove and re-enroll this passkey" in detail