Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SignatureException could include the header and claims #969

Open
mnylensc opened this issue Sep 18, 2024 · 7 comments
Open

SignatureException could include the header and claims #969

mnylensc opened this issue Sep 18, 2024 · 7 comments

Comments

@mnylensc
Copy link

mnylensc commented Sep 18, 2024

Is your feature request related to a problem? Please describe.
If JwtParser#parseSignedClaims call results into SignatureException, it's not possible to parse the JWS header or claims from the exception. This would be useful for collecting metrics per key id and audit logging, when you could log the already parsed header and claims.

Describe the solution you'd like
Similar to ExpiredJwtException#getHeader() and #getClaims(), SignatureException could also have those methods.

I realize the methods are missing probably because this could be quite dangerous if used wrong, but maybe the methods could be named with dangerously prefix to signify the inherent danger in using the return values...?

Describe alternatives you've considered
Decoding the JWS parts myself, but this seems like a waste, when the work to parse the token has already been done by JJWT.

Additional context
JJWT version 0.12.6

@kesrishubham2510
Copy link

Hi @mnylensc , is someone looking into it ?? If not can I take it up ?

@bdemers
Copy link
Member

bdemers commented Oct 18, 2024

Personally, I'd be very cautious about adding this to JJWT, as mentioned above it is potentially dangerous.
All that said, it is a commonly requested bit of functionality, we would either have to mark the method in a very obvious way (as @mnylensc mentioned)

Or maybe, make it possible for a user to do the dangerous bits themselves easier.
I'm guessing @mnylensc's solution does something like: substring, Base64URLDecode, parse json ?

Again, personally I'm reluctant, but I'd still be interested in other thoughts on where a dangerouslyParseTheInvalidJwsBody() method would live (and how users would find it) 😰

@kesrishubham2510
Copy link

kesrishubham2510 commented Oct 19, 2024

Hi @bdemers, consider the below case,

  • Token gets stolen, now the stealer would tamper it, and tries to (as this tempering would lead to Signature exception, and this SignatureException object might be having the information about claims) retrieve the information (like, subject, and other claims) about the original owner of the token.

Is this understanding (regarding the danger associated with this feature) correct ? or I'm thinking in the wrong direction 🤔💭

Hey @mnylen, I want to understand how could a header be useful in logging and secondly the developer team would be knowing the header details and it would probably be same for all the granted/generated tokens. Coming to the payload, what if we could add some logic to log only selective claims instead of all of the claims 🫥.

@mnylensc
Copy link
Author

mnylensc commented Oct 20, 2024

I think the danger here is that someone would catch the SignatureException and use the claims and/or header contained in the exception in a way that leads to the token being accepted.

Consider the following:

public boolean isValidToken(final String token) {
  try {
    jws = parser.parseSignedClaims(token);
    return isValidClaims(jws.getPayload());
  } catch (SignatureException e) {
    return isValidClaims(jws.getPayload());
  }
}

private boolean isValidClaims(Claims claims) {
  return claims.getAudience().equals("https://myserver");
}

Nobody should write code like this, but it's still a mistake that can be made, and someone will eventually end up making it...

However, similar mistake can be already be made with ExpiredJwtException which contains the claims and the header, although arguably the security impact isn't as high due to the signature at least being correct.

Edit: And of course, there are myriad other ways that using claims from an unverified tokens can cause. Some other possible scenarios that popped into my head:

  • One of the claims includes a HTTP endpoint that your claim processing code makes a request to. Obviously, if you invoked this for an unverified claims, that could lead you to invoke an endpoint controlled by an attacker.

  • Denial of service, for example in the way of very deeply nested JSON in the claims that the parser struggles to load.

@mnylensc
Copy link
Author

mnylensc commented Oct 22, 2024

However, these dangers I listed above are imo more about the claims. I don't know why I suggested it originally, because the implementation should not even try parsing the claims if the signature verification failed.

For the header, I don't see similar risks (although, arguably, one can include custom non-standard header fields). The parser anyway needs to parse the header to verify the JWT signature, because the header is needed for locating the key. Having SignatureException#getHeader() method would allow users to audit log and collect metrics for verification failures by key id for example. Beyond obvious attack scenarios, these verification failures can happen due to simple configuration mistakes: using incorrect kid for example, or in case the application is juggling multiple public/private key pairs, using the wrong key.

@kesrishubham2510
Copy link

@mnylensc the use case regarding metrics collection with 'kid' sounds a good measure to track the SignatureExceptions in case we have multiple options for kid. If I'm not mistaken, this 'kid' identifier can be different in same environment because of specific implementation or be different because of different environment specific keys. Please add if I'm missing something here.

Thanks

@nikitocheck
Copy link

Actually, this feature makes sense. In my case jws token is provided by identity & access management proxy server. It is completely safe and there is no need for further verification

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants