From d53af5d737bd28cf240a5344ee877ad9f0d94a20 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Wed, 17 May 2017 13:07:21 +0200 Subject: [PATCH] Fixes #523 - TLS close behaviour breaking session resumption. Since requests cannot be connection delimited, don't call sslEngine.closeInbound() on the server. On the client, added a configuration parameter to allow missing TLS Close Message, since many servers do that. Introduced SslConnection.allowMissingCloseMessage so that it throws in case of truncation attacks. --- .../org/eclipse/jetty/client/HttpClient.java | 6 + .../eclipse/jetty/client/HttpDestination.java | 3 +- .../org/eclipse/jetty/client/HttpProxy.java | 4 +- .../org/eclipse/jetty/client/Socks4Proxy.java | 3 +- .../jetty/client/HttpClientTLSTest.java | 124 ++++++++++++++++++ .../jetty/client/HttpClientTimeoutTest.java | 28 ++-- .../jetty/http2/client/HTTP2Client.java | 7 +- .../io/ssl/SslClientConnectionFactory.java | 12 ++ .../eclipse/jetty/io/ssl/SslConnection.java | 39 +++++- 9 files changed, 193 insertions(+), 33 deletions(-) diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java index 102e0abf32c..e0d6ba49e95 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java @@ -59,6 +59,7 @@ import org.eclipse.jetty.http.HttpScheme; import org.eclipse.jetty.io.ByteBufferPool; import org.eclipse.jetty.io.ClientConnectionFactory; import org.eclipse.jetty.io.MappedByteBufferPool; +import org.eclipse.jetty.io.ssl.SslClientConnectionFactory; import org.eclipse.jetty.util.Fields; import org.eclipse.jetty.util.Jetty; import org.eclipse.jetty.util.Promise; @@ -1060,6 +1061,11 @@ public class HttpClient extends ContainerLifeCycle return HttpScheme.HTTPS.is(scheme) ? port == 443 : port == 80; } + protected ClientConnectionFactory newSslClientConnectionFactory(ClientConnectionFactory connectionFactory) + { + return new SslClientConnectionFactory(getSslContextFactory(), getByteBufferPool(), getExecutor(), connectionFactory); + } + private class ContentDecoderFactorySet implements Set { private final Set set = new HashSet<>(); diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpDestination.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpDestination.java index 54251e3807a..5a21aec22b1 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpDestination.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpDestination.java @@ -33,7 +33,6 @@ import org.eclipse.jetty.http.HttpField; import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpScheme; import org.eclipse.jetty.io.ClientConnectionFactory; -import org.eclipse.jetty.io.ssl.SslClientConnectionFactory; import org.eclipse.jetty.util.BlockingArrayQueue; import org.eclipse.jetty.util.HostPort; import org.eclipse.jetty.util.Promise; @@ -97,7 +96,7 @@ public abstract class HttpDestination extends ContainerLifeCycle implements Dest protected ClientConnectionFactory newSslClientConnectionFactory(ClientConnectionFactory connectionFactory) { - return new SslClientConnectionFactory(client.getSslContextFactory(), client.getByteBufferPool(), client.getExecutor(), connectionFactory); + return client.newSslClientConnectionFactory(connectionFactory); } public HttpClient getHttpClient() diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpProxy.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpProxy.java index 502b9758d77..b90bb504a5f 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpProxy.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpProxy.java @@ -34,7 +34,6 @@ import org.eclipse.jetty.http.HttpScheme; import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.io.ClientConnectionFactory; import org.eclipse.jetty.io.EndPoint; -import org.eclipse.jetty.io.ssl.SslClientConnectionFactory; import org.eclipse.jetty.util.Promise; import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; @@ -205,8 +204,7 @@ public class HttpProxy extends ProxyConfiguration.Proxy context.put(HttpClientTransport.HTTP_CONNECTION_PROMISE_CONTEXT_KEY, promise); HttpDestination destination = (HttpDestination)context.get(HttpClientTransport.HTTP_DESTINATION_CONTEXT_KEY); HttpClient client = destination.getHttpClient(); - ClientConnectionFactory sslConnectionFactory = - new SslClientConnectionFactory(client.getSslContextFactory(), client.getByteBufferPool(), client.getExecutor(), connectionFactory); + ClientConnectionFactory sslConnectionFactory = client.newSslClientConnectionFactory(connectionFactory); HttpConnectionOverHTTP oldConnection = (HttpConnectionOverHTTP)endPoint.getConnection(); org.eclipse.jetty.io.Connection newConnection = sslConnectionFactory.newConnection(endPoint, context); // Creating the connection will link the new Connection the EndPoint, diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/Socks4Proxy.java b/jetty-client/src/main/java/org/eclipse/jetty/client/Socks4Proxy.java index 883b6724319..11e52264b25 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/Socks4Proxy.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/Socks4Proxy.java @@ -31,7 +31,6 @@ import org.eclipse.jetty.http.HttpScheme; import org.eclipse.jetty.io.AbstractConnection; import org.eclipse.jetty.io.ClientConnectionFactory; import org.eclipse.jetty.io.EndPoint; -import org.eclipse.jetty.io.ssl.SslClientConnectionFactory; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.Promise; @@ -198,7 +197,7 @@ public class Socks4Proxy extends ProxyConfiguration.Proxy HttpClient client = destination.getHttpClient(); ClientConnectionFactory connectionFactory = this.connectionFactory; if (HttpScheme.HTTPS.is(destination.getScheme())) - connectionFactory = new SslClientConnectionFactory(client.getSslContextFactory(), client.getByteBufferPool(), client.getExecutor(), connectionFactory); + connectionFactory = client.newSslClientConnectionFactory(connectionFactory); org.eclipse.jetty.io.Connection newConnection = connectionFactory.newConnection(getEndPoint(), context); getEndPoint().upgrade(newConnection); if (LOG.isDebugEnabled()) diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTLSTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTLSTest.java index 5b1e53a3402..6751bbe33a3 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTLSTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTLSTest.java @@ -18,22 +18,34 @@ package org.eclipse.jetty.client; +import java.io.BufferedReader; +import java.io.InputStreamReader; +import java.io.OutputStream; +import java.net.ServerSocket; +import java.net.Socket; +import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; +import javax.net.ssl.SSLException; +import javax.net.ssl.SSLSocket; + import org.eclipse.jetty.client.api.ContentResponse; import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpScheme; import org.eclipse.jetty.http.HttpStatus; +import org.eclipse.jetty.io.ClientConnectionFactory; +import org.eclipse.jetty.io.ssl.SslClientConnectionFactory; import org.eclipse.jetty.io.ssl.SslHandshakeListener; import org.eclipse.jetty.server.Handler; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; import org.eclipse.jetty.util.ssl.SslContextFactory; import org.eclipse.jetty.util.thread.QueuedThreadPool; +import org.hamcrest.Matchers; import org.junit.After; import org.junit.Assert; import org.junit.Test; @@ -392,4 +404,116 @@ public class HttpClientTLSTest Assert.assertTrue(serverLatch.await(1, TimeUnit.SECONDS)); Assert.assertTrue(clientLatch.await(1, TimeUnit.SECONDS)); } + + @Test + public void testClientRawCloseDoesNotInvalidateSession() throws Exception + { + SslContextFactory serverTLSFactory = createSslContextFactory(); + startServer(serverTLSFactory, new EmptyServerHandler()); + + SslContextFactory clientTLSFactory = createSslContextFactory(); + clientTLSFactory.start(); + + String host = "localhost"; + int port = connector.getLocalPort(); + Socket socket = new Socket(host, port); + SSLSocket sslSocket = (SSLSocket)clientTLSFactory.getSslContext().getSocketFactory().createSocket(socket, host, port, true); + CountDownLatch handshakeLatch1 = new CountDownLatch(1); + AtomicReference session1 = new AtomicReference<>(); + sslSocket.addHandshakeCompletedListener(event -> + { + session1.set(event.getSession().getId()); + handshakeLatch1.countDown(); + }); + sslSocket.startHandshake(); + Assert.assertTrue(handshakeLatch1.await(5, TimeUnit.SECONDS)); + + // The client closes abruptly. + socket.close(); + + // Try again and compare the session ids. + socket = new Socket(host, port); + sslSocket = (SSLSocket)clientTLSFactory.getSslContext().getSocketFactory().createSocket(socket, host, port, true); + CountDownLatch handshakeLatch2 = new CountDownLatch(1); + AtomicReference session2 = new AtomicReference<>(); + sslSocket.addHandshakeCompletedListener(event -> + { + session2.set(event.getSession().getId()); + handshakeLatch2.countDown(); + }); + sslSocket.startHandshake(); + Assert.assertTrue(handshakeLatch2.await(5, TimeUnit.SECONDS)); + + Assert.assertArrayEquals(session1.get(), session2.get()); + + sslSocket.close(); + } + + @Test + public void testServerRawCloseDetectedByClient() throws Exception + { + SslContextFactory serverTLSFactory = createSslContextFactory(); + serverTLSFactory.start(); + try (ServerSocket server = new ServerSocket(0)) + { + QueuedThreadPool clientThreads = new QueuedThreadPool(); + clientThreads.setName("client"); + client = new HttpClient(createSslContextFactory()) + { + @Override + protected ClientConnectionFactory newSslClientConnectionFactory(ClientConnectionFactory connectionFactory) + { + SslClientConnectionFactory ssl = (SslClientConnectionFactory)super.newSslClientConnectionFactory(connectionFactory); + ssl.setAllowMissingCloseMessage(false); + return ssl; + } + }; + client.setExecutor(clientThreads); + client.start(); + + CountDownLatch latch = new CountDownLatch(1); + client.newRequest("localhost", server.getLocalPort()) + .scheme(HttpScheme.HTTPS.asString()) + .send(result -> + { + Assert.assertThat(result.getResponseFailure(), Matchers.instanceOf(SSLException.class)); + latch.countDown(); + }); + + Socket socket = server.accept(); + SSLSocket sslSocket = (SSLSocket)serverTLSFactory.getSslContext().getSocketFactory().createSocket(socket, null, socket.getPort(), true); + sslSocket.setUseClientMode(false); + BufferedReader reader = new BufferedReader(new InputStreamReader(sslSocket.getInputStream(), StandardCharsets.UTF_8)); + while (true) + { + String line = reader.readLine(); + if (line == null || line.isEmpty()) + break; + } + + // If the response is Content-Length delimited, allowing the + // missing TLS Close Message is fine because the application + // will see a EOFException anyway. + // If the response is connection delimited, allowing the + // missing TLS Close Message is bad because the application + // will see a successful response with truncated content. + + // Verify that by not allowing the missing + // TLS Close Message we get a response failure. + + byte[] half = new byte[8]; + String response = "HTTP/1.1 200 OK\r\n" + +// "Content-Length: " + (half.length * 2) + "\r\n" + + "Connection: close\r\n" + + "\r\n"; + OutputStream output = sslSocket.getOutputStream(); + output.write(response.getBytes(StandardCharsets.UTF_8)); + output.write(half); + output.flush(); + // Simulate a truncation attack by raw closing. + socket.close(); + + Assert.assertTrue(latch.await(5, TimeUnit.SECONDS)); + } + } } diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTimeoutTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTimeoutTest.java index d5c42d00a61..639d9af7bac 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTimeoutTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTimeoutTest.java @@ -40,8 +40,6 @@ import org.eclipse.jetty.client.api.Destination; import org.eclipse.jetty.client.api.Request; import org.eclipse.jetty.client.api.Response; import org.eclipse.jetty.client.api.Result; -import org.eclipse.jetty.client.http.HttpClientTransportOverHTTP; -import org.eclipse.jetty.client.http.HttpDestinationOverHTTP; import org.eclipse.jetty.client.util.BufferingResponseListener; import org.eclipse.jetty.client.util.InputStreamContentProvider; import org.eclipse.jetty.io.ByteBufferPool; @@ -252,37 +250,29 @@ public class HttpClientTimeoutTest extends AbstractHttpClientServerTest start(new TimeoutHandler(2 * timeout)); client.stop(); final AtomicBoolean sslIdle = new AtomicBoolean(); - client = new HttpClient(new HttpClientTransportOverHTTP() + client = new HttpClient(sslContextFactory) { @Override - public HttpDestination newHttpDestination(Origin origin) + public ClientConnectionFactory newSslClientConnectionFactory(ClientConnectionFactory connectionFactory) { - return new HttpDestinationOverHTTP(getHttpClient(), origin) + return new SslClientConnectionFactory(getSslContextFactory(), getByteBufferPool(), getExecutor(), connectionFactory) { @Override - protected ClientConnectionFactory newSslClientConnectionFactory(ClientConnectionFactory connectionFactory) + protected SslConnection newSslConnection(ByteBufferPool byteBufferPool, Executor executor, EndPoint endPoint, SSLEngine engine) { - HttpClient client = getHttpClient(); - return new SslClientConnectionFactory(client.getSslContextFactory(), client.getByteBufferPool(), client.getExecutor(), connectionFactory) + return new SslConnection(byteBufferPool, executor, endPoint, engine) { @Override - protected SslConnection newSslConnection(ByteBufferPool byteBufferPool, Executor executor, EndPoint endPoint, SSLEngine engine) + protected boolean onReadTimeout() { - return new SslConnection(byteBufferPool, executor, endPoint, engine) - { - @Override - protected boolean onReadTimeout() - { - sslIdle.set(true); - return super.onReadTimeout(); - } - }; + sslIdle.set(true); + return super.onReadTimeout(); } }; } }; } - }, sslContextFactory); + }; client.setIdleTimeout(timeout); client.start(); diff --git a/jetty-http2/http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2Client.java b/jetty-http2/http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2Client.java index 0fa71b70d24..d07a650b85c 100644 --- a/jetty-http2/http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2Client.java +++ b/jetty-http2/http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2Client.java @@ -152,7 +152,7 @@ public class HTTP2Client extends ContainerLifeCycle if (sslContextFactory != null) { ALPNClientConnectionFactory alpn = new ALPNClientConnectionFactory(getExecutor(), h2, getProtocols()); - factory = new SslClientConnectionFactory(sslContextFactory, getByteBufferPool(), getExecutor(), alpn); + factory = newSslClientConnectionFactory(sslContextFactory, alpn); } return factory.newConnection(endPoint, context); }); @@ -173,6 +173,11 @@ public class HTTP2Client extends ContainerLifeCycle return new ClientSelectorManager(getExecutor(), getScheduler(), getSelectors()); } + protected ClientConnectionFactory newSslClientConnectionFactory(SslContextFactory sslContextFactory, ClientConnectionFactory connectionFactory) + { + return new SslClientConnectionFactory(sslContextFactory, getByteBufferPool(), getExecutor(), connectionFactory); + } + public Executor getExecutor() { return executor; diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslClientConnectionFactory.java b/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslClientConnectionFactory.java index 60662741ad8..44493286f99 100644 --- a/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslClientConnectionFactory.java +++ b/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslClientConnectionFactory.java @@ -42,6 +42,7 @@ public class SslClientConnectionFactory implements ClientConnectionFactory private final ByteBufferPool byteBufferPool; private final Executor executor; private final ClientConnectionFactory connectionFactory; + private boolean allowMissingCloseMessage = true; public SslClientConnectionFactory(SslContextFactory sslContextFactory, ByteBufferPool byteBufferPool, Executor executor, ClientConnectionFactory connectionFactory) { @@ -51,6 +52,16 @@ public class SslClientConnectionFactory implements ClientConnectionFactory this.connectionFactory = connectionFactory; } + public boolean isAllowMissingCloseMessage() + { + return allowMissingCloseMessage; + } + + public void setAllowMissingCloseMessage(boolean allowMissingCloseMessage) + { + this.allowMissingCloseMessage = allowMissingCloseMessage; + } + @Override public org.eclipse.jetty.io.Connection newConnection(EndPoint endPoint, Map context) throws IOException { @@ -84,6 +95,7 @@ public class SslClientConnectionFactory implements ClientConnectionFactory SslConnection sslConnection = (SslConnection)connection; sslConnection.setRenegotiationAllowed(sslContextFactory.isRenegotiationAllowed()); sslConnection.setRenegotiationLimit(sslContextFactory.getRenegotiationLimit()); + sslConnection.setAllowMissingCloseMessage(isAllowMissingCloseMessage()); ContainerLifeCycle connector = (ContainerLifeCycle)context.get(ClientConnectionFactory.CONNECTOR_CONTEXT_KEY); connector.getBeans(SslHandshakeListener.class).forEach(sslConnection::addHandshakeListener); } 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 b192a9d9b88..55974eddf2a 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 @@ -92,6 +92,7 @@ public class SslConnection extends AbstractConnection private boolean _renegotiationAllowed; private int _renegotiationLimit = -1; private boolean _closedOutbound; + private boolean _allowMissingCloseMessage = true; private final Runnable _runCompletWrite = new Runnable() { @Override @@ -193,6 +194,16 @@ public class SslConnection extends AbstractConnection _renegotiationLimit = renegotiationLimit; } + public boolean isAllowMissingCloseMessage() + { + return _allowMissingCloseMessage; + } + + public void setAllowMissingCloseMessage(boolean allowMissingCloseMessage) + { + this._allowMissingCloseMessage = allowMissingCloseMessage; + } + @Override public void onOpen() { @@ -602,7 +613,7 @@ public class SslConnection extends AbstractConnection if (_underFlown) { - if (net_filled < 0) + if (net_filled < 0 && _sslEngine.getUseClientMode()) closeInbound(); if (net_filled <= 0) return net_filled; @@ -800,30 +811,46 @@ public class SslConnection extends AbstractConnection { if (LOG.isDebugEnabled()) LOG.debug("Renegotiation denied {}", SslConnection.this); - closeInbound(); + shutdownInput(); return false; } - if (_renegotiationLimit==0) + if (getRenegotiationLimit()==0) { if (LOG.isDebugEnabled()) LOG.debug("Renegotiation limit exceeded {}", SslConnection.this); - closeInbound(); + shutdownInput(); return false; } return true; } - private void closeInbound() + private void shutdownInput() { + try + { + _sslEngine.closeInbound(); + } + catch (Throwable x) + { + LOG.ignore(x); + } + } + + private void closeInbound() throws SSLException + { + HandshakeStatus handshakeStatus = _sslEngine.getHandshakeStatus(); try { _sslEngine.closeInbound(); } catch (SSLException x) { - LOG.ignore(x); + if (handshakeStatus == HandshakeStatus.NOT_HANDSHAKING && !isAllowMissingCloseMessage()) + throw x; + else + LOG.ignore(x); } }