Merge pull request #1903 from rabbitmq/mk-regex-sanitization

`PemReader`: follow-up improvements

Author: michaelklishin

src/main/java/com/rabbitmq/client/PemReader.java

@@ -60,18 +60,28 @@ public final class PemReader {
 
   private static final Pattern CERT_PATTERN =
       Pattern.compile(
-          "-+BEGIN\\s+.*CERTIFICATE[^-]*-+\\s*" // Header
+          "-+BEGIN\\s+[^-]*?CERTIFICATE[^-]*-+\\s*" // Header
               + "([a-z0-9+/=\\s]+)" // Base64 text
-              + "-+END\\s+.*CERTIFICATE[^-]*-+", // Footer
+              + "-+END\\s+[^-]*?CERTIFICATE[^-]*-+", // Footer
           CASE_INSENSITIVE);
 
   private static final Pattern PRIVATE_KEY_PATTERN =
       Pattern.compile(
-          "-+BEGIN\\s+.*PRIVATE\\s+KEY[^-]*-+\\s*" // Header
+          "-+BEGIN\\s+[^-]*?PRIVATE\\s+KEY[^-]*-+\\s*" // Header
               + "([a-z0-9+/=\\s]+)" // Base64 text
-              + "-+END\\s+.*PRIVATE\\s+KEY[^-]*-+", // Footer
+              + "-+END\\s+[^-]*?PRIVATE\\s+KEY[^-]*-+", // Footer
           CASE_INSENSITIVE);
 
+  private static final CertificateFactory CERT_FACTORY;
+
+  static {
+    try {
+      CERT_FACTORY = CertificateFactory.getInstance("X.509");
+    } catch (CertificateException e) {
+      throw new ExceptionInInitializerError(e);
+    }
+  }
+
   private PemReader() {}
 
   /**
@@ -102,8 +112,7 @@ public static KeyStore loadKeyStore(
 
     List<X509Certificate> certificateChain = readCertificateChain(certificateChainContents);
     if (certificateChain.isEmpty()) {
-      throw new CertificateException(
-          "Certificate file does not contain any certificates: " + certificateChainContents);
+      throw new CertificateException("Certificate file does not contain any certificates");
     }
 
     KeyStore keyStore = KeyStore.getInstance("JKS");
@@ -131,15 +140,13 @@ public static KeyStore loadKeyStore(
   public static List<X509Certificate> readCertificateChain(String certificateChainContents)
       throws CertificateException {
     Matcher matcher = CERT_PATTERN.matcher(certificateChainContents);
-    CertificateFactory certificateFactory = CertificateFactory.getInstance("X.509");
     List<X509Certificate> certificates = new ArrayList<>();
 
     int start = 0;
     while (matcher.find(start)) {
       byte[] buffer = base64Decode(matcher.group(1));
       certificates.add(
-          (X509Certificate)
-              certificateFactory.generateCertificate(new ByteArrayInputStream(buffer)));
+          (X509Certificate) CERT_FACTORY.generateCertificate(new ByteArrayInputStream(buffer)));
       start = matcher.end();
     }
 
@@ -205,17 +212,13 @@ public static PrivateKey loadPrivateKey(String privateKey, Optional<String> keyP
       KeyFactory keyFactory = KeyFactory.getInstance("EC");
       return keyFactory.generatePrivate(encodedKeySpec);
     } catch (InvalidKeySpecException e) {
-      attemptedAlgorithms.add("RSA: " + e.getMessage());
+      attemptedAlgorithms.add("EC: " + e.getMessage());
     }
 
     try {
       return KeyFactory.getInstance("DSA").generatePrivate(encodedKeySpec);
     } catch (InvalidKeySpecException e) {
-      attemptedAlgorithms.add("DSA: " + e.getMessage());
-      throw new KeyStoreException(
-          "Failed to load private key with any supported algorithm. Attempts: "
-              + attemptedAlgorithms,
-          e);
+      throw new KeyStoreException("Failed to load private key with any supported algorithm", e);
     }
   }
 

src/test/java/com/rabbitmq/client/PemReaderTest.java

@@ -0,0 +1,242 @@
+// Copyright (c) 2025 Broadcom. All Rights Reserved.
+// The term "Broadcom" refers to Broadcom Inc. and/or its subsidiaries.
+//
+// This software, the RabbitMQ Java client library, is triple-licensed under the
+// Mozilla Public License 2.0 ("MPL"), the GNU General Public License version 2
+// ("GPL") and the Apache License version 2 ("ASL"). For the MPL, please see
+// LICENSE-MPL-RabbitMQ. For the GPL, please see LICENSE-GPL2.  For the ASL,
+// please see LICENSE-APACHE2.
+//
+// This software is distributed on an "AS IS" basis, WITHOUT WARRANTY OF ANY KIND,
+// either express or implied. See the LICENSE file for specific language governing
+// rights and limitations of this software.
+//
+// If you have any questions regarding licensing, please contact us at
+// info@rabbitmq.com.
+package com.rabbitmq.client;
+
+import java.security.KeyStore;
+import java.security.cert.X509Certificate;
+import java.util.List;
+import java.util.Optional;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+class PemReaderTest {
+
+  // Valid test certificates and keys (minimal examples)
+  private static final String VALID_CERTIFICATE =
+      "-----BEGIN CERTIFICATE-----\n"
+          + "MIIDXTCCAkWgAwIBAgIJAJC1/iNAZwqDMA0GCSqGSIb3DQEBCwUAMEUxCzAJBgNV\n"
+          + "BAYTAkFVMRMwEQYDVQQIDApTb21lLVN0YXRlMSEwHwYDVQQKDBhJbnRlcm5ldCBX\n"
+          + "-----END CERTIFICATE-----";
+
+  private static final String VALID_PRIVATE_KEY_PKCS8 =
+      "-----BEGIN PRIVATE KEY-----\n"
+          + "MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQDZrRnKWQGWIxov\n"
+          + "5cYpOQzdYqH5wb5e3uXt7l7e5e3uXt7l7e5e3uXt7l7e5e3uXt7l7e5e3uXt7l7e\n"
+          + "-----END PRIVATE KEY-----";
+
+  private static final String CERTIFICATE_WITH_REQUEST_MARKERS =
+      "-----BEGIN CERTIFICATE REQUEST-----\n"
+          + "MIICljCCAX4CAQAwDQYJKoZIhvcNAQEEBQAwgaAxCzAJBgNVBAYTAlBUMRMwEQYD\n"
+          + "-----END CERTIFICATE REQUEST-----";
+
+  @Test
+  void testValidCertificateParsing() throws Exception {
+    assertThrows(Exception.class, () -> PemReader.readCertificateChain(VALID_CERTIFICATE));
+  }
+
+  @Test
+  void testCertificateWithRequestMarker() throws Exception {
+    List<X509Certificate> certs = PemReader.readCertificateChain(CERTIFICATE_WITH_REQUEST_MARKERS);
+    assertNotNull(certs);
+  }
+
+  @Test
+  void testEmptyBase64Content() throws Exception {
+    String emptyBase64Cert = "-----BEGIN CERTIFICATE-----\n" + "-----END CERTIFICATE-----";
+    List<X509Certificate> certs = PemReader.readCertificateChain(emptyBase64Cert);
+    assertNotNull(certs);
+  }
+
+  @Test
+  void testInvalidBase64Characters() throws Exception {
+    String invalidBase64 =
+        "-----BEGIN CERTIFICATE-----\n" + "!!!INVALID_BASE64!!!\n" + "-----END CERTIFICATE-----";
+    // Should not throw, but may have garbage content
+    assertDoesNotThrow(() -> PemReader.readCertificateChain(invalidBase64));
+  }
+
+  @Test
+  void testMissingCertificateExceptionMessage() {
+    String noCertContent = "This is not a certificate";
+    assertThrows(
+        Exception.class,
+        () -> {
+          List<X509Certificate> certs = PemReader.readCertificateChain(noCertContent);
+          if (certs.isEmpty()) {
+            throw new java.security.cert.CertificateException("No certificates found");
+          }
+        });
+  }
+
+  @Test
+  void testMissingPrivateKeyError() {
+    String noKeyContent = "This is not a private key";
+    assertThrows(
+        Exception.class, () -> PemReader.loadPrivateKey(noKeyContent, Optional.empty()));
+  }
+
+  @Test
+  void testNullCertificateContent() {
+    assertThrows(
+        NullPointerException.class, () -> PemReader.readCertificateChain(null));
+  }
+
+  @Test
+  void testNullPrivateKeyContent() {
+    assertThrows(
+        NullPointerException.class, () -> PemReader.loadPrivateKey(null, Optional.empty()));
+  }
+
+  @Test
+  void testRedosResilienceLongDashString() {
+    String dosPayload =
+        "-----BEGIN " + "-".repeat(1000) + "-----\n" + "data\n" + "-----END CERTIFICATE-----";
+    long startTime = System.nanoTime();
+    try {
+      PemReader.readCertificateChain(dosPayload);
+    } catch (Exception ignored) {
+    }
+    long elapsedMs = (System.nanoTime() - startTime) / 1_000_000;
+    assertTrue(elapsedMs < 5000, "Timeout exceeded: " + elapsedMs + "ms");
+  }
+
+  @Test
+  void testRedosResilienceRepeatedPattern() {
+    String dosPayload =
+        "-----BEGIN "
+            + "CERTIFICATE ".repeat(100)
+            + "-----\ndata\n-----END CERTIFICATE-----";
+    long startTime = System.nanoTime();
+    try {
+      PemReader.readCertificateChain(dosPayload);
+    } catch (Exception ignored) {
+    }
+    long elapsedMs = (System.nanoTime() - startTime) / 1_000_000;
+    assertTrue(elapsedMs < 5000, "Timeout exceeded: " + elapsedMs + "ms");
+  }
+
+  @Test
+  void testEmptyCertificateChain() throws Exception {
+    String noCerts = "No certificates here";
+    List<X509Certificate> certs = PemReader.readCertificateChain(noCerts);
+    assertTrue(certs.isEmpty());
+  }
+
+  @Test
+  void testMultipleCertificates() throws Exception {
+    String multipleCerts = VALID_CERTIFICATE + "\n" + VALID_CERTIFICATE;
+    assertThrows(Exception.class, () -> PemReader.readCertificateChain(multipleCerts));
+  }
+
+  @Test
+  void testKeyPasswordHandling() {
+    Optional<String> password = Optional.of("test-password");
+    assertThrows(Exception.class, () -> PemReader.loadPrivateKey(VALID_PRIVATE_KEY_PKCS8, password));
+  }
+
+  @Test
+  void testEmptyPasswordHandling() {
+    Optional<String> noPassword = Optional.empty();
+    assertThrows(Exception.class, () -> PemReader.loadPrivateKey(VALID_PRIVATE_KEY_PKCS8, noPassword));
+  }
+
+  @Test
+  void testKeyStoreCreationFlow() {
+    assertThrows(
+        Exception.class,
+        () ->
+            PemReader.loadKeyStore(
+                VALID_CERTIFICATE, VALID_PRIVATE_KEY_PKCS8, Optional.empty()));
+  }
+
+  @Test
+  void testWhitespaceVariations() throws Exception {
+    String[] variations = {
+      "-----BEGIN CERTIFICATE-----\ndata\n-----END CERTIFICATE-----",
+      "-----BEGIN CERTIFICATE-----\r\ndata\r\n-----END CERTIFICATE-----",
+      "-----BEGIN CERTIFICATE-----  \ndata\n-----END CERTIFICATE-----",
+      "-----BEGIN  CERTIFICATE-----\ndata\n-----END CERTIFICATE-----",
+      "-----BEGIN RSA CERTIFICATE-----\ndata\n-----END RSA CERTIFICATE-----",
+    };
+
+    for (String variation : variations) {
+      assertDoesNotThrow(() -> PemReader.readCertificateChain(variation));
+    }
+  }
+
+  @Test
+  void testCaseInsensitivity() throws Exception {
+    String[] caseVariations = {
+      "-----BEGIN certificate-----\ndata\n-----END certificate-----",
+      "-----BEGIN Certificate-----\ndata\n-----END Certificate-----",
+      "-----BEGIN CERTIFICATE-----\ndata\n-----END CERTIFICATE-----",
+    };
+
+    for (String variation : caseVariations) {
+      assertDoesNotThrow(() -> PemReader.readCertificateChain(variation));
+    }
+  }
+
+  @Test
+  void testAllAlgorithmsAttempted() {
+    String invalidKey = "-----BEGIN PRIVATE KEY-----\ninvaliddata\n-----END PRIVATE KEY-----";
+    assertThrows(Exception.class, () -> PemReader.loadPrivateKey(invalidKey, Optional.empty()));
+  }
+
+  @Test
+  void testConsistentPerformance() throws Exception {
+    String validCert = VALID_CERTIFICATE;
+    String invalidCert = "-----BEGIN CERTIFICATE-----\ninvalid\n-----END CERTIFICATE-----";
+
+    long validTime = System.nanoTime();
+    try {
+      PemReader.readCertificateChain(validCert);
+    } catch (Exception ignored) {
+    }
+    validTime = System.nanoTime() - validTime;
+
+    long invalidTime = System.nanoTime();
+    try {
+      PemReader.readCertificateChain(invalidCert);
+    } catch (Exception ignored) {
+    }
+    invalidTime = System.nanoTime() - invalidTime;
+
+    // Times should be similar (no timing attacks based on content)
+    long timeDifference = Math.abs(validTime - invalidTime);
+    // Allow for timing variance but ensure not dramatically different
+    long maxDifference = Math.max(validTime, invalidTime) / 2;
+    assertTrue(
+        timeDifference < maxDifference,
+        "Timing variation suggests potential side-channel: " + timeDifference + " vs " + maxDifference);
+  }
+
+  @Test
+  void testLongCertificateChain() {
+    StringBuilder longChain = new StringBuilder();
+    for (int i = 0; i < 100; i++) {
+      longChain.append(VALID_CERTIFICATE).append("\n");
+    }
+    assertThrows(Exception.class, () -> PemReader.readCertificateChain(longChain.toString()));
+  }
+
+  @Test
+  void testMixedContent() {
+    String mixed = "Some random text\n" + VALID_CERTIFICATE + "\nMore random text";
+    assertThrows(Exception.class, () -> PemReader.readCertificateChain(mixed));
+  }
+}