Issue #4217 - SslConnection DecryptedEndpoint flush eternal busy loop

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>

jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTLSTest.java

@@ -23,13 +23,16 @@ import java.io.InputStreamReader;
 import java.io.OutputStream;
 import java.net.ServerSocket;
 import java.net.Socket;
+import java.nio.ByteBuffer;
 import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Executor;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicReference;
-
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLEngineResult;
 import javax.net.ssl.SSLException;
 import javax.net.ssl.SSLSocket;
 
@@ -37,12 +40,20 @@ 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.ByteBufferPool;
 import org.eclipse.jetty.io.ClientConnectionFactory;
+import org.eclipse.jetty.io.EndPoint;
 import org.eclipse.jetty.io.ssl.SslClientConnectionFactory;
+import org.eclipse.jetty.io.ssl.SslConnection;
 import org.eclipse.jetty.io.ssl.SslHandshakeListener;
+import org.eclipse.jetty.server.Connector;
 import org.eclipse.jetty.server.Handler;
+import org.eclipse.jetty.server.HttpConfiguration;
+import org.eclipse.jetty.server.HttpConnectionFactory;
+import org.eclipse.jetty.server.SecureRequestCustomizer;
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.server.ServerConnector;
+import org.eclipse.jetty.server.SslConnectionFactory;
 import org.eclipse.jetty.util.ssl.SslContextFactory;
 import org.eclipse.jetty.util.thread.QueuedThreadPool;
 import org.hamcrest.Matchers;
@@ -50,6 +61,9 @@ import org.junit.After;
 import org.junit.Assert;
 import org.junit.Test;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
 public class HttpClientTLSTest
 {
     private Server server;
@@ -516,4 +530,119 @@ public class HttpClientTLSTest
             Assert.assertTrue(latch.await(5, TimeUnit.SECONDS));
         }
     }
