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

Parser doesn't throw SignatureException if no signature is included #90

Closed
echox opened this issue Feb 4, 2016 · 6 comments
Closed
Labels
stale Stale issues pending deletion due to inactivity

Comments

@echox
Copy link

echox commented Feb 4, 2016

Hi,
I think if a SigningKey is set and the token doesn't contain any signature, a SignatureException should be thrown.

I think this would also address the vulnerabilities mentioned here: https://auth0.com/blog/2015/03/31/critical-vulnerabilities-in-json-web-token-libraries/

Example:

KeyPair pair = RsaProvider.generateKeyPair();
String token = Jwts.builder().setSubject("Joe").signWith(SignatureAlgorithm.RS512, pair.getPrivate()).compact();

try {
    Jwt jwt = Jwts.parser().setSigningKey(pair.getPublic()).parse(token);
} catch (SignatureException e) {
    // this will not happen
    System.out.println("Signature Failed");
}
@lhazlewood
Copy link
Contributor

Hi,

JJWT was mentioned in the article you referenced:

https://auth0.com/blog/2015/03/31/critical-vulnerabilities-in-json-web-token-libraries/#comment-1943426659

It is not vulnerable if you use the API correctly. In this case, you're using the Parser API incorrectly. You should only call parse when you don't know the type of JWT - either an unsigned JWT, a JWS, JWE, etc, and even then you may not be able to trust the returned JWT because parse does only a 'best effort' in determining the type of JWT.

If you know for sure what type of token you require (e.g. JWS), you need to call parsePlaintextJws or parseClaimsJws so that it does the appropriate validation. This is reflected in the various parse* methods' JavaDoc and on the JJWT readme.

That said, if you have recommendations on how to improve the user experience such that this would not have confused you, please do let us know - if you had problems with this, others may as well, and we'd like to make it even easier to use if possible!

Thanks!

@echox
Copy link
Author

echox commented Feb 10, 2016

Thanks for the clarification. You are right.

Why/How I made this mistake:

I just was looking for a method to validate a given signed token string. At this point I'm not interested in checking for any claims.

  • The first examples use parseClaimsJws, which returns Jwt<Header, Claims>, which I skipped, since I just need the token validation.
  • The next examples from the release notes cover "Enforce JWT Claims when Parsing", ok, skipping.
  • I wasn't interested in the specific key, so I also skipped SigningKeyResolver.
  • The end of the page contains the relevant part "Known Type convenience parse methods".
  • At this point I was already checking the Methods of the JwtParser and spotted parse(String) which also throws a SignatureException and looked fine.

Some minor things:

I like the library and this could have been avoided by checking the documentation more carefully. Maybe its a little bit unfortunate that the documentation of the API are the "Release Notes" which are in a chronological order. After the "Usage" Part I would have expected something like "Hey, and these are the Methods you are interested in: [Known Type convenience parse methods]".
Its a little hidden between information like "Minor Bugfixes", "Android Support", ... and vice versa.

The project has a nice JavaDoc documentation, it would be great to have a link to a online version on top of the projects page. I usually don't browse through it in my IDE and often I just want to have a look without setting up a project and adding the library.

Beside that, thanks for providing the library. :-)

@echox
Copy link
Author

echox commented Feb 10, 2016

I'm not sure if Closing the issue prevents commenting, so feel free to close :-)

@lhazlewood
Copy link
Contributor

@echox thanks for the feedback! We'll definitely try to make the docs better. Your bullet list of thought progression was really helpful and can help us figure out how to make the API simpler too.

You can comment on closed issues, but I'll leave this one open to represent any work to simplify the documentation and/or any API methods. Thanks!

@stale stale bot added the stale Stale issues pending deletion due to inactivity label Jul 13, 2019
@jwtk jwtk deleted a comment from stale bot Jul 15, 2019
@stale stale bot removed the stale Stale issues pending deletion due to inactivity label Jul 15, 2019
@stale stale bot added the stale Stale issues pending deletion due to inactivity label Sep 13, 2019
@lhazlewood lhazlewood removed the stale Stale issues pending deletion due to inactivity label Sep 13, 2019
@jwtk jwtk deleted a comment from stale bot Sep 13, 2019
@stale
Copy link

stale bot commented Nov 12, 2019

This issue has been automatically marked as stale due to inactivity for 60 or more days. It will be closed in 7 days if no further activity occurs.

@stale stale bot added the stale Stale issues pending deletion due to inactivity label Nov 12, 2019
@stale
Copy link

stale bot commented Nov 19, 2019

Closed due to inactivity.

@stale stale bot closed this as completed Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Stale issues pending deletion due to inactivity
Projects
None yet
Development

No branches or pull requests

2 participants