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 a80fd2272c..23f76e0a5a 100644 --- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java +++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java @@ -46,6 +46,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; @@ -160,6 +161,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; @@ -339,7 +341,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 @@ -1531,14 +1535,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; @@ -1552,7 +1559,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); @@ -1579,17 +1586,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); } @@ -1656,6 +1682,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(); @@ -1711,6 +1741,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 005f07bbeb..e9388833d1 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java @@ -129,6 +129,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 { @@ -484,6 +485,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"); @@ -1238,7 +1320,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())