+
+    @Test
+    public void testTLSLargeFragments() throws Exception
+    {
+        CountDownLatch serverLatch = new CountDownLatch(1);
+        SslContextFactory serverTLSFactory = createSslContextFactory();
+        QueuedThreadPool serverThreads = new QueuedThreadPool();
+        serverThreads.setName("server");
+        server = new Server(serverThreads);
+        HttpConfiguration httpConfig = new HttpConfiguration();
+        httpConfig.addCustomizer(new SecureRequestCustomizer());
+        HttpConnectionFactory http = new HttpConnectionFactory(httpConfig);
+        SslConnectionFactory ssl = new SslConnectionFactory(serverTLSFactory, http.getProtocol())
+        {
+            @Override
+            protected SslConnection newSslConnection(Connector connector, EndPoint endPoint, SSLEngine engine)
+            {
+                return new SslConnection(connector.getByteBufferPool(), connector.getExecutor(), endPoint, engine, isDirectBuffersForEncryption(), isDirectBuffersForDecryption())
+                {
+                    @Override
+                    protected SSLEngineResult unwrap(SSLEngine sslEngine, ByteBuffer input, ByteBuffer output) throws SSLException
+                    {
+                        int inputBytes = input.remaining();
+                        SSLEngineResult result = super.unwrap(sslEngine, input, output);
+                        if (inputBytes == 5)
+                            serverLatch.countDown();
+                        return result;
+                    }
+                };
+            }
+        };
+        connector = new ServerConnector(server, 1, 1, ssl, http);
+        server.addConnector(connector);
+        server.setHandler(new EmptyServerHandler());
+        server.start();
+
+        long idleTimeout = 2000;
+
+        CountDownLatch clientLatch = new CountDownLatch(1);
+        SslContextFactory clientTLSFactory = createSslContextFactory();
+        QueuedThreadPool clientThreads = new QueuedThreadPool();
+        clientThreads.setName("client");
+        client = new HttpClient(clientTLSFactory)
+        {
+            @Override
+            protected ClientConnectionFactory newSslClientConnectionFactory(ClientConnectionFactory connectionFactory)
+            {
+                SslContextFactory sslContextFactory = getSslContextFactory();
+                return new SslClientConnectionFactory(sslContextFactory, getByteBufferPool(), getExecutor(), connectionFactory)
+                {
+                    @Override
+                    protected SslConnection newSslConnection(ByteBufferPool byteBufferPool, Executor executor, EndPoint endPoint, SSLEngine engine)
+                    {
+                        return new SslConnection(byteBufferPool, executor, endPoint, engine, isDirectBuffersForEncryption(), isDirectBuffersForDecryption())
+                        {
+                            @Override
+                            protected SSLEngineResult wrap(SSLEngine sslEngine, ByteBuffer[] input, ByteBuffer output) throws SSLException
+                            {
+                                try
+                                {
+                                    clientLatch.countDown();
+                                    assertTrue(serverLatch.await(5, TimeUnit.SECONDS));
+                                    return super.wrap(sslEngine, input, output);
+                                }
+                                catch (InterruptedException x)
+                                {
+                                    throw new SSLException(x);
+                                }
+                            }
+                        };
+                    }
+                };
+            }
+        };
+        client.setIdleTimeout(idleTimeout);
+        client.setExecutor(clientThreads);
+        client.start();
+
+        String host = "localhost";
+        int port = connector.getLocalPort();
+
+        CountDownLatch responseLatch = new CountDownLatch(1);
+        client.newRequest(host, port)
+            .scheme(HttpScheme.HTTPS.asString())
+            .send(result ->
+            {
+                assertTrue(result.isSucceeded());
+                assertEquals(HttpStatus.OK_200, result.getResponse().getStatus());
+                responseLatch.countDown();
+            });
+        // Wait for the TLS buffers to be acquired by the client, then the
+        // HTTP request will be paused waiting for the TLS buffer to be expanded.
+        assertTrue(clientLatch.await(5, TimeUnit.SECONDS));
+
+        // Send the large frame bytes that will enlarge the TLS buffers.
+        try (Socket socket = new Socket(host, port))
+        {
+            OutputStream output = socket.getOutputStream();
+            byte[] largeFrameBytes = new byte[5];
+            largeFrameBytes[0] = 22; // Type = handshake
+            largeFrameBytes[1] = 3; // Major TLS version
+            largeFrameBytes[2] = 3; // Minor TLS version
+            // Frame length is 0x7FFF == 32767, i.e. a "large fragment".
+            // Maximum allowed by RFC 8446 is 16384, but SSLEngine supports up to 33093.
+            largeFrameBytes[3] = 0x7F; // Length hi byte
+            largeFrameBytes[4] = (byte)0xFF; // Length lo byte
+            output.write(largeFrameBytes);
+            output.flush();
+            // Just close the connection now, the large frame
+            // length was enough to trigger the buffer expansion.
+        }
+
+        // The HTTP request will resume and be forced to handle the TLS buffer expansion.
+        assertTrue(responseLatch.await(5, TimeUnit.SECONDS));
+    }
 }

jetty-client/src/test/resources/jetty-logging.properties

@@ -1,3 +1,4 @@
 org.eclipse.jetty.util.log.class=org.eclipse.jetty.util.log.StdErrLog
 #org.eclipse.jetty.LEVEL=DEBUG
 #org.eclipse.jetty.client.LEVEL=DEBUG
+#org.eclipse.jetty.io.ssl.LEVEL=DEBUG

jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java

@@ -24,13 +24,14 @@ import java.nio.channels.ClosedChannelException;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.concurrent.Executor;
-
+import java.util.function.ToIntFunction;
 import javax.net.ssl.SSLEngine;
 import javax.net.ssl.SSLEngineResult;
 import javax.net.ssl.SSLEngineResult.HandshakeStatus;
 import javax.net.ssl.SSLEngineResult.Status;
 import javax.net.ssl.SSLException;
 import javax.net.ssl.SSLHandshakeException;
