This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 10.1.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 552327d3829695cd178846357175d6acffe64636 Author: opencode <[email protected]> AuthorDate: Mon Oct 12 13:06:23 2026 +0200 Ignore invalid certificate headers in SSLValve Two robustness problems in the processing of the client certificate header: An invalid percent-escape in the escaped certificate header caused UDecoder.URLDecode to throw an IllegalArgumentException which was not caught, aborting the request with a 500. The exception is now caught, the header is treated as absent and the unescaped header is used as a fallback, as when the escaped header is not present at all. The code comment stated that the trimmed header value was assumed to start with the PEM BEGIN marker, but nothing verified it: any value longer than the marker had its first 27 characters replaced with the marker and was submitted to the certificate factory, and a failure to parse logged the whole bogus value at warn level. A trimmed value that does not start with the BEGIN marker is now ignored, with the header name logged at debug level. --- .../apache/catalina/valves/LocalStrings.properties | 2 ++ java/org/apache/catalina/valves/SSLValve.java | 31 ++++++++++++----- test/org/apache/catalina/valves/TestSSLValve.java | 40 ++++++++++++++++++++++ 3 files changed, 64 insertions(+), 9 deletions(-) diff --git a/java/org/apache/catalina/valves/LocalStrings.properties b/java/org/apache/catalina/valves/LocalStrings.properties index a9146706a8..34c2037981 100644 --- a/java/org/apache/catalina/valves/LocalStrings.properties +++ b/java/org/apache/catalina/valves/LocalStrings.properties @@ -173,6 +173,8 @@ semaphoreValve.invalidConcurrency=Invalid concurrency value [{0}] specified semaphoreValve.invalidStatus=Invalid high concurrency status [{0}] specified sslValve.certError=Failed to process certificate string [{0}] to create a java.security.cert.X509Certificate object +sslValve.invalidCertHeader=The value of the header [{0}] does not start with the PEM certificate marker. The header will be ignored. +sslValve.invalidEscapedCertHeader=The value of the header [{0}] is not valid percent-encoding. The header will be ignored. sslValve.invalidHeader=Invalid value [{0}] for header [{1}] sslValve.invalidProvider=The SSL provider specified on the connector associated with this request of [{0}] is invalid. The certificate data could not be processed. diff --git a/java/org/apache/catalina/valves/SSLValve.java b/java/org/apache/catalina/valves/SSLValve.java index c4f6f8d26f..4b19339d4c 100644 --- a/java/org/apache/catalina/valves/SSLValve.java +++ b/java/org/apache/catalina/valves/SSLValve.java @@ -65,6 +65,8 @@ public class SSLValve extends ValveBase { private static final Log log = LogFactory.getLog(SSLValve.class); + private static final String CERTIFICATE_BEGIN = "-----BEGIN CERTIFICATE-----"; + private String sslClientCertHeader = "ssl_client_cert"; private String sslClientEscapedCertHeader = "ssl_client_escaped_cert"; private String sslCipherHeader = "ssl_cipher"; @@ -199,25 +201,34 @@ public class SSLValve extends ValveBase { * into ' ' - nginx converts the '\n' into multiple ' ' - nginx ssl_client_escaped_cert uses "uri component" * escaping, keeping only ALPHA, DIGIT, "-", ".", "_", "~" * - * The code assumes that the trimmed header value starts with '-----BEGIN CERTIFICATE-----' and ends with - * '-----END CERTIFICATE-----'. + * A header value that, once trimmed, does not start with '-----BEGIN CERTIFICATE-----' is not a PEM encoded + * certificate and is ignored. * * Note: As long as the BEGIN marker and the rest of the content are on separate lines, the CertificateFactory * is tolerant of any additional whitespace. */ - String headerValue; + String headerValue = null; + String certHeaderName = sslClientCertHeader; String headerEscapedValue = mygetHeader(request, sslClientEscapedCertHeader); if (headerEscapedValue != null) { - headerValue = UDecoder.URLDecode(headerEscapedValue, null); - } else { + try { + headerValue = UDecoder.URLDecode(headerEscapedValue, null); + certHeaderName = sslClientEscapedCertHeader; + } catch (IllegalArgumentException e) { + // Invalid percent-encoding. Treat the header as absent and fall back to the unescaped header. + if (log.isDebugEnabled()) { + log.debug(sm.getString("sslValve.invalidEscapedCertHeader", sslClientEscapedCertHeader), e); + } + } + } + if (headerValue == null) { headerValue = mygetHeader(request, sslClientCertHeader); } if (headerValue != null) { headerValue = headerValue.trim(); - if (headerValue.length() > 27) { - String body = headerValue.substring(27); - String header = "-----BEGIN CERTIFICATE-----\n"; - String strcerts = header.concat(body); + if (headerValue.startsWith(CERTIFICATE_BEGIN)) { + String body = headerValue.substring(CERTIFICATE_BEGIN.length()); + String strcerts = CERTIFICATE_BEGIN.concat("\n").concat(body); ByteArrayInputStream bais = new ByteArrayInputStream(strcerts.getBytes(StandardCharsets.ISO_8859_1)); X509Certificate[] jsseCerts = null; String providerName = (String) request.getConnector().getProperty("clientCertProvider"); @@ -238,6 +249,8 @@ public class SSLValve extends ValveBase { } // If parsing fails, remove certificates request.setAttribute(Globals.CERTIFICATES_ATTR, jsseCerts); + } else if (log.isDebugEnabled()) { + log.debug(sm.getString("sslValve.invalidCertHeader", certHeaderName)); } } headerValue = mygetHeader(request, sslCipherHeader); diff --git a/test/org/apache/catalina/valves/TestSSLValve.java b/test/org/apache/catalina/valves/TestSSLValve.java index 67378bd830..a46798da03 100644 --- a/test/org/apache/catalina/valves/TestSSLValve.java +++ b/test/org/apache/catalina/valves/TestSSLValve.java @@ -119,6 +119,10 @@ public class TestSSLValve { EasyMock.replay(connector); mockRequest = new MockRequest(connector); } + // A single MockRequest instance is shared between tests. Remove any + // headers and attributes left over by a previous test. + mockRequest.getCoyoteRequest().getMimeHeaders().recycle(); + mockRequest.getCoyoteRequest().getAttributes().clear(); mockNext.invoke(mockRequest, null); EasyMock.replay(mockNext); } @@ -260,6 +264,42 @@ public class TestSSLValve { String begin = certificateSingleLine(linesBegin, " "); mockRequest.setHeader(valve.getSslClientCertHeader(), begin); + TesterLogValidationFilter f = TesterLogValidationFilter.add(Level.WARNING, null, null, + "org.apache.catalina.valves.SSLValve"); + + valve.invoke(mockRequest, null); + + EasyMock.verify(mockNext); + // A value that does not start with the BEGIN marker is not a PEM + // encoded certificate. The header is ignored. + Assert.assertNull(mockRequest.getAttribute(Globals.CERTIFICATES_ATTR)); + Assert.assertEquals(0, f.getMessageCount()); + } + + + @Test + public void testSslClientCertHeaderEscapedInvalidEncoding() throws Exception { + setUp(); + // Percent-escaping that is not followed by two hex digits + mockRequest.setHeader(valve.getSslClientEscapedCertHeader(), "%zz%0a-----BEGIN%20CERTIFICATE-----%0a"); + + TesterLogValidationFilter f = TesterLogValidationFilter.add(Level.WARNING, null, null, + "org.apache.catalina.valves.SSLValve"); + + valve.invoke(mockRequest, null); + + EasyMock.verify(mockNext); + Assert.assertNull(mockRequest.getAttribute(Globals.CERTIFICATES_ATTR)); + Assert.assertEquals(0, f.getMessageCount()); + } + + + @Test + public void testSslClientCertHeaderEscapedInvalidEncodingFallsBackToPlain() throws Exception { + setUp(); + mockRequest.setHeader(valve.getSslClientEscapedCertHeader(), "%zz%0a-----BEGIN%20CERTIFICATE-----%0a"); + mockRequest.setHeader(valve.getSslClientCertHeader(), certificateSingleLine(" ")); + valve.invoke(mockRequest, null); assertCertificateParsed(); --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
