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 53e265a895..18fbb16ff2 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SniHandlerTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SniHandlerTest.java @@ -180,10 +180,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 4c16eb5196..50cc09ebba 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java @@ -22,9 +22,13 @@ import static org.hamcrest.CoreMatchers.nullValue; import static org.junit.Assert.*; 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; @@ -36,10 +40,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; @@ -54,7 +61,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 { @@ -242,4 +253,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); + } + } }