From bc00be7e98d9b08cff4cc398f5244bd8a7f2d920 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Sat, 26 Sep 2020 08:56:16 +0200 Subject: [PATCH] =?UTF-8?q?Correctly=20report=20back=20when=20we=20fail=20?= =?UTF-8?q?to=20select=20the=20key=20material=20and=20ens=E2=80=A6=20(#106?= =?UTF-8?q?10)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Motivation: We need to let openssl know that we failed to find the key material so it will produce an alert for the remote peer to consume. Beside this we also need to ensure we wrap(...) until we produced everything as otherwise the remote peer may see partial data when an alert is produced in multiple steps. Modifications: - Correctly throw if we could not find the keymaterial - wrap until we produced everything - Add test Result: Correctly handle the case when key material could not be found --- .../ssl/OpenSslKeyMaterialManager.java | 11 +- .../ReferenceCountedOpenSslServerContext.java | 6 +- .../java/io/netty/handler/ssl/SslHandler.java | 7 + .../io/netty/handler/ssl/SslHandlerTest.java | 133 +++++++++++++++++- 4 files changed, 151 insertions(+), 6 deletions(-) diff --git a/handler/src/main/java/io/netty/handler/ssl/OpenSslKeyMaterialManager.java b/handler/src/main/java/io/netty/handler/ssl/OpenSslKeyMaterialManager.java index d3f68e848b..7ba69bfd4d 100644 --- a/handler/src/main/java/io/netty/handler/ssl/OpenSslKeyMaterialManager.java +++ b/handler/src/main/java/io/netty/handler/ssl/OpenSslKeyMaterialManager.java @@ -16,11 +16,13 @@ package io.netty.handler.ssl; import javax.net.ssl.SSLException; +import javax.net.ssl.SSLHandshakeException; import javax.net.ssl.X509ExtendedKeyManager; import javax.net.ssl.X509KeyManager; import javax.security.auth.x500.X500Principal; import java.security.PrivateKey; import java.security.cert.X509Certificate; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; @@ -68,8 +70,10 @@ final class OpenSslKeyMaterialManager { void setKeyMaterialServerSide(ReferenceCountedOpenSslEngine engine) throws SSLException { String[] authMethods = engine.authMethods(); if (authMethods.length == 0) { - return; + throw new SSLHandshakeException("Unable to find key material"); } + + boolean matched = false; // authMethods may contain duplicates but call chooseServerAlias(...) may be expensive. So let's ensure // we filter out duplicates. Set authMethodsSet = new LinkedHashSet(authMethods.length); @@ -83,9 +87,14 @@ final class OpenSslKeyMaterialManager { if (!setKeyMaterial(engine, alias)) { return; } + matched = true; } } } + if (!matched) { + throw new SSLHandshakeException("Unable to find key material for auth method(s): " + + Arrays.toString(authMethods)); + } } void setKeyMaterialClientSide(ReferenceCountedOpenSslEngine engine, String[] keyTypes, diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslServerContext.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslServerContext.java index bac027a3b4..2638cee90b 100644 --- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslServerContext.java +++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslServerContext.java @@ -228,8 +228,12 @@ public final class ReferenceCountedOpenSslServerContext extends ReferenceCounted // OpenJDK SSLEngineImpl does. keyManagerHolder.setKeyMaterialServerSide(engine); } catch (Throwable cause) { - logger.debug("Failed to set the server-side key material", cause); engine.initHandshakeException(cause); + + if (cause instanceof Exception) { + throw (Exception) cause; + } + throw new SSLException(cause); } } } diff --git a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java index de101967ba..55a41a4f51 100644 --- a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java +++ b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java @@ -902,6 +902,13 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH b = null; } finishWrap(ctx, b, p, inUnwrap, false); + // If we are expected to wrap again and we produced some data we need to ensure there + // is something in the queue to process as otherwise we will not try again before there + // was more added. Failing to do so may fail to produce an alert that can be + // consumed by the remote peer. + if (result.bytesProduced() > 0 && pendingUnencryptedWrites.isEmpty()) { + pendingUnencryptedWrites.add(Unpooled.EMPTY_BUFFER); + } break; } case NEED_UNWRAP: diff --git a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java index 7b2e0795a8..7dd5fa8693 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java @@ -62,6 +62,7 @@ import io.netty.util.internal.EmptyArrays; import io.netty.util.internal.PlatformDependent; import org.hamcrest.CoreMatchers; import org.hamcrest.Matchers; +import org.junit.Assume; import org.junit.Test; import java.net.InetSocketAddress; @@ -70,6 +71,7 @@ import java.nio.channels.ClosedChannelException; import java.security.NoSuchAlgorithmException; import java.security.cert.CertificateException; import java.security.cert.X509Certificate; +import java.util.Collections; import java.util.List; import java.util.Queue; import java.util.concurrent.BlockingQueue; @@ -91,10 +93,7 @@ import javax.net.ssl.SSLProtocolException; import javax.net.ssl.X509ExtendedTrustManager; import static io.netty.buffer.Unpooled.wrappedBuffer; -import static org.hamcrest.CoreMatchers.containsString; -import static org.hamcrest.CoreMatchers.instanceOf; -import static org.hamcrest.CoreMatchers.is; -import static org.hamcrest.CoreMatchers.nullValue; +import static org.hamcrest.CoreMatchers.*; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; @@ -1357,4 +1356,130 @@ public class SslHandlerTest { group.shutdownGracefully(); } } + + @Test + public void testHandshakeFailureCipherMissmatchTLSv12Jdk() throws Exception { + testHandshakeFailureCipherMissmatch(SslProvider.JDK, false); + } + + @Test + public void testHandshakeFailureCipherMissmatchTLSv13Jdk() throws Exception { + Assume.assumeTrue(SslProvider.isTlsv13Supported(SslProvider.JDK)); + testHandshakeFailureCipherMissmatch(SslProvider.JDK, true); + } + + @Test + public void testHandshakeFailureCipherMissmatchTLSv12OpenSsl() throws Exception { + Assume.assumeTrue(OpenSsl.isAvailable()); + testHandshakeFailureCipherMissmatch(SslProvider.OPENSSL, false); + } + + @Test + public void testHandshakeFailureCipherMissmatchTLSv13OpenSsl() throws Exception { + Assume.assumeTrue(OpenSsl.isAvailable()); + Assume.assumeTrue(SslProvider.isTlsv13Supported(SslProvider.OPENSSL)); + Assume.assumeFalse("BoringSSL does not support setting ciphers for TLSv1.3 explicit", OpenSsl.isBoringSSL()); + testHandshakeFailureCipherMissmatch(SslProvider.OPENSSL, true); + } + + private static void testHandshakeFailureCipherMissmatch(SslProvider provider, boolean tls13) throws Exception { + final String clientCipher; + final String serverCipher; + final String protocol; + + if (tls13) { + clientCipher = "TLS_AES_128_GCM_SHA256"; + serverCipher = "TLS_AES_256_GCM_SHA384"; + protocol = SslUtils.PROTOCOL_TLS_V1_3; + } else { + clientCipher = "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256"; + serverCipher = "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384"; + protocol = SslUtils.PROTOCOL_TLS_V1_2; + } + final SslContext sslClientCtx = SslContextBuilder.forClient() + .trustManager(InsecureTrustManagerFactory.INSTANCE) + .protocols(protocol) + .ciphers(Collections.singleton(clientCipher)) + .sslProvider(provider).build(); + + final SelfSignedCertificate cert = new SelfSignedCertificate(); + final SslContext sslServerCtx = SslContextBuilder.forServer(cert.key(), cert.cert()) + .protocols(protocol) + .ciphers(Collections.singleton(serverCipher)) + .sslProvider(provider).build(); + + EventLoopGroup group = new NioEventLoopGroup(); + Channel sc = null; + Channel cc = null; + final SslHandler clientSslHandler = sslClientCtx.newHandler(UnpooledByteBufAllocator.DEFAULT); + final SslHandler serverSslHandler = sslServerCtx.newHandler(UnpooledByteBufAllocator.DEFAULT); + + class SslEventHandler extends ChannelInboundHandlerAdapter { + private final AtomicReference ref; + + SslEventHandler(AtomicReference ref) { + this.ref = ref; + } + + @Override + public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exception { + if (evt instanceof SslHandshakeCompletionEvent) { + ref.set((SslHandshakeCompletionEvent) evt); + } + super.userEventTriggered(ctx, evt); + } + } + final AtomicReference clientEvent = + new AtomicReference(); + final AtomicReference serverEvent = + new AtomicReference(); + try { + sc = new ServerBootstrap() + .group(group) + .channel(NioServerSocketChannel.class) + .childHandler(new ChannelInitializer() { + @Override + protected void initChannel(Channel ch) throws Exception { + ch.pipeline().addLast(serverSslHandler); + ch.pipeline().addLast(new SslEventHandler(serverEvent)); + } + }) + .bind(new InetSocketAddress(0)).syncUninterruptibly().channel(); + + ChannelFuture future = new Bootstrap() + .group(group) + .channel(NioSocketChannel.class) + .handler(new ChannelInitializer() { + @Override + protected void initChannel(Channel ch) { + ch.pipeline().addLast(clientSslHandler); + ch.pipeline().addLast(new SslEventHandler(clientEvent)); + } + }).connect(sc.localAddress()); + cc = future.syncUninterruptibly().channel(); + + Throwable clientCause = clientSslHandler.handshakeFuture().await().cause(); + assertThat(clientCause, CoreMatchers.instanceOf(SSLException.class)); + assertThat(clientCause.getCause(), not(CoreMatchers.instanceOf(ClosedChannelException.class))); + Throwable serverCause = serverSslHandler.handshakeFuture().await().cause(); + assertThat(serverCause, CoreMatchers.instanceOf(SSLException.class)); + assertThat(serverCause.getCause(), not(CoreMatchers.instanceOf(ClosedChannelException.class))); + cc.close().syncUninterruptibly(); + sc.close().syncUninterruptibly(); + + Throwable eventClientCause = clientEvent.get().cause(); + assertThat(eventClientCause, CoreMatchers.instanceOf(SSLException.class)); + assertThat(eventClientCause.getCause(), + not(CoreMatchers.instanceOf(ClosedChannelException.class))); + Throwable serverEventCause = serverEvent.get().cause(); + + assertThat(serverEventCause, CoreMatchers.instanceOf(SSLException.class)); + assertThat(serverEventCause.getCause(), + not(CoreMatchers.instanceOf(ClosedChannelException.class))); + } finally { + group.shutdownGracefully(); + ReferenceCountUtil.release(sslClientCtx); + } + } + }