From 99fc0e486da29790a85ab63a824eb71279a511d7 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Thu, 28 Jan 2021 11:00:51 +0100 Subject: [PATCH] Correctly filter out TLSv1.3 ciphers if TLSv1.3 is not enabled (#10919) Motivation: We didnt correctly filter out TLSv1.3 ciphers when TLSv1.3 is not enabled. Modifications: - Filter out ciphers that are not supported due the selected TLS version - Add unit test Result: Fixes https://github.com/netty/netty/issues/10911 Co-authored-by: Bryce Anderson --- .../ssl/ReferenceCountedOpenSslEngine.java | 43 ++++++++-- .../io/netty/handler/ssl/SSLEngineTest.java | 84 ++++++++++++++++++- 2 files changed, 121 insertions(+), 6 deletions(-) diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java index f7fc36d020..7dfb118ac0 100644 --- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java +++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java @@ -53,6 +53,7 @@ import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -154,6 +155,7 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc private volatile boolean destroyed; private volatile String applicationProtocol; private volatile boolean needTask; + private String[] explicitlyEnabledProtocols; // Reference Counting private final ResourceLeakTracker leak; @@ -329,7 +331,9 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc setClientAuth(clientMode ? ClientAuth.NONE : context.clientAuth); if (context.protocols != null) { - setEnabledProtocols(context.protocols); + setEnabledProtocols0(context.protocols, true); + } else { + this.explicitlyEnabledProtocols = getEnabledProtocols(); } // Use SNI if peerHost was specified and a valid hostname @@ -1517,14 +1521,17 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc public final String[] getEnabledCipherSuites() { final String[] extraCiphers; final String[] enabled; + final boolean tls13Enabled; synchronized (this) { if (!isDestroyed()) { enabled = SSL.getCiphers(ssl); int opts = SSL.getOptions(ssl); if (isProtocolEnabled(opts, SSL.SSL_OP_NO_TLSv1_3, PROTOCOL_TLS_V1_3)) { extraCiphers = OpenSsl.EXTRA_SUPPORTED_TLS_1_3_CIPHERS; + tls13Enabled = true; } else { extraCiphers = EmptyArrays.EMPTY_STRINGS; + tls13Enabled = false; } } else { return EmptyArrays.EMPTY_STRINGS; @@ -1538,7 +1545,7 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc for (int i = 0; i < enabled.length; i++) { String mapped = toJavaCipherSuite(enabled[i]); final String cipher = mapped == null ? enabled[i] : mapped; - if (!OpenSsl.isTlsv13Supported() && SslUtils.isTLSv13Cipher(cipher)) { + if ((!tls13Enabled || !OpenSsl.isTlsv13Supported()) && SslUtils.isTLSv13Cipher(cipher)) { continue; } enabledSet.add(cipher); @@ -1565,17 +1572,36 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc } synchronized (this) { if (!isDestroyed()) { - // TODO: Should we also adjust the protocols based on if there are any ciphers left that can be used - // for TLSv1.3 or for previor SSL/TLS versions ? try { // Set non TLSv1.3 ciphers. SSL.setCipherSuites(ssl, cipherSuiteSpec, false); - if (OpenSsl.isTlsv13Supported()) { // Set TLSv1.3 ciphers. SSL.setCipherSuites(ssl, cipherSuiteSpecTLSv13, true); } + // We also need to update the enabled protocols to ensure we disable the protocol if there are + // no compatible ciphers left. + Set protocols = new HashSet(explicitlyEnabledProtocols.length); + Collections.addAll(protocols, explicitlyEnabledProtocols); + + // We have no ciphers that are compatible with none-TLSv1.3, let us explicit disable all other + // protocols. + if (cipherSuiteSpec.isEmpty()) { + protocols.remove(PROTOCOL_TLS_V1); + protocols.remove(PROTOCOL_TLS_V1_1); + protocols.remove(PROTOCOL_TLS_V1_2); + protocols.remove(PROTOCOL_SSL_V3); + protocols.remove(PROTOCOL_SSL_V2); + protocols.remove(PROTOCOL_SSL_V2_HELLO); + } + // We have no ciphers that are compatible with TLSv1.3, let us explicit disable it. + if (cipherSuiteSpecTLSv13.isEmpty()) { + protocols.remove(PROTOCOL_TLS_V1_3); + } + // Update the protocols but not cache the value. We only cache when we call it from the user + // code or when we construct the engine. + setEnabledProtocols0(protocols.toArray(EmptyArrays.EMPTY_STRINGS), false); } catch (Exception e) { throw new IllegalStateException("failed to enable cipher suites: " + cipherSuiteSpec, e); } @@ -1642,6 +1668,10 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc */ @Override public final void setEnabledProtocols(String[] protocols) { + setEnabledProtocols0(protocols, true); + } + + private void setEnabledProtocols0(String[] protocols, boolean cache) { if (protocols == null) { // This is correct from the API docs throw new IllegalArgumentException(); @@ -1704,6 +1734,9 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc } } synchronized (this) { + if (cache) { + this.explicitlyEnabledProtocols = protocols; + } if (!isDestroyed()) { // Clear out options which disable protocols SSL.clearOptions(ssl, SSL.SSL_OP_NO_SSLv2 | SSL.SSL_OP_NO_SSLv3 | SSL.SSL_OP_NO_TLSv1 | diff --git a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java index b3f2000102..7260598080 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java @@ -139,6 +139,7 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.junit.Assume.assumeTrue; import static org.mockito.Mockito.verify; public abstract class SSLEngineTest { @@ -494,6 +495,87 @@ public abstract class SSLEngineTest { } } + @Test + public void testSetSupportedCiphers() throws Exception { + if (protocolCipherCombo != ProtocolCipherCombo.tlsv12()) { + return; + } + SelfSignedCertificate cert = new SelfSignedCertificate(); + serverSslCtx = wrapContext(SslContextBuilder.forServer(cert.key(), cert.cert()) + .protocols(protocols()) + .ciphers(ciphers()) + .sslProvider(sslServerProvider()).build()); + final SSLEngine serverEngine = + wrapEngine(serverSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT)); + clientSslCtx = wrapContext(SslContextBuilder.forClient() + .trustManager(cert.certificate()) + .protocols(protocols()) + .ciphers(ciphers()) + .sslProvider(sslClientProvider()).build()); + final SSLEngine clientEngine = + wrapEngine(clientSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT)); + + final String[] enabledCiphers = new String[]{ ciphers().get(0) }; + + try { + clientEngine.setEnabledCipherSuites(enabledCiphers); + serverEngine.setEnabledCipherSuites(enabledCiphers); + + assertArrayEquals(enabledCiphers, clientEngine.getEnabledCipherSuites()); + assertArrayEquals(enabledCiphers, serverEngine.getEnabledCipherSuites()); + } finally { + cleanupClientSslEngine(clientEngine); + cleanupServerSslEngine(serverEngine); + cert.delete(); + } + } + + @Test(expected = SSLHandshakeException.class) + public void testIncompatibleCiphers() throws Exception { + assumeTrue(SslProvider.isTlsv13Supported(sslClientProvider())); + + SelfSignedCertificate ssc = new SelfSignedCertificate(); + // Select a mandatory cipher from the TLSv1.2 RFC https://www.ietf.org/rfc/rfc5246.txt so handshakes won't fail + // due to no shared/supported cipher. + clientSslCtx = wrapContext(SslContextBuilder.forClient() + .trustManager(InsecureTrustManagerFactory.INSTANCE) + .protocols(PROTOCOL_TLS_V1_3, PROTOCOL_TLS_V1_2, PROTOCOL_TLS_V1) + .sslContextProvider(clientSslContextProvider()) + .sslProvider(sslClientProvider()) + .build()); + + serverSslCtx = wrapContext(SslContextBuilder.forServer(ssc.certificate(), ssc.privateKey()) + .protocols(PROTOCOL_TLS_V1_3, PROTOCOL_TLS_V1_2, PROTOCOL_TLS_V1) + .sslContextProvider(serverSslContextProvider()) + .sslProvider(sslServerProvider()) + .build()); + SSLEngine clientEngine = null; + SSLEngine serverEngine = null; + try { + clientEngine = wrapEngine(clientSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT)); + serverEngine = wrapEngine(serverSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT)); + + // Set the server to only support a single TLSv1.2 cipher + final String serverCipher = "TLS_RSA_WITH_AES_128_CBC_SHA"; + serverEngine.setEnabledCipherSuites(new String[] { serverCipher }); + + // Set the client to only support a single TLSv1.3 cipher + final String clientCipher = "TLS_AES_256_GCM_SHA384"; + clientEngine.setEnabledCipherSuites(new String[] { clientCipher }); + handshake(clientEngine, serverEngine); + + // We shouldn't get here as the handshake should throw a SSLHandshakeException. If + // we do, throw our own exception that is more informative as to what has happened. + fail("Unexpected handshake success. Negotiated TLS version: " + + clientEngine.getSession().getProtocol() + + ", cipher suite negotiated: " + clientEngine.getSession().getCipherSuite()); + } finally { + cleanupClientSslEngine(clientEngine); + cleanupServerSslEngine(serverEngine); + ssc.delete(); + } + } + @Test public void testMutualAuthDiffCerts() throws Exception { File serverKeyFile = ResourcesUtil.getFile(getClass(), "test_encrypted.pem"); @@ -1251,7 +1333,7 @@ public abstract class SSLEngineTest { @Test(timeout = 30000) public void clientInitiatedRenegotiationWithFatalAlertDoesNotInfiniteLoopServer() throws CertificateException, SSLException, InterruptedException, ExecutionException { - Assume.assumeTrue(PlatformDependent.javaVersion() >= 11); + assumeTrue(PlatformDependent.javaVersion() >= 11); final SelfSignedCertificate ssc = new SelfSignedCertificate(); serverSslCtx = wrapContext(SslContextBuilder.forServer(ssc.certificate(), ssc.privateKey()) .sslProvider(sslServerProvider())