diff --git a/fusionauth-jwt.iml b/fusionauth-jwt.iml
index aa5199a..1785c4c 100644
--- a/fusionauth-jwt.iml
+++ b/fusionauth-jwt.iml
@@ -20,7 +20,7 @@
-
+
@@ -31,7 +31,7 @@
-
+
@@ -42,7 +42,7 @@
-
+
diff --git a/src/main/java/io/fusionauth/jwt/ec/ECDSASignature.java b/src/main/java/io/fusionauth/jwt/ec/ECDSASignature.java
index 6e96a0e..aa20872 100644
--- a/src/main/java/io/fusionauth/jwt/ec/ECDSASignature.java
+++ b/src/main/java/io/fusionauth/jwt/ec/ECDSASignature.java
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2018-2019, FusionAuth, All Rights Reserved
+ * Copyright (c) 2018-2024, FusionAuth, All Rights Reserved
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -70,6 +70,7 @@ public byte[] derDecode(Algorithm algorithm) throws IOException {
byte[] r = sequence[0].getPositiveBigInteger().toByteArray();
byte[] s = sequence[1].getPositiveBigInteger().toByteArray();
+ // The length of the result is fixed and discrete per algorithm.
byte[] result;
switch (algorithm) {
case ES256:
@@ -85,11 +86,37 @@ public byte[] derDecode(Algorithm algorithm) throws IOException {
throw new IllegalArgumentException("Unable to decode the signature for algorithm [" + algorithm.name() + "]");
}
- int len = result.length / 2;
- //noinspection ManualMinMaxCalculation
- System.arraycopy(r, r.length > len ? 1 : 0, result, r.length < len ? 1 : 0, r.length > len ? len : r.length);
- //noinspection ManualMinMaxCalculation
- System.arraycopy(s, s.length > len ? 1 : 0, result, s.length < len ? (len + 1) : len, s.length > len ? len : s.length);
+ // Because the response is not encoded, the r and s component must take up an equal amount of the resulting array.
+ // This allows the consumer of this value to always safely split the value in half based upon an index value since
+ // the result is not encoded and does not contain any meta-data about the contents.
+ int componentLength = result.length / 2;
+
+ // The extracted byte array of the DER encoded value can be left padded. For this reason, the component lengths
+ // may be greater than componentLength which is half of the result. So for example, if r is left padded, the
+ // length may be equal to 67 in ES512 even though componentLength is only 66. This is why we must calculate the
+ // source position for reading when we copy the r byte array into the result. The same is potentially true for
+ // either component. We cannot make an assumption that the source position in r or s will be 0.
+ //
+ // Similarly, when the r and s components are not padded, but they are shorter than componentLength, we need to
+ // pad the value to be right aligned in the result. This is why the destination position may not be 0 or
+ // componentLength respectively for
+ // r and s.
+ //
+ // If s is 65 bytes, then the destination position in the 0 initialized resulting array needs to be
+ // componentLength + 1 so that we write the final byte of s at the end of the result.
+ //
+ // For clarity, calculate each input to the arraycopy method first.
+
+ int rSrcPos = r.length > componentLength ? (r.length - componentLength) : 0;
+ int rDstPos = Math.max(0, componentLength - r.length);
+ int rLength = Math.min(r.length, componentLength);
+ System.arraycopy(r, rSrcPos, result, rDstPos, rLength);
+
+ int sSrcPos = s.length > componentLength ? (s.length - componentLength) : 0;
+ int sDstPos = s.length < componentLength ? (componentLength + (componentLength - s.length)) : componentLength;
+ int sLength = Math.min(s.length, componentLength);
+ System.arraycopy(s, sSrcPos, result, sDstPos, sLength);
+
return result;
}
diff --git a/src/test/java/io/fusionauth/jwt/JWTTest.java b/src/test/java/io/fusionauth/jwt/JWTTest.java
index 37bc2b2..a1726df 100644
--- a/src/test/java/io/fusionauth/jwt/JWTTest.java
+++ b/src/test/java/io/fusionauth/jwt/JWTTest.java
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2016-2022, FusionAuth, All Rights Reserved
+ * Copyright (c) 2016-2024, FusionAuth, All Rights Reserved
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -742,8 +742,10 @@ public void test_expiration_clockSkew() {
assertEquals(actual.subject, "1234567890");
}
- @Test
+ @Test(invocationCount = 2_000)
public void test_external_ec_521() {
+ // The purpose of the large invocation is to ensure we are consistently extracting the r and s components of the DER encoded signature.
+ // - Performing this test 1-3k times is generally sufficient to produce at least 1-3 errors prior to fixing the bug.
JWT jwt = new JWT()
.setSubject("1234567890")
.addClaim("name", "John Doe")
@@ -778,8 +780,10 @@ public void test_external_ec_521() {
assertEquals(actual.subject, jwt.subject);
}
- @Test
+ @Test(invocationCount = 2_000)
public void test_external_ec_p256() {
+ // The purpose of the large invocation is to ensure we are consistently extracting the r and s components of the DER encoded signature.
+ // - Performing this test 1-3k times is generally sufficient to produce at least 1-3 errors prior to fixing the bug.
JWT jwt = new JWT()
.setSubject("1234567890")
.addClaim("name", "John Doe")
@@ -806,8 +810,10 @@ public void test_external_ec_p256() {
assertEquals(actual.subject, jwt.subject);
}
- @Test
+ @Test(invocationCount = 2_000)
public void test_external_ec_p384() {
+ // The purpose of the large invocation is to ensure we are consistently extracting the r and s components of the DER encoded signature.
+ // - Performing this test 1-3k times is generally sufficient to produce at least 1-3 errors prior to fixing the bug.
JWT jwt = new JWT()
.setSubject("1234567890")
.addClaim("name", "John Doe")
@@ -887,7 +893,7 @@ public void test_multipleSignersAndVerifiers() throws Exception {
verifiers.put("verifier2", verifier2);
verifiers.put("verifier3", verifier3);
- // decode all of the encoded JWTs and ensure they come out the same.
+ // decode all the encoded JWTs and ensure they come out the same.
JWT jwt1 = JWT.getDecoder().decode(encodedJWT1, verifiers);
JWT jwt2 = JWT.getDecoder().decode(encodedJWT2, verifiers);
JWT jwt3 = JWT.getDecoder().decode(encodedJWT3, verifiers);
@@ -965,8 +971,10 @@ public void test_openssl_keys_p_256() {
assertEquals(actual.subject, jwt.subject);
}
- @Test
+ @Test(invocationCount = 2_000)
public void test_openssl_keys_p_521() {
+ // The purpose of the large invocation is to ensure we are consistently extracting the r and s components of the DER encoded signature.
+ // - Performing this test 1-3k times is generally sufficient to produce at least 1-3 errors prior to fixing the bug.
JWT jwt = new JWT()
.setSubject("1234567890")
.addClaim("name", "John Doe")