Skip to content

Commit 4cc4f9d

Browse files
michaelklishinmichaelklishin
committed
PemReader: follow-up updates
This improves regular expression patterns, error handling and logging in `PemReader`. References #1840.
1 parent 5359ec5 commit 4cc4f9d

2 files changed

Lines changed: 308 additions & 10 deletions

File tree

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

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -60,18 +60,28 @@ public final class PemReader {
6060

6161
private static final Pattern CERT_PATTERN =
6262
Pattern.compile(
63-
"-+BEGIN\\s+.*CERTIFICATE[^-]*-+\\s*" // Header
63+
"-+BEGIN\\s+[^-]*?CERTIFICATE[^-]*-+\\s*" // Header
6464
+ "([a-z0-9+/=\\s]+)" // Base64 text
65-
+ "-+END\\s+.*CERTIFICATE[^-]*-+", // Footer
65+
+ "-+END\\s+[^-]*?CERTIFICATE[^-]*-+", // Footer
6666
CASE_INSENSITIVE);
6767

6868
private static final Pattern PRIVATE_KEY_PATTERN =
6969
Pattern.compile(
70-
"-+BEGIN\\s+.*PRIVATE\\s+KEY[^-]*-+\\s*" // Header
70+
"-+BEGIN\\s+[^-]*?PRIVATE\\s+KEY[^-]*-+\\s*" // Header
7171
+ "([a-z0-9+/=\\s]+)" // Base64 text
72-
+ "-+END\\s+.*PRIVATE\\s+KEY[^-]*-+", // Footer
72+
+ "-+END\\s+[^-]*?PRIVATE\\s+KEY[^-]*-+", // Footer
7373
CASE_INSENSITIVE);
7474

75+
private static final CertificateFactory CERT_FACTORY;
76+
77+
static {
78+
try {
79+
CERT_FACTORY = CertificateFactory.getInstance("X.509");
80+
} catch (CertificateException e) {
81+
throw new ExceptionInInitializerError(e);
82+
}
83+
}
84+
7585
private PemReader() {}
7686

7787
/**
@@ -102,8 +112,7 @@ public static KeyStore loadKeyStore(
102112

103113
List<X509Certificate> certificateChain = readCertificateChain(certificateChainContents);
104114
if (certificateChain.isEmpty()) {
105-
throw new CertificateException(
106-
"Certificate file does not contain any certificates: " + certificateChainContents);
115+
throw new CertificateException("Certificate file does not contain any certificates");
107116
}
108117

109118
KeyStore keyStore = KeyStore.getInstance("JKS");
@@ -131,15 +140,13 @@ public static KeyStore loadKeyStore(
131140
public static List<X509Certificate> readCertificateChain(String certificateChainContents)
132141
throws CertificateException {
133142
Matcher matcher = CERT_PATTERN.matcher(certificateChainContents);
134-
CertificateFactory certificateFactory = CertificateFactory.getInstance("X.509");
135143
List<X509Certificate> certificates = new ArrayList<>();
136144

137145
int start = 0;
138146
while (matcher.find(start)) {
139147
byte[] buffer = base64Decode(matcher.group(1));
140148
certificates.add(
141-
(X509Certificate)
142-
certificateFactory.generateCertificate(new ByteArrayInputStream(buffer)));
149+
(X509Certificate) CERT_FACTORY.generateCertificate(new ByteArrayInputStream(buffer)));
143150
start = matcher.end();
144151
}
145152

@@ -205,7 +212,7 @@ public static PrivateKey loadPrivateKey(String privateKey, Optional<String> keyP
205212
KeyFactory keyFactory = KeyFactory.getInstance("EC");
206213
return keyFactory.generatePrivate(encodedKeySpec);
207214
} catch (InvalidKeySpecException e) {
208-
attemptedAlgorithms.add("RSA: " + e.getMessage());
215+
attemptedAlgorithms.add("EC: " + e.getMessage());
209216
}
210217

211218
try {

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

Lines changed: 291 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,291 @@
1+
// Copyright (c) 2025 Broadcom. All Rights Reserved.
2+
// The term "Broadcom" refers to Broadcom Inc. and/or its subsidiaries.
3+
//
4+
// This software, the RabbitMQ Java client library, is triple-licensed under the
5+
// Mozilla Public License 2.0 ("MPL"), the GNU General Public License version 2
6+
// ("GPL") and the Apache License version 2 ("ASL"). For the MPL, please see
7+
// LICENSE-MPL-RabbitMQ. For the GPL, please see LICENSE-GPL2. For the ASL,
8+
// please see LICENSE-APACHE2.
9+
//
10+
// This software is distributed on an "AS IS" basis, WITHOUT WARRANTY OF ANY KIND,
11+
// either express or implied. See the LICENSE file for specific language governing
12+
// rights and limitations of this software.
13+
//
14+
// If you have any questions regarding licensing, please contact us at
15+
// info@rabbitmq.com.
16+
package com.rabbitmq.client;
17+
18+
import java.security.KeyStore;
19+
import java.security.PrivateKey;
20+
import java.security.cert.X509Certificate;
21+
import java.util.List;
22+
import java.util.Optional;
23+
import org.junit.jupiter.api.DisplayName;
24+
import org.junit.jupiter.api.Test;
25+
26+
import static org.junit.jupiter.api.Assertions.*;
27+
28+
@DisplayName("PemReader Security Tests")
29+
class PemReaderTest {
30+
31+
// Valid test certificates and keys (minimal examples)
32+
private static final String VALID_CERTIFICATE =
33+
"-----BEGIN CERTIFICATE-----\n"
34+
+ "MIIDXTCCAkWgAwIBAgIJAJC1/iNAZwqDMA0GCSqGSIb3DQEBCwUAMEUxCzAJBgNV\n"
35+
+ "BAYTAkFVMRMwEQYDVQQIDApTb21lLVN0YXRlMSEwHwYDVQQKDBhJbnRlcm5ldCBX\n"
36+
+ "-----END CERTIFICATE-----";
37+
38+
private static final String VALID_PRIVATE_KEY_PKCS8 =
39+
"-----BEGIN PRIVATE KEY-----\n"
40+
+ "MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQDZrRnKWQGWIxov\n"
41+
+ "5cYpOQzdYqH5wb5e3uXt7l7e5e3uXt7l7e5e3uXt7l7e5e3uXt7l7e5e3uXt7l7e\n"
42+
+ "-----END PRIVATE KEY-----";
43+
44+
private static final String CERTIFICATE_WITH_REQUEST_MARKERS =
45+
"-----BEGIN CERTIFICATE REQUEST-----\n"
46+
+ "MIICljCCAX4CAQAwDQYJKoZIhvcNAQEEBQAwgaAxCzAJBgNVBAYTAlBUMRMwEQYD\n"
47+
+ "-----END CERTIFICATE REQUEST-----";
48+
49+
private static final String ENCRYPTED_PRIVATE_KEY =
50+
"-----BEGIN ENCRYPTED PRIVATE KEY-----\n"
51+
+ "MIIFDjBABgkqhkiG9w0BBQ0wMzAbBgkqhkiG9w0BBQwwDgQI1234567890ABCDE\n"
52+
+ "-----END ENCRYPTED PRIVATE KEY-----";
53+
54+
@Test
55+
@DisplayName("Iteration 1: Regex Fix - Valid Certificate Parsing")
56+
void testValidCertificateParsing() throws Exception {
57+
List<X509Certificate> certs = PemReader.readCertificateChain(VALID_CERTIFICATE);
58+
assertNotNull(certs);
59+
// Note: parsing may fail due to invalid cert data, but regex should match
60+
}
61+
62+
@Test
63+
@DisplayName("Iteration 1: Regex Fix - Certificate with REQUEST marker")
64+
void testCertificateWithRequestMarker() throws Exception {
65+
List<X509Certificate> certs = PemReader.readCertificateChain(CERTIFICATE_WITH_REQUEST_MARKERS);
66+
assertNotNull(certs);
67+
}
68+
69+
@Test
70+
@DisplayName("Iteration 2: Base64 Decoding - Empty Certificate Content")
71+
void testEmptyBase64Content() throws Exception {
72+
String emptyBase64Cert = "-----BEGIN CERTIFICATE-----\n" + "-----END CERTIFICATE-----";
73+
List<X509Certificate> certs = PemReader.readCertificateChain(emptyBase64Cert);
74+
assertNotNull(certs);
75+
assertDoesNotThrow(() -> PemReader.readCertificateChain(emptyBase64Cert));
76+
}
77+
78+
@Test
79+
@DisplayName("Iteration 2: Base64 Decoding - Invalid Base64 Characters")
80+
void testInvalidBase64Characters() throws Exception {
81+
String invalidBase64 =
82+
"-----BEGIN CERTIFICATE-----\n" + "!!!INVALID_BASE64!!!\n" + "-----END CERTIFICATE-----";
83+
// Should not throw, but may have garbage content
84+
assertDoesNotThrow(() -> PemReader.readCertificateChain(invalidBase64));
85+
}
86+
87+
@Test
88+
@DisplayName("Iteration 3: Exception Handling - Missing Certificate")
89+
void testMissingCertificateExceptionMessage() {
90+
String noCertContent = "This is not a certificate";
91+
assertThrows(
92+
Exception.class,
93+
() -> {
94+
List<X509Certificate> certs = PemReader.readCertificateChain(noCertContent);
95+
if (certs.isEmpty()) {
96+
throw new java.security.cert.CertificateException("No certificates found");
97+
}
98+
});
99+
}
100+
101+
@Test
102+
@DisplayName("Iteration 3: Exception Handling - Missing Private Key")
103+
void testMissingPrivateKeyError() {
104+
String noKeyContent = "This is not a private key";
105+
assertThrows(
106+
Exception.class, () -> PemReader.loadPrivateKey(noKeyContent, Optional.empty()));
107+
}
108+
109+
@Test
110+
@DisplayName("Iteration 5: Input Validation - Null Certificate Content")
111+
void testNullCertificateContent() {
112+
assertThrows(
113+
NullPointerException.class, () -> PemReader.readCertificateChain(null));
114+
}
115+
116+
@Test
117+
@DisplayName("Iteration 5: Input Validation - Null Private Key Content")
118+
void testNullPrivateKeyContent() {
119+
assertThrows(
120+
NullPointerException.class, () -> PemReader.loadPrivateKey(null, Optional.empty()));
121+
}
122+
123+
@Test
124+
@DisplayName("Iteration 7: ReDoS Resilience - Long Dashes in Header")
125+
void testRedosResilienceLongDashString() {
126+
String dosPayload =
127+
"-----BEGIN " + "-".repeat(1000) + "-----\n" + "data\n" + "-----END CERTIFICATE-----";
128+
long startTime = System.nanoTime();
129+
List<X509Certificate> result = null;
130+
try {
131+
result = PemReader.readCertificateChain(dosPayload);
132+
} catch (Exception e) {
133+
// Acceptable to fail, but should not hang
134+
}
135+
long endTime = System.nanoTime();
136+
long elapsedMs = (endTime - startTime) / 1_000_000;
137+
assertTrue(
138+
elapsedMs < 5000,
139+
"ReDoS vulnerability detected: parsing took " + elapsedMs + "ms for malicious input");
140+
}
141+
142+
@Test
143+
@DisplayName("Iteration 7: ReDoS Resilience - Repeated Pattern")
144+
void testRedosResilienceRepeatedPattern() {
145+
String dosPayload =
146+
"-----BEGIN "
147+
+ "CERTIFICATE ".repeat(100)
148+
+ "-----\ndata\n-----END CERTIFICATE-----";
149+
long startTime = System.nanoTime();
150+
List<X509Certificate> result = null;
151+
try {
152+
result = PemReader.readCertificateChain(dosPayload);
153+
} catch (Exception e) {
154+
// Acceptable to fail, but should not hang
155+
}
156+
long endTime = System.nanoTime();
157+
long elapsedMs = (endTime - startTime) / 1_000_000;
158+
assertTrue(
159+
elapsedMs < 5000, "ReDoS vulnerability detected: parsing took " + elapsedMs + "ms");
160+
}
161+
162+
@Test
163+
@DisplayName("Iteration 8: Certificate Validation - Empty Certificate Chain")
164+
void testEmptyCertificateChain() throws Exception {
165+
String noCerts = "No certificates here";
166+
List<X509Certificate> certs = PemReader.readCertificateChain(noCerts);
167+
assertTrue(certs.isEmpty());
168+
}
169+
170+
@Test
171+
@DisplayName("Iteration 8: Certificate Validation - Multiple Certificates")
172+
void testMultipleCertificates() throws Exception {
173+
String multipleCerts = VALID_CERTIFICATE + "\n" + VALID_CERTIFICATE;
174+
// Should parse without error
175+
assertDoesNotThrow(() -> PemReader.readCertificateChain(multipleCerts));
176+
}
177+
178+
@Test
179+
@DisplayName("Iteration 9: Memory Safety - Key Password Handling")
180+
void testKeyPasswordHandling() {
181+
// Document that password is converted to char array
182+
Optional<String> password = Optional.of("test-password");
183+
assertDoesNotThrow(() -> PemReader.loadPrivateKey(VALID_PRIVATE_KEY_PKCS8, password));
184+
}
185+
186+
@Test
187+
@DisplayName("Iteration 9: Memory Safety - Empty Password")
188+
void testEmptyPasswordHandling() {
189+
Optional<String> noPassword = Optional.empty();
190+
assertDoesNotThrow(() -> PemReader.loadPrivateKey(VALID_PRIVATE_KEY_PKCS8, noPassword));
191+
}
192+
193+
@Test
194+
@DisplayName("Iteration 10: Comprehensive - KeyStore Creation Flow")
195+
void testKeyStoreCreationFlow() {
196+
assertThrows(
197+
Exception.class,
198+
() ->
199+
PemReader.loadKeyStore(
200+
VALID_CERTIFICATE, VALID_PRIVATE_KEY_PKCS8, Optional.empty()));
201+
}
202+
203+
@Test
204+
@DisplayName("Iteration 6: Regex Pattern Matching - Whitespace Variations")
205+
void testWhitespaceVariations() throws Exception {
206+
String[] variations = {
207+
"-----BEGIN CERTIFICATE-----\ndata\n-----END CERTIFICATE-----",
208+
"-----BEGIN CERTIFICATE-----\r\ndata\r\n-----END CERTIFICATE-----",
209+
"-----BEGIN CERTIFICATE----- \ndata\n-----END CERTIFICATE-----",
210+
"-----BEGIN CERTIFICATE-----\ndata\n-----END CERTIFICATE-----",
211+
"-----BEGIN RSA CERTIFICATE-----\ndata\n-----END RSA CERTIFICATE-----",
212+
};
213+
214+
for (String variation : variations) {
215+
assertDoesNotThrow(() -> PemReader.readCertificateChain(variation));
216+
}
217+
}
218+
219+
@Test
220+
@DisplayName("Iteration 6: Regex Pattern Matching - Case Insensitivity")
221+
void testCaseInsensitivity() throws Exception {
222+
String[] caseVariations = {
223+
"-----BEGIN certificate-----\ndata\n-----END certificate-----",
224+
"-----BEGIN Certificate-----\ndata\n-----END Certificate-----",
225+
"-----BEGIN CERTIFICATE-----\ndata\n-----END CERTIFICATE-----",
226+
};
227+
228+
for (String variation : caseVariations) {
229+
assertDoesNotThrow(() -> PemReader.readCertificateChain(variation));
230+
}
231+
}
232+
233+
@Test
234+
@DisplayName("Iteration 4: Logic Bug - All Three Key Algorithms Attempted")
235+
void testAllAlgorithmsAttempted() {
236+
String invalidKey = "-----BEGIN PRIVATE KEY-----\ninvaliddata\n-----END PRIVATE KEY-----";
237+
Exception exception =
238+
assertThrows(Exception.class, () -> PemReader.loadPrivateKey(invalidKey, Optional.empty()));
239+
String message = exception.getMessage();
240+
assertNotNull(message);
241+
assertFalse(
242+
message.contains("RSA: RSA:"),
243+
"Error message should not duplicate algorithm names");
244+
}
245+
246+
@Test
247+
@DisplayName("Iteration 7: Timing Side-Channel - Consistent Performance")
248+
void testConsistentPerformance() throws Exception {
249+
String validCert = VALID_CERTIFICATE;
250+
String invalidCert = "-----BEGIN CERTIFICATE-----\ninvalid\n-----END CERTIFICATE-----";
251+
252+
long validTime = System.nanoTime();
253+
try {
254+
PemReader.readCertificateChain(validCert);
255+
} catch (Exception ignored) {
256+
}
257+
validTime = System.nanoTime() - validTime;
258+
259+
long invalidTime = System.nanoTime();
260+
try {
261+
PemReader.readCertificateChain(invalidCert);
262+
} catch (Exception ignored) {
263+
}
264+
invalidTime = System.nanoTime() - invalidTime;
265+
266+
// Times should be similar (no timing attacks based on content)
267+
long timeDifference = Math.abs(validTime - invalidTime);
268+
// Allow for timing variance but ensure not dramatically different
269+
long maxDifference = Math.max(validTime, invalidTime) / 2;
270+
assertTrue(
271+
timeDifference < maxDifference,
272+
"Timing variation suggests potential side-channel: " + timeDifference + " vs " + maxDifference);
273+
}
274+
275+
@Test
276+
@DisplayName("Iteration 10: Edge Case - Very Long Certificate Chain")
277+
void testLongCertificateChain() {
278+
StringBuilder longChain = new StringBuilder();
279+
for (int i = 0; i < 100; i++) {
280+
longChain.append(VALID_CERTIFICATE).append("\n");
281+
}
282+
assertDoesNotThrow(() -> PemReader.readCertificateChain(longChain.toString()));
283+
}
284+
285+
@Test
286+
@DisplayName("Iteration 10: Edge Case - Mixed Valid and Invalid Content")
287+
void testMixedContent() {
288+
String mixed = "Some random text\n" + VALID_CERTIFICATE + "\nMore random text";
289+
assertDoesNotThrow(() -> PemReader.readCertificateChain(mixed));
290+
}
291+
}

0 commit comments

Comments
 (0)