From 268b901844c620337bc11ae82e99163a1781b96c Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Fri, 16 Feb 2018 17:25:47 +0100 Subject: [PATCH] SSL connection not closed properly after handshake failure Motivation: When SSL handshake fails, the connection should be closed. This is not true anymore after 978a46c. Modifications: - Ensure we always flush and close the channel on handshake failure. - Add testcase. Result: Fixes [#7724]. --- .../netty/handler/ssl/AbstractSniHandler.java | 2 +- .../java/io/netty/handler/ssl/SslHandler.java | 26 ++++----- .../java/io/netty/handler/ssl/SslUtils.java | 2 +- .../io/netty/handler/ssl/SslHandlerTest.java | 54 +++++++++++++++++++ 4 files changed, 69 insertions(+), 15 deletions(-) diff --git a/handler/src/main/java/io/netty/handler/ssl/AbstractSniHandler.java b/handler/src/main/java/io/netty/handler/ssl/AbstractSniHandler.java index ecbf8263c1..05fcaffdfc 100644 --- a/handler/src/main/java/io/netty/handler/ssl/AbstractSniHandler.java +++ b/handler/src/main/java/io/netty/handler/ssl/AbstractSniHandler.java @@ -81,7 +81,7 @@ public abstract class AbstractSniHandler extends ByteToMessageDecoder impleme "not an SSL/TLS record: " + ByteBufUtil.hexDump(in)); in.skipBytes(in.readableBytes()); ctx.fireUserEventTriggered(new SniCompletionEvent(e)); - SslUtils.notifyHandshakeFailure(ctx, e, true); + SslUtils.handleHandshakeFailure(ctx, e, true); throw e; } if (len == SslUtils.NOT_ENOUGH_DATA || 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 a5ebc363f3..1f22943caf 100644 --- a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java +++ b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java @@ -865,7 +865,7 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH /** * This method will not call - * {@link #setHandshakeFailure(ChannelHandlerContext, Throwable, boolean, boolean)} or + * {@link #setHandshakeFailure(ChannelHandlerContext, Throwable, boolean, boolean, boolean)} or * {@link #setHandshakeFailure(ChannelHandlerContext, Throwable)}. * @return {@code true} if this method ends on {@link SSLEngineResult.HandshakeStatus#NOT_HANDSHAKING}. */ @@ -1002,7 +1002,7 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH public void channelInactive(ChannelHandlerContext ctx) throws Exception { // Make sure to release SSLEngine, // and notify the handshake future if the connection has been closed during handshake. - setHandshakeFailure(ctx, CHANNEL_CLOSED, !outboundClosed, handshakeStarted); + setHandshakeFailure(ctx, CHANNEL_CLOSED, !outboundClosed, handshakeStarted, false); // Ensure we always notify the sslClosePromise as well notifyClosePromise(CHANNEL_CLOSED); @@ -1191,7 +1191,8 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH logger.debug("SSLException during trying to call SSLEngine.wrap(...)" + " because of an previous SSLException, ignoring...", ex); } finally { - setHandshakeFailure(ctx, cause, true, false); + // ensure we always flush and close the channel. + setHandshakeFailure(ctx, cause, true, false, true); } PlatformDependent.throwException(cause); } @@ -1498,13 +1499,14 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH * Notify all the handshake futures about the failure during the handshake. */ private void setHandshakeFailure(ChannelHandlerContext ctx, Throwable cause) { - setHandshakeFailure(ctx, cause, true, true); + setHandshakeFailure(ctx, cause, true, true, false); } /** * Notify all the handshake futures about the failure during the handshake. */ - private void setHandshakeFailure(ChannelHandlerContext ctx, Throwable cause, boolean closeInbound, boolean notify) { + private void setHandshakeFailure(ChannelHandlerContext ctx, Throwable cause, boolean closeInbound, + boolean notify, boolean alwaysFlushAndClose) { try { // Release all resources such as internal buffers that SSLEngine // is managing. @@ -1526,7 +1528,9 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH } } } - notifyHandshakeFailure(cause, notify); + if (handshakePromise.tryFailure(cause) || alwaysFlushAndClose) { + SslUtils.handleHandshakeFailure(ctx, cause, notify); + } } finally { // Ensure we remove and fail all pending writes in all cases and so release memory quickly. releaseAndFailAll(cause); @@ -1539,12 +1543,6 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH } } - private void notifyHandshakeFailure(Throwable cause, boolean notify) { - if (handshakePromise.tryFailure(cause)) { - SslUtils.notifyHandshakeFailure(ctx, cause, notify); - } - } - private void notifyClosePromise(Throwable cause) { if (cause == null) { if (sslClosePromise.trySuccess(ctx.channel())) { @@ -1725,7 +1723,9 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH return; } try { - notifyHandshakeFailure(HANDSHAKE_TIMED_OUT, true); + if (handshakePromise.tryFailure(HANDSHAKE_TIMED_OUT)) { + SslUtils.handleHandshakeFailure(ctx, HANDSHAKE_TIMED_OUT, true); + } } finally { releaseAndFailAll(HANDSHAKE_TIMED_OUT); } diff --git a/handler/src/main/java/io/netty/handler/ssl/SslUtils.java b/handler/src/main/java/io/netty/handler/ssl/SslUtils.java index 175238d41e..3dd40fdf37 100644 --- a/handler/src/main/java/io/netty/handler/ssl/SslUtils.java +++ b/handler/src/main/java/io/netty/handler/ssl/SslUtils.java @@ -310,7 +310,7 @@ final class SslUtils { return packetLength; } - static void notifyHandshakeFailure(ChannelHandlerContext ctx, Throwable cause, boolean notify) { + static void handleHandshakeFailure(ChannelHandlerContext ctx, Throwable cause, boolean notify) { // We have may haven written some parts of data before an exception was thrown so ensure we always flush. // See https://github.com/netty/netty/issues/3900#issuecomment-172481830 ctx.flush(); 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 5366de289f..f3c31ed6dd 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java @@ -599,4 +599,58 @@ public class SslHandlerTest { ReferenceCountUtil.release(sslClientCtx); } } + + @Test(timeout = 10000) + public void testCloseOnHandshakeFailure() throws Exception { + final SelfSignedCertificate ssc = new SelfSignedCertificate(); + + final SslContext sslServerCtx = SslContextBuilder.forServer(ssc.key(), ssc.cert()).build(); + final SslContext sslClientCtx = SslContextBuilder.forClient() + .trustManager(new SelfSignedCertificate().cert()) + .build(); + + EventLoopGroup group = new NioEventLoopGroup(1); + Channel sc = null; + Channel cc = null; + try { + LocalAddress address = new LocalAddress(getClass().getSimpleName() + ".testCloseOnHandshakeFailure"); + ServerBootstrap sb = new ServerBootstrap() + .group(group) + .channel(LocalServerChannel.class) + .childHandler(new ChannelInitializer() { + @Override + protected void initChannel(Channel ch) { + ch.pipeline().addLast(sslServerCtx.newHandler(ch.alloc())); + } + }); + sc = sb.bind(address).syncUninterruptibly().channel(); + + Bootstrap b = new Bootstrap() + .group(group) + .channel(LocalChannel.class) + .handler(new ChannelInitializer() { + @Override + protected void initChannel(Channel ch) { + ch.pipeline().addLast(sslClientCtx.newHandler(ch.alloc())); + } + }); + cc = b.connect(sc.localAddress()).syncUninterruptibly().channel(); + SslHandler handler = cc.pipeline().get(SslHandler.class); + handler.handshakeFuture().awaitUninterruptibly(); + assertFalse(handler.handshakeFuture().isSuccess()); + + cc.closeFuture().syncUninterruptibly(); + } finally { + if (cc != null) { + cc.close().syncUninterruptibly(); + } + if (sc != null) { + sc.close().syncUninterruptibly(); + } + group.shutdownGracefully(); + + ReferenceCountUtil.release(sslServerCtx); + ReferenceCountUtil.release(sslClientCtx); + } + } }