Fix padding when extracting the r and s components of the EC signatur…

…e from a DER encoded sequence.
FusionAuth · Feb 28, 2024 · af79d25 · af79d25
1 parent 8db30c7
commit af79d25
Show file tree

Hide file tree

Showing 5 changed files with 45 additions and 99 deletions.
diff --git a/fusionauth-jwt.iml b/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>

diff --git 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.
@@ -86,10 +86,17 @@ public byte[] derDecode(Algorithm algorithm) throws IOException {
     }
 
     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);
+
+    int rSrcPos = r.length > len ? (r.length - len) : 0;
+    int rDstPos = Math.max(0, len - r.length);
+    int rLength = Math.min(r.length, len);
+    System.arraycopy(r, rSrcPos, result, rDstPos, rLength);
+
+    int sSrcPos = s.length > len ? (s.length - len) : 0;
+    int sDstPos = s.length < len ? (len + (len - s.length)) : len;
+    int sLength = Math.min(s.length, len);
+    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
@@ -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(invocationCount = 1000)
+  @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")
@@ -996,33 +1004,6 @@ public void test_openssl_keys_p_521() {
     assertEquals(actual.subject, jwt.subject);
   }
 
-  @Test(invocationCount = 1000)
-  public void test_openssl_keys_p_521_failed_validation() {
-    JWT jwt = new JWT()
-        .setSubject("1234567890")
-        .addClaim("name", "John Doe 208")
-        .addClaim("admin", true)
-        .addClaim("iat", 1516239022);
-
-    // PKCS#8 PEM, needs no encapsulation
-    Signer signer = ECSigner.newSHA512Signer("-----BEGIN PRIVATE KEY-----\n" +
-        "MF8CAQAwEAYHKoZIzj0CAQYFK4EEACMESDBGAgEBBEHNkRW+AZ87Hobcc0BIW1YV\n" +
-        "Ia5uQ0QVj8kkAN7sgTzKmawnf82FqFnHzqyhshHfwx9eIv723E3/XrA+3nw+8/l9\n" +
-        "ew==\n" +
-        "-----END PRIVATE KEY-----");
-    String encodedJWT = JWT.getEncoder().encode(jwt, signer, header
-        -> header.set("kid", "S-CdQFQmkHB1rFfzQS4kxhzKk8xtZPAECItUvB0plIM"));
-
-    Verifier verifier = ECVerifier.newVerifier("-----BEGIN PUBLIC KEY-----\n" +
-        "MIGbMBAGByqGSM49AgEGBSuBBAAjA4GGAAQBa5VzP87t5eGBqoZFLs9xQiodlD7E\n" +
-        "0xCzD7SrjshhOfOYei9kb7Yt5bvXShTfji16r1tgpaVzc5ffG693O4RVsUsBV7nl\n" +
-        "mCRG7g5x9FD2VIiSwnrDHyVf0bYNMeBM4vrt245VNT0E1xkJWhUB2JIl4dGvv0qE\n" +
-        "dYamwqrHFpoN7QW530Y=\n" +
-        "-----END PUBLIC KEY-----");
-    JWT actual = JWT.getDecoder().decode(encodedJWT, verifier);
-    assertEquals(actual.subject, jwt.subject);
-  }
-
   @Test
   public void test_zonedDateTime() {
     ZonedDateTime expiration = ZonedDateTime.now(ZoneOffset.UTC).plusMinutes(60).truncatedTo(ChronoUnit.SECONDS);

diff --git a/src/test/java/io/fusionauth/jwt/ec/ECSignerTest.java b/src/test/java/io/fusionauth/jwt/ec/ECSignerTest.java
@@ -16,15 +16,8 @@
 
 package io.fusionauth.jwt.ec;
 
-import io.fusionauth.jwks.JSONWebKeyBuilder;
 import io.fusionauth.jwt.BaseJWTTest;
-import io.fusionauth.jwt.InvalidJWTSignatureException;
 import io.fusionauth.jwt.InvalidKeyTypeException;
-import io.fusionauth.jwt.JWTUtils;
-import io.fusionauth.jwt.Signer;
-import io.fusionauth.jwt.Verifier;
-import io.fusionauth.jwt.domain.JWT;
-import io.fusionauth.pem.PEMEncoder;
 import io.fusionauth.pem.domain.PEM;
 import io.fusionauth.security.BCFIPSCryptoProvider;
 import org.testng.annotations.Test;
@@ -34,7 +27,6 @@
 import java.nio.file.Paths;
 import java.security.KeyPair;
 import java.security.KeyPairGenerator;
-import java.security.SecureRandom;
 import java.security.Signature;
 import java.security.interfaces.ECPrivateKey;
 import java.security.interfaces.ECPublicKey;
@@ -67,61 +59,28 @@ public void test_invalidKey() {
     }
   }
 
