Skip to content

Commit 3b93748

Browse files
vyvy
authored
Fix host name verification in SSLSocketManager (#4002)
1 parent 37a0383 commit 3b93748

File tree

6 files changed

+238
-63
lines changed

6 files changed

+238
-63
lines changed

‎log4j-core-test/src/test/java/org/apache/logging/log4j/core/net/SslSocketManagerTest.java‎

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one or more
3+
* contributor license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright ownership.
5+
* The ASF licenses this file to you under the Apache License, Version 2.0
6+
* (the "License"); you may not use this file except in compliance with
7+
* the License. You may obtain a copy of the License at
8+
*
9+
* http://www.apache.org/licenses/LICENSE-2.0
10+
*
11+
* Unless required by applicable law or agreed to in writing, software
12+
* distributed under the License is distributed on an "AS IS" BASIS,
13+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
* See the License for the specific language governing permissions and
15+
* limitations under the License.
16+
*/
17+
package org.apache.logging.log4j.core.net;
18+
19+
import static org.apache.logging.log4j.core.net.SslSocketManager.createSniHostName;
20+
import static org.assertj.core.api.Assertions.assertThat;
21+
import static org.assertj.core.api.Assertions.assertThatCode;
22+
23+
import javax.net.ssl.SNIHostName;
24+
import javax.net.ssl.SSLParameters;
25+
import javax.net.ssl.SSLSocket;
26+
import org.apache.logging.log4j.core.Layout;
27+
import org.apache.logging.log4j.core.layout.PatternLayout;
28+
import org.apache.logging.log4j.core.net.ssl.SslConfiguration;
29+
import org.apache.logging.log4j.core.net.ssl.SslKeyStoreConstants;
30+
import org.apache.logging.log4j.core.net.ssl.TrustStoreConfiguration;
31+
import org.apache.logging.log4j.test.junit.UsingStatusListener;
32+
import org.apache.logging.log4j.util.Strings;
33+
import org.junit.jupiter.api.Test;
34+
import org.junit.jupiter.params.ParameterizedTest;
35+
import org.junit.jupiter.params.provider.MethodSource;
36+
import org.junitpioneer.jupiter.Issue;
37+
38+
class SslSocketManagerTest {
39+
40+
private static final String HOST_NAME = "apache.org";
41+
42+
private static final int HOST_PORT = 443;
43+
44+
private static final Layout<?> LAYOUT = PatternLayout.createDefaultLayout();
45+
46+
@Test
47+
@Issue("https://github.com/apache/logging-log4j2/issues/3947")
48+
@UsingStatusListener // Suppresses `StatusLogger` output, unless there is a failure
49+
void should_not_throw_exception_when_configuration_without_KeyStore() throws Exception {
50+
final TrustStoreConfiguration trustStoreConfig = new TrustStoreConfiguration(
51+
SslKeyStoreConstants.TRUSTSTORE_LOCATION,
52+
SslKeyStoreConstants::TRUSTSTORE_PWD,
53+
SslKeyStoreConstants.TRUSTSTORE_TYPE,
54+
null);
55+
final SslConfiguration sslConfig = SslConfiguration.createSSLConfiguration(null, null, trustStoreConfig);
56+
assertThatCode(() -> {
57+
// noinspection EmptyTryBlock
58+
try (final SslSocketManager ignored = createSocketManager(sslConfig)) {
59+
// Do nothing
60+
}
61+
})
62+
.doesNotThrowAnyException();
63+
}
64+
65+
@Test
66+
void host_name_verification_should_take_effect() {
67+
final SslConfiguration sslConfig = SslConfiguration.createSSLConfiguration(
68+
null,
69+
null,
70+
null,
71+
// Explicitly enable hostname verification
72+
true);
73+
try (final SslSocketManager ssm = createSocketManager(sslConfig)) {
74+
final SSLSocket sslSocket = (SSLSocket) ssm.getSocket();
75+
final SSLParameters sslParams = sslSocket.getSSLParameters();
76+
assertThat(sslParams.getEndpointIdentificationAlgorithm()).isEqualTo("HTTPS");
77+
assertThat(sslParams.getServerNames()).containsOnly(new SNIHostName(HOST_NAME));
78+
}
79+
}
80+
81+
private static SslSocketManager createSocketManager(final SslConfiguration sslConfig) {
82+
return SslSocketManager.getSocketManager(
83+
sslConfig, SslSocketManagerTest.HOST_NAME, HOST_PORT, 0, 0, true, LAYOUT, 8192, null);
84+
}
85+
86+
static String[] hostNamesAllowedForSniServerName() {
87+
return new String[] {
88+
"apache.org",
89+
"a.b",
90+
"sub.domain.co.uk",
91+
"xn--bcher-kva.example", // valid IDN/punycode
92+
"my-host-123.example",
93+
"a1234567890.b1234567890.c1234567890", // numeric/alpha labels
94+
"EXAMPLE.COM", // case-insensitive
95+
"service--node.example", // double hyphens allowed
96+
Strings.repeat("l", 63) + ".com", // 63-char label, valid
97+
"a.b.c.d.e.f.g.h.i.j", // many labels, short each
98+
};
99+
}
100+
101+
@ParameterizedTest
102+
@MethodSource("hostNamesAllowedForSniServerName")
103+
void createSniHostName_should_work_with_allowed_input(String hostName) {
104+
assertThat(createSniHostName(hostName)).isNotNull();
105+
}
106+
107+
static String[] hostNamesNotAllowedForSniServerName() {
108+
return new String[] {
109+
// Invalid because IP literals not allowed
110+
"192.168.1.1", // IPv4 literal
111+
"2001:db8::1", // IPv6 literal
112+
"[2001:db8::1]", // IPv6 URL literal form
113+
// Invalid because illegal characters
114+
"exa_mple.com", // underscore not allowed
115+
"host name.com", // space not allowed
116+
"example!.com", // symbol not allowed
117+
// Invalid because label syntax violations
118+
"-example.com", // leading hyphen
119+
"example-.com", // trailing hyphen
120+
".example.com", // leading dot / empty label
121+
// Invalid because label too long
122+
Strings.repeat("l", 64) + ".com",
123+
};
124+
}
125+
126+
@ParameterizedTest
127+
@MethodSource("hostNamesNotAllowedForSniServerName")
128+
void createSniHostName_should_fail_with_not_allowed_input(String hostName) {
129+
assertThat(createSniHostName(hostName)).isNull();
130+
}
131+
}

‎log4j-core-test/src/test/java/org/apache/logging/log4j/core/net/ssl/SslSocketManagerTest.java‎

Lines changed: 0 additions & 45 deletions
This file was deleted.

‎log4j-core/src/main/java/org/apache/logging/log4j/core/net/SslSocketManager.java‎

Lines changed: 84 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -30,19 +30,22 @@
3030
import java.util.Objects;
3131
import java.util.stream.Collectors;
3232
import java.util.stream.Stream;
33+
import javax.net.ssl.SNIHostName;
3334
import javax.net.ssl.SSLContext;
35+
import javax.net.ssl.SSLParameters;
3436
import javax.net.ssl.SSLSocket;
3537
import javax.net.ssl.SSLSocketFactory;
3638
import org.apache.logging.log4j.core.Layout;
3739
import org.apache.logging.log4j.core.net.ssl.SslConfiguration;
3840
import org.apache.logging.log4j.util.Strings;
41+
import org.jspecify.annotations.Nullable;
3942

40-
/**
41-
*
42-
*/
4343
public class SslSocketManager extends TcpSocketManager {
44+
4445
public static final int DEFAULT_PORT = 6514;
46+
4547
private static final SslSocketManagerFactory FACTORY = new SslSocketManagerFactory();
48+
4649
private final SslConfiguration sslConfig;
4750

4851
/**
@@ -285,10 +288,7 @@ private static String createSslConfigurationId(final SslConfiguration sslConfig)
285288

286289
@Override
287290
protected Socket createSocket(final InetSocketAddress socketAddress) throws IOException {
288-
final SSLSocketFactory socketFactory = createSslSocketFactory(sslConfig);
289-
final Socket newSocket = socketFactory.createSocket();
290-
newSocket.connect(socketAddress, getConnectTimeoutMillis());
291-
return newSocket;
291+
return createSocket(getHost(), socketAddress, getConnectTimeoutMillis(), sslConfig, getSocketOptions());
292292
}
293293

294294
private static SSLSocketFactory createSslSocketFactory(final SslConfiguration sslConf) {
@@ -333,32 +333,102 @@ Socket createSocket(final SslFactoryData data) throws IOException {
333333
for (InetSocketAddress socketAddress : socketAddresses) {
334334
try {
335335
return SslSocketManager.createSocket(
336-
socketAddress, data.connectTimeoutMillis, data.sslConfiguration, data.socketOptions);
336+
data.host,
337+
socketAddress,
338+
data.connectTimeoutMillis,
339+
data.sslConfiguration,
340+
data.socketOptions);
337341
} catch (IOException ex) {
338-
ioe = ex;
342+
final String message = String.format(
343+
"failed create a socket to `%s:%s` that is resolved to address `%s`",
344+
data.host, data.port, socketAddress);
345+
final IOException newEx = new IOException(message, ex);
346+
if (ioe == null) {
347+
ioe = newEx;
348+
} else {
349+
ioe.addSuppressed(newEx);
350+
}
339351
}
340352
}
341353
throw new IOException(errorMessage(data, socketAddresses), ioe);
342354
}
343355
}
344356

345-
static Socket createSocket(
357+
private static Socket createSocket(
358+
final String hostName,
346359
final InetSocketAddress socketAddress,
347360
final int connectTimeoutMillis,
348361
final SslConfiguration sslConfiguration,
349362
final SocketOptions socketOptions)
350363
throws IOException {
364+
365+
// Create the `SSLSocket`
351366
final SSLSocketFactory socketFactory = createSslSocketFactory(sslConfiguration);
352367
final SSLSocket socket = (SSLSocket) socketFactory.createSocket();
368+
369+
// Apply socket options before `connect()`
353370
if (socketOptions != null) {
354-
// Not sure which options must be applied before or after the connect() call.
355371
socketOptions.apply(socket);
356372
}
373+
374+
// Connect the socket
357375
socket.connect(socketAddress, connectTimeoutMillis);
358-
if (socketOptions != null) {
359-
// Not sure which options must be applied before or after the connect() call.
360-
socketOptions.apply(socket);
376+
377+
// Verify the host name
378+
if (sslConfiguration.isVerifyHostName()) {
379+
// Allowed endpoint identification algorithms: HTTPS and LDAPS.
380+
// https://docs.oracle.com/en/java/javase/17/docs/specs/security/standard-names.html#endpoint-identification-algorithms
381+
final SSLParameters sslParameters = socket.getSSLParameters();
382+
sslParameters.setEndpointIdentificationAlgorithm("HTTPS");
383+
final SNIHostName serverName = createSniHostName(hostName);
384+
if (serverName != null) {
385+
sslParameters.setServerNames(Collections.singletonList(serverName));
386+
}
387+
socket.setSSLParameters(sslParameters);
361388
}
389+
390+
// Force the handshake right after `connect()` instead of waiting for read/write to trigger it indirectly at a
391+
// later stage
392+
socket.startHandshake();
393+
362394
return socket;
363395
}
396+
397+
/**
398+
* {@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}
399+
*
400+
* @param hostName a host name
401+
*
402+
* @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>
403+
* @see <a href="https://www.rfc-editor.org/rfc/rfc1035.html">Domain Names - Implementation and Specification (RFC 1035)</a>
404+
*/
405+
@Nullable
406+
static SNIHostName createSniHostName(String hostName) {
407+
// The actual check should be
408+
//
409+
// !isIPv4(h) && !isIPv6(h) && isValidHostName(h)
410+
//
411+
// Though we translate this into
412+
//
413+
// !h.matches("\d+[.]\d+[.]\d+[.]\d+") && new SNIServerName(h)
414+
//
415+
// This simplification is possible because
416+
//
417+
// - The `\d+[.]\d+[.]\d+[.]\d+` is sufficient to eliminate IPv4 addresses.
418+
// Any sequence of four numeric labels (e.g., `1234.2345.3456.4567`) is not a valid host name.
419+
// Hence, false positives are not a problem, they would be eliminated by `isValidHostName()` anyway.
420+
//
421+
// - `SNIServerName::new` throws an exception on invalid host names.
422+
// This check is performed using `IDN.toASCII(hostName, IDN.USE_STD3_ASCII_RULES)`.
423+
// IPv6 literals don't qualify as a valid host name by `IDN::toASCII`.
424+
// This assumption on `IDN` is unlikely to change in the foreseeable future.
425+
if (!hostName.matches("\\d+[.]\\d+[.]\\d+[.]\\d+")) {
426+
try {
427+
return new SNIHostName(hostName);
428+
} catch (IllegalArgumentException ignored) {
429+
// Do nothing
430+
}
431+
}
432+
return null;
433+
}
364434
}

‎log4j-core/src/main/java/org/apache/logging/log4j/core/net/ssl/SslConfiguration.java‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -132,8 +132,8 @@ private static SSLContext createSslContext(
132132
@Nullable final TrustStoreConfiguration trustStoreConfig) {
133133
try {
134134
final SSLContext sslContext = SSLContext.getInstance(protocol);
135-
final KeyManager[] keyManagers = loadKeyManagers(keyStoreConfig);
136-
final TrustManager[] trustManagers = loadTrustManagers(trustStoreConfig);
135+
@Nullable final KeyManager[] keyManagers = loadKeyManagers(keyStoreConfig);
136+
@Nullable final TrustManager[] trustManagers = loadTrustManagers(trustStoreConfig);
137137
sslContext.init(keyManagers, trustManagers, null);
138138
return sslContext;
139139
} catch (final Exception error) {
@@ -144,9 +144,11 @@ private static SSLContext createSslContext(
144144
}
145145
}
146146

147+
@Nullable
148+
@NullUnmarked
147149
private static KeyManager[] loadKeyManagers(@Nullable final KeyStoreConfiguration config) throws Exception {
148150
if (config == null) {
149-
return new KeyManager[0];
151+
return null;
150152
}
151153
final KeyManagerFactory factory = KeyManagerFactory.getInstance(config.getKeyManagerFactoryAlgorithm());
152154
final char[] password = config.getPasswordAsCharArray();
@@ -158,9 +160,11 @@ private static KeyManager[] loadKeyManagers(@Nullable final KeyStoreConfiguratio
158160
return factory.getKeyManagers();
159161
}
160162

163+
@Nullable
164+
@NullUnmarked
161165
private static TrustManager[] loadTrustManagers(@Nullable final TrustStoreConfiguration config) throws Exception {
162166
if (config == null) {
163-
return new TrustManager[0];
167+
return null;
164168
}
165169
final TrustManagerFactory factory = TrustManagerFactory.getInstance(config.getTrustManagerFactoryAlgorithm());
166170
factory.init(config.getKeyStore());

‎src/changelog/.2.x.x/4002_fix_SslSocketAppender_verifyHostName.xml‎

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

‎src/site/antora/modules/ROOT/pages/manual/appenders/network.adoc‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,9 @@ https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/JSSERefGuid
6767
If `true`, the host name in X509 certificate will be compared to the requested host name.
6868
In the case of a mismatch, the connection will fail.
6969
70+
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].
71+
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].
72+
7073
See also
7174
xref:manual/systemproperties.adoc#log4j2.sslVerifyHostName[`log4j2.sslVerifyHostName`].
7275
|===

0 commit comments

Comments
 (0)