+import javax.net.ssl.SSLSession;
 
 import org.eclipse.jetty.io.AbstractConnection;
 import org.eclipse.jetty.io.AbstractEndPoint;
@@ -38,6 +39,7 @@ import org.eclipse.jetty.io.ByteBufferPool;
 import org.eclipse.jetty.io.Connection;
 import org.eclipse.jetty.io.EndPoint;
 import org.eclipse.jetty.io.EofException;
+import org.eclipse.jetty.io.RuntimeIOException;
 import org.eclipse.jetty.io.SelectChannelEndPoint;
 import org.eclipse.jetty.io.WriteFlusher;
 import org.eclipse.jetty.util.BufferUtil;
@@ -212,6 +214,57 @@ public class SslConnection extends AbstractConnection
         this._allowMissingCloseMessage = allowMissingCloseMessage;
     }
 
+    private int getApplicationBufferSize()
+    {
+        return getBufferSize(SSLSession::getApplicationBufferSize);
+    }
+
+    private int getPacketBufferSize()
+    {
+        return getBufferSize(SSLSession::getPacketBufferSize);
+    }
+
+    private int getBufferSize(ToIntFunction<SSLSession> bufferSizeFn)
+    {
+        SSLSession hsSession = _sslEngine.getHandshakeSession();
+        SSLSession session = _sslEngine.getSession();
+        int size = bufferSizeFn.applyAsInt(session);
+        if (hsSession == null || hsSession == session)
+            return size;
+        int hsSize = bufferSizeFn.applyAsInt(hsSession);
+        return Math.max(hsSize, size);
+    }
+
+    private void acquireEncryptedInput()
+    {
+        if (_encryptedInput == null)
+            _encryptedInput = _bufferPool.acquire(getPacketBufferSize(), _encryptedDirectBuffers);
+    }
+
+    private void acquireEncryptedOutput()
+    {
+        if (_encryptedOutput == null)
+            _encryptedOutput = _bufferPool.acquire(getPacketBufferSize(), _encryptedDirectBuffers);
+    }
+
+    private void releaseEncryptedInputBuffer()
+    {
+        if (_encryptedInput != null && !_encryptedInput.hasRemaining())
+        {
+            _bufferPool.release(_encryptedInput);
+            _encryptedInput = null;
+        }
+    }
+
+    protected void releaseDecryptedInputBuffer()
+    {
+        if (_decryptedInput != null && !_decryptedInput.hasRemaining())
+        {
+            _bufferPool.release(_decryptedInput);
+            _decryptedInput = null;
+        }
+    }
+
     @Override
     public void onOpen()
     {
@@ -298,6 +351,16 @@ public class SslConnection extends AbstractConnection
             _decryptedEndPoint.getWriteFlusher().onFail(cause);
     }
 
