Repository: jwtk/jjwt (java)

Issue:
DefaultJwtParser ignores custom base64 parser it was built with in a scenario
In cases where a JwtParser is built using b64Url() to provide a custom base64 decoder, the expectation is that all base64 decoding will use the custom decoder:

```
Jwts.parser()
    .verifyWith(publicKey)
    .b64Url(decoder) // expect this decoder to be used for all base64 operations of parseSignedClaims()
    .build()
    .parseSignedClaims(jwt);
```

However due to https://github.com/jwtk/jjwt/blob/754324879d55a374856e9e5d48e028b906fa293a/impl/src/main/java/io/jsonwebtoken/impl/DefaultJwtParser.java#L644 a new DefaultJws is constructed. The constructor for DefaultJws is hard-coded to use its own base64 decoder (namely Decoders.BASE64URL):

https://github.com/jwtk/jjwt/blob/754324879d55a374856e9e5d48e028b906fa293a/impl/src/main/java/io/jsonwebtoken/impl/DefaultJws.java#L30

This violates the expectation that the custom base64 decoder will be used for all decoding. This behavior has changed somewhere between 0.11.5 and 0.12.5 which in turn breaks our application if we update our jjwt dependency version to the latest.

Pull request description:
Fixes #947

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 70c53ed..deca6a4 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,13 @@
 ## Release Notes
 
