Skip to content

Commit 3028f3f

Browse files
SephsterSephster
authored
Merge pull request thephpleague#1326 from thephpleague/enforce-code-verifier-if-challenge-present
Prevent PKCE Downgrade Attack
2 parents b8436ac + 0d523dd commit 3028f3f

File tree

3 files changed

+118
-31
lines changed

3 files changed

+118
-31
lines changed

‎CHANGELOG.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
88
### Added
99
- You can now set a leeway for time drift between servers when validating a JWT (PR #1304)
1010

11+
### Security
12+
- Access token requests that contain a code_verifier but are not bound to a code_challenge will be rejected to prevent
13+
a PKCE downgrade attack (PR #1326)
14+
1115
### [8.3.6] - released 2022-11-14
1216
### Fixed
1317
- Use LooseValidAt instead of StrictValidAt so that users aren't forced to use claims such as NBF in their JWT tokens (PR #1312)

‎src/Grant/AuthCodeGrant.php‎

Lines changed: 43 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -127,39 +127,18 @@ public function respondToAccessTokenRequest(
127127
throw OAuthServerException::invalidRequest('code', 'Cannot decrypt the authorization code', $e);
128128
}
129129

130-
// Validate code challenge
131-
if (!empty($authCodePayload->code_challenge)) {
132-
$codeVerifier = $this->getRequestParameter('code_verifier', $request, null);
133-
134-
if ($codeVerifier === null) {
135-
throw OAuthServerException::invalidRequest('code_verifier');
136-
}
130+
$codeVerifier = $this->getRequestParameter('code_verifier', $request, null);
137131

138-
// Validate code_verifier according to RFC-7636
139-
// @see: https://tools.ietf.org/html/rfc7636#section-4.1
140-
if (\preg_match('/^[A-Za-z0-9-._~]{43,128}$/', $codeVerifier) !== 1) {
141-
throw OAuthServerException::invalidRequest(
142-
'code_verifier',
143-
'Code Verifier must follow the specifications of RFC-7636.'
144-
);
145-
}
132+
// If a code challenge isn't present but a code verifier is, reject the request to block PKCE downgrade attack
133+
if (empty($authCodePayload->code_challenge) && $codeVerifier !== null) {
134+
throw OAuthServerException::invalidRequest(
135+
'code_challenge',
136+
'code_verifier received when no code_challenge is present'
137+
);
138+
}
146139

147-
if (\property_exists($authCodePayload, 'code_challenge_method')) {
148-
if (isset($this->codeChallengeVerifiers[$authCodePayload->code_challenge_method])) {
149-
$codeChallengeVerifier = $this->codeChallengeVerifiers[$authCodePayload->code_challenge_method];
150-
151-
if ($codeChallengeVerifier->verifyCodeChallenge($codeVerifier, $authCodePayload->code_challenge) === false) {
152-
throw OAuthServerException::invalidGrant('Failed to verify `code_verifier`.');
153-
}
154-
} else {
155-
throw OAuthServerException::serverError(
156-
\sprintf(
157-
'Unsupported code challenge method `%s`',
158-
$authCodePayload->code_challenge_method
159-
)
160-
);
161-
}
162-
}
140+
if (!empty($authCodePayload->code_challenge)) {
141+
$this->validateCodeChallenge($authCodePayload, $codeVerifier);
163142
}
164143

165144
// Issue and persist new access token
@@ -181,6 +160,39 @@ public function respondToAccessTokenRequest(
181160
return $responseType;
182161
}
183162

163+
private function validateCodeChallenge($authCodePayload, $codeVerifier)
164+
{
165+
if ($codeVerifier === null) {
166+
throw OAuthServerException::invalidRequest('code_verifier');
167+
}
168+
169+
// Validate code_verifier according to RFC-7636
170+
// @see: https://tools.ietf.org/html/rfc7636#section-4.1
171+
if (\preg_match('/^[A-Za-z0-9-._~]{43,128}$/', $codeVerifier) !== 1) {
172+
throw OAuthServerException::invalidRequest(
173+
'code_verifier',
174+
'Code Verifier must follow the specifications of RFC-7636.'
175+
);
176+
}
177+
178+
if (\property_exists($authCodePayload, 'code_challenge_method')) {
179+
if (isset($this->codeChallengeVerifiers[$authCodePayload->code_challenge_method])) {
180+
$codeChallengeVerifier = $this->codeChallengeVerifiers[$authCodePayload->code_challenge_method];
181+
182+
if ($codeChallengeVerifier->verifyCodeChallenge($codeVerifier, $authCodePayload->code_challenge) === false) {
183+
throw OAuthServerException::invalidGrant('Failed to verify `code_verifier`.');
184+
}
185+
} else {
186+
throw OAuthServerException::serverError(
187+
\sprintf(
188+
'Unsupported code challenge method `%s`',
189+
$authCodePayload->code_challenge_method
190+
)
191+
);
192+
}
193+
}
194+
}
195+
184196
/**
185197
* Validate the authorization code.
186198
*

‎tests/Grant/AuthCodeGrantTest.php‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -917,6 +917,77 @@ public function testRespondToAccessTokenRequestCodeChallengeS256()
917917
$this->assertInstanceOf(RefreshTokenEntityInterface::class, $response->getRefreshToken());
918918
}
919919

920+
public function testPKCEDowngradeBlocked()
921+
{
922+
$client = new ClientEntity();
923+
$client->setIdentifier('foo');
924+
$client->setRedirectUri('http://foo/bar');
925+
$client->setConfidential();
926+
$clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock();
927+
$clientRepositoryMock->method('getClientEntity')->willReturn($client);
928+
929+
$scopeRepositoryMock = $this->getMockBuilder(ScopeRepositoryInterface::class)->getMock();
930+
$scopeEntity = new ScopeEntity();
931+
$scopeRepositoryMock->method('getScopeEntityByIdentifier')->willReturn($scopeEntity);
932+
$scopeRepositoryMock->method('finalizeScopes')->willReturnArgument(0);
933+
934+
$accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock();
935+
$accessTokenRepositoryMock->method('getNewToken')->willReturn(new AccessTokenEntity());
936+
$accessTokenRepositoryMock->method('persistNewAccessToken')->willReturnSelf();
937+
938+
$refreshTokenRepositoryMock = $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock();
939+
$refreshTokenRepositoryMock->method('persistNewRefreshToken')->willReturnSelf();
940+
$refreshTokenRepositoryMock->method('getNewRefreshToken')->willReturn(new RefreshTokenEntity());
941+
942+
$grant = new AuthCodeGrant(
943+
$this->getMockBuilder(AuthCodeRepositoryInterface::class)->getMock(),
944+
$this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock(),
945+
new DateInterval('PT10M')
946+
);
947+
948+
$grant->setClientRepository($clientRepositoryMock);
949+
$grant->setScopeRepository($scopeRepositoryMock);
950+
$grant->setAccessTokenRepository($accessTokenRepositoryMock);
951+
$grant->setRefreshTokenRepository($refreshTokenRepositoryMock);
952+
$grant->setEncryptionKey($this->cryptStub->getKey());
953+
$grant->setPrivateKey(new CryptKey('file://' . __DIR__ . '/../Stubs/private.key'));
954+
955+
$request = new ServerRequest(
956+
[],
957+
[],
958+
null,
959+
'POST',
960+
'php://input',
961+
[],
962+
[],
963+
[],
964+
[
965+
'grant_type' => 'authorization_code',
966+
'client_id' => 'foo',
967+
'redirect_uri' => 'http://foo/bar',
968+
'code_verifier' => self::CODE_VERIFIER,
969+
'code' => $this->cryptStub->doEncrypt(
970+
\json_encode(
971+
[
972+
'auth_code_id' => \uniqid(),
973+
'expire_time' => \time() + 3600,
974+
'client_id' => 'foo',
975+
'user_id' => 123,
976+
'scopes' => ['foo'],
977+
'redirect_uri' => 'http://foo/bar',
978+
]
979+
)
980+
),
981+
]
982+
);
983+
984+
$this->expectException(\League\OAuth2\Server\Exception\OAuthServerException::class);
985+
$this->expectExceptionCode(3);
986+
987+
/* @var StubResponseType $response */
988+
$grant->respondToAccessTokenRequest($request, new StubResponseType(), new DateInterval('PT10M'));
989+
}
990+
920991
public function testRespondToAccessTokenRequestMissingRedirectUri()
921992
{
922993
$client = new ClientEntity();

0 commit comments

Comments
 (0)