From 45828ee906b350ea3d21faaf6808c7da8d0d4688 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Wed, 16 Oct 2013 16:27:36 +0200 Subject: [PATCH] 418892 - SSL session caching so unreliable it effectively does not work. Fixed by making sure that we completely decrypt read bytes. Before the fix, it was possible that we returned after the decryption of one TLS frame, while another was still present in the _encryptedBuffer. This lead to non-clean closes of the connection, which hampered the capability of session reuse by clients. Now we decrypt in a loop and only return if there is nothing more that we can decrypt. --- .../jetty/client/ssl/SslBytesServerTest.java | 90 ++++++- .../jetty/client/ssl/SslBytesTest.java | 9 +- .../eclipse/jetty/io/ssl/SslConnection.java | 243 +++++++++--------- 3 files changed, 220 insertions(+), 122 deletions(-) diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/ssl/SslBytesServerTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/ssl/SslBytesServerTest.java index e0130ddb196..4802db7338a 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/ssl/SslBytesServerTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/ssl/SslBytesServerTest.java @@ -84,6 +84,7 @@ public class SslBytesServerTest extends SslBytesTest private ExecutorService threadPool; private Server server; private SslContextFactory sslContextFactory; + private int serverPort; private SSLContext sslContext; private SimpleProxy proxy; private Runnable idleHook; @@ -211,7 +212,7 @@ public class SslBytesServerTest extends SslBytesTest } }); server.start(); - int serverPort = connector.getLocalPort(); + serverPort = connector.getLocalPort(); sslContext = sslContextFactory.getSslContext(); @@ -292,6 +293,88 @@ public class SslBytesServerTest extends SslBytesTest closeClient(client); } + @Test + public void testHandshakeWithResumedSessionThenClose() throws Exception + { + // First socket will establish the SSL session + SSLSocket client1 = newClient(); + SimpleProxy.AutomaticFlow automaticProxyFlow = proxy.startAutomaticFlow(); + client1.startHandshake(); + client1.close(); + Assert.assertTrue(automaticProxyFlow.stop(5, TimeUnit.SECONDS)); + int proxyPort = proxy.getPort(); + proxy.stop(); + + proxy = new SimpleProxy(threadPool, proxyPort, "localhost", serverPort); + proxy.start(); + logger.info("proxy:{} <==> server:{}", proxy.getPort(), serverPort); + + final SSLSocket client2 = newClient(proxy); + + Future handshake = threadPool.submit(new Callable() + { + @Override + public Object call() throws Exception + { + client2.startHandshake(); + return null; + } + }); + + // Client Hello with SessionID + TLSRecord record = proxy.readFromClient(); + Assert.assertNotNull(record); + proxy.flushToServer(record); + + // Server Hello + record = proxy.readFromServer(); + Assert.assertNotNull(record); + proxy.flushToClient(record); + + // Change Cipher Spec + record = proxy.readFromServer(); + Assert.assertNotNull(record); + proxy.flushToClient(record); + + // Server Done + record = proxy.readFromServer(); + Assert.assertNotNull(record); + proxy.flushToClient(record); + + // Client Key Exchange + record = proxy.readFromClient(); + Assert.assertNotNull(record); + // Client Done + TLSRecord doneRecord = proxy.readFromClient(); + Assert.assertNotNull(doneRecord); + // Close + client2.close(); + TLSRecord closeRecord = proxy.readFromClient(); + Assert.assertNotNull(closeRecord); + Assert.assertEquals(TLSRecord.Type.ALERT, closeRecord.getType()); + // Flush to server Client Key Exchange + Client Done + Close in one chunk + byte[] recordBytes = record.getBytes(); + byte[] doneBytes = doneRecord.getBytes(); + byte[] closeRecordBytes = closeRecord.getBytes(); + byte[] chunk = new byte[recordBytes.length + doneBytes.length + closeRecordBytes.length]; + System.arraycopy(recordBytes, 0, chunk, 0, recordBytes.length); + System.arraycopy(doneBytes, 0, chunk, recordBytes.length, doneBytes.length); + System.arraycopy(closeRecordBytes, 0, chunk, recordBytes.length + doneBytes.length, closeRecordBytes.length); + proxy.flushToServer(0, chunk); + // Close the raw socket + proxy.flushToServer(null); + + // Expect the server to send a FIN as well + record = proxy.readFromServer(); + Assert.assertNull(record); + + // Check that we did not spin + TimeUnit.MILLISECONDS.sleep(500); + Assert.assertThat(sslFills.get(), Matchers.lessThan(20)); + Assert.assertThat(sslFlushes.get(), Matchers.lessThan(20)); + Assert.assertThat(httpParses.get(), Matchers.lessThan(20)); + } + @Test public void testHandshakeWithSplitBoundary() throws Exception { @@ -1794,6 +1877,11 @@ public class SslBytesServerTest extends SslBytesTest } private SSLSocket newClient() throws IOException, InterruptedException + { + return newClient(proxy); + } + + private SSLSocket newClient(SimpleProxy proxy) throws IOException, InterruptedException { SSLSocket client = (SSLSocket)sslContext.getSocketFactory().createSocket("localhost", proxy.getPort()); client.setUseClientMode(true); diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/ssl/SslBytesTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/ssl/SslBytesTest.java index b0f576995e7..b24b4572e85 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/ssl/SslBytesTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/ssl/SslBytesTest.java @@ -105,6 +105,7 @@ public abstract class SslBytesTest { private final CountDownLatch latch = new CountDownLatch(1); private final ExecutorService threadPool; + private final int proxyPort; private final String serverHost; private final int serverPort; private volatile ServerSocket serverSocket; @@ -112,15 +113,21 @@ public abstract class SslBytesTest private volatile Socket client; public SimpleProxy(ExecutorService threadPool, String serverHost, int serverPort) + { + this(threadPool, 0, serverHost, serverPort); + } + + public SimpleProxy(ExecutorService threadPool, int proxyPort, String serverHost, int serverPort) { this.threadPool = threadPool; + this.proxyPort = proxyPort; this.serverHost = serverHost; this.serverPort = serverPort; } public void start() throws Exception { - serverSocket = new ServerSocket(0); + serverSocket = new ServerSocket(proxyPort); Thread acceptor = new Thread(this); acceptor.start(); server = new Socket(serverHost, serverPort); diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java b/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java index 05e00c92f5f..43a99696565 100644 --- a/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java +++ b/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java @@ -189,12 +189,12 @@ public class SslConnection extends AbstractConnection // filling. if (DEBUG) - LOG.debug("onFillable enter {}", getEndPoint()); + LOG.debug("onFillable enter {}", _decryptedEndPoint); // We have received a close handshake, close the end point to send FIN. if (_decryptedEndPoint.isInputShutdown()) - getEndPoint().close(); - + _decryptedEndPoint.close(); + // wake up whoever is doing the fill or the flush so they can // do all the filling, unwrapping, wrapping and flushing _decryptedEndPoint.getFillInterest().fillable(); @@ -210,7 +210,7 @@ public class SslConnection extends AbstractConnection } if (DEBUG) - LOG.debug("onFillable exit {}", getEndPoint()); + LOG.debug("onFillable exit {}", _decryptedEndPoint); } @Override @@ -312,7 +312,7 @@ public class SslConnection extends AbstractConnection fail_filler = true; } } - + final boolean filler_failed=fail_filler; getExecutor().execute(new Runnable() @@ -508,142 +508,142 @@ public class SslConnection extends AbstractConnection int net_filled = getEndPoint().fill(_encryptedInput); if (DEBUG) LOG.debug("{} filled {} encrypted bytes", SslConnection.this, net_filled); - if (net_filled > 0) - _underFlown = false; - // Let's try the SSL thang even if we have no net data because in that - // case we want to fall through to the handshake handling - int pos = BufferUtil.flipToFill(app_in); - - SSLEngineResult unwrapResult = _sslEngine.unwrap(_encryptedInput, app_in); - - BufferUtil.flipToFlush(app_in, pos); - if (DEBUG) - LOG.debug("{} unwrap {}", SslConnection.this, unwrapResult); - - Status unwrapResultStatus = unwrapResult.getStatus(); - HandshakeStatus unwrapHandshakeStatus = unwrapResult.getHandshakeStatus(); - HandshakeStatus handshakeStatus = _sslEngine.getHandshakeStatus(); - - // and deal with the results - switch (unwrapResultStatus) + decryption: while (true) { - case BUFFER_OVERFLOW: - throw new IllegalStateException(); + // Let's unwrap even if we have no net data because in that + // case we want to fall through to the handshake handling + int pos = BufferUtil.flipToFill(app_in); + SSLEngineResult unwrapResult = _sslEngine.unwrap(_encryptedInput, app_in); + BufferUtil.flipToFlush(app_in, pos); + if (DEBUG) + LOG.debug("{} unwrap {}", SslConnection.this, unwrapResult); - case CLOSED: - // Dang! we have to care about the handshake state specially for close - switch (handshakeStatus) - { - case NOT_HANDSHAKING: - // We were not handshaking, so just tell the app we are closed - return -1; + HandshakeStatus handshakeStatus = _sslEngine.getHandshakeStatus(); + HandshakeStatus unwrapHandshakeStatus = unwrapResult.getHandshakeStatus(); + Status unwrapResultStatus = unwrapResult.getStatus(); - case NEED_TASK: - // run the task - _sslEngine.getDelegatedTask().run(); - continue; + _underFlown = unwrapResultStatus == Status.BUFFER_UNDERFLOW; - case NEED_WRAP: - // we need to send some handshake data (probably to send a close handshake). - // but that will not enable any extra data to fill, so we just return -1 - // The wrapping can be done by any output drivers doing flushing or shutdown output. - return -1; - } - throw new IllegalStateException(); - - default: - if (unwrapHandshakeStatus == HandshakeStatus.FINISHED && !_handshaken) - { - _handshaken = true; - if (DEBUG) - LOG.debug("{} handshake completed client-side", SslConnection.this); - } - - // Check whether renegotiation is allowed - if (_handshaken && handshakeStatus != HandshakeStatus.NOT_HANDSHAKING && !isRenegotiationAllowed()) - { - if (DEBUG) - LOG.debug("{} renegotiation denied", SslConnection.this); + if (_underFlown) + { + if (net_filled < 0) closeInbound(); - return -1; - } + if (net_filled <= 0) + return net_filled; + } - if (unwrapResultStatus == Status.BUFFER_UNDERFLOW) - _underFlown = true; - - // If bytes were produced, don't bother with the handshake status; - // pass the decrypted data to the application, which will perform - // another call to fill() or flush(). - if (unwrapResult.bytesProduced() > 0) + switch (unwrapResultStatus) + { + case CLOSED: { - if (app_in == buffer) - return unwrapResult.bytesProduced(); - return BufferUtil.flipPutFlip(_decryptedInput, buffer); - } - - // Dang! we have to care about the handshake state - switch (handshakeStatus) - { - case NOT_HANDSHAKING: - // we just didn't read anything. - if (net_filled < 0) + switch (handshakeStatus) + { + case NOT_HANDSHAKING: { - closeInbound(); + // We were not handshaking, so just tell the app we are closed return -1; } - return 0; - - case NEED_TASK: - // run the task - _sslEngine.getDelegatedTask().run(); - continue; - - case NEED_WRAP: - // we need to send some handshake data - - // if we are called from flush - if (buffer == __FLUSH_CALLED_FILL) - return 0; // let it do the wrapping - - _fillRequiresFlushToProgress = true; - flush(__FILL_CALLED_FLUSH); - if (BufferUtil.isEmpty(_encryptedOutput)) + case NEED_TASK: { - // the flush completed so continue - _fillRequiresFlushToProgress = false; + _sslEngine.getDelegatedTask().run(); continue; } - return 0; - - case NEED_UNWRAP: - // if we just filled some net data - if (net_filled < 0) + case NEED_WRAP: { - closeInbound(); + // We need to send some handshake data (probably the close handshake). + // We return -1 so that the application can drive the close by flushing + // or shutting down the output. return -1; } - else if (net_filled > 0) + default: { - // maybe we will fill some more on a retry + throw new IllegalStateException(); + } + } + } + case BUFFER_UNDERFLOW: + case OK: + { + if (unwrapHandshakeStatus == HandshakeStatus.FINISHED && !_handshaken) + { + _handshaken = true; + if (DEBUG) + LOG.debug("{} {} handshake completed", SslConnection.this, + _sslEngine.getUseClientMode() ? "client-side" : "resumed session server-side"); + } + + // Check whether renegotiation is allowed + if (_handshaken && handshakeStatus != HandshakeStatus.NOT_HANDSHAKING && !isRenegotiationAllowed()) + { + if (DEBUG) + LOG.debug("{} renegotiation denied", SslConnection.this); + closeInbound(); + return -1; + } + + // If bytes were produced, don't bother with the handshake status; + // pass the decrypted data to the application, which will perform + // another call to fill() or flush(). + if (unwrapResult.bytesProduced() > 0) + { + if (app_in == buffer) + return unwrapResult.bytesProduced(); + return BufferUtil.flipPutFlip(_decryptedInput, buffer); + } + + switch (handshakeStatus) + { + case NOT_HANDSHAKING: + { + if (_underFlown) + break decryption; continue; } - else + case NEED_TASK: { - if (_encryptedInput.hasRemaining()) + _sslEngine.getDelegatedTask().run(); + continue; + } + case NEED_WRAP: + { + // If we are called from flush() + // return to let it do the wrapping. + if (buffer == __FLUSH_CALLED_FILL) + return 0; + + _fillRequiresFlushToProgress = true; + flush(__FILL_CALLED_FLUSH); + if (BufferUtil.isEmpty(_encryptedOutput)) { - // if there are more encrypted bytes, - // then we need to unwrap more, we don't - // care if net_filled is zero + // The flush wrote all the encrypted bytes so continue to fill + _fillRequiresFlushToProgress = false; continue; } - // we need to wait for more net data - return 0; + else + { + // The flush did not complete, return from fill() + // and let the write completion mechanism to kick in. + return 0; + } } - - case FINISHED: - throw new IllegalStateException(); + case NEED_UNWRAP: + { + if (_underFlown) + break decryption; + continue; + } + default: + { + throw new IllegalStateException(); + } + } } + default: + { + throw new IllegalStateException(); + } + } } } } @@ -778,7 +778,7 @@ public class SslConnection extends AbstractConnection { _handshaken = true; if (DEBUG) - LOG.debug("{} handshake completed server-side", SslConnection.this); + LOG.debug("{} {} handshake completed", SslConnection.this, "server-side"); } HandshakeStatus handshakeStatus = _sslEngine.getHandshakeStatus(); @@ -824,7 +824,7 @@ public class SslConnection extends AbstractConnection if (handshakeStatus == HandshakeStatus.NEED_WRAP) continue; } - return allConsumed&&BufferUtil.isEmpty(_encryptedOutput); + return allConsumed && BufferUtil.isEmpty(_encryptedOutput); case FINISHED: throw new IllegalStateException(); @@ -860,17 +860,18 @@ public class SslConnection extends AbstractConnection public void shutdownOutput() { boolean ishut = isInputShutdown(); + boolean oshut = isOutputShutdown(); if (DEBUG) - LOG.debug("{} shutdownOutput: oshut={}, ishut={}", SslConnection.this, isOutputShutdown(), ishut); + LOG.debug("{} shutdownOutput: oshut={}, ishut={}", SslConnection.this, oshut, ishut); if (ishut) { // Aggressively close, since inbound close alert has already been processed // and the TLS specification allows to close the connection directly, which // is what most other implementations expect: a FIN rather than a TLS close - // reply. If a TLS close reply is sent, most implementation send a RST. + // reply. If a TLS close reply is sent, most implementations send a RST. getEndPoint().close(); } - else + else if (!oshut) { try { @@ -895,6 +896,8 @@ public class SslConnection extends AbstractConnection @Override public void close() { + // First send the TLS Close Alert, then the FIN + shutdownOutput(); getEndPoint().close(); }