Apache Log4j Core: Missing TLS hostname verification in Socket appender

@@ -30,19 +30,22 @@
 import java.util.Objects;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
+import javax.net.ssl.SNIHostName;
 import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLParameters;
 import javax.net.ssl.SSLSocket;
 import javax.net.ssl.SSLSocketFactory;
 import org.apache.logging.log4j.core.Layout;
 import org.apache.logging.log4j.core.net.ssl.SslConfiguration;
 import org.apache.logging.log4j.util.Strings;
+import org.jspecify.annotations.Nullable;
 
-/**
- *
- */
 public class SslSocketManager extends TcpSocketManager {
+
     public static final int DEFAULT_PORT = 6514;
+
     private static final SslSocketManagerFactory FACTORY = new SslSocketManagerFactory();
+
     private final SslConfiguration sslConfig;
 
     /**
@@ -285,10 +288,7 @@ private static String createSslConfigurationId(final SslConfiguration sslConfig)
 
     @Override
     protected Socket createSocket(final InetSocketAddress socketAddress) throws IOException {
-        final SSLSocketFactory socketFactory = createSslSocketFactory(sslConfig);
-        final Socket newSocket = socketFactory.createSocket();
-        newSocket.connect(socketAddress, getConnectTimeoutMillis());
-        return newSocket;
+        return createSocket(getHost(), socketAddress, getConnectTimeoutMillis(), sslConfig, getSocketOptions());
     }
 
     private static SSLSocketFactory createSslSocketFactory(final SslConfiguration sslConf) {
@@ -333,32 +333,102 @@ Socket createSocket(final SslFactoryData data) throws IOException {
             for (InetSocketAddress socketAddress : socketAddresses) {
                 try {
                     return SslSocketManager.createSocket(
-                            socketAddress, data.connectTimeoutMillis, data.sslConfiguration, data.socketOptions);
+                            data.host,
+                            socketAddress,
+                            data.connectTimeoutMillis,
+                            data.sslConfiguration,
+                            data.socketOptions);
                 } catch (IOException ex) {
-                    ioe = ex;
+                    final String message = String.format(
+                            "failed create a socket to `%s:%s` that is resolved to address `%s`",
+                            data.host, data.port, socketAddress);
+                    final IOException newEx = new IOException(message, ex);
+                    if (ioe == null) {
+                        ioe = newEx;
+                    } else {
+                        ioe.addSuppressed(newEx);
+                    }
                 }
             }
             throw new IOException(errorMessage(data, socketAddresses), ioe);
         }
     }
 
-    static Socket createSocket(
+    private static Socket createSocket(
+            final String hostName,
             final InetSocketAddress socketAddress,
             final int connectTimeoutMillis,
             final SslConfiguration sslConfiguration,
             final SocketOptions socketOptions)
             throws IOException {
+
+        // Create the `SSLSocket`
         final SSLSocketFactory socketFactory = createSslSocketFactory(sslConfiguration);
         final SSLSocket socket = (SSLSocket) socketFactory.createSocket();
+
+        // Apply socket options before `connect()`
         if (socketOptions != null) {
-            // Not sure which options must be applied before or after the connect() call.
             socketOptions.apply(socket);
         }
+
+        // Connect the socket
         socket.connect(socketAddress, connectTimeoutMillis);
-        if (socketOptions != null) {
-            // Not sure which options must be applied before or after the connect() call.
-            socketOptions.apply(socket);
+
+        // Verify the host name
+        if (sslConfiguration.isVerifyHostName()) {
+            // Allowed endpoint identification algorithms: HTTPS and LDAPS.
+            // https://docs.oracle.com/en/java/javase/17/docs/specs/security/standard-names.html#endpoint-identification-algorithms
+            final SSLParameters sslParameters = socket.getSSLParameters();
+            sslParameters.setEndpointIdentificationAlgorithm("HTTPS");
+            final SNIHostName serverName = createSniHostName(hostName);
+            if (serverName != null) {
+                sslParameters.setServerNames(Collections.singletonList(serverName));
+            }
+            socket.setSSLParameters(sslParameters);
         }
+
+        // Force the handshake right after `connect()` instead of waiting for read/write to trigger it indirectly at a
+        // later stage
+        socket.startHandshake();
+
         return socket;
     }
+
+    /**
+     * {@return an {@link SNIHostName} instance if the provided host name is not an IP literal (RFC 6066), and constitutes a valid host name (RFC 1035); null otherwise}
+     *
+     * @param hostName a host name
+     *
+     * @see <a href="https://www.rfc-editor.org/rfc/rfc6066.html#section-3">Literal IPv4 and IPv6 addresses are not permitted in "HostName" (RFC 6066)</a>
+     * @see <a href="https://www.rfc-editor.org/rfc/rfc1035.html">Domain Names - Implementation and Specification (RFC 1035)</a>
+     */
+    @Nullable
+    static SNIHostName createSniHostName(String hostName) {
+        // The actual check should be
+        //
+        //     !isIPv4(h) && !isIPv6(h) && isValidHostName(h)
+        //
+        // Though we translate this into
+        //
+        //     !h.matches("\d+[.]\d+[.]\d+[.]\d+") && new SNIServerName(h)
+        //
+        // This simplification is possible because
+        //
+        // - The `\d+[.]\d+[.]\d+[.]\d+` is sufficient to eliminate IPv4 addresses.
+        //   Any sequence of four numeric labels (e.g., `1234.2345.3456.4567`) is not a valid host name.
+        //   Hence, false positives are not a problem, they would be eliminated by `isValidHostName()` anyway.
+        //
+        // - `SNIServerName::new` throws an exception on invalid host names.
+        //   This check is performed using `IDN.toASCII(hostName, IDN.USE_STD3_ASCII_RULES)`.
+        //   IPv6 literals don't qualify as a valid host name by `IDN::toASCII`.
+        //   This assumption on `IDN` is unlikely to change in the foreseeable future.
+        if (!hostName.matches("\\d+[.]\\d+[.]\\d+[.]\\d+")) {
+            try {
+                return new SNIHostName(hostName);
+            } catch (IllegalArgumentException ignored) {
+                // Do nothing
+            }
+        }
+        return null;
+    }
 }