+### 0.12.6
+
+This patch release:
+
+* Ensures that after successful JWS signature verification, an application-configured Base64Url `Decoder` output is
+  used to construct a `Jws` instance (instead of JJWT's default decoder). See
+  [Issue 947](https://github.com/jwtk/jjwt/issues/947).
+
 ### 0.12.5
 
 This patch release:
diff --git a/impl/src/main/java/io/jsonwebtoken/impl/DefaultJws.java b/impl/src/main/java/io/jsonwebtoken/impl/DefaultJws.java
index 0bf0fdf..dadaa51 100644
--- a/impl/src/main/java/io/jsonwebtoken/impl/DefaultJws.java
+++ b/impl/src/main/java/io/jsonwebtoken/impl/DefaultJws.java
@@ -18,7 +18,6 @@ package io.jsonwebtoken.impl;
 import io.jsonwebtoken.Jws;
 import io.jsonwebtoken.JwsHeader;
 import io.jsonwebtoken.JwtVisitor;
-import io.jsonwebtoken.io.Decoders;
 
 public class DefaultJws<P> extends DefaultProtectedJwt<JwsHeader, P> implements Jws<P> {
 
@@ -26,9 +25,9 @@ public class DefaultJws<P> extends DefaultProtectedJwt<JwsHeader, P> implements
 
     private final String signature;
 
-    public DefaultJws(JwsHeader header, P payload, String signature) {
-        super(header, payload, Decoders.BASE64URL.decode(signature), DIGEST_NAME);
-        this.signature = signature;
+    public DefaultJws(JwsHeader header, P payload, byte[] signature, String b64UrlSig) {
+        super(header, payload, signature, DIGEST_NAME);
+        this.signature = b64UrlSig;
     }
 
     @Override
diff --git a/impl/src/main/java/io/jsonwebtoken/impl/DefaultJwtParser.java b/impl/src/main/java/io/jsonwebtoken/impl/DefaultJwtParser.java
index beb36f4..ad2faad 100644
--- a/impl/src/main/java/io/jsonwebtoken/impl/DefaultJwtParser.java
+++ b/impl/src/main/java/io/jsonwebtoken/impl/DefaultJwtParser.java
@@ -265,8 +265,8 @@ public class DefaultJwtParser extends AbstractParser<Jwt<?, ?>> implements JwtPa
         return header != null && Strings.hasText(header.getContentType());
     }
 
-    private void verifySignature(final TokenizedJwt tokenized, final JwsHeader jwsHeader, final String alg,
-                                 @SuppressWarnings("deprecation") SigningKeyResolver resolver, Claims claims, Payload payload) {
+    private byte[] verifySignature(final TokenizedJwt tokenized, final JwsHeader jwsHeader, final String alg,
+                                   @SuppressWarnings("deprecation") SigningKeyResolver resolver, Claims claims, Payload payload) {
 
         Assert.notNull(resolver, "SigningKeyResolver instance cannot be null.");
 
@@ -354,6 +354,8 @@ public class DefaultJwtParser extends AbstractParser<Jwt<?, ?>> implements JwtPa
         } finally {
             Streams.reset(payloadStream);
         }
+
+        return signature;
     }
 
     @Override
@@ -485,7 +487,7 @@ public class DefaultJwtParser extends AbstractParser<Jwt<?, ?>> implements JwtPa
         }
 
         byte[] iv = null;
-        byte[] tag = null;
+        byte[] digest = null; // either JWE AEAD tag or JWS signature after Base64Url-decoding
         if (tokenized instanceof TokenizedJwe) {
 
             TokenizedJwe tokenizedJwe = (TokenizedJwe) tokenized;
@@ -521,8 +523,8 @@ public class DefaultJwtParser extends AbstractParser<Jwt<?, ?>> implements JwtPa
             base64Url = base64UrlDigest;
             //guaranteed to be non-empty via the `alg` + digest check above:
             Assert.hasText(base64Url, "JWE AAD Authentication Tag cannot be null or empty.");
-            tag = decode(base64Url, "JWE AAD Authentication Tag");
-            if (Bytes.isEmpty(tag)) {
+            digest = decode(base64Url, "JWE AAD Authentication Tag");
+            if (Bytes.isEmpty(digest)) {
                 String msg = "Compact JWE strings must always contain an AAD Authentication Tag.";
                 throw new MalformedJwtException(msg);
             }
@@ -564,7 +566,7 @@ public class DefaultJwtParser extends AbstractParser<Jwt<?, ?>> implements JwtPa
             // TODO: add encProvider(Provider) builder method that applies to this request only?
             InputStream ciphertext = payload.toInputStream();
             ByteArrayOutputStream plaintext = new ByteArrayOutputStream(8192);
-            DecryptAeadRequest dreq = new DefaultDecryptAeadRequest(ciphertext, cek, aad, iv, tag);
+            DecryptAeadRequest dreq = new DefaultDecryptAeadRequest(ciphertext, cek, aad, iv, digest);
             encAlg.decrypt(dreq, plaintext);
             payload = new Payload(plaintext.toByteArray(), header.getContentType());
 
@@ -574,7 +576,7 @@ public class DefaultJwtParser extends AbstractParser<Jwt<?, ?>> implements JwtPa
             // not using a signing key resolver, so we can verify the signature before reading the payload, which is
             // always safer:
             JwsHeader jwsHeader = Assert.stateIsInstance(JwsHeader.class, header, "Not a JwsHeader. ");
-            verifySignature(tokenized, jwsHeader, alg, new LocatingKeyResolver(this.keyLocator), null, payload);
+            digest = verifySignature(tokenized, jwsHeader, alg, new LocatingKeyResolver(this.keyLocator), null, payload);
             integrityVerified = true; // no exception means signature verified
         }
 
@@ -635,26 +637,28 @@ public class DefaultJwtParser extends AbstractParser<Jwt<?, ?>> implements JwtPa
             }
         }
 
+        // =============== Post-SKR Signature Check =================
+        if (hasDigest && signingKeyResolver != null) { // TODO: remove for 1.0
+            // A SigningKeyResolver has been configured, and due to it's API, we have to verify the signature after
+            // parsing the body.  This can be a security risk, so it needs to be removed before 1.0
+            JwsHeader jwsHeader = Assert.stateIsInstance(JwsHeader.class, header, "Not a JwsHeader. ");
+            digest = verifySignature(tokenized, jwsHeader, alg, this.signingKeyResolver, claims, payload);
+            //noinspection UnusedAssignment
+            integrityVerified = true; // no exception means verified successfully
+        }
+
         Jwt<?, ?> jwt;
         Object body = claims != null ? claims : payloadBytes;
         if (header instanceof JweHeader) {
-            jwt = new DefaultJwe<>((JweHeader) header, body, iv, tag);
+            jwt = new DefaultJwe<>((JweHeader) header, body, iv, digest);
         } else if (hasDigest) {
             JwsHeader jwsHeader = Assert.isInstanceOf(JwsHeader.class, header, "JwsHeader required.");
-            jwt = new DefaultJws<>(jwsHeader, body, base64UrlDigest.toString());
+            jwt = new DefaultJws<>(jwsHeader, body, digest, base64UrlDigest.toString());
         } else {
             //noinspection rawtypes
             jwt = new DefaultJwt(header, body);
         }
 
-        // =============== Signature =================
-        if (hasDigest && signingKeyResolver != null) { // TODO: remove for 1.0
-            // A SigningKeyResolver has been configured, and due to it's API, we have to verify the signature after
-            // parsing the body.  This can be a security risk, so it needs to be removed before 1.0
-            JwsHeader jwsHeader = Assert.stateIsInstance(JwsHeader.class, header, "Not a JwsHeader. ");
-            verifySignature(tokenized, jwsHeader, alg, this.signingKeyResolver, claims, payload);
-        }
-
         final boolean allowSkew = this.allowedClockSkewMillis > 0;
 
         //since 0.3:


Test patch:
diff --git a/impl/src/test/groovy/io/jsonwebtoken/impl/DefaultJwsTest.groovy b/impl/src/test/groovy/io/jsonwebtoken/impl/DefaultJwsTest.groovy
index a55e550..1511eb1 100644
--- a/impl/src/test/groovy/io/jsonwebtoken/impl/DefaultJwsTest.groovy
+++ b/impl/src/test/groovy/io/jsonwebtoken/impl/DefaultJwsTest.groovy
@@ -17,8 +17,12 @@ package io.jsonwebtoken.impl
 
 import io.jsonwebtoken.JwsHeader
 import io.jsonwebtoken.Jwts
+import io.jsonwebtoken.impl.lang.Bytes
+import io.jsonwebtoken.io.Encoders
 import org.junit.Test
 
+import java.security.MessageDigest
+
 import static org.junit.Assert.*
 
 class DefaultJwsTest {
@@ -26,10 +30,13 @@ class DefaultJwsTest {
     @Test
     void testConstructor() {
         JwsHeader header = new DefaultJwsHeader([:])
-        def jws = new DefaultJws<String>(header, 'foo', 'sig')
+        byte[] sig = Bytes.random(32)
+        String b64u = Encoders.BASE64URL.encode(sig)
+        def jws = new DefaultJws<String>(header, 'foo', sig, b64u)
         assertSame jws.getHeader(), header
         assertEquals jws.getPayload(), 'foo'
-        assertEquals jws.getSignature(), 'sig'
+        assertTrue MessageDigest.isEqual(sig, jws.getDigest())
+        assertEquals b64u, jws.getSignature()
     }
 
     @Test