+    protected SSLEngineResult wrap(SSLEngine sslEngine, ByteBuffer[] input, ByteBuffer output) throws SSLException
+    {
+        return sslEngine.wrap(input, output);
+    }
+
+    protected SSLEngineResult unwrap(SSLEngine sslEngine, ByteBuffer input, ByteBuffer output) throws SSLException
+    {
+        return sslEngine.unwrap(input, output);
+    }
+
     @Override
     public String toString()
     {
@@ -540,9 +603,12 @@ public class SslConnection extends AbstractConnection
         {
             if (connection instanceof AbstractConnection)
             {
-                AbstractConnection a = (AbstractConnection)connection;
+                // This is an optimization to avoid that upper layer connections use small
-                if (a.getInputBufferSize()<_sslEngine.getSession().getApplicationBufferSize())
+                // buffers and we need to copy decrypted data rather than decrypting in place.
-                    a.setInputBufferSize(_sslEngine.getSession().getApplicationBufferSize());
+                AbstractConnection c = (AbstractConnection)connection;
+                int appBufferSize = getApplicationBufferSize();
+                if (c.getInputBufferSize() < appBufferSize)
+                    c.setInputBufferSize(appBufferSize);
             }
             super.setConnection(connection);
         }
@@ -572,18 +638,22 @@ public class SslConnection extends AbstractConnection
                         else
                             BufferUtil.compact(_encryptedInput);
 
-                        // We also need an app buffer, but can use the passed buffer if it is big enough
-                        ByteBuffer app_in;
-                        if (BufferUtil.space(buffer) > _sslEngine.getSession().getApplicationBufferSize())
-                            app_in = buffer;
-                        else if (_decryptedInput == null)
-                            app_in = _decryptedInput = _bufferPool.acquire(_sslEngine.getSession().getApplicationBufferSize(), _decryptedDirectBuffers);
-                        else
-                            app_in = _decryptedInput;
 
                         // loop filling and unwrapping until we have something
                         while (true)
                         {
+                            // We also need an app buffer, but can use the passed buffer if it is big enough
+                            ByteBuffer app_in;
+                            int appBufferSize = getApplicationBufferSize();
+                            if (BufferUtil.space(buffer) > appBufferSize)
+                                app_in = buffer;
+                            else if (_decryptedInput == null)
+                                app_in = _decryptedInput = _bufferPool.acquire(appBufferSize, _decryptedDirectBuffers);
+                            else
+                                app_in = _decryptedInput;
+
+                            acquireEncryptedInput();
+
                             // Let's try reading some encrypted data... even if we have some already.
                             int net_filled = getEndPoint().fill(_encryptedInput);
 
@@ -598,17 +668,20 @@ public class SslConnection extends AbstractConnection
                                 SSLEngineResult unwrapResult;
                                 try
                                 {
-                                    unwrapResult = _sslEngine.unwrap(_encryptedInput, app_in);
+                                    unwrapResult = unwrap(_sslEngine, _encryptedInput, app_in);
                                 }
                                 finally
                                 {
                                     BufferUtil.flipToFlush(app_in, pos);
                                 }
+
                                 if (LOG.isDebugEnabled())
-                                {
+                                    LOG.debug("unwrap net_filled={} {} encryptedBuffer={} unwrapBuffer={} appBuffer={}",
-                                    LOG.debug("net={} unwrap {} {}", net_filled, unwrapResult.toString().replace('\n',' '), SslConnection.this);
+                                        net_filled,
-                                    LOG.debug("filled {} {}",BufferUtil.toHexSummary(buffer), SslConnection.this);
+                                        unwrapResult.toString().replace('\n',' '),
-                                }
+                                        BufferUtil.toSummaryString(_encryptedInput),
+                                        BufferUtil.toDetailString(app_in),
+                                        BufferUtil.toDetailString(buffer));
 
                                 HandshakeStatus handshakeStatus = _sslEngine.getHandshakeStatus();
                                 HandshakeStatus unwrapHandshakeStatus = unwrapResult.getHandshakeStatus();
@@ -662,7 +735,41 @@ public class SslConnection extends AbstractConnection
                                             }
                                         }
                                     }
+                                    case BUFFER_OVERFLOW:
+                                        // It's possible that SSLSession.applicationBufferSize has been expanded
+                                        // by the SSLEngine implementation. Unwrapping a large encrypted buffer
+                                        // causes BUFFER_OVERFLOW because the (old) applicationBufferSize is
+                                        // too small. Release the decrypted input buffer so it will be re-acquired
+                                        // with the larger capacity.
+                                        // See also system property "jsse.SSLEngine.acceptLargeFragments".
+                                        if (BufferUtil.isEmpty(_decryptedInput) && appBufferSize < getApplicationBufferSize())
+                                        {
+                                            releaseDecryptedInputBuffer();
+                                            break decryption;
+                                        }
+                                        throw new IllegalStateException("Unexpected unwrap result " + unwrapResultStatus);
+
                                     case BUFFER_UNDERFLOW:
