jwtk / jjwt

Java JWT: JSON Web Token for Java and Android
Apache License 2.0
10.31k stars 1.34k forks source link

ES signature validation fails (sometimes) #491

Closed rmalchow closed 4 years ago

rmalchow commented 5 years ago

hi, i am using jjwt,

i am using this to sign:

    JwtBuilder b = Jwts.builder();

    [...]   

    if(pk.getAlgorithm().equals("RSA")) {
        b = b.signWith(SignatureAlgorithm.RS256, pk);
    } else if(epk.getAlgorithm().equals("EC")) {
        b = b.signWith(SignatureAlgorithm.ES256, pk);
    } else {
        throw new NoSuchAlgorithmException();
    }
    return b.compact();

and this to read:

        JwtParser p = Jwts.parser();
        p = p.setSigningKeyResolver(resolver);

with the resolver:

           Long s = Long.parseLong(header.get("serial")+"");
               Key k = publicKeyRepo.getBySerial(s);
                   return k;

this works fine ALWAYS with RSA, but with EC, i OCASSIONALLY get this exception:

io.jsonwebtoken.SignatureException: Unable to verify Elliptic Curve signature using configured ECPublicKey. Could not verify signature
    at io.jsonwebtoken.impl.crypto.EllipticCurveSignatureValidator.isValid(EllipticCurveSignatureValidator.java:55)
    at io.jsonwebtoken.impl.crypto.DefaultJwtSignatureValidator.isValid(DefaultJwtSignatureValidator.java:47)
    at io.jsonwebtoken.impl.DefaultJwtParser.parse(DefaultJwtParser.java:351)
    at io.jsonwebtoken.impl.DefaultJwtParser.parse(DefaultJwtParser.java:481)
    at io.jsonwebtoken.impl.DefaultJwtParser.parseClaimsJws(DefaultJwtParser.java:541)
    at  foobar.UserTokenReader.readToken(UserTokenReader.java:43)
Caused by: java.security.SignatureException: Could not verify signature
    at sun.security.ec.ECDSASignature.engineVerify(ECDSASignature.java:393)
    at java.security.Signature$Delegate.engineVerify(Signature.java:1222)
    at java.security.Signature.verify(Signature.java:655)
    at io.jsonwebtoken.impl.crypto.EllipticCurveSignatureValidator.doVerify(EllipticCurveSignatureValidator.java:63)
    at io.jsonwebtoken.impl.crypto.EllipticCurveSignatureValidator.isValid(EllipticCurveSignatureValidator.java:52)
    ... 56 common frames omitted
Caused by: java.security.SignatureException: Invalid encoding for signature
    at sun.security.ec.ECDSASignature.decodeSignature(ECDSASignature.java:468)
    at sun.security.ec.ECDSASignature.engineVerify(ECDSASignature.java:390)
    ... 60 common frames omitted
Caused by: java.io.IOException: DerInputStream.getLength(): lengthTag=35, too big.
    at sun.security.util.DerInputStream.getLength(DerInputStream.java:599)
    at sun.security.util.DerInputStream.readVector(DerInputStream.java:382)
    at sun.security.util.DerInputStream.getSequence(DerInputStream.java:331)
    at sun.security.ec.ECDSASignature.decodeSignature(ECDSASignature.java:444)
    ... 61 common frames omitted

this happens rarely, but it does happen. am i holding it wrong? or is there some error in the encoding of the signature?

rmalchow commented 5 years ago

i just ran 10000 cycles of generate keypair, encode, decode .... 37 failed. that's about 0.4% .... there''s actually TWO errors:

one is:

Caused by: java.io.IOException: DerInputStream.getLength(): lengthTag=16, too big.
    at sun.security.util.DerInputStream.getLength(DerInputStream.java:599)
    at sun.security.util.DerInputStream.readVector(DerInputStream.java:382)

and the other one:

Caused by: java.io.IOException: insufficient data
    at sun.security.util.DerInputBuffer.truncate(DerInputBuffer.java:135)
    at sun.security.util.DerInputStream.subStream(DerInputStream.java:160)
lhazlewood commented 5 years ago

Can you share your test code with us? A test case?

Also, are you generating KeyPairs appropriately?

Keys.keyPairFor(SignatureAlgorithm) ?

Try the test case without the SigningKeyResolver using the appropriately generated keys. Does it always work? If so, the problem is probably with the key returned by publicKeyRepo.getBySerial(s) (perhaps the key was generated incorrectly).

lhazlewood commented 5 years ago

Also which JVM/JDK implementation are you using?

rmalchow commented 5 years ago

hi

i generate keypairs like this:

KeyPairGenerator kpg = KeyPairGenerator.getInstance("EC");
kpg.initialize(new ECGenParameterSpec("secp384r1"));
return kpg.generateKeyPair();

error happens with both (these are the only ones installed on this machine, i've seen this error on other machines on other JDKs):

bash$ java -version java version "1.8.0_172" Java(TM) SE Runtime Environment (build 1.8.0_172-b11) Java HotSpot(TM) 64-Bit Server VM (build 25.172-b11, mixed mode)

bash$ java -version java version "10.0.1" 2018-04-17 Java(TM) SE Runtime Environment 18.3 (build 10.0.1+10) Java HotSpot(TM) 64-Bit Server VM 18.3 (build 10.0.1+10, mixed mode)

i just stripped our code to minimize dependencies, still getting that exception:

https://github.com/rmalchow/test-jjwt

rmalchow commented 5 years ago

replacing:

KeyPairGenerator kpg = KeyPairGenerator.getInstance("EC");
kpg.initialize(new ECGenParameterSpec("secp384r1"));
KeyPair kp = kpg.generateKeyPair();    

with

KeyPair kp = Keys.keyPairFor(SignatureAlgorithm.ES256);

makes this go away. this is not the case with RSA keys ... what is the difference here? why would a "regular" EC keypair not work for this? i've signed and checked signature on loads of things ... (admittedly, i've never fully read the JWT specs though)

lhazlewood commented 5 years ago

The EC curves/signature algorithms are defined in JWT's JWA specification:

https://tools.ietf.org/html/rfc7518#section-3.4

The only differences I can tell between your implementation and JJWT's is JJWT generates a keypair by initializing the keypair generator with seeded SecureRandom, which is always recommended for new secure keys, e.g.

KeyPairGenerator kpg = KeyPairGenerator.getInstance("EC");
ECGenParameterSpec spec = new ECGenParameterSpec("secp384r1");
kpg.initialize(spec, secureRandom); // <--- JJWT's difference
return kpg.generateKeyPair();

The SecureRandom instance itself is seeded as follows:

https://github.com/jwtk/jjwt/blob/0.10.7/impl/src/main/java/io/jsonwebtoken/impl/crypto/SignatureProvider.java#L30-L48

Also, your first example key generation code generates a key using a 384-bit named curve identifier, but you were specifying SignatureAlgorithm.ES256. This signature algorithm creates 256-bit SHA256withECDSA signatures as required by the JWA spec per https://tools.ietf.org/html/rfc7518#appendix-A.1.

It is unclear to me whether the lack of random seeding the keypair generator or the difference in curve name (384) w/ the signature length (256) is causing the problem. Please let me know if you find out!

lhazlewood commented 5 years ago

P.S. Thank You for the sample/test project. That makes a world of difference for us to help troubleshoot!

lhazlewood commented 5 years ago

Yep, with your test project, using secp256r1 does not result in any problems.

lhazlewood commented 5 years ago

Similarly, keeping secp384r and changing the sample project's TokenWriter.java line 26 to use SignatureAlgorithm.ES384 also works without any errors.

rmalchow commented 5 years ago

hi,

first off - thanks for providing this library. i am using it with a bit of spring sugar sprinkled on top in this one here:

 https://github.com/mcon-group/spring-jwt

concerning this issue ... first, i was under the impression that all the default getInstance() methods do internally instantiate a new SecureRandom anyways? anyway, I'll add that explicitely.

the error is most definitely in chosing the wrong signature algorithm, as you pointed out, both variants with the same number of bits work just fine. but assertValid in SignatureAlgorithm checks all sorts of things, but for this, it only checks for >= not for exact size, and this check actually kicks in when i switch my mistake around and do a 256 bit key with ES384 ... so basically the check is not correct ... or the check is fine, and something goes wrong when the signature is created?

if it's the latter ... just a wild guess ... but it seems that EllipticCureProvider.getSignatureByteArrayLength() might be a possible candidate. i'll clone your code tomorrow and dig in a bit more.

again, thank you very much for your patience and help

.rm

rmalchow commented 5 years ago

hi,

some progress: the signature produced in EllipticCurveSigner.doSign is totally valid & verifiable, even with mismatched key / signing algorithm. and even if the wrapping / unwrapping gives an error. my suspicion is now that somehow, EllipticCurveProvider.transcodeSignatureToConcat is miscalculating the size for some reason. Unfortunately, I dont really get what's happening there ...

in my code, i now do this:

        int bl = ecpk.getParams().getOrder().bitLength();
        if(bl==384) {
            b = b.signWith(epk.getPrivateKey(),SignatureAlgorithm.ES384);
        } else if (bl==256) {
            b = b.signWith(epk.getPrivateKey(),SignatureAlgorithm.ES256);
        } else if (bl==512) {
            b = b.signWith(epk.getPrivateKey(),SignatureAlgorithm.ES512);
        }

to match sig algorithm to key. i still have the feeling that this probably shouldn't be necessary. one other thing that confused me here is that in RSA256, the "256" refers to the hashing algorithm and has nothing to do with the key size, whereas in EC256, it means both key size AND hashing algorithm. I don't think this link is necessarily correct. If it is a hard connection, then that would mean the check in SignatureAlgorithm.assertValid is too lenient - it should check for == instead >=

.rm

lhazlewood commented 5 years ago

Hi @rmalchow

Usually cryptographic keys of sizes larger than a hash algorithm's digest length are valid (e.g. it's ok to use a 512 bit secret key to create a hmac sha 256 digest) - the algorithms usually truncate or hash the key to the algorithm's necessary length.

It is unclear to me if this is allowed with ECDSA signatures - I thought it was. If not allowed, then yes SignatureAlgorithm.assertValid must assert length equality (not just >=) for EC keys.

If allowed, then there could indeed be a problem with the transcodeSignatureToConcat implementation. That implementation was (very kindly) contributed to us by @martintreurnicht in PR #137, so I'm curious if he has any feedback or comments if he's aware of anything that might be amiss.

Hopefully we can get to the bottom of this soon!

lhazlewood commented 5 years ago

@rmalchow just a note: You don't need the if/then/else bit-length logic in your code. You can just call b.signWith(epk.getPrivateKey()) and JJWT will default to the strongest algorithm supportable with the specified key: https://github.com/jwtk/jjwt#signing-key

stale[bot] commented 4 years ago

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[bot] commented 4 years ago

Closed due to inactivity.