From 2a8c22c49caffe71ebf62683dcc4a87d1e81ae41 Mon Sep 17 00:00:00 2001 From: Andrew Millington Date: Wed, 15 Feb 2023 15:49:41 +0000 Subject: [PATCH 1/4] Move code challenge verification to private function --- src/Grant/AuthCodeGrant.php | 74 ++++++++++++++++++------------- tests/Grant/AuthCodeGrantTest.php | 71 +++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 31 deletions(-) diff --git a/src/Grant/AuthCodeGrant.php b/src/Grant/AuthCodeGrant.php index 3fac0344e..1bde2044e 100644 --- a/src/Grant/AuthCodeGrant.php +++ b/src/Grant/AuthCodeGrant.php @@ -127,39 +127,18 @@ public function respondToAccessTokenRequest( throw OAuthServerException::invalidRequest('code', 'Cannot decrypt the authorization code', $e); } - // Validate code challenge - if (!empty($authCodePayload->code_challenge)) { - $codeVerifier = $this->getRequestParameter('code_verifier', $request, null); - - if ($codeVerifier === null) { - throw OAuthServerException::invalidRequest('code_verifier'); - } + $codeVerifier = $this->getRequestParameter('code_verifier', $request, null); - // Validate code_verifier according to RFC-7636 - // @see: https://tools.ietf.org/html/rfc7636#section-4.1 - if (\preg_match('/^[A-Za-z0-9-._~]{43,128}$/', $codeVerifier) !== 1) { - throw OAuthServerException::invalidRequest( - 'code_verifier', - 'Code Verifier must follow the specifications of RFC-7636.' - ); - } + if (!empty($authCodePayload->code_challenge)) { + $this->validateCodeChallenge($authCodePayload, $codeVerifier); + } - if (\property_exists($authCodePayload, 'code_challenge_method')) { - if (isset($this->codeChallengeVerifiers[$authCodePayload->code_challenge_method])) { - $codeChallengeVerifier = $this->codeChallengeVerifiers[$authCodePayload->code_challenge_method]; - - if ($codeChallengeVerifier->verifyCodeChallenge($codeVerifier, $authCodePayload->code_challenge) === false) { - throw OAuthServerException::invalidGrant('Failed to verify `code_verifier`.'); - } - } else { - throw OAuthServerException::serverError( - \sprintf( - 'Unsupported code challenge method `%s`', - $authCodePayload->code_challenge_method - ) - ); - } - } + // If a code challenge isn't present but a code verifier is, reject the request to block PKCE downgrade attack + if (empty($authCodePayload->code_challenge) && $codeVerifier !== null) { + throw OAuthServerException::invalidRequest( + 'code_challenge', + 'code_verifier received when no code_challenge is present' + ); } // Issue and persist new access token @@ -181,6 +160,39 @@ public function respondToAccessTokenRequest( return $responseType; } + private function validateCodeChallenge($authCodePayload, $codeVerifier) + { + if ($codeVerifier === null) { + throw OAuthServerException::invalidRequest('code_verifier'); + } + + // Validate code_verifier according to RFC-7636 + // @see: https://tools.ietf.org/html/rfc7636#section-4.1 + if (\preg_match('/^[A-Za-z0-9-._~]{43,128}$/', $codeVerifier) !== 1) { + throw OAuthServerException::invalidRequest( + 'code_verifier', + 'Code Verifier must follow the specifications of RFC-7636.' + ); + } + + if (\property_exists($authCodePayload, 'code_challenge_method')) { + if (isset($this->codeChallengeVerifiers[$authCodePayload->code_challenge_method])) { + $codeChallengeVerifier = $this->codeChallengeVerifiers[$authCodePayload->code_challenge_method]; + + if ($codeChallengeVerifier->verifyCodeChallenge($codeVerifier, $authCodePayload->code_challenge) === false) { + throw OAuthServerException::invalidGrant('Failed to verify `code_verifier`.'); + } + } else { + throw OAuthServerException::serverError( + \sprintf( + 'Unsupported code challenge method `%s`', + $authCodePayload->code_challenge_method + ) + ); + } + } + } + /** * Validate the authorization code. * diff --git a/tests/Grant/AuthCodeGrantTest.php b/tests/Grant/AuthCodeGrantTest.php index 860c6e863..a8e4feef3 100644 --- a/tests/Grant/AuthCodeGrantTest.php +++ b/tests/Grant/AuthCodeGrantTest.php @@ -917,6 +917,77 @@ public function testRespondToAccessTokenRequestCodeChallengeS256() $this->assertInstanceOf(RefreshTokenEntityInterface::class, $response->getRefreshToken()); } + public function testPKCEDowngradeBlocked() + { + $client = new ClientEntity(); + $client->setIdentifier('foo'); + $client->setRedirectUri('http://foo/bar'); + $client->setConfidential(); + $clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock(); + $clientRepositoryMock->method('getClientEntity')->willReturn($client); + + $scopeRepositoryMock = $this->getMockBuilder(ScopeRepositoryInterface::class)->getMock(); + $scopeEntity = new ScopeEntity(); + $scopeRepositoryMock->method('getScopeEntityByIdentifier')->willReturn($scopeEntity); + $scopeRepositoryMock->method('finalizeScopes')->willReturnArgument(0); + + $accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock(); + $accessTokenRepositoryMock->method('getNewToken')->willReturn(new AccessTokenEntity()); + $accessTokenRepositoryMock->method('persistNewAccessToken')->willReturnSelf(); + + $refreshTokenRepositoryMock = $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock(); + $refreshTokenRepositoryMock->method('persistNewRefreshToken')->willReturnSelf(); + $refreshTokenRepositoryMock->method('getNewRefreshToken')->willReturn(new RefreshTokenEntity()); + + $grant = new AuthCodeGrant( + $this->getMockBuilder(AuthCodeRepositoryInterface::class)->getMock(), + $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock(), + new DateInterval('PT10M') + ); + + $grant->setClientRepository($clientRepositoryMock); + $grant->setScopeRepository($scopeRepositoryMock); + $grant->setAccessTokenRepository($accessTokenRepositoryMock); + $grant->setRefreshTokenRepository($refreshTokenRepositoryMock); + $grant->setEncryptionKey($this->cryptStub->getKey()); + $grant->setPrivateKey(new CryptKey('file://' . __DIR__ . '/../Stubs/private.key')); + + $request = new ServerRequest( + [], + [], + null, + 'POST', + 'php://input', + [], + [], + [], + [ + 'grant_type' => 'authorization_code', + 'client_id' => 'foo', + 'redirect_uri' => 'http://foo/bar', + 'code_verifier' => self::CODE_VERIFIER, + 'code' => $this->cryptStub->doEncrypt( + \json_encode( + [ + 'auth_code_id' => \uniqid(), + 'expire_time' => \time() + 3600, + 'client_id' => 'foo', + 'user_id' => 123, + 'scopes' => ['foo'], + 'redirect_uri' => 'http://foo/bar', + ] + ) + ), + ] + ); + + $this->expectException(\League\OAuth2\Server\Exception\OAuthServerException::class); + $this->expectExceptionCode(3); + + /** @var StubResponseType $response */ + $grant->respondToAccessTokenRequest($request, new StubResponseType(), new DateInterval('PT10M')); + } + public function testRespondToAccessTokenRequestMissingRedirectUri() { $client = new ClientEntity(); From c9255bd524f2e40219698e46c2ba7eacc79e7ae1 Mon Sep 17 00:00:00 2001 From: Andrew Millington Date: Wed, 15 Feb 2023 15:54:09 +0000 Subject: [PATCH 2/4] StyleCI fix --- tests/Grant/AuthCodeGrantTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Grant/AuthCodeGrantTest.php b/tests/Grant/AuthCodeGrantTest.php index a8e4feef3..536e071d8 100644 --- a/tests/Grant/AuthCodeGrantTest.php +++ b/tests/Grant/AuthCodeGrantTest.php @@ -984,7 +984,7 @@ public function testPKCEDowngradeBlocked() $this->expectException(\League\OAuth2\Server\Exception\OAuthServerException::class); $this->expectExceptionCode(3); - /** @var StubResponseType $response */ + /* @var StubResponseType $response */ $grant->respondToAccessTokenRequest($request, new StubResponseType(), new DateInterval('PT10M')); } From 1480564d751fadf600f9bd7448c5888a8588c1f9 Mon Sep 17 00:00:00 2001 From: Andrew Millington Date: Wed, 15 Feb 2023 15:57:35 +0000 Subject: [PATCH 3/4] Update changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3366be840..2e886b227 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Added - You can now set a leeway for time drift between servers when validating a JWT (PR #1304) +### Security +- Access token requests that contain a code_verifier but are not bound to a code_challenge will be rejected to prevent +a PKCE downgrade attack (PR #1326) + ### [8.3.6] - released 2022-11-14 ### Fixed - Use LooseValidAt instead of StrictValidAt so that users aren't forced to use claims such as NBF in their JWT tokens (PR #1312) From 0d523ddedd4d519a954610ac10cbbdbd64b983ba Mon Sep 17 00:00:00 2001 From: Andrew Millington Date: Wed, 15 Feb 2023 15:59:31 +0000 Subject: [PATCH 4/4] Move pkce check so it happens prior to validation of code challenge --- src/Grant/AuthCodeGrant.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Grant/AuthCodeGrant.php b/src/Grant/AuthCodeGrant.php index 1bde2044e..8336cf649 100644 --- a/src/Grant/AuthCodeGrant.php +++ b/src/Grant/AuthCodeGrant.php @@ -129,10 +129,6 @@ public function respondToAccessTokenRequest( $codeVerifier = $this->getRequestParameter('code_verifier', $request, null); - if (!empty($authCodePayload->code_challenge)) { - $this->validateCodeChallenge($authCodePayload, $codeVerifier); - } - // If a code challenge isn't present but a code verifier is, reject the request to block PKCE downgrade attack if (empty($authCodePayload->code_challenge) && $codeVerifier !== null) { throw OAuthServerException::invalidRequest( @@ -141,6 +137,10 @@ public function respondToAccessTokenRequest( ); } + if (!empty($authCodePayload->code_challenge)) { + $this->validateCodeChallenge($authCodePayload, $codeVerifier); + } + // Issue and persist new access token $accessToken = $this->issueAccessToken($accessTokenTTL, $client, $authCodePayload->user_id, $scopes); $this->getEmitter()->emit(new RequestAccessTokenEvent(RequestEvent::ACCESS_TOKEN_ISSUED, $request, $accessToken));