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.

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;
+            forceFlush(ctx);
-            ctx.flush();
         }
     }
 
@@ -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
+        try {
-        // is managing.
+            // Release all resources such as internal buffers that SSLEngine
-        engine.closeOutbound();
+            // is managing.
+            engine.closeOutbound();
 
-        if (closeInbound) {
+            if (closeInbound) {
-            try {
+                try {
-                engine.closeInbound();
+                    engine.closeInbound();
-            } catch (SSLException e) {
+                } catch (SSLException e) {
-                // only log in debug mode as it most likely harmless and latest chrome still trigger
+                    // only log in debug mode as it most likely harmless and latest chrome still trigger
-                // this all the time.
+                    // this all the time.
-                //
+                    //
-                // See https://github.com/netty/netty/issues/1340
+                    // See https://github.com/netty/netty/issues/1340
-                String msg = e.getMessage();
+                    String msg = e.getMessage();
-                if (msg == null || !msg.contains("possible truncation attack")) {
+                    if (msg == null || !msg.contains("possible truncation attack")) {
-                    logger.debug("{} SSLEngine.closeInbound() raised an exception.", ctx.channel(), e);
+                        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 (Throwable e) {
-        } catch (Exception e) {
+            setHandshakeFailure(ctx, e);
-            notifyHandshakeFailure(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
      */

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
+            // This should produce an alert
-            // while the JDK SSLEngineImpl does not atm.
+            assertTrue(ch.finish());
-            // See https://github.com/netty/netty/issues/5874
-            ch.finish();
 
             assertThat(handler.hostname(), is("chat4.leancloud.cn"));
             assertThat(handler.sslContext(), is(leanContext));

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<Void> promise = group.next().newPromise();
+            sc = new ServerBootstrap()
+                    .group(group)
+                    .channel(NioServerSocketChannel.class)
+                    .childHandler(new ChannelInitializer<Channel>() {
+                        @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<Channel>() {
+                        @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);
+        }
+    }
 }