Skip to content

Commit

Permalink
Merge pull request #4218 from eclipse/jetty-9.4.x-4217-tls_flush_buff…
Browse files Browse the repository at this point in the history 
…er_overflow_busy_loop

Fixes #4217 - SslConnection.DecryptedEnpoint.flush eternal busy loop.
  • Loading branch information
@sbordet
sbordet authored Oct 21, 2019
2 parents a547a77 + 73eb82c commit add8ffc
Show file tree
Hide file tree
Showing 2 changed files with 205 additions and 23 deletions.
116 changes: 116 additions & 0 deletions jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTLSTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -807,4 +807,120 @@ protected SSLEngineResult wrap(SSLEngine sslEngine, ByteBuffer[] input, ByteBuff
Throwable cause = failure.getCause();
assertThat(cause, Matchers.instanceOf(SSLHandshakeException.class));
}

@Test
public void testTLSLargeFragments() throws Exception
{
CountDownLatch serverLatch = new CountDownLatch(1);
SslContextFactory serverTLSFactory = createServerSslContextFactory();
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 = createClientSslContextFactory();
QueuedThreadPool clientThreads = new QueuedThreadPool();
clientThreads.setName("client");
client = new HttpClient(clientTLSFactory)
{
@Override
protected ClientConnectionFactory newSslClientConnectionFactory(SslContextFactory sslContextFactory, ClientConnectionFactory connectionFactory)
{
if (sslContextFactory == null)
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));
}
}
112 changes: 89 additions & 23 deletions jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,14 @@
import java.util.List;
import java.util.concurrent.Executor;
import java.util.concurrent.atomic.AtomicReference;
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;
Expand Down Expand Up @@ -308,10 +310,37 @@ private boolean isHandshakeComplete()
return state == HandshakeState.SUCCEEDED || state == HandshakeState.FAILED;
}

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(_sslEngine.getSession().getPacketBufferSize(), _encryptedDirectBuffers);
_encryptedInput = _bufferPool.acquire(getPacketBufferSize(), _encryptedDirectBuffers);
}

private void acquireEncryptedOutput()
{
if (_encryptedOutput == null)
_encryptedOutput = _bufferPool.acquire(getPacketBufferSize(), _encryptedDirectBuffers);
}

@Override
Expand Down Expand Up @@ -409,6 +438,24 @@ public String toConnectionString()
connection instanceof AbstractConnection ? ((AbstractConnection)connection).toConnectionString() : connection);
}

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;
}
}

private void releaseEncryptedOutputBuffer()
{
if (!Thread.holdsLock(_decryptedEndPoint))
Expand Down Expand Up @@ -544,9 +591,12 @@ public void setConnection(Connection connection)
{
if (connection instanceof AbstractConnection)
{
AbstractConnection a = (AbstractConnection)connection;
if (a.getInputBufferSize() < _sslEngine.getSession().getApplicationBufferSize())
a.setInputBufferSize(_sslEngine.getSession().getApplicationBufferSize());
// This is an optimization to avoid that upper layer connections use small
// buffers and we need to copy decrypted data rather than decrypting in place.
AbstractConnection c = (AbstractConnection)connection;
int appBufferSize = getApplicationBufferSize();
if (c.getInputBufferSize() < appBufferSize)
c.setInputBufferSize(appBufferSize);
}
super.setConnection(connection);
}
Expand Down Expand Up @@ -613,12 +663,13 @@ public int fill(ByteBuffer buffer) throws IOException

// can we use the passed buffer if it is big enough
ByteBuffer appIn;
int appBufferSize = getApplicationBufferSize();
if (_decryptedInput == null)
{
if (BufferUtil.space(buffer) > _sslEngine.getSession().getApplicationBufferSize())
if (BufferUtil.space(buffer) > appBufferSize)
appIn = buffer;
else
appIn = _decryptedInput = _bufferPool.acquire(_sslEngine.getSession().getApplicationBufferSize(), _decryptedDirectBuffers);
appIn = _decryptedInput = _bufferPool.acquire(appBufferSize, _decryptedDirectBuffers);
}
else
{
Expand Down Expand Up @@ -698,8 +749,21 @@ public int fill(ByteBuffer buffer) throws IOException
}
return filled = netFilled;

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();
continue;
}
throw new IllegalStateException("Unexpected unwrap result " + unwrap);

case OK:
{
if (unwrapResult.getHandshakeStatus() == HandshakeStatus.FINISHED)
handshakeSucceeded();

Expand All @@ -717,7 +781,6 @@ public int fill(ByteBuffer buffer) throws IOException
}

break;
}

default:
throw new IllegalStateException("Unexpected unwrap result " + unwrap);
Expand All @@ -737,17 +800,8 @@ public int fill(ByteBuffer buffer) throws IOException
}
finally
{
if (_encryptedInput != null && !_encryptedInput.hasRemaining())
{
_bufferPool.release(_encryptedInput);
_encryptedInput = null;
}

if (_decryptedInput != null && !_decryptedInput.hasRemaining())
{
_bufferPool.release(_decryptedInput);
_decryptedInput = null;
}
releaseEncryptedInputBuffer();
releaseDecryptedInputBuffer();

if (_flushState == FlushState.WAIT_FOR_FILL)
{
Expand Down Expand Up @@ -974,16 +1028,17 @@ public boolean flush(ByteBuffer... appOuts) throws IOException
throw new IllegalStateException("Unexpected HandshakeStatus " + status);
}

if (_encryptedOutput == null)
_encryptedOutput = _bufferPool.acquire(_sslEngine.getSession().getPacketBufferSize(), _encryptedDirectBuffers);
int packetBufferSize = getPacketBufferSize();
acquireEncryptedOutput();

if (_handshake.compareAndSet(HandshakeState.INITIAL, HandshakeState.HANDSHAKE))
{
if (LOG.isDebugEnabled())
LOG.debug("flush starting handshake {}", SslConnection.this);
}

// We call sslEngine.wrap to try to take bytes from appOut buffers and encrypt them into the _netOut buffer
// 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;
Expand Down Expand Up @@ -1032,7 +1087,18 @@ public boolean flush(ByteBuffer... appOuts) throws IOException
case BUFFER_OVERFLOW:
if (!flushed)
return result = false;
continue;
// 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 " + wrap);

case OK:
if (wrapResult.getHandshakeStatus() == HandshakeStatus.FINISHED)
Expand Down

0 comments on commit add8ffc

Please sign in to comment.