From 69609db8af780b34737ce6be808c8bb069031c23 Mon Sep 17 00:00:00 2001 From: Lyra Bot Date: Sun, 27 Sep 2026 10:38:02 +0000 Subject: [PATCH] Use a lenient signature-counter check for passkeys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The library default (ThrowExceptionIfInvalid) requires the reported counter to be strictly greater than the stored one. Synchronised passkeys report a constant 0 forever, so the default rejects a brand-new credential on its first login — and only on real hardware, never in a unit test that increments the counter. The replacement still rejects a counter that moves backwards, which is the only signal the counter can carry. Clone detection remains explicitly not a property this feature claims; see SECURITY.md. --- src/Service/PasskeyCeremonyFactory.php | 64 ++++++++++ src/Service/PasskeyCounterChecker.php | 56 +++++++++ .../Service/PasskeyCounterCheckerTest.php | 116 ++++++++++++++++++ 3 files changed, 236 insertions(+) create mode 100644 src/Service/PasskeyCounterChecker.php create mode 100644 tests/Unit/Service/PasskeyCounterCheckerTest.php diff --git a/src/Service/PasskeyCeremonyFactory.php b/src/Service/PasskeyCeremonyFactory.php index a050da2..80d358f 100644 --- a/src/Service/PasskeyCeremonyFactory.php +++ b/src/Service/PasskeyCeremonyFactory.php @@ -4,10 +4,15 @@ declare(strict_types=1); namespace App\Service; +use Cose\Algorithm\Manager; +use Cose\Algorithm\Signature\ECDSA\ES256; use Symfony\Component\Serializer\SerializerInterface; use Throwable; use Webauthn\AttestationStatement\AttestationStatementSupportManager; use Webauthn\AttestationStatement\NoneAttestationStatementSupport; +use Webauthn\AuthenticatorAssertionResponseValidator; +use Webauthn\AuthenticatorAttestationResponseValidator; +use Webauthn\CeremonyStep\CeremonyStepManagerFactory; use Webauthn\CredentialRecord; use Webauthn\Denormalizer\WebauthnSerializerFactory; use Webauthn\Exception\InvalidDataException; @@ -32,11 +37,70 @@ final readonly class PasskeyCeremonyFactory { private AttestationStatementSupportManager $attestationStatementSupportManager; + private PasskeyCounterChecker $counterChecker; + public function __construct() { $manager = AttestationStatementSupportManager::create(); $manager->add(NoneAttestationStatementSupport::create()); $this->attestationStatementSupportManager = $manager; + $this->counterChecker = new PasskeyCounterChecker(); + } + + /** + * Validator for the registration ceremony. + * + * The origins are passed in rather than read from a request, so the scheme + * and host can only ever come from configuration. This is what makes D4 + * enforceable: an `http://` origin is never presented to the library as + * acceptable, no matter how the request arrived at the container. + * + * @param string[] $allowedOrigins + */ + public function creationCeremonyValidator(array $allowedOrigins): AuthenticatorAttestationResponseValidator + { + return AuthenticatorAttestationResponseValidator::create( + $this->ceremonyStepManagerFactory($allowedOrigins)->creationCeremony(), + ); + } + + /** + * Validator for the login (assertion) ceremony. + * + * @param string[] $allowedOrigins + */ + public function requestCeremonyValidator(array $allowedOrigins): AuthenticatorAssertionResponseValidator + { + return AuthenticatorAssertionResponseValidator::create( + $this->ceremonyStepManagerFactory($allowedOrigins)->requestCeremony(), + ); + } + + public function counterChecker(): PasskeyCounterChecker + { + return $this->counterChecker; + } + + /** + * The ceremony steps shared by both ceremonies. + * + * `setSecuredRelyingPartyId()` is deliberately never called: it is deprecated + * in 5.2 and, more importantly, it is the escape hatch that would let an + * `http://` origin through. Development uses real TLS instead (D4). + * + * @param string[] $allowedOrigins + */ + private function ceremonyStepManagerFactory(array $allowedOrigins): CeremonyStepManagerFactory + { + $factory = new CeremonyStepManagerFactory(); + $factory->setAllowedOrigins($allowedOrigins); + $factory->setAlgorithmManager(Manager::create()->add(ES256::create())); + $factory->setAttestationStatementSupportManager($this->attestationStatementSupportManager); + /* replace the library default, which rejects the constant-zero counter + * that synchronised passkeys report — see PasskeyCounterChecker */ + $factory->setCounterChecker($this->counterChecker); + + return $factory; } /** diff --git a/src/Service/PasskeyCounterChecker.php b/src/Service/PasskeyCounterChecker.php new file mode 100644 index 0000000..226ed1a --- /dev/null +++ b/src/Service/PasskeyCounterChecker.php @@ -0,0 +1,56 @@ + `ThrowExceptionIfInvalid` throws `CounterException` + * + * so with the default checker a brand-new synced passkey fails on its *first* + * login — and only on real hardware, never in a unit test that increments the + * counter. That is the worst possible failure shape, so the default is not used. + * + * **What is kept.** A counter that goes *backwards* still fails. That is the one + * signal the counter can carry (a cloned authenticator replaying an older + * assertion), and rejecting it costs nothing because a genuine synchronised + * passkey only ever reports the same value or a larger one. + * + * Note this is defence in depth and not relied upon for security: a + * synchronised passkey's counter carries no clone signal at all, which is why + * `SECURITY.md` records that clone detection is explicitly not a property this + * feature claims. The real protections are per-credential challenge binding, + * origin/RP-ID checks, and the user-verification requirement. + */ +final readonly class PasskeyCounterChecker implements CounterChecker +{ + /** + * @throws CounterException when the reported counter moves backwards + */ + #[Override] + public function check(CredentialRecord $credentialRecord, int $currentCounter): void + { + if ($currentCounter < $credentialRecord->counter) { + throw CounterException::create( + $currentCounter, + $credentialRecord->counter, + 'The signature counter moved backwards, which can indicate a cloned authenticator.', + ); + } + } +} diff --git a/tests/Unit/Service/PasskeyCounterCheckerTest.php b/tests/Unit/Service/PasskeyCounterCheckerTest.php new file mode 100644 index 0000000..c433ccb --- /dev/null +++ b/tests/Unit/Service/PasskeyCounterCheckerTest.php @@ -0,0 +1,116 @@ +makeRecord(0); + + $checker->check($record, 0); + $checker->check($record, 0); + + /* reaching this point without an exception is the assertion */ + self::assertSame(0, $record->counter); + } + + /** + * Documents *why* the library default cannot be used: it rejects the exact + * scenario above. If a future library version relaxes this, the test fails + * and the custom checker can be reconsidered rather than kept by habit. + */ + public function test_the_library_default_would_reject_a_constant_zero_counter(): void + { + $this->expectException(CounterException::class); + + (new ThrowExceptionIfInvalid())->check($this->makeRecord(0), 0); + } + + public function test_a_counter_that_moves_forward_is_accepted(): void + { + $checker = new PasskeyCounterChecker(); + $record = $this->makeRecord(5); + + $checker->check($record, 6); + $checker->check($record, PHP_INT_MAX); + + self::assertSame(5, $record->counter); + } + + public function test_a_counter_that_moves_backwards_is_rejected(): void + { + $checker = new PasskeyCounterChecker(); + $record = $this->makeRecord(5); + + $this->expectException(CounterException::class); + + $checker->check($record, 4); + } + + /** + * The exception carries both values, which the listener logs. Asserted so a + * future refactor cannot quietly drop the diagnostic detail. + */ + public function test_the_rejection_reports_both_counters(): void + { + $checker = new PasskeyCounterChecker(); + $record = $this->makeRecord(9); + + try { + $checker->check($record, 3); + self::fail('Expected a CounterException.'); + } catch (CounterException $exception) { + self::assertSame(3, $exception->currentCounter); + self::assertSame(9, $exception->authenticatorCounter); + } + } + + /** + * The checker under test must differ from the library default, otherwise + * wiring the default back in by accident would go unnoticed. + */ + public function test_it_is_not_the_library_default(): void + { + self::assertNotInstanceOf(ThrowExceptionIfInvalid::class, new PasskeyCounterChecker()); + } +}