@@ -132,8 +132,8 @@ private static SSLContext createSslContext(
             @Nullable final TrustStoreConfiguration trustStoreConfig) {
         try {
             final SSLContext sslContext = SSLContext.getInstance(protocol);
-            final KeyManager[] keyManagers = loadKeyManagers(keyStoreConfig);
-            final TrustManager[] trustManagers = loadTrustManagers(trustStoreConfig);
+            @Nullable final KeyManager[] keyManagers = loadKeyManagers(keyStoreConfig);
+            @Nullable final TrustManager[] trustManagers = loadTrustManagers(trustStoreConfig);
             sslContext.init(keyManagers, trustManagers, null);
             return sslContext;
         } catch (final Exception error) {
@@ -144,9 +144,11 @@ private static SSLContext createSslContext(
         }
     }
 
+    @Nullable
+    @NullUnmarked
     private static KeyManager[] loadKeyManagers(@Nullable final KeyStoreConfiguration config) throws Exception {
         if (config == null) {
-            return new KeyManager[0];
+            return null;
         }
         final KeyManagerFactory factory = KeyManagerFactory.getInstance(config.getKeyManagerFactoryAlgorithm());
         final char[] password = config.getPasswordAsCharArray();
@@ -158,9 +160,11 @@ private static KeyManager[] loadKeyManagers(@Nullable final KeyStoreConfiguratio
         return factory.getKeyManagers();
     }
 
+    @Nullable
+    @NullUnmarked
     private static TrustManager[] loadTrustManagers(@Nullable final TrustStoreConfiguration config) throws Exception {
         if (config == null) {
-            return new TrustManager[0];
+            return null;
         }
         final TrustManagerFactory factory = TrustManagerFactory.getInstance(config.getTrustManagerFactoryAlgorithm());
         factory.init(config.getKeyStore());

@@ -0,0 +1,131 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.logging.log4j.core.net;
+
+import static org.apache.logging.log4j.core.net.SslSocketManager.createSniHostName;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatCode;
+
+import javax.net.ssl.SNIHostName;
+import javax.net.ssl.SSLParameters;
+import javax.net.ssl.SSLSocket;
+import org.apache.logging.log4j.core.Layout;
+import org.apache.logging.log4j.core.layout.PatternLayout;
+import org.apache.logging.log4j.core.net.ssl.SslConfiguration;
+import org.apache.logging.log4j.core.net.ssl.SslKeyStoreConstants;
+import org.apache.logging.log4j.core.net.ssl.TrustStoreConfiguration;
+import org.apache.logging.log4j.test.junit.UsingStatusListener;
+import org.apache.logging.log4j.util.Strings;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
+import org.junitpioneer.jupiter.Issue;
+
+class SslSocketManagerTest {
+
+    private static final String HOST_NAME = "apache.org";
+
+    private static final int HOST_PORT = 443;
+
+    private static final Layout<?> LAYOUT = PatternLayout.createDefaultLayout();
+
+    @Test
+    @Issue("https://github.com/apache/logging-log4j2/issues/3947")
+    @UsingStatusListener // Suppresses `StatusLogger` output, unless there is a failure
+    void should_not_throw_exception_when_configuration_without_KeyStore() throws Exception {
+        final TrustStoreConfiguration trustStoreConfig = new TrustStoreConfiguration(
+                SslKeyStoreConstants.TRUSTSTORE_LOCATION,
+                SslKeyStoreConstants::TRUSTSTORE_PWD,
+                SslKeyStoreConstants.TRUSTSTORE_TYPE,
+                null);
+        final SslConfiguration sslConfig = SslConfiguration.createSSLConfiguration(null, null, trustStoreConfig);
+        assertThatCode(() -> {
+                    // noinspection EmptyTryBlock
+                    try (final SslSocketManager ignored = createSocketManager(sslConfig)) {
+                        // Do nothing
+                    }
+                })
+                .doesNotThrowAnyException();
+    }
+
+    @Test
+    void host_name_verification_should_take_effect() {
+        final SslConfiguration sslConfig = SslConfiguration.createSSLConfiguration(
+                null,
+                null,
+                null,
+                // Explicitly enable hostname verification
+                true);
+        try (final SslSocketManager ssm = createSocketManager(sslConfig)) {
+            final SSLSocket sslSocket = (SSLSocket) ssm.getSocket();
+            final SSLParameters sslParams = sslSocket.getSSLParameters();
+            assertThat(sslParams.getEndpointIdentificationAlgorithm()).isEqualTo("HTTPS");
+            assertThat(sslParams.getServerNames()).containsOnly(new SNIHostName(HOST_NAME));
+        }
+    }
+
+    private static SslSocketManager createSocketManager(final SslConfiguration sslConfig) {
+        return SslSocketManager.getSocketManager(
+                sslConfig, SslSocketManagerTest.HOST_NAME, HOST_PORT, 0, 0, true, LAYOUT, 8192, null);
+    }
+
+    static String[] hostNamesAllowedForSniServerName() {
+        return new String[] {
+            "apache.org",
+            "a.b",
+            "sub.domain.co.uk",
+            "xn--bcher-kva.example", // valid IDN/punycode
+            "my-host-123.example",
+            "a1234567890.b1234567890.c1234567890", // numeric/alpha labels
+            "EXAMPLE.COM", // case-insensitive
+            "service--node.example", // double hyphens allowed
+            Strings.repeat("l", 63) + ".com", // 63-char label, valid
+            "a.b.c.d.e.f.g.h.i.j", // many labels, short each
+        };
+    }
+
+    @ParameterizedTest
+    @MethodSource("hostNamesAllowedForSniServerName")
+    void createSniHostName_should_work_with_allowed_input(String hostName) {
+        assertThat(createSniHostName(hostName)).isNotNull();
+    }
+
+    static String[] hostNamesNotAllowedForSniServerName() {
+        return new String[] {
+            // Invalid because IP literals not allowed
+            "192.168.1.1", // IPv4 literal
+            "2001:db8::1", // IPv6 literal
+            "[2001:db8::1]", // IPv6 URL literal form
+            // Invalid because illegal characters
+            "exa_mple.com", // underscore not allowed
+            "host name.com", // space not allowed
+            "example!.com", // symbol not allowed
+            // Invalid because label syntax violations
+            "-example.com", // leading hyphen
+            "example-.com", // trailing hyphen
+            ".example.com", // leading dot / empty label
+            // Invalid because label too long
+            Strings.repeat("l", 64) + ".com",
+        };
+    }
+
+    @ParameterizedTest
+    @MethodSource("hostNamesNotAllowedForSniServerName")
+    void createSniHostName_should_fail_with_not_allowed_input(String hostName) {
+        assertThat(createSniHostName(hostName)).isNull();
+    }
+}

@@ -1,45 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one or more
- * contributor license agreements.  See the NOTICE file distributed with
- * this work for additional information regarding copyright ownership.
- * The ASF licenses this file to you under the Apache License, Version 2.0
- * (the "License"); you may not use this file except in compliance with
- * the License.  You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-package org.apache.logging.log4j.core.net.ssl;
-
-import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
-
-import org.apache.logging.log4j.core.layout.PatternLayout;
-import org.apache.logging.log4j.core.net.SslSocketManager;
-import org.junit.jupiter.api.Test;
-import org.junitpioneer.jupiter.Issue;
-
-class SslSocketManagerTest {
-    @Issue("https://github.com/apache/logging-log4j2/issues/3947")
-    @Test
-    void shouldNotThrowExceptionWhenConfiguringTrustStore() {
-        final TrustStoreConfiguration trustStoreConfiguration = assertDoesNotThrow(() -> new TrustStoreConfiguration(
-                SslKeyStoreConstants.TRUSTSTORE_LOCATION,
-                SslKeyStoreConstants::TRUSTSTORE_PWD,
-                SslKeyStoreConstants.TRUSTSTORE_TYPE,
-                null));
-        final SslConfiguration sslConfiguration =
-                SslConfiguration.createSSLConfiguration(null, null, trustStoreConfiguration);
-        assertDoesNotThrow(() -> {
-            // noinspection EmptyTryBlock (try-with-resources to close `SslSocketManager`, even on failure
-            try (final SslSocketManager ignored = SslSocketManager.getSocketManager(
-                    sslConfiguration, "localhost", 0, 0, 0, true, PatternLayout.createDefaultLayout(), 8192, null)) {
-                // Do nothing
-            }
-        });
-    }
-}

@@ -0,0 +1,12 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns="https://logging.apache.org/xml/ns"
+       xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xsi:schemaLocation="
+           https://logging.apache.org/xml/ns
+           https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="fixed">
+  <issue id="4002" link="https://github.com/apache/logging-log4j2/pull/4002"/>
+  <description format="asciidoc">
+    Fix incorrect handling of the host name verification (i.e., `verifyHostName`) in `SslSocketManager`, which is used by Socket Appender when SSL/TLS is enabled
+  </description>
+</entry>

@@ -67,6 +67,9 @@ https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/JSSERefGuid
 If `true`, the host name in X509 certificate will be compared to the requested host name.
 In the case of a mismatch, the connection will fail.
 
+Note that SSL/TLS does not allow IP literals for host name verification – see https://www.rfc-editor.org/rfc/rfc6066.html#section-3[RFC 6066].
+Host name verification will only be effective if the provided host name is not an IP literal and a valid host name per https://www.rfc-editor.org/rfc/rfc1035.html[RFC 1035].
+
 See also
 xref:manual/systemproperties.adoc#log4j2.sslVerifyHostName[`log4j2.sslVerifyHostName`].
 |===