-  @Test(invocationCount = 1000)
+  @Test
   public void round_trip_raw1() throws Exception {
     // Generate a key-pair and sign and verify a message
     KeyPairGenerator g = KeyPairGenerator.getInstance("EC");
-    ECGenParameterSpec parameterSpec = new ECGenParameterSpec("secp384r1");
+    ECGenParameterSpec parameterSpec = new ECGenParameterSpec("secp256r1");
     g.initialize(parameterSpec);
     KeyPair pair = g.generateKeyPair();
-    System.out.println(new PEMEncoder().encode(pair.getPublic()));
-    System.out.println(new PEMEncoder().encode(pair.getPrivate()));
-    System.out.println(JWTUtils.generateJWS_kid_S256(new JSONWebKeyBuilder().build(pair.getPublic())));
 
     // Instance of signature class with SHA256withECDSA algorithm
-    Signature signature = Signature.getInstance("SHA384withECDSA");
+    Signature signature = Signature.getInstance("SHA256withECDSA");
     signature.initSign(pair.getPrivate());
-    Signer signer = ECSigner.newSHA384Signer(pair.getPrivate());
-    Verifier verifier = ECVerifier.newVerifier(pair.getPublic());
-
-    for (int i = 0; i < 250; i++) {
-      JWT jwt = new JWT()
-          .setSubject("1234567890")
-          .addClaim("name", "John Doe " + i)
-          .addClaim("admin", true)
-          .addClaim("iat", 1516239022);
-
-      String encodedJWT = JWT.getEncoder().encode(jwt, signer, header
-          -> header.set("kid", JWTUtils.generateJWS_kid_S256(new JSONWebKeyBuilder().build(pair.getPublic()))));
-
-        JWT actual = JWT.getDecoder().decode(encodedJWT, verifier);
-        assertEquals(actual.subject, jwt.subject);
-        assertEquals(actual.getString("name"), jwt.getString("name"));
-//      try {
-//        System.out.println("Good " + encodedJWT);
-//      } catch (InvalidJWTSignatureException e) {
-//        System.out.println("Bad " + encodedJWT);
-//      }
-    }
 
-//    for (int i = 0; i < 250; i++) {
-//      // Sign a message
-//      byte[] bytes = new byte[i+1];
-//      new SecureRandom().nextBytes(bytes);
-//      signature.update(bytes);
-//      byte[] signatureBytes = signature.sign();
-//
-//      // Validation
-//      Signature verifier = Signature.getInstance("SHA512withECDSA");
-//      verifier.initVerify(pair.getPublic());
-//      verifier.update(bytes);
-//      boolean verified = verifier.verify(signatureBytes);
-//      if (!verified) {
-//        System.out.println("Public: " + new String(pair.getPublic().getEncoded(), StandardCharsets.UTF_8));
-//        System.out.println("Private: " + new String(pair.getPrivate().getEncoded(), StandardCharsets.UTF_8));
-//      }
-//      assertTrue(verified);
-//    }
+    // Sign a message
+    String message = "text ecdsa with sha256";
+    signature.update((message).getBytes(StandardCharsets.UTF_8));
+    byte[] signatureBytes = signature.sign();
+
+    // Validation
+    Signature verifier = Signature.getInstance("SHA256withECDSA");
+    verifier.initVerify(pair.getPublic());
+    verifier.update(message.getBytes(StandardCharsets.UTF_8));
+    assertTrue(verifier.verify(signatureBytes));
   }
 
   @Test

diff --git a/src/test/java/io/fusionauth/security/KeyUtilsTests.java b/src/test/java/io/fusionauth/security/KeyUtilsTests.java
@@ -16,7 +16,6 @@
 
 package io.fusionauth.security;
 
-import io.fusionauth.pem.PEMDecoder;
 import io.fusionauth.pem.domain.PEM;
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
@@ -35,8 +34,8 @@ public class KeyUtilsTests {
   @DataProvider(name = "ecKeyLengths")
   public Object[][] ecKeyLengths() {
     return new Object[][]{
-//        {"EC", 256, 256, 256},
-//        {"EC", 384, 384, 384},
+        {"EC", 256, 256, 256},
+        {"EC", 384, 384, 384},
         {"EC", 521, 521, 521}
     };
   }
@@ -64,7 +63,7 @@ public void problematicKey() {
   // failures where the key is not an exact size and we have to figure out which key size it should be reported as.
   // - For testing locally, you can ramp up this invocation count to 100k or something like that to prove that we have
   //   consistency over time.
-  @Test(dataProvider = "ecKeyLengths", invocationCount = 20000)
+  @Test(dataProvider = "ecKeyLengths", invocationCount = 500)
   public void ec_getKeyLength(String algorithm, int keySize, int privateKeySize, int publicKeySize) throws Exception {
     KeyPairGenerator keyPairGenerator = KeyPairGenerator.getInstance(algorithm);
     keyPairGenerator.initialize(keySize);