Merge pull request #56 from FusionAuth/spencer/ec521-validation

Correct padding when extracting r and s components of the DER encoded EC signature

fusionauth-jwt.iml

@@ -20,7 +20,7 @@
         </CLASSES>
         <JAVADOC />
         <SOURCES>
-          <root url="jar://$MODULE_DIR$/.savant/cache/com/fasterxml/jackson/core/jackson-databind/2.15.2/jackson-databind-2.15.2-sources.jar!/" />
+          <root url="jar://$MODULE_DIR$/.savant/cache/com/fasterxml/jackson/core/jackson-databind/2.15.2/jackson-databind-2.15.2-src.jar!/" />
         </SOURCES>
       </library>
     </orderEntry>
@@ -31,7 +31,7 @@
         </CLASSES>
         <JAVADOC />
         <SOURCES>
-          <root url="jar://$MODULE_DIR$/.savant/cache/com/fasterxml/jackson/core/jackson-annotations/2.15.2/jackson-annotations-2.15.2-sources.jar!/" />
+          <root url="jar://$MODULE_DIR$/.savant/cache/com/fasterxml/jackson/core/jackson-annotations/2.15.2/jackson-annotations-2.15.2-src.jar!/" />
         </SOURCES>
       </library>
     </orderEntry>
@@ -42,7 +42,7 @@
         </CLASSES>
         <JAVADOC />
         <SOURCES>
-          <root url="jar://$MODULE_DIR$/.savant/cache/com/fasterxml/jackson/core/jackson-core/2.15.2/jackson-core-2.15.2-sources.jar!/" />
+          <root url="jar://$MODULE_DIR$/.savant/cache/com/fasterxml/jackson/core/jackson-core/2.15.2/jackson-core-2.15.2-src.jar!/" />
         </SOURCES>
       </library>
     </orderEntry>

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;
   }
 

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")