From 7ce0f35b69a30fcfcd37c9602a1be30077336d3e Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Thu, 1 Dec 2016 09:41:30 +0100 Subject: [PATCH] Correctly not try to call handshake() when engine is already closed. Motivation: We need to ensure we not call handshake() when the engine is already closed. Beside this our implementation of isOutboundDone() was not correct as it not took the pending data in the outbound buffer into acount (which may be also generated as part of an ssl alert). Beside this we also called SSL_shutdown(...) while we were still in init state which will produce an error and so noise in the log with openssl later versions. This is also in some extend related to #5931 . Modifications: - Ensure we not call handshake() when already closed - Correctly implement isOutboundDone() - Not call SSL_shutdown(...) when still in init state - Added test-cases Result: More correct behaviour of our openssl SSLEngine implementation. --- .../ssl/ReferenceCountedOpenSslEngine.java | 96 ++++++++-------- .../io/netty/handler/ssl/SSLEngineTest.java | 103 ++++++++++++++++++ 2 files changed, 156 insertions(+), 43 deletions(-) diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java index 8bff5858e9..fdae240202 100644 --- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java +++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java @@ -240,8 +240,7 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc // SSL Engine status variables private boolean isInboundDone; - private boolean isOutboundDone; - private boolean engineClosed; + private boolean outboundClosed; private final boolean clientMode; private final ByteBufAllocator alloc; @@ -274,7 +273,7 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc this.alloc = checkNotNull(alloc, "alloc"); apn = (OpenSslApplicationProtocolNegotiator) context.applicationProtocolNegotiator(); ssl = SSL.newSSL(context.ctx, !context.isClient()); - session = new ReferenceCountedOpenSslEngine.OpenSslSession(context.sessionContext()); + session = new OpenSslSession(context.sessionContext()); networkBIO = SSL.makeNetworkBIO(ssl); clientMode = context.isClient(); engineMap = context.engineMap; @@ -366,8 +365,7 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc SSL.freeBIO(networkBIO); ssl = networkBIO = 0; - // internal errors can cause shutdown without marking the engine closed - isInboundDone = isOutboundDone = engineClosed = true; + isInboundDone = outboundClosed = true; } // On shutdown clear all errors @@ -545,16 +543,22 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc bytesProduced += produced; pendingNet -= produced; } - // If isOuboundDone is set, then the data from the network BIO - // was the close_notify message -- we are not required to wait + + SSLEngineResult.HandshakeStatus hs = mayFinishHandshake( + status != FINISHED ? getHandshakeStatus(pendingNet) : status); + final SSLEngineResult.Status rs; + + // If isOutboundDone, then the data from the network BIO + // was the close_notify message and all was consumed we are not required to wait // for the receipt the peer's close_notify message -- shutdown. - if (isOutboundDone) { + if (isOutboundDone()) { + rs = CLOSED; shutdown(); + } else { + rs = OK; } - return new SSLEngineResult(getEngineStatus(), - mayFinishHandshake(status != FINISHED ? getHandshakeStatus(pendingNet) : status), - bytesConsumed, bytesProduced); + return new SSLEngineResult(rs, hs, bytesConsumed, bytesProduced); } return null; } @@ -581,8 +585,9 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc } synchronized (this) { - // Check to make sure the engine has not been closed - if (isDestroyed()) { + // Check to make sure the engine has not been closed and the outbound is not considered as done while the + // handshake was not started at all. + if (isDestroyed() || (handshakeState == HandshakeState.NOT_STARTED && isOutboundDone())) { return CLOSED_NOT_HANDSHAKING; } @@ -596,11 +601,15 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc status = handshake(); if (status == NEED_UNWRAP) { - return NEED_UNWRAP_OK; + // Signal if the outbound is done or not. + return isOutboundDone() ? NEED_UNWRAP_CLOSED : NEED_UNWRAP_OK; } - if (engineClosed) { - return NEED_UNWRAP_CLOSED; + // Explicit use outboundClosed and not outboundClosed() as we want to drain any bytes that are still + // present. + if (outboundClosed) { + SSLEngineResult pendingNetResult = readPendingBytesFromBIO(dst, 0, 0, status); + return pendingNetResult != null ? pendingNetResult : NEED_UNWRAP_CLOSED; } } @@ -656,7 +665,7 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc // has been closed. [1] https://www.openssl.org/docs/manmaster/ssl/SSL_write.html pendingNetResult = readPendingBytesFromBIO(dst, bytesConsumed, bytesProduced, status); return pendingNetResult != null ? pendingNetResult : - new SSLEngineResult(getEngineStatus(), + new SSLEngineResult(isOutboundDone() ? CLOSED : OK, NEED_UNWRAP, bytesConsumed, bytesProduced); case SSL.SSL_ERROR_WANT_WRITE: // SSL_ERROR_WANT_WRITE typically means that the underlying transport is not writable @@ -689,7 +698,7 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc } } - return newResult(bytesConsumed, bytesProduced, status); + return newResult(isOutboundDone() ? CLOSED : OK, bytesConsumed, bytesProduced, status); } } @@ -760,16 +769,13 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc } synchronized (this) { - // Check to make sure the engine has not been closed - if (isDestroyed()) { + // Check to make sure the engine has not been closed and the inbound was not marked as done. + if (isDestroyed() || (handshakeState == HandshakeState.NOT_STARTED && isInboundDone())) { return CLOSED_NOT_HANDSHAKING; } // protect against protocol overflow attack vector if (len > MAX_ENCRYPTED_PACKET_LENGTH) { - isInboundDone = true; - isOutboundDone = true; - engineClosed = true; shutdown(); throw ENCRYPTED_PACKET_OVERSIZED; } @@ -786,7 +792,8 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc if (status == NEED_WRAP) { return NEED_WRAP_OK; } - if (engineClosed) { + // Check if the inbound is considered to be closed if so let us try to wrap again. + if (isInboundDone) { return NEED_WRAP_CLOSED; } } @@ -884,7 +891,7 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc idx++; } else { // We read everything return now. - return newResult(bytesConsumed, bytesProduced, status); + return newResult(isInboundDone() ? CLOSED : OK, bytesConsumed, bytesProduced, status); } } else { int sslError = SSL.getError(ssl, bytesRead); @@ -898,7 +905,7 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc case SSL.SSL_ERROR_WANT_READ: case SSL.SSL_ERROR_WANT_WRITE: // break to the outer loop - return newResult(bytesConsumed, bytesProduced, status); + return newResult(isInboundDone() ? CLOSED : OK, bytesConsumed, bytesProduced, status); default: return sslReadErrorResult(SSL.getLastErrorNumber(), bytesConsumed, bytesProduced); } @@ -927,7 +934,7 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc closeAll(); } - return newResult(bytesConsumed, bytesProduced, status); + return newResult(isInboundDone() ? CLOSED : OK, bytesConsumed, bytesProduced, status); } } @@ -955,11 +962,10 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc return handshakeState == HandshakeState.FINISHED ? SSL.pendingReadableBytesInSSL(ssl) : 0; } - private SSLEngineResult newResult( + private SSLEngineResult newResult(SSLEngineResult.Status resultStatus, int bytesConsumed, int bytesProduced, SSLEngineResult.HandshakeStatus status) throws SSLException { - return new SSLEngineResult( - getEngineStatus(), mayFinishHandshake(status != FINISHED ? getHandshakeStatus() : status) - , bytesConsumed, bytesProduced); + return new SSLEngineResult(resultStatus, mayFinishHandshake(status != FINISHED ? getHandshakeStatus() : status), + bytesConsumed, bytesProduced); } private void closeAll() throws SSLException { @@ -1052,7 +1058,6 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc } isInboundDone = true; - engineClosed = true; shutdown(); @@ -1064,19 +1069,26 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc @Override public final synchronized boolean isInboundDone() { - return isInboundDone || engineClosed; + return isInboundDone; } @Override public final synchronized void closeOutbound() { - if (isOutboundDone) { + if (outboundClosed) { return; } - isOutboundDone = true; - engineClosed = true; + outboundClosed = true; if (handshakeState != HandshakeState.NOT_STARTED && !isDestroyed()) { + if (SSL.isInInit(ssl) != 0) { + // Only try to call SSL_shutdown if we are not in the init state anymore. + // Otherwise we will see 'error:140E0197:SSL routines:SSL_shutdown:shutdown while in init' in our logs. + // + // See also http://hg.nginx.org/nginx/rev/062c189fee20 + return; + } + int mode = SSL.getShutdown(ssl); if ((mode & SSL.SSL_SENT_SHUTDOWN) != SSL.SSL_SENT_SHUTDOWN) { int err = SSL.shutdownSSL(ssl); @@ -1114,7 +1126,9 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc @Override public final synchronized boolean isOutboundDone() { - return isOutboundDone; + // Check if there is anything left in the outbound buffer. + // We need to ensure we only call SSL.pendingWrittenBytesInBIO(...) if the engine was not destroyed yet. + return outboundClosed && (networkBIO == 0 || SSL.pendingWrittenBytesInBIO(networkBIO) == 0); } @Override @@ -1348,7 +1362,7 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc } private void checkEngineClosed(SSLException cause) throws SSLException { - if (engineClosed || isDestroyed()) { + if (isDestroyed()) { throw cause; } } @@ -1428,10 +1442,6 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc return FINISHED; } - private SSLEngineResult.Status getEngineStatus() { - return engineClosed? CLOSED : OK; - } - private SSLEngineResult.HandshakeStatus mayFinishHandshake(SSLEngineResult.HandshakeStatus status) throws SSLException { if (status == NOT_HANDSHAKING && handshakeState != HandshakeState.FINISHED) { @@ -1455,7 +1465,7 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc private boolean needPendingStatus() { return handshakeState != HandshakeState.NOT_STARTED && !isDestroyed() - && (handshakeState != HandshakeState.FINISHED || engineClosed); + && (handshakeState != HandshakeState.FINISHED || isInboundDone() || isOutboundDone()); } /** diff --git a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java index 2ccda58fdd..98d629206b 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java @@ -1111,4 +1111,107 @@ public abstract class SSLEngineTest { cleanupClientSslEngine(client); } } + + @Test + public void testBeginHandshakeAfterEngineClosed() throws SSLException { + clientSslCtx = SslContextBuilder + .forClient() + .sslProvider(sslClientProvider()) + .build(); + SSLEngine client = clientSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT); + + try { + client.closeInbound(); + client.closeOutbound(); + try { + client.beginHandshake(); + fail(); + } catch (SSLException expected) { + // expected + } + } finally { + cleanupClientSslEngine(client); + } + } + + @Test + public void testBeginHandshakeCloseOutbound() throws Exception { + SelfSignedCertificate cert = new SelfSignedCertificate(); + + clientSslCtx = SslContextBuilder + .forClient() + .sslProvider(sslClientProvider()) + .build(); + SSLEngine client = clientSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT); + + serverSslCtx = SslContextBuilder + .forServer(cert.certificate(), cert.privateKey()) + .sslProvider(sslServerProvider()) + .build(); + SSLEngine server = serverSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT); + + try { + testBeginHandshakeCloseOutbound(client); + testBeginHandshakeCloseOutbound(server); + } finally { + cleanupClientSslEngine(client); + cleanupServerSslEngine(server); + } + } + + private static void testBeginHandshakeCloseOutbound(SSLEngine engine) throws SSLException { + ByteBuffer dst = ByteBuffer.allocateDirect(engine.getSession().getPacketBufferSize()); + ByteBuffer empty = ByteBuffer.allocateDirect(0); + engine.beginHandshake(); + engine.closeOutbound(); + + SSLEngineResult result; + for (;;) { + result = engine.wrap(empty, dst); + dst.flip(); + + assertEquals(0, result.bytesConsumed()); + assertEquals(dst.remaining(), result.bytesProduced()); + if (result.getHandshakeStatus() != SSLEngineResult.HandshakeStatus.NEED_WRAP) { + break; + } + dst.clear(); + } + assertEquals(SSLEngineResult.Status.CLOSED, result.getStatus()); + } + + @Test + public void testCloseInboundAfterBeginHandshake() throws Exception { + SelfSignedCertificate cert = new SelfSignedCertificate(); + + clientSslCtx = SslContextBuilder + .forClient() + .sslProvider(sslClientProvider()) + .build(); + SSLEngine client = clientSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT); + + serverSslCtx = SslContextBuilder + .forServer(cert.certificate(), cert.privateKey()) + .sslProvider(sslServerProvider()) + .build(); + SSLEngine server = serverSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT); + + try { + testCloseInboundAfterBeginHandshake(client); + testCloseInboundAfterBeginHandshake(server); + } finally { + cleanupClientSslEngine(client); + cleanupServerSslEngine(server); + } + } + + private static void testCloseInboundAfterBeginHandshake(SSLEngine engine) throws SSLException { + engine.beginHandshake(); + try { + engine.closeInbound(); + fail(); + } catch (SSLException expected) { + // expected + } + } }