Harden RSA1_5 JWE decryption against timing-based padding oracles by hagerm98 · Pull Request #53085 · keycloak/keycloak

hagerm98 · 2026-09-23T01:45:20Z

Summary

This is a follow-up to #52205, which was narrowed to address the OID4VCI error response. The RSA1_5 CEK-decoding hardening discussed there is not specific to OID4VCI, so this PR implements it at the algorithm-provider level instead of adding RSA1_5-specific handling to JWE.

The new wrapper generates a random fallback CEK before delegating RSA1_5 decoding. It uses the decoded CEK only when it has the expected length; otherwise, processing continues with the fallback CEK through content authentication. Encoding is delegated unchanged, and decoding exceptions are available in trace logs.

The wrapper is registered for RSA1_5 in the default, FIPS, and Elytron crypto providers. Other key-management algorithms are unchanged.

Tests

Added tests for the wrapper's valid, null, wrong-length, and exception paths, and for unchanged encoding.
Added a shared JWE regression test comparing an invalid encrypted CEK with invalid authenticated content, plus a check that RSA-OAEP behavior is unchanged.
Ran the wrapper tests and the default, Elytron, and FIPS JWE suites. The FIPS suite was run in non-approved-only mode using a process-scoped FIPS test flag.
Spotless checks passed for the changed modules.

Closes #53084.

Copilot

Copilot review overview

🟡 Changes recommended

The critical timing-oracle risk from exception trace logging must be resolved before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

Avoid timing leaks from per-attempt RSA unwrap exception logging · New

What changed in this PR

Hardens RSA1_5 JWE decryption by substituting random fallback CEKs after unwrap failures.

Changes:

Adds and registers an RSA1_5 wrapper across all crypto providers.
Adds wrapper and JWE regression tests.

File	Description
`crypto/fips1402/src/test/java/org/keycloak/crypto/fips/test/FIPS1402JWETest.java`	Adds FIPS regression coverage.
`crypto/fips1402/src/main/java/org/keycloak/crypto/fips/FIPS1402Provider.java`	Registers the wrapper for FIPS.
`crypto/elytron/src/main/java/org/keycloak/crypto/elytron/WildFlyElytronProvider.java`	Registers the wrapper for Elytron.
`crypto/default/src/main/java/org/keycloak/crypto/def/DefaultCryptoProvider.java`	Registers the wrapper for the default provider.
`core/src/test/java/org/keycloak/jose/JWETest.java`	Adds RSA1_5 and RSA-OAEP regression tests.
`core/src/test/java/org/keycloak/jose/jwe/alg/RSA15WrapperAlgorithmProviderTest.java`	Tests wrapper behavior and encoding delegation.
`core/src/main/java/org/keycloak/jose/jwe/alg/RSA15WrapperAlgorithmProvider.java`	Implements fallback CEK handling. Critical: Exception trace logging adds failure-specific work before authentication, potentially restoring a timing oracle; suppress per-attempt unwrap exception logging or use aggregate metrics.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

keycloak-github-bot · 2026-09-23T02:32:28Z

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.federation.ldap.LDAPUserLoginTest#loginLDAPUserCredentialVaultAuthenticationNoneEncryptionStartTLS

Keycloak CI - Java Distribution IT (windows-latest - temurin - 21)

org.openqa.selenium.TimeoutException: 
java.net.SocketTimeoutException: Read timed out
Build info: version: '4.46.0', revision: 'df5a634 df5a6341cbb5f8f06b836bd595752775627c80dc'
System info: os.name: 'Windows Server 2025', os.arch: 'amd64', os.version: '10.0', java.version: '21.0.12.1'
Driver info: driver.version: HtmlUnitDriver
...

Report flaky test

keycloak-github-bot

Unreported flaky test detected, please review

oleksii-udovychenko · 2026-09-23T14:42:05Z


        providers.put(CryptoConstants.A128KW, new AesKeyWrapAlgorithmProvider());
