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

Critical vulnerabilites in JWT #20

Closed
KPRATIK opened this issue Apr 2, 2015 · 17 comments
Closed

Critical vulnerabilites in JWT #20

KPRATIK opened this issue Apr 2, 2015 · 17 comments

Comments

@KPRATIK
Copy link

KPRATIK commented Apr 2, 2015

Wanted to know if your libray takes care of security vulnerabilites https://auth0.com/blog/2015/03/31/critical-vulnerabilities-in-json-web-token-libraries/

Please update

@lhazlewood
Copy link
Contributor

JJWT is not vulnerable to the listed attacks as long as you use it correctly - I had security in mind from the beginning when designing and implementing JJWT :)

Here are the two discussed attack vectors and verification that you are not susceptible to attack:

  1. Discussed attack vector 1: client sends the server an unsigned JWT with a missing alg header field or the alg header field is none.

If your server is expecting a signed JWT with a claims body (known as a 'Claims JWS'), you should be calling the parseClaimsJws method in JJWT to indicate this fact:

Jwts.parser().setSigningKey(privateKey).parseClaimsJws(jwtString);

An unsigned token will cause this method to throw an UnsupportedJwtException and so you are not susceptible to this attack vector.

Now, if you called Jwts.parser().setSigningKey(privateKey).parse(jwtString) instead, then it would not throw an exception, but this is expected: the generic parse(String) method supports any of the 4 types of JWTs. If you do not want to allow any of the 4 types, and you know exactly the type of JWT you should receive (as you should in the server), then you should use the parse(String, JwtHandler) method or one of the four parseFooJwx variants.
2. Discussed attack vector 2: client creates a signed JWT using an RSA public key as an HMAC signing key.

When using RSA public/private keys, your server should always use the private key during token creation and verification (clients would use the public key). As long as you do this, you are not susceptible to this attack vector. Here is a (Groovy) test case that verifies it:

@Test
void testForgedTokenWhenUsingRsaPublicKeyAsHmacSigningKey() {

    //Create a legitimate RSA public and private key pair:
    KeyPairGenerator keyGenerator = KeyPairGenerator.getInstance("RSA");
    keyGenerator.initialize(1024);
    KeyPair kp = keyGenerator.genKeyPair();
    PublicKey publicKey = kp.getPublic();
    PrivateKey privateKey = kp.getPrivate();

    // Now for the forgery: simulate a client using the RSA public key to sign a token, but
    // using it as an HMAC signing key instead of RSA:
    String forged = Jwts.builder().setPayload('foo').signWith(SignatureAlgorithm.HS256, publicKey).compact();

    // Assert that the server (that should always use the private key) does not recognized the forged token:
    try {
        Jwts.parser().setSigningKey(privateKey).parse(forged);
        fail("Forged token must not be successfully parsed.")
    } catch (SignatureException expected) {
        assertEquals expected.getMessage(), 'JWT signature does not match locally computed signature. JWT validity cannot be asserted and should not be trusted.'
    }
}

So, the takeaway: use the specific parse* method that corresponds to your known use case and use the server private key during token creation and verification, and you're safe.

lhazlewood added a commit that referenced this issue Apr 4, 2015
…or in JJWT is not susceptible to the discussed attack vectors.
@lhazlewood
Copy link
Contributor

P.S. For good measure, I added 3 test cases in commit 060666a that demonstrate JJWT is/was not vulnerable.

@KPRATIK
Copy link
Author

KPRATIK commented Apr 7, 2015

Hey, was going through your new test case and had a doubt in testForgedTokenWhenUsingRsaPublicKeyAsHmacSigningKey()

Was thinking of use case which is reverse of yours. So server signs with private key and client verifies using public key. Do you think it will still work?

Thanks,
Pratik

@lhazlewood
Copy link
Contributor

Do you think it will still work?

I'm confused by your question. Public/private key pairs are designed specifically to allow anyone to verify a signature using the public key. The important point is that only the holder of the private key should be able to create the signature. This is the expected behavior of any public/private signature key mechanism and JJWT works this way for RSA as expected.

In other words, this is correct and expected:

  1. The server creates a JWT and signs it with the private key that no one else can access
  2. The server sends the signed JWT (aka a 'JWS') to anyone. The public key can be exposed as well since anyone is 'allowed' to see the public key.
  3. Anyone (e.g. a client) can verify the JWT signature with the public key.

RSA public keys should never be used by anyone to sign anything. JJWT will throw an exception if you attempt to use an RSA public key to sign a JWT using an RSA algorithm, preventing you from accidentally making that mistake.

@KPRATIK
Copy link
Author

KPRATIK commented Apr 8, 2015

I was talking about use case where some one uses public key for signing. So if this line in the test changes :
Jwts.parser().setSigningKey(privateKey).parse(forged);
to
Jwts.parser().setSigningKey(publicKey).parse(forged);

Should not it throw exception as it is a possible attack?

@christopherlakey
Copy link

If the public key approach can be trusted for verification, then this would limit the need to distribute the private key to each micro service that needs to validate tokens but not create them.

Is there a simple way to use the publication key for verification and avoid vector 2 above?

@lhazlewood
Copy link
Contributor

@christopherlakey and @KPRATIK good idea - this will be a simple addition - I'll create a ticket as a feature enhancement

@lhazlewood
Copy link
Contributor

See #25

@christopherlakey
Copy link

Thanks!

@KPRATIK
Copy link
Author

KPRATIK commented May 5, 2015

When are you planning to release 0.5 version?

@lhazlewood
Copy link
Contributor

As soon as @rhyscoronado or @abrodersen (or any other developer that can test on Android) can confirm the fix for #18 works as expected. We're waiting on them.

If I don't hear an answer back within a day or two, I'll cut the release and people can issue pull requests if it doesn't work.

@KPRATIK
Copy link
Author

KPRATIK commented May 8, 2015

Any updated on this?

@lhazlewood
Copy link
Contributor

This issue is closed. If you're referring to the release, I hope to get it out today.

@lhazlewood
Copy link
Contributor

@KPRATIK btw, if you need it earlier, you can build from master - that has the fix in it and the branch is stable.

@KPRATIK
Copy link
Author

KPRATIK commented May 11, 2015

Is the release out now?

@lhazlewood
Copy link
Contributor

No, I needed to do some code cleanup to get higher test code coverage and include elliptic curve signature support. You will know when a release has been completed by watching the releases page.

@lhazlewood
Copy link
Contributor

@KPRATIK 0.5 has just been released. Please wait for an hour or so from now and it should be accessible in Maven Central.

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

3 participants