From fffc1ba872dff0d67a88cb5e2b56fdc548916248 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Wed, 16 Nov 2016 15:47:07 +0100 Subject: [PATCH] Ensure alert is send when SSLException happens during calling SslHandler.unwrap(...) Motivation: When the SslHandler.unwrap(...) (which is called via decode(...)) method did produce an SSLException it was possible that the produced alert was not send to the remote peer. This could lead to staling connections if the remote peer did wait for such an alert and the connection was not closed. Modifications: - Ensure we try to flush any pending data when a SSLException is thrown during unwrapping. - Fix SniHandlerTest to correct test this - Add explicit new test in SslHandlerTest to verify behaviour with all SslProviders. Result: The alert is correctly send to the remote peer in all cases. --- .../java/io/netty/handler/ssl/SslHandler.java | 101 +++++++++------ .../io/netty/handler/ssl/SniHandlerTest.java | 6 +- .../io/netty/handler/ssl/SslHandlerTest.java | 120 ++++++++++++++++++ 3 files changed, 181 insertions(+), 46 deletions(-) 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 c1ee34c4b1..edcd0ec2ca 100644 --- a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java +++ b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java @@ -523,9 +523,19 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH if (startTls && !sentFirstMessage) { sentFirstMessage = true; pendingUnencryptedWrites.removeAndWriteAll(); - ctx.flush(); + forceFlush(ctx); return; } + + try { + wrapAndFlush(ctx); + } catch (Throwable cause) { + setHandshakeFailure(ctx, cause); + PlatformDependent.throwException(cause); + } + } + + private void wrapAndFlush(ChannelHandlerContext ctx) throws SSLException { if (pendingUnencryptedWrites.isEmpty()) { // It's important to NOT use a voidPromise here as the user // may want to add a ChannelFutureListener to the ChannelPromise later. @@ -538,18 +548,14 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH } try { wrap(ctx, false); - } catch (Throwable cause) { - // Fail pending writes. - pendingUnencryptedWrites.removeAndFailAll(cause); - - PlatformDependent.throwException(cause); } finally { // We may have 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(); + forceFlush(ctx); } } + // This method will not call setHandshakeFailure(...) ! private void wrap(ChannelHandlerContext ctx, boolean inUnwrap) throws SSLException { ByteBuf out = null; ChannelPromise promise = null; @@ -607,9 +613,6 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH } } } - } catch (SSLException e) { - setHandshakeFailure(ctx, e); - throw e; } finally { finishWrap(ctx, out, promise, inUnwrap, needUnwrap); } @@ -641,6 +644,7 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH } } + // This method will not call setHandshakeFailure(...) ! private void wrapNonAppData(ChannelHandlerContext ctx, boolean inUnwrap) throws SSLException { ByteBuf out = null; ByteBufAllocator alloc = ctx.alloc(); @@ -697,13 +701,6 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH break; } } - } catch (SSLException e) { - setHandshakeFailure(ctx, e); - - // We may have 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 - flushIfNeeded(ctx); - throw e; } finally { if (out != null) { out.release(); @@ -949,7 +946,21 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH in.skipBytes(totalLength); - firedChannelRead = unwrap(ctx, in, startOffset, totalLength) || firedChannelRead; + try { + firedChannelRead = unwrap(ctx, in, startOffset, totalLength) || firedChannelRead; + } catch (Throwable cause) { + try { + // We need to flush one time as there may be an alert that we should send to the remote peer because + // of the SSLException reported here. + wrapAndFlush(ctx); + } catch (SSLException ex) { + logger.debug("SSLException during trying to call SSLEngine.wrap(...)" + + " because of an previous SSLException, ignoring...", ex); + } finally { + setHandshakeFailure(ctx, cause); + } + PlatformDependent.throwException(cause); + } } if (nonSslRecord) { @@ -989,8 +1000,7 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH private void flushIfNeeded(ChannelHandlerContext ctx) { if (needsFlush) { - needsFlush = false; - ctx.flush(); + forceFlush(ctx); } } @@ -1109,9 +1119,6 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH if (notifyClosure) { sslCloseFuture.trySuccess(ctx.channel()); } - } catch (SSLException e) { - setHandshakeFailure(ctx, e); - throw e; } finally { if (decodeOut.isReadable()) { decoded = true; @@ -1235,26 +1242,30 @@ 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, boolean closeInbound) { - // Release all resources such as internal buffers that SSLEngine - // is managing. - engine.closeOutbound(); + try { + // Release all resources such as internal buffers that SSLEngine + // is managing. + engine.closeOutbound(); - if (closeInbound) { - try { - engine.closeInbound(); - } catch (SSLException e) { - // only log in debug mode as it most likely harmless and latest chrome still trigger - // this all the time. - // - // See https://github.com/netty/netty/issues/1340 - String msg = e.getMessage(); - if (msg == null || !msg.contains("possible truncation attack")) { - logger.debug("{} SSLEngine.closeInbound() raised an exception.", ctx.channel(), e); + if (closeInbound) { + try { + engine.closeInbound(); + } catch (SSLException e) { + // only log in debug mode as it most likely harmless and latest chrome still trigger + // this all the time. + // + // See https://github.com/netty/netty/issues/1340 + String msg = e.getMessage(); + if (msg == null || !msg.contains("possible truncation attack")) { + logger.debug("{} SSLEngine.closeInbound() raised an exception.", ctx.channel(), e); + } } } + notifyHandshakeFailure(cause); + } finally { + // Ensure we remove and fail all pending writes in all cases and so release memory quickly. + pendingUnencryptedWrites.removeAndFailAll(cause); } - notifyHandshakeFailure(cause); - pendingUnencryptedWrites.removeAndFailAll(cause); } private void notifyHandshakeFailure(Throwable cause) { @@ -1389,9 +1400,10 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH try { engine.beginHandshake(); wrapNonAppData(ctx, false); - ctx.flush(); - } catch (Exception e) { - notifyHandshakeFailure(e); + } catch (Throwable e) { + setHandshakeFailure(ctx, e); + } finally { + forceFlush(ctx); } // Set timeout if necessary. @@ -1419,6 +1431,11 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH }); } + private void forceFlush(ChannelHandlerContext ctx) { + needsFlush = false; + ctx.flush(); + } + /** * Issues an initial TLS handshake once connected when used in client-mode */ diff --git a/handler/src/test/java/io/netty/handler/ssl/SniHandlerTest.java b/handler/src/test/java/io/netty/handler/ssl/SniHandlerTest.java index 640c050cc0..b7a0e7c588 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SniHandlerTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SniHandlerTest.java @@ -168,10 +168,8 @@ public class SniHandlerTest { // expected } - // Just call finish and not assert the return value. This is because OpenSSL correct produces an alert - // while the JDK SSLEngineImpl does not atm. - // See https://github.com/netty/netty/issues/5874 - ch.finish(); + // This should produce an alert + assertTrue(ch.finish()); assertThat(handler.hostname(), is("chat4.leancloud.cn")); assertThat(handler.sslContext(), is(leanContext)); 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 7d50460312..fc565b442a 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java @@ -25,9 +25,13 @@ import static org.junit.Assert.*; import static org.junit.Assert.fail; import static org.junit.Assume.assumeTrue; +import javax.net.ssl.ManagerFactoryParameters; import javax.net.ssl.SSLContext; import javax.net.ssl.SSLEngine; +import javax.net.ssl.SSLException; import javax.net.ssl.SSLProtocolException; +import javax.net.ssl.TrustManager; +import javax.net.ssl.X509TrustManager; import io.netty.bootstrap.Bootstrap; import io.netty.bootstrap.ServerBootstrap; @@ -39,10 +43,13 @@ import io.netty.channel.socket.nio.NioServerSocketChannel; import io.netty.channel.socket.nio.NioSocketChannel; import io.netty.handler.codec.CodecException; import io.netty.handler.ssl.util.InsecureTrustManagerFactory; +import io.netty.handler.ssl.util.SimpleTrustManagerFactory; import io.netty.util.IllegalReferenceCountException; import io.netty.util.concurrent.Future; import io.netty.util.concurrent.FutureListener; import io.netty.util.concurrent.Promise; +import io.netty.util.internal.EmptyArrays; +import org.junit.Assume; import org.junit.Test; import io.netty.buffer.ByteBufAllocator; @@ -57,7 +64,11 @@ import io.netty.handler.ssl.util.SelfSignedCertificate; import io.netty.util.ReferenceCountUtil; import io.netty.util.ReferenceCounted; +import java.io.File; import java.net.InetSocketAddress; +import java.security.KeyStore; +import java.security.cert.CertificateException; +import java.security.cert.X509Certificate; public class SslHandlerTest { @@ -245,4 +256,113 @@ public class SslHandlerTest { } }; } + + @Test(timeout = 10000) + public void testAlertProducedAndSendJdk() throws Exception { + testAlertProducedAndSend(SslProvider.JDK); + } + + @Test(timeout = 10000) + public void testAlertProducedAndSendOpenSsl() throws Exception { + assumeTrue(OpenSsl.isAvailable()); + testAlertProducedAndSend(SslProvider.OPENSSL); + testAlertProducedAndSend(SslProvider.OPENSSL_REFCNT); + } + + private void testAlertProducedAndSend(SslProvider provider) throws Exception { + SelfSignedCertificate ssc = new SelfSignedCertificate(); + + final SslContext sslServerCtx = SslContextBuilder.forServer(ssc.certificate(), ssc.privateKey()) + .sslProvider(provider) + .trustManager(new SimpleTrustManagerFactory() { + @Override + protected void engineInit(KeyStore keyStore) { } + @Override + protected void engineInit(ManagerFactoryParameters managerFactoryParameters) { } + + @Override + protected TrustManager[] engineGetTrustManagers() { + return new TrustManager[] { new X509TrustManager() { + + @Override + public void checkClientTrusted(X509Certificate[] x509Certificates, String s) + throws CertificateException { + // Fail verification which should produce an alert that is send back to the client. + throw new CertificateException(); + } + + @Override + public void checkServerTrusted(X509Certificate[] x509Certificates, String s) { + // NOOP + } + + @Override + public X509Certificate[] getAcceptedIssuers() { + return EmptyArrays.EMPTY_X509_CERTIFICATES; + } + } }; + } + }).clientAuth(ClientAuth.REQUIRE).build(); + + final SslContext sslClientCtx = SslContextBuilder.forClient() + .trustManager(InsecureTrustManagerFactory.INSTANCE) + .keyManager(new File(getClass().getResource("test.crt").getFile()), + new File(getClass().getResource("test_unencrypted.pem").getFile())) + .sslProvider(provider).build(); + + NioEventLoopGroup group = new NioEventLoopGroup(); + Channel sc = null; + Channel cc = null; + try { + final Promise promise = group.next().newPromise(); + sc = new ServerBootstrap() + .group(group) + .channel(NioServerSocketChannel.class) + .childHandler(new ChannelInitializer() { + @Override + protected void initChannel(Channel ch) throws Exception { + ch.pipeline().addLast(sslServerCtx.newHandler(ch.alloc())); + ch.pipeline().addLast(new ChannelInboundHandlerAdapter() { + @Override + public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) { + // Just trigger a close + ctx.close(); + } + }); + } + }).bind(new InetSocketAddress(0)).syncUninterruptibly().channel(); + + cc = new Bootstrap() + .group(group) + .channel(NioSocketChannel.class) + .handler(new ChannelInitializer() { + @Override + protected void initChannel(Channel ch) throws Exception { + ch.pipeline().addLast(sslClientCtx.newHandler(ch.alloc())); + ch.pipeline().addLast(new ChannelInboundHandlerAdapter() { + @Override + public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) { + if (cause.getCause() instanceof SSLException) { + // We received the alert and so produce an SSLException. + promise.setSuccess(null); + } + } + }); + } + }).connect(sc.localAddress()).syncUninterruptibly().channel(); + + promise.syncUninterruptibly(); + } finally { + if (cc != null) { + cc.close().syncUninterruptibly(); + } + if (sc != null) { + sc.close().syncUninterruptibly(); + } + group.shutdownGracefully(); + + ReferenceCountUtil.release(sslServerCtx); + ReferenceCountUtil.release(sslClientCtx); + } + } }