Repository navigation
Decode the JWS signature segment strictly in JWSInput - #54278
Open
naruto-lgtm wants to merge 1 commit into
Open
naruto-lgtm wants to merge 1 commit into
naruto-lgtm wants to merge 1 commit into
Conversation
Base64Url.decode cuts the input at the first '=', maps '+' and '/' and ignores non-zero unused bits, while the signing input is the raw header.payload string. A signed JWS could therefore be rewritten into many distinct strings that all verify. Only accept the unpadded base64url form of the signature (RFC 7515 section 2). The wrong-kb-signature SD-JWT fixture had a non-canonical last character; clear its unused bits so the test still exercises signature verification. Signed-off-by: Nayyar <nayyar@bugqore.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The narrowly scoped validation has focused regression coverage and no identified blocking issues; tests were not executed.
No open issues 🎉
What changed in this PR
This PR hardens JWS parsing by requiring canonical, unpadded base64url signatures, preventing alternate encodings of the same signature from being accepted. Header and payload decoding remain unchanged.
Changes:
- Validate signatures with a URL-safe decoder and an exact encoding round-trip.
- Add regression tests for canonical, unsigned, and malformed signature inputs.
- Correct the invalid-signature SD-JWT fixture so it still reaches signature verification.
| File | Description |
|---|---|
core/src/test/resources/sdjwt/s20.1-sdjwt+kb--wrong-kb-signature.txt |
Canonicalizes the deliberately incorrect signature. |
core/src/test/java/org/keycloak/jose/jws/JWSInputTest.java |
Adds seven signature parsing tests. |
core/src/main/java/org/keycloak/jose/jws/JWSInput.java |
Enforces canonical signature encoding during parsing. |
Review effort: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
JWSInput decodes the signature segment with the lenient Base64Url.decode, which truncates at the first '=', accepts '+' and '/', and ignores non-zero unused bits, while verification runs over the raw header.payload string, so any signed JWS can be rewritten into an unbounded number of distinct strings that still verify (raised by @oleksii-udovychenko on #53928, which kept it out of scope). The signature segment now has to round-trip through the unpadded base64url encoder per RFC 7515 section 2, enforced in the parser so it covers every algorithm and caller; header and payload stay lenient since they enter the signing input as raw strings, and JWSBuilder already emits the canonical form, so Keycloak-issued tokens are unaffected, though a third-party issuer that pads its signatures would now be rejected. JWSInputTest fails 5 of 7 cases before the change, and one SD-JWT fixture with a hand-edited, non-canonical signature was corrected so it still exercises signature verification.