This is an automated email from the ASF dual-hosted git repository. coheigea pushed a commit to branch coheigea/trust-manager in repository https://gitbox.apache.org/repos/asf/cxf.git
commit 2e41300d91c438cdaabf3cab05c40515a286f362 Author: Colm O hEigeartaigh <[email protected]> AuthorDate: Fri Aug 28 17:00:22 2026 +0100 Don't skip hostname verification with a custom trust manager --- .../org/apache/cxf/transport/https/SSLUtils.java | 54 ++++++++++++------ .../apache/cxf/transport/https/SSLUtilsTest.java | 64 ++++++++++++++++++++++ 2 files changed, 100 insertions(+), 18 deletions(-) diff --git a/rt/transports/http/src/main/java/org/apache/cxf/transport/https/SSLUtils.java b/rt/transports/http/src/main/java/org/apache/cxf/transport/https/SSLUtils.java index 970f9382710..f1085557b8a 100644 --- a/rt/transports/http/src/main/java/org/apache/cxf/transport/https/SSLUtils.java +++ b/rt/transports/http/src/main/java/org/apache/cxf/transport/https/SSLUtils.java @@ -45,6 +45,7 @@ import javax.net.ssl.SSLParameters; import javax.net.ssl.SSLPeerUnverifiedException; import javax.net.ssl.SSLSession; import javax.net.ssl.SSLSessionContext; +import javax.net.ssl.SSLSocket; import javax.net.ssl.StandardConstants; import javax.net.ssl.TrustManager; import javax.net.ssl.X509ExtendedTrustManager; @@ -245,6 +246,11 @@ public final class SSLUtils { } else { delegate.checkServerTrusted(chain, s); } + // certificates are valid, now check the hostname regardless of the + // delegate's type - see the SSLEngine overload below + if (socket instanceof SSLSocket) { + verifyPeerHostname(chain, ((SSLSocket)socket).getHandshakeSession()); + } } private String getHostName(List<SNIServerName> names) { @@ -268,27 +274,39 @@ public final class SSLUtils { throws CertificateException { if (extendedDelegate != null) { extendedDelegate.checkServerTrusted(chain, s, new SSLEngineWrapper(engine)); - //certificates are valid, now check hostnames - SSLSession session = engine.getHandshakeSession(); - List<SNIServerName> names = null; - if (session instanceof ExtendedSSLSession) { - ExtendedSSLSession extSession = (ExtendedSSLSession)session; - names = extSession.getRequestedServerNames(); - } - - boolean identifiable = false; - String peerHost = session.getPeerHost(); - String hostname = getHostName(names); - session = new SSLSessionWrapper(session, chain); - if (hostname != null && verifier.verify(hostname, session)) { - identifiable = true; - } - if (!identifiable && !verifier.verify(peerHost, session)) { - throw new CertificateException("No name matching " + peerHost + " found"); - } } else { delegate.checkServerTrusted(chain, s); } + // certificates are valid, now check the hostname. This must run regardless + // of the delegate's type: JSSE endpoint identification is deliberately + // suppressed (SSLEngineWrapper.getSSLParameters), so if the verifier were + // skipped for a plain X509TrustManager delegate no hostname check would + // happen anywhere and any certificate the delegate trusts would enable MITM. + verifyPeerHostname(chain, engine.getHandshakeSession()); + } + + private void verifyPeerHostname(X509Certificate[] chain, SSLSession session) + throws CertificateException { + if (session == null) { + throw new CertificateException( + "No handshake session available to verify the peer hostname"); + } + List<SNIServerName> names = null; + if (session instanceof ExtendedSSLSession) { + ExtendedSSLSession extSession = (ExtendedSSLSession)session; + names = extSession.getRequestedServerNames(); + } + + boolean identifiable = false; + String peerHost = session.getPeerHost(); + String hostname = getHostName(names); + SSLSession wrappedSession = new SSLSessionWrapper(session, chain); + if (hostname != null && verifier.verify(hostname, wrappedSession)) { + identifiable = true; + } + if (!identifiable && !verifier.verify(peerHost, wrappedSession)) { + throw new CertificateException("No name matching " + peerHost + " found"); + } } @Override diff --git a/rt/transports/http/src/test/java/org/apache/cxf/transport/https/SSLUtilsTest.java b/rt/transports/http/src/test/java/org/apache/cxf/transport/https/SSLUtilsTest.java index 66a1cc27867..4c56afd0f50 100644 --- a/rt/transports/http/src/test/java/org/apache/cxf/transport/https/SSLUtilsTest.java +++ b/rt/transports/http/src/test/java/org/apache/cxf/transport/https/SSLUtilsTest.java @@ -20,11 +20,18 @@ package org.apache.cxf.transport.https; import java.security.NoSuchAlgorithmException; +import java.security.cert.CertificateException; +import java.security.cert.X509Certificate; +import java.util.Collections; +import javax.net.ssl.ExtendedSSLSession; +import javax.net.ssl.HostnameVerifier; import javax.net.ssl.SSLContext; import javax.net.ssl.SSLEngine; +import javax.net.ssl.X509TrustManager; import org.apache.cxf.transport.https.SSLUtils.SSLEngineWrapper; +import org.apache.cxf.transport.https.SSLUtils.X509TrustManagerWrapper; import org.junit.After; import org.junit.Before; @@ -34,6 +41,13 @@ import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.CoreMatchers.not; import static org.hamcrest.CoreMatchers.nullValue; import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.Assert.fail; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; public class SSLUtilsTest { @@ -61,4 +75,54 @@ public class SSLUtilsTest { assertThat(wrapper.getSSLParameters(), is(not(nullValue()))); } + + /** + * Regression test: a plain (non-extended) X509TrustManager delegate must still get + * hostname verification. JSSE endpoint identification is suppressed by + * SSLEngineWrapper, so skipping the CXF HostnameVerifier for plain delegates left + * no hostname check at all - any certificate the trust manager accepted (e.g. any + * cert from a pinned corporate CA, issued for any host) enabled silent MITM. + */ + @Test + public void testPlainTrustManagerStillGetsHostnameVerification() throws Exception { + X509TrustManager plainTrustManager = mock(X509TrustManager.class); + HostnameVerifier failingVerifier = mock(HostnameVerifier.class); + when(failingVerifier.verify(anyString(), any())).thenReturn(false); + + ExtendedSSLSession session = mock(ExtendedSSLSession.class); + when(session.getPeerHost()).thenReturn("evil.example.net"); + when(session.getRequestedServerNames()).thenReturn(Collections.emptyList()); + SSLEngine mockEngine = mock(SSLEngine.class); + when(mockEngine.getHandshakeSession()).thenReturn(session); + + X509TrustManagerWrapper wrapper = + new X509TrustManagerWrapper(plainTrustManager, failingVerifier); + X509Certificate[] chain = new X509Certificate[0]; + try { + wrapper.checkServerTrusted(chain, "RSA", mockEngine); + fail("hostname verification must run for plain X509TrustManager delegates"); + } catch (CertificateException expected) { + // expected: no name matching evil.example.net + } + // the delegate's chain validation was still consulted + verify(plainTrustManager).checkServerTrusted(chain, "RSA"); + } + + @Test + public void testPlainTrustManagerAcceptedWhenHostnameMatches() throws Exception { + X509TrustManager plainTrustManager = mock(X509TrustManager.class); + HostnameVerifier passingVerifier = mock(HostnameVerifier.class); + when(passingVerifier.verify(anyString(), any())).thenReturn(true); + + ExtendedSSLSession session = mock(ExtendedSSLSession.class); + when(session.getPeerHost()).thenReturn("service.example.com"); + when(session.getRequestedServerNames()).thenReturn(Collections.emptyList()); + SSLEngine mockEngine = mock(SSLEngine.class); + when(mockEngine.getHandshakeSession()).thenReturn(session); + + X509TrustManagerWrapper wrapper = + new X509TrustManagerWrapper(plainTrustManager, passingVerifier); + wrapper.checkServerTrusted(new X509Certificate[0], "RSA", mockEngine); + verify(passingVerifier).verify(eq("service.example.com"), any()); + } }
