Repository navigation
Validate ECDSA signature length before JWS verification - #53928
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The centralized validation correctly closes the signature-malleability issue with comprehensive provider coverage.
0 open findings
What changed in this PR
Enforces RFC 7518 ECDSA signature lengths before provider-specific JWS verification.
Changes:
- Rejects truncated or trailing-byte ECDSA signatures.
- Adds shared ES256/384/512 regression tests across all crypto providers.
| File | Description |
|---|---|
core/.../ECDSAAlgorithm.java |
Validates exact signature length. |
core/.../ECDSASignatureVerifierContextTest.java |
Adds shared regression coverage. |
crypto/default/.../DefaultCryptoECDSASignatureVerifierContextTest.java |
Runs tests with the default provider. |
crypto/fips1402/.../FIPS1402ECDSASignatureVerifierContextTest.java |
Runs tests with the FIPS provider. |
crypto/elytron/.../ElytronCryptoECDSASignatureVerifierContextTest.java |
Runs tests with the Elytron provider. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Should the signature segment also be decoded strictly, so the same token can't be rewritten at the base64url level?
With this PR, a signature with extra bytes after R || S is rejected for ES256, ES384 and ES512. A token with =anything-at-all appended to its signature segment still verifies, so one valid token still has an unlimited number of accepted string forms.
JWSInput decodes the signature with Base64Url.decode, which passes it through encodeBase64ToBase64Url. That function cuts the input at the first = and maps + and / to - and _, and Base64.getUrlDecoder() ignores non-zero trailing bits. So the decoded bytes are still the 64, 96 or 132 octets the new check expects.
How I tested, on this PR's head with a probe test in crypto/default (default provider):
- Generated a key per curve and signed an
AccessTokenwithJWSBuilderandECDSASignatureSignerContext. - Rewrote only the signature segment, then checked each string with both
ECDSASignatureVerifierContextandTokenVerifier.create(...).publicKey(...).verify(). "Accepted" means both passed.
The header and payload can't be rewritten this way, because the signing input is built from the raw strings. I only ran ES256/384/512; Base64Url.decode isn't specific to ECDSA, so other algorithms may behave the same.
| Signature segment | Expected | Actual |
|---|---|---|
| Unchanged | accepted | accepted (all three) |
AA appended, 2 extra bytes after R || S |
rejected | rejected (all three, this PR) |
=anything-at-all appended |
rejected | accepted (all three) |
- and _ replaced with + and / |
rejected | accepted (all three) |
== padding appended (ES256, 86 chars) |
rejected | accepted |
| Last character changed to one of the other 15 values with the same top 2 bits (ES256, 4 unused bits) | rejected | accepted, 15 of 15 |
|
Agreed, that's real, and your table matches the code: I'd keep it out of this PR though. It sits in Happy to open a separate issue for that and follow up with a PR against it, so this one stays scoped to the R||S length check. |
|
@naruto-lgtm Thanks! There is failing test, which is probably related to your changes. Can you please doublecheck it? Failing test is: |
Closes keycloak#53927 Signed-off-by: Nayyar <nayyar@bugqore.com>
f703643 to
74d81df
Compare
|
Good catch, that one's on me. Pushed a fix (amended into the single commit): the three ECDSA verifier contexts now return false for a wrong-length Verified locally by driving |
There was a problem hiding this comment.
@naruto-lgtm Thanks!
There was a problem hiding this comment.
@naruto-lgtm thanks! The fix looks good to me, approving. I just left one non-blocking inline comment on the FIPS test guard, feel free to address it here.
| @Before | ||
| public void before() { | ||
| // Run this test just if java is not in FIPS mode | ||
| Assume.assumeFalse("Java is in FIPS mode. Skipping the test.", Environment.isJavaInFipsMode()); |
There was a problem hiding this comment.
FIPS1402ECDSASignatureVerifierContextTest uses assumeFalse(isJavaInFipsMode()), so it's skipped in the FIPS UT job (which runs with -Dcom.redhat.fips=true). I'd drop the @Before so it runs in both jobs, like BCFIPSECDSACryptoProviderTest.
There was a problem hiding this comment.
Good catch! The test should be aligned with other tests in the crypto/fips module.
@naruto-lgtm Could you please fix this?
There was a problem hiding this comment.
Makes sense. Dropped the @Before in 17c9205, so it now runs in both the regular and the FIPS UT job like BCFIPSECDSACryptoProviderTest. The default-provider subclass keeps its assumeFalse since it shouldn't run under FIPS.
There was a problem hiding this comment.
The length checks look correct at 74d81df. The regression tests pass across default, Elytron, and BCFIPS providers, and running them with the pre-fix core classes reproduces the original failure. I also verified the server verifier and TokenVerifier rejection paths. No blocking findings; ready for the scoped signature-length fix.
There was a problem hiding this comment.
Marking as "request changes" due the FIPS1402ECDSASignatureVerifierContextTest (see inline comment).
@naruto-lgtm Could you please take a look?
Closes keycloak#53927 Signed-off-by: Nayyar <nayyar@bugqore.com>
There was a problem hiding this comment.
@naruto-lgtm @lhanusov @graziang Thanks for the PR and reviews!
concatenatedRSToASN1DER reads only the first signLength bytes of the concatenated R||S value in every crypto provider, so an ES256/384/512 JWS signature with trailing bytes appended still verifies, even though RFC 7518 section 3.4 fixes it to exactly 2 * curve-size octets, and anyone holding a valid token can derive unlimited distinct strings that all pass verification. Reject wrong-length signatures at the one chokepoint the three ECDSA verifier contexts funnel through, so the check holds under the default, FIPS and Elytron providers alike; the regression test is a shared base in the core test-jar with a subclass per crypto module, matching how the existing crypto tests are organized.
Closes #53927.
AI tooling was used to help prepare this change.