jwtk/jjwt

Option to enable strict duplicate detection

laurids opened this issue · 4 comments

laurids commented

Is your feature request related to a problem? Please describe.
For parsing, we are using the JacksonDeserializer via the constructor public JacksonDeserializer(Map<String, Class<?>> claimTypeMap).
When parsing a JWT with duplicate properties, the last one will be used, and the rest are ignored. Although this behavior is allowed by the RFCs, we would prefer for the parser to throw an exception if there are duplicate keys.
In general, it can create vulnerabilities if e.g. different parsers are used in a stack, where some use the first item, and some use the last item, in cases of duplicates. This is of course not allowed in JWT context, but we would also like to fail fast, if the next parser fails.
This can be done by enabling JsonParser.Feature.STRICT_DUPLICATE_DETECTION on the underlying ObjectMapper.
The problem is that there is no way to access the objectMapper in JacksonDeserializer.

Describe the solution you'd like
An option to enable JsonParser.Feature.STRICT_DUPLICATE_DETECTION on the objectMapper in JacksonDeserializer. Whether it be by adding a getter for the objectMapper or an extra constructor that also takes JsonParser.Features?

Describe alternatives you've considered
We could write our own implementation of AbstractSerializer, but it would be a copy of the JacksonDeserializer class except for enabling the strict duplicate detection.

Additional context
The RFCs allows both behaviors for parsing duplicates in both header and payload:
https://www.rfc-editor.org/rfc/rfc7515.html#section-4
https://www.rfc-editor.org/rfc/rfc7519#section-4
Strict duplicate detection:
https://fasterxml.github.io/jackson-core/javadoc/2.9/com/fasterxml/jackson/core/JsonParser.Feature.html#STRICT_DUPLICATE_DETECTION

lhazlewood commented

FWIW, this is also in the the documentation: https://github.com/jwtk/jjwt#jackson-json-processor

But that section mostly talks about your own application-wide ObjectMapper instead of being clearer that it can also be used for other things (like setting JsonParser.Features), nor do we discuss duplicate key behavior in the main documentation, so I understand how it may not be readily obvious to many readers.

It is unclear to me if we should change the default behavior of implicitly-created ObjectMapper instances since the RFCs explicitly allow this behavior. Assuming signature verification or AEAD decryption are successful before accessing the payload, what vulnerabilities are you referring to?

laurids commented

Hi @bdemers and @lhazlewood, thanks for your quick responses.
Yes, I did notice the constructor taking an ObjectMapper.
However, I wanted to use the MappedTypeDeserializer as well, since it was conveniently implemented there:)
Maybe I was not clear enough about that part.
So my request was for an option to configure the ObjectMapper, specifically when using the JacksonDeserializer(Map<String, Class<?>> claimTypeMap) constructor.

Assuming signature verification or AEAD decryption are successful before accessing the payload, what vulnerabilities are you referring to?

In general, I would think duplicate properties in a JWT is a sign of a buggy or compromised authorization server. I don't think allowing both behaviors in the RFCs was a good idea, since a JWT going through different parsers/verifiers could be rejected at any of them simply depending on the choice of implementation.
If multiple parsers are used in a stack including JJWT, and some parsers are wrongly implemented to use the first value instead of the last, that could lead to vulnerabilities. However, if JJWT rejects it, this type of vulnerability is not possible.
Examples of possible vulnerabilities:
https://bishopfox.com/blog/json-interoperability-vulnerabilities
(Referenced from an interesting presentation:) https://i.blackhat.com/BH-US-23/Presentations/US-23-Tervoort-Three-New-Attacks-Against-JSON-Web-Tokens.pdf

By the way, with strict duplicate detection enabled, duplicates in the header results in e.g. Malformed protected header JSON: Unable to deserialize: Duplicate field 'alg' but duplicates in the payload results in a less specific error message Unexpected content JWS. This is because when the JSON serialization of the payload fails, it tries to handle it as a byte array:
https://github.com/jwtk/jjwt/blob/0.12.3/impl/src/main/java/io/jsonwebtoken/impl/DefaultJwtParser.java#L617
And then it ends up in https://github.com/jwtk/jjwt/blob/0.12.3/api/src/main/java/io/jsonwebtoken/SupportedJwtVisitor.java#L129
I understand if there isn't a better way to handle this though.

lhazlewood commented

Thanks for the additional context @laurids , that's helpful.

Maybe I was not clear enough about that part. So my request was for an option to configure the ObjectMapper, specifically when using the JacksonDeserializer(Map<String, Class<?>> claimTypeMap) constructor.

However, I wanted to use the MappedTypeDeserializer as well, since it was conveniently implemented there:)

You were clear 😄 . We were just basically implying that, if the default implementation doesn't suit your needs, you should be using the ObjectMapper constructor and implement the same or similar MappedTypeDeserializer logic in your own project.

But I can see how that could be a 'heavier' solution than what you desire. Perhaps a general solution would be to make MappedTypeDeserializer a public class in the jackson module so that devs can use it explicitly, because this could be useful regardless of the duplicate key issue.

A more targeted solution for your particular use case would be to just default to enabling STRICT_DUPLICATE_DETECTION. I agree this would likely be a better default, and, since the RFC allows it, application developers can override/unset that feature if they desire. So I think we can use this issue to track that change in the codebase.

Implementation note: when changing the default, it doesn't make sense to also enable generic configuration options on the JacksonDeserializer class (e.g. jacksonDeserializer.enable(JsonParser.Feature);). JacksonDeserializer is intended to be a lightweight wrapper around an ObjectMapper, so any such ObjectMapper configuration should be done using native Jackson APIs first before instantiating JacksonDeserializer.

  • 👍1