From c3d7b3a79a1dccc25ada53e3aa48ee066fa6cc24 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 27 May 2019 11:34:03 -0700 Subject: [PATCH 01/11] Create LoginResponseInterface --- src/ChallengeProvider.php | 1 - src/ClientData.php | 5 +++++ src/LoginResponseInterface.php | 17 +++++++++++++++++ src/ResponseTrait.php | 5 +++++ src/Server.php | 25 ++++++------------------- src/SignResponse.php | 21 ++++++++++++++++++++- 6 files changed, 53 insertions(+), 21 deletions(-) create mode 100644 src/LoginResponseInterface.php diff --git a/src/ChallengeProvider.php b/src/ChallengeProvider.php index 9aa5681..334b117 100644 --- a/src/ChallengeProvider.php +++ b/src/ChallengeProvider.php @@ -5,6 +5,5 @@ interface ChallengeProvider { - public function getChallenge(): string; } diff --git a/src/ClientData.php b/src/ClientData.php index 73b1ee9..58ea494 100644 --- a/src/ClientData.php +++ b/src/ClientData.php @@ -28,6 +28,11 @@ public static function fromJson(string $json) return $ret; } + public function getApplicationParameter(): string + { + return hash('sha256', $this->origin, true); + } + /** * Checks the 'typ' field against the allowed types in the U2F spec (sec. * 7.1) diff --git a/src/LoginResponseInterface.php b/src/LoginResponseInterface.php new file mode 100644 index 0000000..b092f6d --- /dev/null +++ b/src/LoginResponseInterface.php @@ -0,0 +1,17 @@ +clientData; } + public function getChallengeProvider(): ChallengeProvider + { + return $this->clientData; + } + protected function setSignature(string $signature): self { $this->signature = $signature; diff --git a/src/Server.php b/src/Server.php index 92d0f02..6c59777 100644 --- a/src/Server.php +++ b/src/Server.php @@ -61,19 +61,19 @@ public function __construct() // @codeCoverageIgnoreEnd } /** - * This method authenticates a `SignResponse` against outstanding + * This method authenticates a `LoginResponseInterface` against outstanding * registrations and their corresponding `SignRequest`s. If the response's * signature validates and the counter hasn't done anything strange, the * registration will be returned with an updated counter value, which *must* * be persisted for the next authentication. If any verification component * fails, a `SE` will be thrown. * - * @param SignResponse $response the parsed response from the user + * @param LoginResponseInterface $response the parsed response from the user * @return RegistrationInterface if authentication succeeds * @throws SE if authentication fails * @throws BadMethodCallException if a precondition is not met */ - public function authenticate(SignResponse $response): RegistrationInterface + public function authenticate(LoginResponseInterface $response): RegistrationInterface { if (!$this->registrations) { throw new BadMethodCallException( @@ -117,28 +117,15 @@ public function authenticate(SignResponse $response): RegistrationInterface // match the one in the signing request, the client signed the // wrong thing. This could possibly be an attempt at a replay // attack. - $this->validateChallenge($response->getClientData(), $request); + $this->validateChallenge($response->getChallengeProvider(), $request); $pem = $registration->getPublicKeyPem(); - // U2F Spec: - // https://fidoalliance.org/specs/fido-u2f-v1.0-nfc-bt-amendment-20150514/fido-u2f-raw-message-formats.html#authentication-response-message-success - $to_verify = sprintf( - '%s%s%s%s', - $request->getApplicationParameter(), - chr($response->getUserPresenceByte()), - pack('N', $response->getCounter()), - // Note: Spec says this should be from the request, but that's not - // actually available via the JS API. Because we assert the - // challenge *value* from the Client Data matches the trusted one - // from the SignRequest and that value is included in the Challenge - // Parameter, this is safe unless/until SHA-256 is broken. - $response->getClientData()->getChallengeParameter() - ); + $toVerify = $response->getSignedData(); // Signature must validate against $sig_check = openssl_verify( - $to_verify, + $toVerify, $response->getSignature(), $pem, \OPENSSL_ALGO_SHA256 diff --git a/src/SignResponse.php b/src/SignResponse.php index 6a63f56..f642332 100644 --- a/src/SignResponse.php +++ b/src/SignResponse.php @@ -5,7 +5,7 @@ use Firehed\U2F\InvalidDataException as IDE; -class SignResponse +class SignResponse implements LoginResponseInterface { use ResponseTrait; @@ -17,11 +17,30 @@ public function getCounter(): int { return $this->counter; } + public function getUserPresenceByte(): int { return $this->user_presence; } + public function getSignedData(): string + { + // U2F Spec: + // https://fidoalliance.org/specs/fido-u2f-v1.0-nfc-bt-amendment-20150514/fido-u2f-raw-message-formats.html#authentication-response-message-success + return sprintf( + '%s%s%s%s', + $this->getClientData()->getApplicationParameter(), + chr($this->getUserPresenceByte()), + pack('N', $this->getCounter()), + // Note: Spec says this should be from the request, but that's not + // actually available via the JS API. Because we assert the + // challenge *value* from the Client Data matches the trusted one + // from the SignRequest and that value is included in the Challenge + // Parameter, this is safe unless/until SHA-256 is broken. + $this->getClientData()->getChallengeParameter() + ); + } + protected function parseResponse(array $response): self { $this->validateKeyInArray('keyHandle', $response); From 80c65935589890e33ec9f15ff2a3c065bcf69826 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 27 May 2019 12:32:39 -0700 Subject: [PATCH 02/11] RegistrationResponseInterface --- src/RegisterResponse.php | 20 ++++++++++- src/RegistrationResponseInterface.php | 25 +++++++++++++ src/Server.php | 51 +++++++++++++-------------- 3 files changed, 69 insertions(+), 27 deletions(-) create mode 100644 src/RegistrationResponseInterface.php diff --git a/src/RegisterResponse.php b/src/RegisterResponse.php index be0b6df..df4f891 100644 --- a/src/RegisterResponse.php +++ b/src/RegisterResponse.php @@ -5,7 +5,7 @@ use Firehed\U2F\InvalidDataException as IDE; -class RegisterResponse +class RegisterResponse implements RegistrationResponseInterface { use AttestationCertificateTrait; use ECPublicKeyTrait; @@ -108,4 +108,22 @@ protected function parseResponse(array $response): self return $this; } + + public function getSignedData(): string + { + // https://fidoalliance.org/specs/fido-u2f-v1.0-nfc-bt-amendment-20150514/fido-u2f-raw-message-formats.html#fig-authentication-request-message + return sprintf( + '%s%s%s%s%s', + chr(0), + $this->getClientData()->getApplicationParameter(), + $this->getClientData()->getChallengeParameter(), + $this->getKeyHandleBinary(), + $this->getPublicKeyBinary() + ); + } + + public function getRpIdHash(): string + { + return $this->getClientData()->getApplicationParameter(); + } } diff --git a/src/RegistrationResponseInterface.php b/src/RegistrationResponseInterface.php new file mode 100644 index 0000000..8c361ce --- /dev/null +++ b/src/RegistrationResponseInterface.php @@ -0,0 +1,25 @@ +registerRequest) { throw new BadMethodCallException( @@ -194,28 +194,27 @@ public function register(RegisterResponse $resp): RegistrationInterface 'with setRegisterRequest()' ); } - $this->validateChallenge($resp->getClientData(), $this->registerRequest); - // Check the Application Parameter? - - // https://fidoalliance.org/specs/fido-u2f-v1.0-nfc-bt-amendment-20150514/fido-u2f-raw-message-formats.html#registration-response-message-success - $signed_data = sprintf( - '%s%s%s%s%s', - chr(0), + $this->validateChallenge($response->getChallengeProvider(), $this->registerRequest); + // Check the Application Parameter + // Note: this is a bit delicate at the moment, since different + // protocols have different rules around the handling of Relying Party + // verification. Expect this to be revised. + if (!hash_equals( $this->registerRequest->getApplicationParameter(), - $resp->getClientData()->getChallengeParameter(), - $resp->getKeyHandleBinary(), - $resp->getPublicKeyBinary() - ); + $response->getRpIdHash() + )) { + throw new SE(SE::SIGNATURE_INVALID); + } - $pem = $resp->getAttestationCertificatePem(); + $pem = $response->getAttestationCertificatePem(); if ($this->verifyCA) { - $resp->verifyIssuerAgainstTrustedCAs($this->trustedCAs); + $response->verifyIssuerAgainstTrustedCAs($this->trustedCAs); } // Signature must validate against device issuer's public key $sig_check = openssl_verify( - $signed_data, - $resp->getSignature(), + $response->getSignedData(), + $response->getSignature(), $pem, \OPENSSL_ALGO_SHA256 ); @@ -224,10 +223,10 @@ public function register(RegisterResponse $resp): RegistrationInterface } return (new Registration()) - ->setAttestationCertificate($resp->getAttestationCertificateBinary()) + ->setAttestationCertificate($response->getAttestationCertificateBinary()) ->setCounter(0) // The response does not include this - ->setKeyHandle($resp->getKeyHandleBinary()) - ->setPublicKey($resp->getPublicKeyBinary()); + ->setKeyHandle($response->getKeyHandleBinary()) + ->setPublicKey($response->getPublicKeyBinary()); } /** From 96a9071ba0dd37ca69d3893dc4b396e65fd3dfa6 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Mon, 27 May 2019 12:50:02 -0700 Subject: [PATCH 03/11] Invert control of attestation cert verification --- src/AttestationCertificateTrait.php | 13 --------- src/RegistrationResponseInterface.php | 2 -- src/Server.php | 26 +++++++++++++++-- tests/AttestationCertificateTraitTest.php | 34 ----------------------- 4 files changed, 24 insertions(+), 51 deletions(-) diff --git a/src/AttestationCertificateTrait.php b/src/AttestationCertificateTrait.php index 6620773..3acb9b8 100644 --- a/src/AttestationCertificateTrait.php +++ b/src/AttestationCertificateTrait.php @@ -31,17 +31,4 @@ public function setAttestationCertificate(string $cert): self $this->attest = $cert; return $this; } - - public function verifyIssuerAgainstTrustedCAs(array $trusted_cas): bool - { - $result = openssl_x509_checkpurpose( - $this->getAttestationCertificatePem(), - \X509_PURPOSE_ANY, - $trusted_cas - ); - if ($result !== true) { - throw new SecurityException(SecurityException::NO_TRUSTED_CA); - } - return $result; - } } diff --git a/src/RegistrationResponseInterface.php b/src/RegistrationResponseInterface.php index 8c361ce..c9f20a9 100644 --- a/src/RegistrationResponseInterface.php +++ b/src/RegistrationResponseInterface.php @@ -20,6 +20,4 @@ public function getRpIdHash(): string; public function getSignature(): string; public function getSignedData(): string; - - public function verifyIssuerAgainstTrustedCAs(array $trustedCAs): bool; } diff --git a/src/Server.php b/src/Server.php index 06042df..433a479 100644 --- a/src/Server.php +++ b/src/Server.php @@ -206,12 +206,12 @@ public function register(RegistrationResponseInterface $response): RegistrationI throw new SE(SE::SIGNATURE_INVALID); } - $pem = $response->getAttestationCertificatePem(); if ($this->verifyCA) { - $response->verifyIssuerAgainstTrustedCAs($this->trustedCAs); + $this->verifyAttestationCertAgainstTrustedCAs($response); } // Signature must validate against device issuer's public key + $pem = $response->getAttestationCertificatePem(); $sig_check = openssl_verify( $response->getSignedData(), $response->getSignature(), @@ -404,4 +404,26 @@ private function validateChallenge( // TOOD: generate and compare timestamps return true; } + + /** + * Asserts that the attestation cert provided by the registration is issued + * by the set of trusted CAs. + * + * @param RegistrationResponseInterface $response The response to validate + * @throws SecurityException upon failure + * @return void + */ + private function verifyAttestationCertAgainstTrustedCAs(RegistrationResponseInterface $response): void + { + $pem = $response->getAttestationCertificatePem(); + + $result = openssl_x509_checkpurpose( + $pem, + \X509_PURPOSE_ANY, + $this->trustedCAs + ); + if ($result !== true) { + throw new SE(SE::NO_TRUSTED_CA); + } + } } diff --git a/tests/AttestationCertificateTraitTest.php b/tests/AttestationCertificateTraitTest.php index 15f4d89..63860b2 100644 --- a/tests/AttestationCertificateTraitTest.php +++ b/tests/AttestationCertificateTraitTest.php @@ -52,40 +52,6 @@ public function testGetAttestationCertificatePem() ); } - /** - * @covers ::verifyIssuerAgainstTrustedCAs - */ - public function testSuccessfulCAVerification() - { - $class = $this->getObjectWithYubicoCert(); - $certs = [dirname(__DIR__).'/CAcerts/yubico.pem']; - $this->assertTrue($class->verifyIssuerAgainstTrustedCAs($certs)); - } - - /** - * @covers ::verifyIssuerAgainstTrustedCAs - */ - public function testFailedCAVerification() - { - $class = $this->getObjectWithYubicoCert(); - $certs = [__DIR__.'/verisign_only_for_unit_tests.pem']; - $this->expectException(SecurityException::class); - $this->expectExceptionCode(SecurityException::NO_TRUSTED_CA); - $class->verifyIssuerAgainstTrustedCAs($certs); - } - - /** - * @covers ::verifyIssuerAgainstTrustedCAs - */ - public function testFailedCAVerificationFromNoCAs() - { - $class = $this->getObjectWithYubicoCert(); - $certs = []; - $this->expectException(SecurityException::class); - $this->expectExceptionCode(SecurityException::NO_TRUSTED_CA); - $class->verifyIssuerAgainstTrustedCAs($certs); - } - // -(Helper methods)------------------------------------------------------- /** From 9d78a4760dc2b08e27e1fe712a5181d0e83f4e23 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Wed, 29 May 2019 19:29:05 -0700 Subject: [PATCH 04/11] test getSignedData for registration response --- tests/RegisterResponseTest.php | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/RegisterResponseTest.php b/tests/RegisterResponseTest.php index 2137d43..0947ab6 100644 --- a/tests/RegisterResponseTest.php +++ b/tests/RegisterResponseTest.php @@ -138,6 +138,40 @@ public function testDataAccuracyAfterSuccessfulParsing() ); } + /** + * @covers ::getSignedData + */ + public function testGetSignedData() + { + $json = file_get_contents(__DIR__ . '/register_response.json'); + assert($json !== false); + $response = RegisterResponse::fromJson($json); + + $expectedSignedData = sprintf( + '%s%s%s%s%s', + "\x00", + hash('sha256', 'https://u2f.ericstern.com', true), + hash( + 'sha256', + '{'. + '"typ":"navigator.id.finishEnrollment",'. + '"challenge":"PfsWR1Umy2V5Al1Bam2tG0yfPLeJElfwRzzAzkYPgzo",'. + '"origin":"https://u2f.ericstern.com",'. + '"cid_pubkey":""'. + '}', + true + ), + $response->getKeyHandleBinary(), + $response->getPublicKeyBinary() + ); + + $this->assertSame( + $expectedSignedData, + $response->getSignedData(), + 'Wrong signed data' + ); + } + // -( DataProviders )------------------------------------------------------ public function clientErrors() From 464077c45eca1b13f558bc8892055d20dbc66ce4 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Wed, 29 May 2019 19:34:12 -0700 Subject: [PATCH 05/11] test getSignedData for sign/login response --- tests/SignResponseTest.php | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/SignResponseTest.php b/tests/SignResponseTest.php index 76cf0c9..4c4d6e9 100644 --- a/tests/SignResponseTest.php +++ b/tests/SignResponseTest.php @@ -180,6 +180,40 @@ public function testFromJsonWithInvalidSignatureData() SignResponse::fromJson($json); } + /** + * @covers ::getSignedData + */ + public function testGetSignedData() + { + $json = file_get_contents(__DIR__ . '/sign_response.json'); + assert($json !== false); + $response = SignResponse::fromJson($json); + print_r($response); + + $expectedSignedData = sprintf( + '%s%s%s%s', + hash('sha256', 'https://u2f.ericstern.com', true), + "\x01", // user presence + "\x00\x00\x00\x2d", // counter (int(45)) + hash( + 'sha256', + '{'. + '"typ":"navigator.id.getAssertion",'. + '"challenge":"wt2ze8IskcTO3nIsO2D2hFjE5tVD041NpnYesLpJweg",'. + '"origin":"https://u2f.ericstern.com",'. + '"cid_pubkey":""'. + '}', + true + ), + ); + + $this->assertSame( + $expectedSignedData, + $response->getSignedData(), + 'Wrong signed data' + ); + } + /** * @dataProvider clientErrors */ From b4b4c882bd77fc1e571e6fbb78d2f23c0903db4b Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Wed, 29 May 2019 19:37:02 -0700 Subject: [PATCH 06/11] test client data getApplicationParameter --- tests/ClientDataTest.php | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/ClientDataTest.php b/tests/ClientDataTest.php index cbf2f89..9dff028 100644 --- a/tests/ClientDataTest.php +++ b/tests/ClientDataTest.php @@ -58,6 +58,26 @@ public function testGetChallengeParameter() ); } + /** + * @covers ::getApplicationParameter + */ + public function testGetApplicationParameter() + { + $goodData = [ + 'typ' => 'navigator.id.finishEnrollment', + 'challenge' => 'PfsWR1Umy2V5Al1Bam2tG0yfPLeJElfwRzzAzkYPgzo', + 'origin' => 'https://u2f.ericstern.com', + 'cid_pubkey' => '', + ]; + $goodJson = json_encode($goodData); + assert($goodJson !== false); + $clientData = ClientData::fromJson($goodJson); + $this->assertSame( + hash('sha256', 'https://u2f.ericstern.com', true), + $clientData->getApplicationParameter() + ); + } + /** * @covers ::fromJson */ From 3f2de1290239a70c9f1d7f37a0de9d2279015ddd Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Wed, 29 May 2019 19:38:28 -0700 Subject: [PATCH 07/11] getChallengeProvider test --- tests/ResponseTraitTest.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/ResponseTraitTest.php b/tests/ResponseTraitTest.php index f7bd109..fd696d7 100644 --- a/tests/ResponseTraitTest.php +++ b/tests/ResponseTraitTest.php @@ -27,6 +27,7 @@ protected function parseResponse(array $response): self /** * @covers ::fromJson * @covers ::getSignature + * @covers ::getChallengeProvider * @covers ::getClientData */ public function testValidJson() @@ -55,6 +56,11 @@ public function testValidJson() $response->getClientData(), 'ClientData was not parsed correctly' ); + $this->assertInstanceOf( + ChallengeProvider::class, + $response->getChallengeProvider(), + 'Did not get a challenge provider' + ); $this->assertSame( __METHOD__, From 652626caea849fbd8f582789552fb0f4f4b6c0b0 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Wed, 29 May 2019 19:42:01 -0700 Subject: [PATCH 08/11] Trim old syntax --- tests/SignResponseTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/SignResponseTest.php b/tests/SignResponseTest.php index 4c4d6e9..772d423 100644 --- a/tests/SignResponseTest.php +++ b/tests/SignResponseTest.php @@ -204,7 +204,7 @@ public function testGetSignedData() '"cid_pubkey":""'. '}', true - ), + ) ); $this->assertSame( From 8e06b1d5717bcaf4f407da90533842d958c7c95f Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Wed, 29 May 2019 19:42:51 -0700 Subject: [PATCH 09/11] Remove debug --- tests/SignResponseTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/SignResponseTest.php b/tests/SignResponseTest.php index 772d423..1d3e7c3 100644 --- a/tests/SignResponseTest.php +++ b/tests/SignResponseTest.php @@ -188,7 +188,6 @@ public function testGetSignedData() $json = file_get_contents(__DIR__ . '/sign_response.json'); assert($json !== false); $response = SignResponse::fromJson($json); - print_r($response); $expectedSignedData = sprintf( '%s%s%s%s', From aa7998e2f5df30317f50773dacb6c97630d89362 Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Wed, 29 May 2019 19:53:32 -0700 Subject: [PATCH 10/11] test getRpIdHash --- tests/RegisterResponseTest.php | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/RegisterResponseTest.php b/tests/RegisterResponseTest.php index 0947ab6..77f3137 100644 --- a/tests/RegisterResponseTest.php +++ b/tests/RegisterResponseTest.php @@ -172,6 +172,21 @@ public function testGetSignedData() ); } + /** + * @covers ::getRpIdHash + */ + public function testGetRpIdHash() + { + $json = file_get_contents(__DIR__ . '/register_response.json'); + assert($json !== false); + $response = RegisterResponse::fromJson($json); + + $this->assertSame( + hash('sha256', 'https://u2f.ericstern.com', true), + $response->getRpIdHash() + ); + } + // -( DataProviders )------------------------------------------------------ public function clientErrors() From fd921166c62a8893634dd1987f30075a58ef01ef Mon Sep 17 00:00:00 2001 From: Eric Stern Date: Wed, 29 May 2019 20:13:46 -0700 Subject: [PATCH 11/11] Adjust relying party verification --- src/AppIdTrait.php | 8 ++++++++ src/SecurityException.php | 2 ++ src/Server.php | 19 ++++++++++--------- tests/AppIdTraitTest.php | 17 +++++++++++++++++ tests/ClientDataTest.php | 1 - tests/ServerTest.php | 19 ++++++++++++------- 6 files changed, 49 insertions(+), 17 deletions(-) diff --git a/src/AppIdTrait.php b/src/AppIdTrait.php index c54e828..eb10a2d 100644 --- a/src/AppIdTrait.php +++ b/src/AppIdTrait.php @@ -25,4 +25,12 @@ public function getApplicationParameter(): string { return hash('sha256', $this->appId, true); } + + /** + * @return string The raw SHA-256 hash of the Relying Party ID + */ + public function getRpIdHash(): string + { + return hash('sha256', $this->appId, true); + } } diff --git a/src/SecurityException.php b/src/SecurityException.php index d31431d..2d2ed73 100644 --- a/src/SecurityException.php +++ b/src/SecurityException.php @@ -12,6 +12,7 @@ class SecurityException extends Exception const CHALLENGE_MISMATCH = 3; const KEY_HANDLE_UNRECOGNIZED = 4; const NO_TRUSTED_CA = 5; + const WRONG_RELYING_PARTY = 6; const MESSAGES = [ self::SIGNATURE_INVALID => 'Signature verification failed', @@ -23,6 +24,7 @@ class SecurityException extends Exception self::CHALLENGE_MISMATCH => 'Response challenge does not match request', self::KEY_HANDLE_UNRECOGNIZED => 'Key handle has not been registered', self::NO_TRUSTED_CA => 'The attestation certificate was not signed by any trusted Certificate Authority', + self::WRONG_RELYING_PARTY => 'Relying party invalid for this server', ]; public function __construct(int $code) diff --git a/src/Server.php b/src/Server.php index 433a479..c4b7975 100644 --- a/src/Server.php +++ b/src/Server.php @@ -196,15 +196,7 @@ public function register(RegistrationResponseInterface $response): RegistrationI } $this->validateChallenge($response->getChallengeProvider(), $this->registerRequest); // Check the Application Parameter - // Note: this is a bit delicate at the moment, since different - // protocols have different rules around the handling of Relying Party - // verification. Expect this to be revised. - if (!hash_equals( - $this->registerRequest->getApplicationParameter(), - $response->getRpIdHash() - )) { - throw new SE(SE::SIGNATURE_INVALID); - } + $this->validateRelyingParty($response->getRpIdHash()); if ($this->verifyCA) { $this->verifyAttestationCertAgainstTrustedCAs($response); @@ -380,6 +372,15 @@ private function generateChallenge(): string return toBase64Web(\random_bytes(16)); } + private function validateRelyingParty(string $rpIdHash): void + { + // Note: this is a bit delicate at the moment, since different + // protocols have different rules around the handling of Relying Party + // verification. Expect this to be revised. + if (!hash_equals($this->getRpIdHash(), $rpIdHash)) { + throw new SE(SE::WRONG_RELYING_PARTY); + } + } /** * Compares the Challenge value from a known source against the * user-provided value. A mismatch will throw a SE. Future diff --git a/tests/AppIdTraitTest.php b/tests/AppIdTraitTest.php index e6c85fe..1f5a710 100644 --- a/tests/AppIdTraitTest.php +++ b/tests/AppIdTraitTest.php @@ -50,4 +50,21 @@ public function testGetApplicationParameter() 'getApplicationParamter should return the raw SHA256 hash of the application id' ); } + + /** + * @covers ::getRpIdHash + */ + public function testGetRpIdHash() + { + $obj = new class { + use AppIdTrait; + }; + $appId = 'https://u2f.example.com'; + $obj->setAppId($appId); + $this->assertSame( + hash('sha256', $appId, true), + $obj->getRpIdHash(), + 'getRpIdHash should return the raw SHA256 hash of the application id' + ); + } } diff --git a/tests/ClientDataTest.php b/tests/ClientDataTest.php index 9dff028..b369220 100644 --- a/tests/ClientDataTest.php +++ b/tests/ClientDataTest.php @@ -10,7 +10,6 @@ */ class ClientDataTest extends \PHPUnit\Framework\TestCase { - /** * @covers ::fromJson */ diff --git a/tests/ServerTest.php b/tests/ServerTest.php index 76248fa..b81dbc6 100644 --- a/tests/ServerTest.php +++ b/tests/ServerTest.php @@ -12,7 +12,7 @@ */ class ServerTest extends \PHPUnit\Framework\TestCase { - const APP_ID = 'https://u2f.example.com'; + const APP_ID = 'https://u2f.ericstern.com'; const ENCODED_KEY_HANDLE = 'JUnVTStPn-V2-bCu0RlvPbukBpHTD5Mi1ZGglDOcN0vD45rnTD0BXdkRt78huTwJ7tVax'. @@ -326,14 +326,19 @@ public function testRegisterWorksWithCAList() */ public function testRegisterThrowsWithChangedApplicationParameter() { - $request = (new RegisterRequest()) - ->setAppId('https://not.my.u2f.example.com') - ->setChallenge('PfsWR1Umy2V5Al1Bam2tG0yfPLeJElfwRzzAzkYPgzo'); + $request = $this->getDefaultRegisterRequest(); - $response = $this->getDefaultRegisterResponse(); + $challengeProvider = $this->createMock(ChallengeProvider::class); + $challengeProvider->method('getChallenge') + ->willReturn($request->getChallenge()); + $response = $this->createMock(RegistrationResponseInterface::class); + $response->method('getChallengeProvider') + ->willReturn($challengeProvider); + $response->method('getRpIdHash') + ->willReturn(hash('sha256', 'https://some.otherdomain.com', true)); $this->expectException(SecurityException::class); - $this->expectExceptionCode(SecurityException::SIGNATURE_INVALID); + $this->expectExceptionCode(SecurityException::WRONG_RELYING_PARTY); $this->server ->setRegisterRequest($request) ->register($response); @@ -349,7 +354,7 @@ public function testRegisterThrowsWithChangedChallengeParameter() $data = $this->readJsonFile('register_response.json'); $cli = fromBase64Web($data['clientData']); $obj = json_decode($cli, true); - $obj['origin'] = 'https://not.my.u2f.example.com'; + $obj['cid_pubkey'] = 'nonsense'; $cli = toBase64Web($this->safeEncode($obj)); $data['clientData'] = $cli; $response = RegisterResponse::fromJson($this->safeEncode($data));