Repository navigation
Add explicit EC point-on-curve validation in ElytronEcdhEsAlgorithmProvider closes #53639 - #53644
atiqur-rahman-pro wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The regression test uses a noncanonical coordinate encoding, and the new field-boundary checks lack coverage.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Adds explicit EC point validation to the Elytron ECDH-ES provider.
Changes:
- Validates coordinate bounds and curve equation before key construction.
- Adds an off-curve EPK regression test.
| File | Description |
|---|---|
ElytronEcdhEsAlgorithmProvider.java |
Implements fail-fast point validation. |
ElytronEcdhEsAlgorithmProviderTest.java |
Tests rejection of an off-curve point. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (x.signum() < 0 || x.compareTo(p) >= 0) { | ||
| throw new IllegalArgumentException("EPK x coordinate is not in the field"); | ||
| } | ||
| if (y.signum() < 0 || y.compareTo(p) >= 0) { | ||
| throw new IllegalArgumentException("EPK y coordinate is not in the field"); |
|
The Windows/JDK 17 job fails before compiling project sources, while |
| * and cofactor one, so this check is sufficient to reject invalid-curve attacks. | ||
| */ | ||
| private static void validatePointOnCurve(BigInteger x, BigInteger y, ECParameterSpec spec) { | ||
| BigInteger p = ((java.security.spec.ECFieldFp) spec.getCurve().getField()).getP(); |
There was a problem hiding this comment.
Add import at the top. Check before perform a cast.
// Add to the imports at the top of ElytronEcdhEsAlgorithmProvider.java
import java.security.spec.ECFieldFp;
// Inside validatePointOnCurve — guard the cast
if (!(spec.getCurve().getField() instanceof ECFieldFp)) {
throw new IllegalArgumentException("Only prime-field (Fp) curves are supported for EPK validation");
}
BigInteger p = ((ECFieldFp) spec.getCurve().getField()).getP();
| .ephemeralPublicKey(jwk) | ||
| .build(); | ||
| PrivateKey decryptionKey = getPrivateKey("P-256", "VEmDZpDXXK8p8N0Cndsxs924q6nS1RXFASRl6BfUqdw"); | ||
| try { |
There was a problem hiding this comment.
use assertThrows instead if try/catch
There was a problem hiding this comment.
Thanks @atiqur-rahman-pro for the PR and the contribution. The PR solves the issue #53639.
Please check my comments
|
Thank you for the review! I have addressed your comments in the latest commit:
All 195 crypto Elytron tests pass cleanly. |
…ovider closes keycloak#53639 Signed-off-by: Atiqur Rahman <rahman.atiqur.pro@gmail.com>
Signed-off-by: Atiqur Rahman <rahman.atiqur.pro@gmail.com>
… test Signed-off-by: Atiqur Rahman <rahman.atiqur.pro@gmail.com>
9cc2015 to
fd1716d
Compare
|
Hi @atiqur-rahman-pro, I believe you meant to tag @jimmychakkalakal for this review rather than me. |
|
Thanks to @vramik for bringing this to our attention! @jimmychakkalakal, could you please take a look at this PR when you have a chance? It adds EC point-on-curve validation to The branch has been rebased against the latest |
There was a problem hiding this comment.
Thanks @atiqur-rahman-pro for the PR and changes.
I aprove the changes from my end
Closes #53639
Description
In
ElytronEcdhEsAlgorithmProvider.toPublicKey(), ephemeral public keys (EPK) from incoming JWE headers were decoded using jose4j'sEcKeyUtil.publicKey()and forwarded directly toKeyAgreement.getInstance("ECDH")without explicitly validating that the point lies on the specified elliptic curve.While #48960 added explicit point-on-curve checks to
BCEcdhEsAlgorithmProviderandBCFIPSEcdhEsAlgorithmProvider,ElytronEcdhEsAlgorithmProviderremained dependent on downstream JCA provider resolution order duringdoPhase().This PR:
validatePointOnCurve) inElytronEcdhEsAlgorithmProvider.toPublicKey(), checking both prime field membership (0 <= x < p, 0 <= y < p) and the curve equation (y² ≡ x³ + ax + b mod p).java.security.spec.ECFieldFp,BigInteger) to respect the Elytron module's independence from BouncyCastle.toPublicKeyRejectsOffCurvePointinElytronEcdhEsAlgorithmProviderTestverifying rejection of off-curve points.