From 58dc7f790231428edf2502ee4c4ca6ff29280832 Mon Sep 17 00:00:00 2001 From: Michael Bildner Date: Fri, 4 Sep 2015 09:23:06 -0400 Subject: [PATCH] Do not bother closing SSL enging inbound if the outbound has already been closed. Motivation: Invoking the javax.net.ssl.SSLEngine.closeInbound() method will send a fatal alert and invalidate the SSL session if a close_notify alert has not been received. From the javadoc: If the application initiated the closing process by calling closeOutbound(), under some circumstances it is not required that the initiator wait for the peer's corresponding close message. (See section 7.2.1 of the TLS specification (RFC 2246) for more information on waiting for closure alerts.) In such cases, this method need not be called. Always invoking the closeInbound() method without regard to whether or not the closeOutbound() method has been invoked could lead to invalidating perfectly valid SSL sessions. Modifications: Added an instance variable to track whether the SSLEngine.closeOutbound() method has been invoked. When the instance variable is true, the SSLEngine.closeInbound() method doesn't need to be invoked. Result: SSL sessions will not be invalidated if the outbound side has been closed but a close_notify alert hasn't been received. --- .../java/io/netty/handler/ssl/SslHandler.java | 35 ++- .../socket/SocketSslSessionReuseTest.java | 217 ++++++++++++++++++ 2 files changed, 241 insertions(+), 11 deletions(-) create mode 100644 testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketSslSessionReuseTest.java 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 85103f30c5..07f59e3fc5 100644 --- a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java +++ b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java @@ -236,6 +236,8 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH */ private boolean needsFlush; + private boolean outboundClosed; + private int packetLength; /** @@ -394,6 +396,7 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH ctx.executor().execute(new Runnable() { @Override public void run() { + SslHandler.this.outboundClosed = true; engine.closeOutbound(); try { write(ctx, Unpooled.EMPTY_BUFFER, future); @@ -702,7 +705,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); + setHandshakeFailure(ctx, CHANNEL_CLOSED, !outboundClosed); super.channelInactive(ctx); } @@ -1249,20 +1252,29 @@ 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); + } + + /** + * 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 { - 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); @@ -1287,6 +1299,7 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH return; } + outboundClosed = true; engine.closeOutbound(); ChannelPromise closeNotifyFuture = ctx.newPromise(); diff --git a/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketSslSessionReuseTest.java b/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketSslSessionReuseTest.java new file mode 100644 index 0000000000..9441ce15b2 --- /dev/null +++ b/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketSslSessionReuseTest.java @@ -0,0 +1,217 @@ +/* + * Copyright 2015 The Netty Project + * + * The Netty Project licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ +package io.netty.testsuite.transport.socket; + +import io.netty.bootstrap.Bootstrap; +import io.netty.bootstrap.ServerBootstrap; +import io.netty.buffer.ByteBuf; +import io.netty.buffer.ByteBufUtil; +import io.netty.buffer.Unpooled; +import io.netty.channel.Channel; +import io.netty.channel.ChannelHandlerContext; +import io.netty.channel.ChannelHandler.Sharable; +import io.netty.channel.ChannelInitializer; +import io.netty.channel.SimpleChannelInboundHandler; +import io.netty.channel.socket.SocketChannel; +import io.netty.handler.ssl.JdkSslClientContext; +import io.netty.handler.ssl.JdkSslContext; +import io.netty.handler.ssl.JdkSslServerContext; +import io.netty.handler.ssl.SslContext; +import io.netty.handler.ssl.SslHandler; +import io.netty.handler.ssl.util.SelfSignedCertificate; +import io.netty.util.internal.logging.InternalLogger; +import io.netty.util.internal.logging.InternalLoggerFactory; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameters; + +import javax.net.ssl.SSLEngine; +import javax.net.ssl.SSLSessionContext; + +import java.io.File; +import java.io.IOException; +import java.net.InetSocketAddress; +import java.security.cert.CertificateException; +import java.util.Collection; +import java.util.Collections; +import java.util.Enumeration; +import java.util.HashSet; +import java.util.Set; +import java.util.concurrent.atomic.AtomicReference; + +import static org.junit.Assert.*; + +@RunWith(Parameterized.class) +public class SocketSslSessionReuseTest extends AbstractSocketTest { + + private static final InternalLogger logger = InternalLoggerFactory.getInstance(SocketSslSessionReuseTest.class); + + private static final File CERT_FILE; + private static final File KEY_FILE; + + static { + SelfSignedCertificate ssc; + try { + ssc = new SelfSignedCertificate(); + } catch (CertificateException e) { + throw new Error(e); + } + CERT_FILE = ssc.certificate(); + KEY_FILE = ssc.privateKey(); + } + + @Parameters(name = "{index}: serverEngine = {0}, clientEngine = {1}") + public static Collection data() throws Exception { + return Collections.singletonList(new Object[] { + new JdkSslServerContext(CERT_FILE, KEY_FILE), + new JdkSslClientContext(CERT_FILE) + }); + } + + private final SslContext serverCtx; + private final SslContext clientCtx; + + public SocketSslSessionReuseTest(SslContext serverCtx, SslContext clientCtx) { + this.serverCtx = serverCtx; + this.clientCtx = clientCtx; + } + + @Test(timeout = 30000) + public void testSslSessionReuse() throws Throwable { + run(); + } + + public void testSslSessionReuse(ServerBootstrap sb, Bootstrap cb) throws Throwable { + final ReadAndDiscardHandler sh = new ReadAndDiscardHandler(true, true); + final ReadAndDiscardHandler ch = new ReadAndDiscardHandler(false, true); + final String[] protocols = new String[]{ "TLSv1", "TLSv1.1", "TLSv1.2" }; + + sb.childHandler(new ChannelInitializer() { + @Override + protected void initChannel(SocketChannel sch) throws Exception { + SSLEngine engine = serverCtx.newEngine(sch.alloc()); + engine.setUseClientMode(false); + engine.setEnabledProtocols(protocols); + + sch.pipeline().addLast(new SslHandler(engine)); + sch.pipeline().addLast(sh); + } + }); + final Channel sc = sb.bind().sync().channel(); + + cb.handler(new ChannelInitializer() { + @Override + protected void initChannel(SocketChannel sch) throws Exception { + InetSocketAddress serverAddr = (InetSocketAddress) sc.localAddress(); + SSLEngine engine = clientCtx.newEngine(sch.alloc(), serverAddr.getHostString(), serverAddr.getPort()); + engine.setUseClientMode(true); + engine.setEnabledProtocols(protocols); + + sch.pipeline().addLast(new SslHandler(engine)); + sch.pipeline().addLast(ch); + } + }); + + try { + SSLSessionContext clientSessionCtx = ((JdkSslContext) clientCtx).sessionContext(); + ByteBuf msg = Unpooled.wrappedBuffer(new byte[] { 0xa, 0xb, 0xc, 0xd }, 0, 4); + Channel cc = cb.connect().sync().channel(); + cc.writeAndFlush(msg).sync(); + cc.closeFuture().sync(); + rethrowHandlerExceptions(sh, ch); + Set sessions = sessionIdSet(clientSessionCtx.getIds()); + + msg = Unpooled.wrappedBuffer(new byte[] { 0xa, 0xb, 0xc, 0xd }, 0, 4); + cc = cb.connect().sync().channel(); + cc.writeAndFlush(msg).sync(); + cc.closeFuture().sync(); + assertEquals("Expected no new sessions", sessions, sessionIdSet(clientSessionCtx.getIds())); + rethrowHandlerExceptions(sh, ch); + } finally { + sc.close().awaitUninterruptibly(); + } + } + + private void rethrowHandlerExceptions(ReadAndDiscardHandler sh, ReadAndDiscardHandler ch) throws Throwable { + if (sh.exception.get() != null && !(sh.exception.get() instanceof IOException)) { + throw sh.exception.get(); + } + if (ch.exception.get() != null && !(ch.exception.get() instanceof IOException)) { + throw ch.exception.get(); + } + if (sh.exception.get() != null) { + throw sh.exception.get(); + } + if (ch.exception.get() != null) { + throw ch.exception.get(); + } + } + + private Set sessionIdSet(Enumeration sessionIds) { + Set idSet = new HashSet(); + byte[] id; + while (sessionIds.hasMoreElements()) { + id = sessionIds.nextElement(); + idSet.add(ByteBufUtil.hexDump(Unpooled.wrappedBuffer(id))); + } + return idSet; + } + + @Sharable + private static class ReadAndDiscardHandler extends SimpleChannelInboundHandler { + final AtomicReference exception = new AtomicReference(); + private final boolean server; + private final boolean autoRead; + + ReadAndDiscardHandler(boolean server, boolean autoRead) { + this.server = server; + this.autoRead = autoRead; + } + + @Override + public void channelRead0(ChannelHandlerContext ctx, ByteBuf in) throws Exception { + byte[] actual = new byte[in.readableBytes()]; + in.readBytes(actual); + ctx.close(); + } + + @Override + public void channelReadComplete(ChannelHandlerContext ctx) throws Exception { + try { + ctx.flush(); + } finally { + if (!autoRead) { + ctx.read(); + } + } + } + + @Override + public void exceptionCaught(ChannelHandlerContext ctx, + Throwable cause) throws Exception { + if (logger.isWarnEnabled()) { + logger.warn( + "Unexpected exception from the " + + (server? "server" : "client") + " side", cause); + } + + exception.compareAndSet(null, cause); + ctx.close(); + } + } +}