Merge pull request #1326 from thephpleague/enforce-code-verifier-if-c…

…hallenge-present Prevent PKCE Downgrade Attack
thephpleague · Feb 15, 2023 · 3028f3f · 3028f3f
2 parents b8436ac + 0d523dd
commit 3028f3f
Show file tree

Hide file tree

Showing 3 changed files with 118 additions and 31 deletions.
diff --git 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)

diff --git 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 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'
+            );
+        }
 
-            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 (!empty($authCodePayload->code_challenge)) {
+            $this->validateCodeChallenge($authCodePayload, $codeVerifier);
         }
 
         // 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
@@ -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();