Skip to content

compare pkce code verifier and client secret hash in constant time#3170

Merged
coheigea merged 2 commits into
apache:mainfrom
dxbjavid:oauth2-constant-time-pkce-secret
Jun 3, 2026
Merged

compare pkce code verifier and client secret hash in constant time#3170
coheigea merged 2 commits into
apache:mainfrom
dxbjavid:oauth2-constant-time-pkce-secret

Conversation

@dxbjavid
Copy link
Copy Markdown
Contributor

@dxbjavid dxbjavid commented Jun 2, 2026

Two token-endpoint checks still compare a client-supplied secret against a server-held value with String.equals. compareCodeVerifierWithChallenge matches the stored PKCE code_challenge against the transformed code_verifier, and with the plain transformer the challenge is the verifier itself, so the secret leaks byte by byte through timing. validateClientSecret in ClientSecretHashVerifier compares the secret hashes the same way. Route both through MessageDigest.isEqual, as the default client-secret check in AbstractTokenService and the OAuth2 token comparisons already do.

Copy link
Copy Markdown
Contributor

@coheigea coheigea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use OAuthUtils.compareTokens instead?

@dxbjavid
Copy link
Copy Markdown
Contributor Author

dxbjavid commented Jun 3, 2026

Good call, that's cleaner. Switched both sites to OAuthUtils.compareTokens so it's consistent with the other token checks. Dropped the now-unused MessageDigest/StandardCharsets imports.

@coheigea coheigea merged commit 8574198 into apache:main Jun 3, 2026
2 of 5 checks passed
coheigea pushed a commit that referenced this pull request Jun 3, 2026
…3170)

* compare pkce code verifier and client secret hash in constant time

* use OAuthUtils.compareTokens for the constant-time comparisons

(cherry picked from commit 8574198)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants