search

WrenSecurity / wrensec-commons

Community fork of ForgeRock Commons, which contains common utility code used by multiple products originally developed by ForgeRock.
http://wrensecurity.org
0 stars 10 forks source link

json-web-token (20.0.0) fails tests #1

Closed Kortanul closed 6 years ago

Kortanul commented 6 years ago

on both JDK 1.8.0_141-b15, and 1.8.0_131-8u131-b11-0ubuntu1.16.04.2-b11, trying to build json-web-token in 20.0.0 of wrensec-commons results in the following test failures:

-------------------------------------------------------
 T E S T S
-------------------------------------------------------
Running TestSuite
Configuring TestNG with: TestNG652Configurator
Tests run: 169, Failures: 6, Errors: 0, Skipped: 7, Time elapsed: 13.75 sec <<< FAILURE! - in TestSuite
shouldDetectTampering(org.forgerock.json.jose.jws.handlers.ECDSASigningHandlerTest)  Time elapsed: 0.053 sec  <<< FAILURE!
org.forgerock.json.jose.exceptions.JwsSigningException: java.security.SignatureException: Could not verify signature
        at sun.security.ec.ECDSASignature.decodeSignature(ECDSASignature.java:377)
        at sun.security.ec.ECDSASignature.engineVerify(ECDSASignature.java:318)
        at java.security.Signature$Delegate.engineVerify(Signature.java:1219)
        at java.security.Signature.verify(Signature.java:652)
        at org.forgerock.json.jose.jws.handlers.ECDSASigningHandler.verify(ECDSASigningHandler.java:97)
        at org.forgerock.json.jose.jws.handlers.ECDSASigningHandlerTest.shouldDetectTampering(ECDSASigningHandlerTest.java:87)

shouldDetectTampering(org.forgerock.json.jose.jws.handlers.ECDSASigningHandlerTest)  Time elapsed: 0.017 sec  <<< FAILURE!
org.forgerock.json.jose.exceptions.JwsSigningException: java.security.SignatureException: Could not verify signature
        at sun.security.ec.ECDSASignature.decodeSignature(ECDSASignature.java:377)
        at sun.security.ec.ECDSASignature.engineVerify(ECDSASignature.java:318)
        at java.security.Signature$Delegate.engineVerify(Signature.java:1219)
        at java.security.Signature.verify(Signature.java:652)
        at org.forgerock.json.jose.jws.handlers.ECDSASigningHandler.verify(ECDSASigningHandler.java:97)
        at org.forgerock.json.jose.jws.handlers.ECDSASigningHandlerTest.shouldDetectTampering(ECDSASigningHandlerTest.java:87)

shouldDetectTampering(org.forgerock.json.jose.jws.handlers.ECDSASigningHandlerTest)  Time elapsed: 0.024 sec  <<< FAILURE!
org.forgerock.json.jose.exceptions.JwsSigningException: java.security.SignatureException: Could not verify signature
        at sun.security.util.DerInputStream.getLength(DerInputStream.java:606)
        at sun.security.util.DerInputStream.readVector(DerInputStream.java:383)
        at sun.security.util.DerInputStream.getSequence(DerInputStream.java:332)
        at sun.security.ec.ECDSASignature.decodeSignature(ECDSASignature.java:372)
        at sun.security.ec.ECDSASignature.engineVerify(ECDSASignature.java:318)
        at java.security.Signature$Delegate.engineVerify(Signature.java:1219)
        at java.security.Signature.verify(Signature.java:652)
        at org.forgerock.json.jose.jws.handlers.ECDSASigningHandler.verify(ECDSASigningHandler.java:97)
        at org.forgerock.json.jose.jws.handlers.ECDSASigningHandlerTest.shouldDetectTampering(ECDSASigningHandlerTest.java:87)

shouldVerifyCorrectly(org.forgerock.json.jose.jws.handlers.ECDSASigningHandlerTest)  Time elapsed: 0.008 sec  <<< FAILURE!
org.forgerock.json.jose.exceptions.JwsSigningException: java.security.SignatureException: Could not verify signature
        at sun.security.ec.ECDSASignature.decodeSignature(ECDSASignature.java:377)
        at sun.security.ec.ECDSASignature.engineVerify(ECDSASignature.java:318)
        at java.security.Signature$Delegate.engineVerify(Signature.java:1219)
        at java.security.Signature.verify(Signature.java:652)
        at org.forgerock.json.jose.jws.handlers.ECDSASigningHandler.verify(ECDSASigningHandler.java:97)
        at org.forgerock.json.jose.jws.handlers.ECDSASigningHandlerTest.shouldVerifyCorrectly(ECDSASigningHandlerTest.java:70)

