Reject JWS with an unprotected critical b64 header#210
Conversation
The only critical header that go-jose supports is b64 from RFC7797. go-jose correctly only respects that header if it appears in a protected header. go-jose correctly rejects unknown critical headers. However, go-jose does not reject a JWS which contains a critical b64 unprotected header. I don't believe this has any security impact, as the only place this is exposed is via the Signature header members which expose unprotected headers, and so a library user is already aware they are not to be trusted. The 'critical' behavior (of not base64-encoding the value, defined in RFC7797) is not influenced here.
There was a problem hiding this comment.
Pull Request Overview
This PR improves security validation for JSON Web Signature (JWS) by rejecting JWS tokens that contain critical headers in unprotected headers, specifically addressing the case where the critical b64 header appears in an unprotected context.
Key changes:
- Adds validation to reject critical headers in unprotected headers
- Refactors critical header validation logic with a new helper method
- Introduces a new error type for unsupported critical headers
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| shared.go | Adds new error constant and helper method for validating critical headers |
| signing.go | Updates signature verification to check critical headers in both protected and unprotected contexts |
| signing_test.go | Adds test case for rejecting JWS with unprotected critical b64 header |
| crypter.go | Refactors encryption validation to use the new critical header validation method |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Instead of passing an empty set to checkSupportedCritical, add a dedicated method for checking there are no critical headers present.
Co-authored-by: Jacob Hoffman-Andrews <github@hoffman-andrews.com>
Co-authored-by: Jacob Hoffman-Andrews <github@hoffman-andrews.com>
|
Thanks! I'm going to leave this open til tomorrow morning for any extra feedback, at which point I'll merge and cut a new release. |
|
@mcpherrinm Thanks for the quick response and clear assessment. Your rationale for not treating this as a security issue is reasonable, and I appreciate the follow‑up changes to explicitly reject critical headers in unprotected sections , tightening validation and fixing the bug—this improves robustness and spec compliance. |
| // Protected Header." | ||
| err = signature.header.checkNoCritical() | ||
| if err != nil { | ||
| continue outer |
The only critical header that go-jose supports is b64 from RFC7797.
go-jose correctly only respects that header if it appears in a protected header. go-jose correctly rejects unknown critical headers.
However, go-jose does not reject a JWS which contains a critical b64 unprotected header.
I don't believe this has any security impact, as the only place this is exposed is via the Signature header members which expose unprotected headers, and so a library user is already aware they are not to be trusted. The 'critical' behavior (of not base64-encoding the value, defined in RFC7797) is not influenced here.
Return a new ErrUnsupportedCriticalHeader error exported as a constant.
Reported by: Muhammad Noman Ilyas @AL-Cybision