-        providers.put(CryptoConstants.RSA1_5, new DefaultRsaKeyEncryptionJWEAlgorithmProvider("RSA/ECB/PKCS1Padding"));
+        providers.put(CryptoConstants.RSA1_5, new RSA15WrapperAlgorithmProvider(new DefaultRsaKeyEncryptionJWEAlgorithmProvider("RSA/ECB/PKCS1Padding")));


Is the RSA1_5-only scope deliberate, or should the same wrapper go around RSA_OAEP and RSA_OAEP_256?

RSA15WrapperAlgorithmProvider is registered for CryptoConstants.RSA1_5 in all three crypto providers, and substitutes a random CEK when decodeCek throws or returns a wrong-length one. The two lines below it keep the plain OAEP providers, and testRSAOAEP_CekUnwrapFailureIsNotSubstituted pins that with assertNotEquals(AEADBadTagException.class, cause.getClass()).

I ran all three algs on main against A128GCM, comparing an all-zero encrypted CEK with a one-bit-flipped ciphertext. They behave identically: BadPaddingException("Padding error in decryption") for the bad CEK, AEADBadTagException("mac check in GCM failed") for the bad ciphertext. So the difference this PR removes for v1.5 is still there for OAEP.

@oleksii-udovychenko Thanks for checking this across all three algorithms. Actually the initial scope and concern of this issue (and its precedent issue/PR [#52205]) was deliberately limited to RSA1_5 to mitigate the Bleichenbacher-style padding oracle attack which applies to RSA1_5 (and its implicit timing based in this PR)

but I think you're right that the OAEP providers still stop before content authentication when CEK decoding fails, and even though they're not vulnerable to that specific Bleichenbacher attack, that doesn't guarantee immunity against others, so I think it makes sense to apply the same hardening to them as well.

I'll address this and update the PR, @mposolda FYI

mposolda

@hagerm98 Thanks for the PR!

What do you think about the inline comment by @oleksii-udovychenko ? I am leaving to your decision whether you prefer to keep PR as it is or whether to add that wrapper against other RSA ciphers. Hopefully same wrapper can be used for all algorithms? If you think it makes sense, please rename class to RSAWrapperAlgorithmProvider .

Closes keycloak#53084 Signed-off-by: Hager Khamis <hagerm98@hotmail.com>

Copilot

Copilot review overview

🟡 Changes recommended

OAEP registrations unintentionally alter unwrap and configuration failure behavior.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 3

Open (3)

Resolved since last review (1)

Avoid timing leaks from per-attempt RSA unwrap exception logging

+        providers.put(CryptoConstants.RSA_OAEP, new RSAWrapperAlgorithmProvider(new DefaultRsaKeyEncryptionJWEAlgorithmProvider("RSA/ECB/OAEPWithSHA-1AndMGF1Padding")));
+        providers.put(CryptoConstants.RSA_OAEP_256, new RSAWrapperAlgorithmProvider(new DefaultRsaKeyEncryption256JWEAlgorithmProvider("RSA/ECB/OAEPWithSHA-256AndMGF1Padding")));


+        providers.put(CryptoConstants.RSA_OAEP, new RSAWrapperAlgorithmProvider(new ElytronRsaKeyEncryptionJWEAlgorithmProvider("RSA/ECB/OAEPWithSHA-1AndMGF1Padding")));
+        providers.put(CryptoConstants.RSA_OAEP_256, new RSAWrapperAlgorithmProvider(new ElytronRsaKeyEncryption256JWEAlgorithmProvider("RSA/ECB/OAEPWithSHA-256AndMGF1Padding")));


+        providers.put(CryptoConstants.RSA_OAEP, new RSAWrapperAlgorithmProvider(new FIPSRsaKeyEncryptionJWEAlgorithmProvider(FipsRSA.WRAP_OAEP)));
+        providers.put(CryptoConstants.RSA_OAEP_256, new RSAWrapperAlgorithmProvider(new FIPSRsaKeyEncryptionJWEAlgorithmProvider(FipsRSA.WRAP_OAEP.withDigest(FipsSHS.Algorithm.SHA256))));


-        providers.put(CryptoConstants.RSA1_5, new DefaultRsaKeyEncryptionJWEAlgorithmProvider("RSA/ECB/PKCS1Padding"));
-        providers.put(CryptoConstants.RSA_OAEP, new DefaultRsaKeyEncryptionJWEAlgorithmProvider("RSA/ECB/OAEPWithSHA-1AndMGF1Padding"));
-        providers.put(CryptoConstants.RSA_OAEP_256, new DefaultRsaKeyEncryption256JWEAlgorithmProvider("RSA/ECB/OAEPWithSHA-256AndMGF1Padding"));
+        providers.put(CryptoConstants.RSA1_5, new RSAWrapperAlgorithmProvider(new DefaultRsaKeyEncryptionJWEAlgorithmProvider("RSA/ECB/PKCS1Padding")));


Copilot AI balanced review requested due to automatic review settings September 23, 2026 01:45

hagerm98 requested a review from a team as a code owner September 23, 2026 01:45

Copilot started reviewing on behalf of hagerm98 September 23, 2026 01:45 View session

Copilot AI reviewed Sep 23, 2026

View reviewed changes

Comment thread core/src/main/java/org/keycloak/jose/jwe/alg/RSAWrapperAlgorithmProvider.java

github-advanced-security Bot found potential problems Sep 23, 2026

View reviewed changes

Comment thread crypto/default/src/main/java/org/keycloak/crypto/def/DefaultCryptoProvider.java Fixed

keycloak-github-bot Bot added the flaky-test label Sep 23, 2026

keycloak-github-bot Bot reviewed Sep 23, 2026

View reviewed changes

oleksii-udovychenko reviewed Sep 23, 2026

View reviewed changes

mposolda self-assigned this Sep 23, 2026

mposolda reviewed Sep 23, 2026

View reviewed changes

Harden RSA1_5 JWE decryption against timing-based padding oracles

0312cd9

Closes keycloak#53084 Signed-off-by: Hager Khamis <hagerm98@hotmail.com>

Copilot AI review requested due to automatic review settings September 24, 2026 00:17

hagerm98 force-pushed the fix/rsa15-jwe-cek-hardening branch from 6953ffa to 0312cd9 Compare September 24, 2026 00:17

Copilot started reviewing on behalf of hagerm98 September 24, 2026 00:17 View session

Copilot AI reviewed Sep 24, 2026

View reviewed changes

github-advanced-security Bot found potential problems Sep 24, 2026

View reviewed changes

Navigation Menu

Harden RSA1_5 JWE decryption against timing-based padding oracles - #53085

Harden RSA1_5 JWE decryption against timing-based padding oracles#53085
hagerm98 wants to merge 1 commit into
keycloak:mainfrom
hagerm98:fix/rsa15-jwe-cek-hardening

hagerm98 commented Sep 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

keycloak-github-bot Bot commented Sep 23, 2026

Uh oh!

keycloak-github-bot Bot left a comment

Uh oh!

oleksii-udovychenko Sep 23, 2026

Uh oh!

hagerm98 Sep 23, 2026

Uh oh!

mposolda left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		providers.put(CryptoConstants.RSA_OAEP, new RSAWrapperAlgorithmProvider(new DefaultRsaKeyEncryptionJWEAlgorithmProvider("RSA/ECB/OAEPWithSHA-1AndMGF1Padding")));
		providers.put(CryptoConstants.RSA_OAEP_256, new RSAWrapperAlgorithmProvider(new DefaultRsaKeyEncryption256JWEAlgorithmProvider("RSA/ECB/OAEPWithSHA-256AndMGF1Padding")));

Conversation

hagerm98 commented Sep 23, 2026

Summary

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

keycloak-github-bot Bot commented Sep 23, 2026

Unreported flaky test detected

org.keycloak.testsuite.federation.ldap.LDAPUserLoginTest#loginLDAPUserCredentialVaultAuthenticationNoneEncryptionStartTLS

Uh oh!

keycloak-github-bot Bot left a comment

Choose a reason for hiding this comment

Uh oh!

oleksii-udovychenko Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

hagerm98 Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

mposolda left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants