diff --git a/logback-android/src/main/java/ch/qos/logback/core/net/ssl/ConfigurableSSLSocketFactory.java b/logback-android/src/main/java/ch/qos/logback/core/net/ssl/ConfigurableSSLSocketFactory.java index 0341fb8d5..4170ff516 100644 --- a/logback-android/src/main/java/ch/qos/logback/core/net/ssl/ConfigurableSSLSocketFactory.java +++ b/logback-android/src/main/java/ch/qos/logback/core/net/ssl/ConfigurableSSLSocketFactory.java @@ -62,8 +62,7 @@ public Socket createSocket(InetAddress address, int port, InetAddress localAddress, int localPort) throws IOException { SSLSocket socket = (SSLSocket) delegate.createSocket(address, port, localAddress, localPort); - parameters.configure(new SSLConfigurableSocket(socket)); - return socket; + return configure(socket); } /** @@ -72,8 +71,7 @@ public Socket createSocket(InetAddress address, int port, @Override public Socket createSocket(InetAddress host, int port) throws IOException { SSLSocket socket = (SSLSocket) delegate.createSocket(host, port); - parameters.configure(new SSLConfigurableSocket(socket)); - return socket; + return configure(socket); } /** @@ -84,8 +82,7 @@ public Socket createSocket(String host, int port, InetAddress localHost, int localPort) throws IOException, UnknownHostException { SSLSocket socket = (SSLSocket) delegate.createSocket(host, port, localHost, localPort); - parameters.configure(new SSLConfigurableSocket(socket)); - return socket; + return configure(socket); } /** @@ -95,8 +92,13 @@ public Socket createSocket(String host, int port, InetAddress localHost, public Socket createSocket(String host, int port) throws IOException, UnknownHostException { SSLSocket socket = (SSLSocket) delegate.createSocket(host, port); - parameters.configure(new SSLConfigurableSocket(socket)); - return socket; + return configure(socket); + } + + private SSLSocket configure(SSLSocket socket) { + boolean verifyHostname = parameters.configureExceptHostnameVerification( + new SSLConfigurableSocket(socket)); + return SSLConfigurableSocket.applyHostnameVerification(socket, verifyHostname); } } diff --git a/logback-android/src/main/java/ch/qos/logback/core/net/ssl/SSLConfigurableSocket.java b/logback-android/src/main/java/ch/qos/logback/core/net/ssl/SSLConfigurableSocket.java index 596a9d9c2..0baeb90f8 100644 --- a/logback-android/src/main/java/ch/qos/logback/core/net/ssl/SSLConfigurableSocket.java +++ b/logback-android/src/main/java/ch/qos/logback/core/net/ssl/SSLConfigurableSocket.java @@ -76,13 +76,28 @@ public void setWantClientAuth(boolean state) { @Override public void setHostnameVerification(boolean hostnameVerification) { + applyHostnameVerification(delegate, hostnameVerification); + } + + /** + * Makes the socket verify that the server's certificate matches its hostname. + *

+ * This returns the socket so that a caller can return the result: CodeQL's + * java/unsafe-cert-trust query only sees hostname verification when + * {@code setSSLParameters()} is called in the method the socket flows through. + * + * @param socket the socket to configure + * @param hostnameVerification whether to verify the server's hostname + * @return {@code socket} + */ + static SSLSocket applyHostnameVerification(SSLSocket socket, boolean hostnameVerification) { // SSLParameters.setEndpointIdentificationAlgorithm() is API 24+; below // that, SSLParametersConfiguration warns that the hostname is not verified - if (!hostnameVerification || Build.VERSION.SDK_INT < Build.VERSION_CODES.N) { - return; + if (hostnameVerification && Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { + SSLParameters sslParameters = socket.getSSLParameters(); + sslParameters.setEndpointIdentificationAlgorithm("HTTPS"); + socket.setSSLParameters(sslParameters); } - SSLParameters sslParameters = delegate.getSSLParameters(); - sslParameters.setEndpointIdentificationAlgorithm("HTTPS"); - delegate.setSSLParameters(sslParameters); + return socket; } } diff --git a/logback-android/src/main/java/ch/qos/logback/core/net/ssl/SSLParametersConfiguration.java b/logback-android/src/main/java/ch/qos/logback/core/net/ssl/SSLParametersConfiguration.java index e542eea40..a9df9ca42 100644 --- a/logback-android/src/main/java/ch/qos/logback/core/net/ssl/SSLParametersConfiguration.java +++ b/logback-android/src/main/java/ch/qos/logback/core/net/ssl/SSLParametersConfiguration.java @@ -50,6 +50,18 @@ public class SSLParametersConfiguration extends ContextAwareBase { * @param socket the subject configurable */ public void configure(SSLConfigurable socket) { + socket.setHostnameVerification(configureExceptHostnameVerification(socket)); + } + + /** + * Configures every SSL parameter but hostname verification on an + * {@link SSLConfigurable}, and decides whether the peer's hostname should be + * verified. {@link ConfigurableSSLSocketFactory} applies that decision to the + * {@link javax.net.ssl.SSLSocket} it returns itself. + * @param socket the subject configurable + * @return whether the peer's hostname should be verified + */ + boolean configureExceptHostnameVerification(SSLConfigurable socket) { socket.setEnabledProtocols(enabledProtocols( socket.getSupportedProtocols(), socket.getDefaultProtocols())); socket.setEnabledCipherSuites(enabledCipherSuites( @@ -66,7 +78,7 @@ public void configure(SSLConfigurable socket) { addWarn("hostnameVerification requires Android 7.0 (API 24) or newer;" + " the peer's hostname will not be verified"); } - socket.setHostnameVerification(verifyHostname); + return verifyHostname; } /** diff --git a/logback-android/src/test/java/ch/qos/logback/core/net/ssl/SSLHostnameVerificationTest.java b/logback-android/src/test/java/ch/qos/logback/core/net/ssl/SSLHostnameVerificationTest.java index e0f5812e9..089f51e44 100644 --- a/logback-android/src/test/java/ch/qos/logback/core/net/ssl/SSLHostnameVerificationTest.java +++ b/logback-android/src/test/java/ch/qos/logback/core/net/ssl/SSLHostnameVerificationTest.java @@ -16,15 +16,22 @@ package ch.qos.logback.core.net.ssl; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertSame; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import java.io.IOException; +import java.net.InetAddress; +import java.net.Socket; + import javax.net.ssl.SSLParameters; import javax.net.ssl.SSLServerSocket; import javax.net.ssl.SSLSocket; +import javax.net.ssl.SSLSocketFactory; import android.os.Build; @@ -87,6 +94,27 @@ public void clientSocketHostnameVerificationCanBeDisabled() { verify(socket, never()).setSSLParameters(any(SSLParameters.class)); } + @Test + @Config(sdk = 29) + public void socketFactoryVerifiesHostnameByDefault() throws IOException { + Socket created = newSocketFactory().createSocket(InetAddress.getLoopbackAddress(), 4560); + + assertSame(socket, created); + ArgumentCaptor captor = ArgumentCaptor.forClass(SSLParameters.class); + verify(socket).setSSLParameters(captor.capture()); + assertEquals("HTTPS", captor.getValue().getEndpointIdentificationAlgorithm()); + } + + @Test + @Config(sdk = 29) + public void socketFactoryHostnameVerificationCanBeDisabled() throws IOException { + configuration.setHostnameVerification(false); + Socket created = newSocketFactory().createSocket("localhost", 4560); + + assertSame(socket, created); + verify(socket, never()).setSSLParameters(any(SSLParameters.class)); + } + @Test @Config(sdk = 29) public void serverSocketDoesNotVerifyHostnameByDefault() { @@ -122,4 +150,11 @@ public void belowApi24HostnameIsNotVerifiedAndAWarningIsLogged() { verify(socket, never()).setSSLParameters(any(SSLParameters.class)); assertEquals(Status.WARN, new StatusUtil(context).getHighestLevel(0)); } + + private ConfigurableSSLSocketFactory newSocketFactory() throws IOException { + SSLSocketFactory delegate = mock(SSLSocketFactory.class); + when(delegate.createSocket(any(InetAddress.class), anyInt())).thenReturn(socket); + when(delegate.createSocket(any(String.class), anyInt())).thenReturn(socket); + return new ConfigurableSSLSocketFactory(configuration, delegate); + } }