shouldVerifyCorrectly(org.forgerock.json.jose.jws.handlers.ECDSASigningHandlerTest)  Time elapsed: 0.015 sec  <<< FAILURE!
org.forgerock.json.jose.exceptions.JwsSigningException: java.security.SignatureException: Could not verify signature
        at sun.security.ec.ECDSASignature.decodeSignature(ECDSASignature.java:377)
        at sun.security.ec.ECDSASignature.engineVerify(ECDSASignature.java:318)
        at java.security.Signature$Delegate.engineVerify(Signature.java:1219)
        at java.security.Signature.verify(Signature.java:652)
        at org.forgerock.json.jose.jws.handlers.ECDSASigningHandler.verify(ECDSASigningHandler.java:97)
        at org.forgerock.json.jose.jws.handlers.ECDSASigningHandlerTest.shouldVerifyCorrectly(ECDSASigningHandlerTest.java:70)

shouldVerifyCorrectly(org.forgerock.json.jose.jws.handlers.ECDSASigningHandlerTest)  Time elapsed: 0.022 sec  <<< FAILURE!
org.forgerock.json.jose.exceptions.JwsSigningException: java.security.SignatureException: Could not verify signature
        at sun.security.util.DerInputStream.getLength(DerInputStream.java:606)
        at sun.security.util.DerInputStream.readVector(DerInputStream.java:383)
        at sun.security.util.DerInputStream.getSequence(DerInputStream.java:332)
        at sun.security.ec.ECDSASignature.decodeSignature(ECDSASignature.java:372)
        at sun.security.ec.ECDSASignature.engineVerify(ECDSASignature.java:318)
        at java.security.Signature$Delegate.engineVerify(Signature.java:1219)
        at java.security.Signature.verify(Signature.java:652)
        at org.forgerock.json.jose.jws.handlers.ECDSASigningHandler.verify(ECDSASigningHandler.java:97)
        at org.forgerock.json.jose.jws.handlers.ECDSASigningHandlerTest.shouldVerifyCorrectly(ECDSASigningHandlerTest.java:70)

Results :

Failed tests:
org.forgerock.json.jose.jws.handlers.ECDSASigningHandlerTest.shouldDetectTampering(org.forgerock.json.jose.jws.handlers.ECDSASigningHandlerTest)
  Run 1: ECDSASigningHandlerTest.shouldDetectTampering:87 ▒ JwsSigning java.security.Si...
  Run 2: ECDSASigningHandlerTest.shouldDetectTampering:87 ▒ JwsSigning java.security.Si...
  Run 3: ECDSASigningHandlerTest.shouldDetectTampering:87 ▒ JwsSigning java.security.Si...

org.forgerock.json.jose.jws.handlers.ECDSASigningHandlerTest.shouldVerifyCorrectly(org.forgerock.json.jose.jws.handlers.ECDSASigningHandlerTest)
  Run 1: ECDSASigningHandlerTest.shouldVerifyCorrectly:70 ▒ JwsSigning java.security.Si...
  Run 2: ECDSASigningHandlerTest.shouldVerifyCorrectly:70 ▒ JwsSigning java.security.Si...
  Run 3: ECDSASigningHandlerTest.shouldVerifyCorrectly:70 ▒ JwsSigning java.security.Si...

Tests run: 162, Failures: 2, Errors: 0, Skipped: 4

[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:09 min
[INFO] Finished at: 2017-09-17T19:59:34-04:00
[INFO] Final Memory: 22M/130M
[INFO] ------------------------------------------------------------------------

as far as I can tell, we haven't done anything that should cause this.

Kortanul commented 6 years ago

looks like this may be a regression caused by a change in Java 8 update 121: https://bugs.openjdk.java.net/browse/JDK-8174719

siepkes commented 6 years ago

@Kortanul I don't think this caused by JDK-8174719.

As far as I can tell wrensec-commons does all its own DER encoding (see DerUtils class). Whether this is a good idea is another discussion (we might think about off-loading all this kind of low-level stuff to a good library like BouncyCastle).

Also JDK-8174719 seems to be caused by information getting lost during Base64 encoding of the signature. However in our case the signature always stays "binary" (it's never encoded / decoded).

pavelhoral commented 6 years ago

Summary of findings discussed on Gitter:

The culprit here is that the implemented DER encoding is not correctly producing shortest form (i.e. do not have leading or trailing zeros). This was not an issue until JDK added strict signature validation.

There are 3 issues in the code:

  1. ECDSASigningHandler#derEncode is returning full array buffer with trailing zeros by calling ByteBuffer#array.
  2. DerUtils#writeLength is using two's-complement logic from BigInteger which introduces leading zeros for values bigger than 127.
  3. DerUtils#writeInteger is not removing leading zeros in the byte array.

Those issues violate several BER/DER encoding rules.

Kortanul commented 6 years ago

Closed by #2.

Kortanul commented 6 years ago

thanks to @pavelhoral and @siepkes for getting this fixed so quickly!