+                                        if (net_filled > 0)
+                                            break decryption; // try filling some more
+                                        _underFlown = true;
+                                        if (net_filled < 0 && _sslEngine.getUseClientMode())
+                                        {
+                                            try
+                                            {
+                                                closeInbound();
+                                            }
+                                            catch (SSLException closeFailure)
+                                            {
+                                                Throwable handshakeFailure = new SSLHandshakeException("Abruptly closed by peer");
+                                                if (closeFailure != null)
+                                                    handshakeFailure.initCause(closeFailure);
+                                                throw handshakeFailure;
+                                            }
+
+                                            return -1;
+                                        }
+                                        return net_filled;
                                     case OK:
                                     {
                                         if (unwrapHandshakeStatus == HandshakeStatus.FINISHED)
@@ -769,23 +876,17 @@ public class SslConnection extends AbstractConnection
                             getExecutor().execute(failure == null ? _runCompletWrite : new FailWrite(failure));
                         }
 
-                        if (_encryptedInput != null && !_encryptedInput.hasRemaining())
+                        releaseEncryptedInputBuffer();
-                        {
+                        releaseDecryptedInputBuffer();
-                            _bufferPool.release(_encryptedInput);
-                            _encryptedInput = null;
-                        }
-                        if (_decryptedInput != null && !_decryptedInput.hasRemaining())
-                        {
-                            _bufferPool.release(_decryptedInput);
-                            _decryptedInput = null;
-                        }
                     }
                 }
             }
             catch (Throwable x)
             {
                 close(x);
-                throw x;
+                if(x instanceof IOException)
+                    throw (IOException) x;
+                throw new RuntimeIOException(x);
             }
         }
 
@@ -895,19 +996,19 @@ public class SslConnection extends AbstractConnection
                             return false;
                         }
 
-                        // We will need a network buffer
-                        if (_encryptedOutput == null)
-                            _encryptedOutput = _bufferPool.acquire(_sslEngine.getSession().getPacketBufferSize(), _encryptedDirectBuffers);
-
                         while (true)
                         {
-                            // We call sslEngine.wrap to try to take bytes from appOut buffers and encrypt them into the _netOut buffer
+                            int packetBufferSize = getPacketBufferSize();
+                            acquireEncryptedOutput();
+
+                            // We call sslEngine.wrap to try to take bytes from appOuts
+                            // buffers and encrypt them into the _encryptedOutput buffer.
                             BufferUtil.compact(_encryptedOutput);
                             int pos = BufferUtil.flipToFill(_encryptedOutput);
                             SSLEngineResult wrapResult;
                             try
                             {
-                                wrapResult = _sslEngine.wrap(appOuts, _encryptedOutput);
+                                wrapResult = wrap(_sslEngine, appOuts,_encryptedOutput);
                             }
                             finally
                             {
@@ -948,6 +1049,21 @@ public class SslConnection extends AbstractConnection
                                     }
                                     return allConsumed;
                                 }
+                                case BUFFER_OVERFLOW:
+                                {
+                                    // It's possible that SSLSession.packetBufferSize has been expanded
+                                    // by the SSLEngine implementation. Wrapping a large application buffer
+                                    // causes BUFFER_OVERFLOW because the (old) packetBufferSize is
+                                    // too small. Release the encrypted output buffer so that it will
+                                    // be re-acquired with the larger capacity.
+                                    // See also system property "jsse.SSLEngine.acceptLargeFragments".
+                                    if (packetBufferSize < getPacketBufferSize())
+                                    {
+                                        releaseEncryptedOutputBuffer();
+                                        continue;
+                                    }
+                                    throw new IllegalStateException("Unexpected wrap result " + wrapResultStatus);
+                                }
                                 case BUFFER_UNDERFLOW:
                                 {
                                     throw new IllegalStateException();