Visitar URL original
Merge pull request #4218 from eclipse/jetty-9.4.x-4217-tls_flush_buff… · jetty/jetty.project@add8ffc · GitHub
Skip to content

Commit add8ffc

Browse files
sbordetsbordet
authored
Merge pull request #4218 from eclipse/jetty-9.4.x-4217-tls_flush_buffer_overflow_busy_loop
Fixes #4217 - SslConnection.DecryptedEnpoint.flush eternal busy loop.
2 parents a547a77 + 73eb82c commit add8ffc

2 files changed

Lines changed: 205 additions & 23 deletions

File tree

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

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -807,4 +807,120 @@ protected SSLEngineResult wrap(SSLEngine sslEngine, ByteBuffer[] input, ByteBuff
807807
Throwable cause = failure.getCause();
808808
assertThat(cause, Matchers.instanceOf(SSLHandshakeException.class));
809809
}
810+
811+
@Test
812+
public void testTLSLargeFragments() throws Exception
813+
{
814+
CountDownLatch serverLatch = new CountDownLatch(1);
815+
SslContextFactory serverTLSFactory = createServerSslContextFactory();
816+
QueuedThreadPool serverThreads = new QueuedThreadPool();
817+
serverThreads.setName("server");
818+
server = new Server(serverThreads);
819+
HttpConfiguration httpConfig = new HttpConfiguration();
820+
httpConfig.addCustomizer(new SecureRequestCustomizer());
821+
HttpConnectionFactory http = new HttpConnectionFactory(httpConfig);
822+
SslConnectionFactory ssl = new SslConnectionFactory(serverTLSFactory, http.getProtocol())
823+
{
824+
@Override
825+
protected SslConnection newSslConnection(Connector connector, EndPoint endPoint, SSLEngine engine)
826+
{
827+
return new SslConnection(connector.getByteBufferPool(), connector.getExecutor(), endPoint, engine, isDirectBuffersForEncryption(), isDirectBuffersForDecryption())
828+
{
829+
@Override
830+
protected SSLEngineResult unwrap(SSLEngine sslEngine, ByteBuffer input, ByteBuffer output) throws SSLException
831+
{
832+
int inputBytes = input.remaining();
833+
SSLEngineResult result = super.unwrap(sslEngine, input, output);
834+
if (inputBytes == 5)
835+
serverLatch.countDown();
836+
return result;
837+
}
838+
};
839+
}
840+
};
841+
connector = new ServerConnector(server, 1, 1, ssl, http);
842+
server.addConnector(connector);
843+
server.setHandler(new EmptyServerHandler());
844+
server.start();
845+
846+
long idleTimeout = 2000;
847+
848+
CountDownLatch clientLatch = new CountDownLatch(1);
849+
SslContextFactory clientTLSFactory = createClientSslContextFactory();
850+
QueuedThreadPool clientThreads = new QueuedThreadPool();
851+
clientThreads.setName("client");
852+
client = new HttpClient(clientTLSFactory)
853+
{
854+
@Override
855+
protected ClientConnectionFactory newSslClientConnectionFactory(SslContextFactory sslContextFactory, ClientConnectionFactory connectionFactory)
856+
{
857+
if (sslContextFactory == null)
858+
sslContextFactory = getSslContextFactory();
859+
return new SslClientConnectionFactory(sslContextFactory, getByteBufferPool(), getExecutor(), connectionFactory)
860+
{
861+
@Override
862+
protected SslConnection newSslConnection(ByteBufferPool byteBufferPool, Executor executor, EndPoint endPoint, SSLEngine engine)
863+
{
864+
return new SslConnection(byteBufferPool, executor, endPoint, engine, isDirectBuffersForEncryption(), isDirectBuffersForDecryption())
865+
{
866+
@Override
867+
protected SSLEngineResult wrap(SSLEngine sslEngine, ByteBuffer[] input, ByteBuffer output) throws SSLException
868+
{
869+
try
870+
{
871+
clientLatch.countDown();
872+
assertTrue(serverLatch.await(5, TimeUnit.SECONDS));
873+
return super.wrap(sslEngine, input, output);
874+
}
875+
catch (InterruptedException x)
876+
{
877+
throw new SSLException(x);
878+
}
879+
}
880+
};
881+
}
882+
};
883+
}
884+
};
885+
client.setIdleTimeout(idleTimeout);
886+
client.setExecutor(clientThreads);
887+
client.start();
888+
889+
String host = "localhost";
890+
int port = connector.getLocalPort();
891+
892+
CountDownLatch responseLatch = new CountDownLatch(1);
893+
client.newRequest(host, port)
894+
.scheme(HttpScheme.HTTPS.asString())
895+
.send(result ->
896+
{
897+
assertTrue(result.isSucceeded());
898+
assertEquals(HttpStatus.OK_200, result.getResponse().getStatus());
899+
responseLatch.countDown();
900+
});
901+
// Wait for the TLS buffers to be acquired by the client, then the
902+
// HTTP request will be paused waiting for the TLS buffer to be expanded.
903+
assertTrue(clientLatch.await(5, TimeUnit.SECONDS));
904+
905+
// Send the large frame bytes that will enlarge the TLS buffers.
906+
try (Socket socket = new Socket(host, port))
907+
{
908+
OutputStream output = socket.getOutputStream();
909+
byte[] largeFrameBytes = new byte[5];
910+
largeFrameBytes[0] = 22; // Type = handshake
911+
largeFrameBytes[1] = 3; // Major TLS version
912+
largeFrameBytes[2] = 3; // Minor TLS version
913+
// Frame length is 0x7FFF == 32767, i.e. a "large fragment".
914+
// Maximum allowed by RFC 8446 is 16384, but SSLEngine supports up to 33093.
915+
largeFrameBytes[3] = 0x7F; // Length hi byte
916+
largeFrameBytes[4] = (byte)0xFF; // Length lo byte
917+
output.write(largeFrameBytes);
918+
output.flush();
919+
// Just close the connection now, the large frame
920+
// length was enough to trigger the buffer expansion.
921+
}
922+
923+
// The HTTP request will resume and be forced to handle the TLS buffer expansion.
924+
assertTrue(responseLatch.await(5, TimeUnit.SECONDS));
925+
}
810926
}

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

Lines changed: 89 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,14 @@
2525
import java.util.List;
2626
import java.util.concurrent.Executor;
2727
import java.util.concurrent.atomic.AtomicReference;
28+
import java.util.function.ToIntFunction;
2829
import javax.net.ssl.SSLEngine;
2930
import javax.net.ssl.SSLEngineResult;
3031
import javax.net.ssl.SSLEngineResult.HandshakeStatus;
3132
import javax.net.ssl.SSLEngineResult.Status;
3233
import javax.net.ssl.SSLException;
3334
import javax.net.ssl.SSLHandshakeException;
35+
import javax.net.ssl.SSLSession;
3436

3537
import org.eclipse.jetty.io.AbstractConnection;
3638
import org.eclipse.jetty.io.AbstractEndPoint;
@@ -308,10 +310,37 @@ private boolean isHandshakeComplete()
308310
return state == HandshakeState.SUCCEEDED || state == HandshakeState.FAILED;
309311
}
310312

313+
private int getApplicationBufferSize()
314+
{
315+
return getBufferSize(SSLSession::getApplicationBufferSize);
316+
}
317+
318+
private int getPacketBufferSize()
319+
{
320+
return getBufferSize(SSLSession::getPacketBufferSize);
321+
}
322+
323+
private int getBufferSize(ToIntFunction<SSLSession> bufferSizeFn)
324+
{
325+
SSLSession hsSession = _sslEngine.getHandshakeSession();
326+
SSLSession session = _sslEngine.getSession();
327+
int size = bufferSizeFn.applyAsInt(session);
328+
if (hsSession == null || hsSession == session)
329+
return size;
330+
int hsSize = bufferSizeFn.applyAsInt(hsSession);
331+
return Math.max(hsSize, size);
332+
}
333+
311334
private void acquireEncryptedInput()
312335
{
313336
if (_encryptedInput == null)
314-
_encryptedInput = _bufferPool.acquire(_sslEngine.getSession().getPacketBufferSize(), _encryptedDirectBuffers);
337+
_encryptedInput = _bufferPool.acquire(getPacketBufferSize(), _encryptedDirectBuffers);
338+
}
339+
340+
private void acquireEncryptedOutput()
341+
{
342+
if (_encryptedOutput == null)
343+
_encryptedOutput = _bufferPool.acquire(getPacketBufferSize(), _encryptedDirectBuffers);
315344
}
316345

317346
@Override
@@ -409,6 +438,24 @@ public String toConnectionString()
409438
connection instanceof AbstractConnection ? ((AbstractConnection)connection).toConnectionString() : connection);
410439
}
411440

441+
private void releaseEncryptedInputBuffer()
442+
{
443+
if (_encryptedInput != null && !_encryptedInput.hasRemaining())
444+
{
445+
_bufferPool.release(_encryptedInput);
446+
_encryptedInput = null;
447+
}
448+
}
449+
450+
protected void releaseDecryptedInputBuffer()
451+
{
452+
if (_decryptedInput != null && !_decryptedInput.hasRemaining())
453+
{
454+
_bufferPool.release(_decryptedInput);
455+
_decryptedInput = null;
456+
}
457+
}
458+
412459
private void releaseEncryptedOutputBuffer()
413460
{
414461
if (!Thread.holdsLock(_decryptedEndPoint))
@@ -544,9 +591,12 @@ public void setConnection(Connection connection)
544591
{
545592
if (connection instanceof AbstractConnection)
546593
{
547-
AbstractConnection a = (AbstractConnection)connection;
548-
if (a.getInputBufferSize() < _sslEngine.getSession().getApplicationBufferSize())
549-
a.setInputBufferSize(_sslEngine.getSession().getApplicationBufferSize());
594+
// This is an optimization to avoid that upper layer connections use small
595+
// buffers and we need to copy decrypted data rather than decrypting in place.
596+
AbstractConnection c = (AbstractConnection)connection;
597+
int appBufferSize = getApplicationBufferSize();
598+
if (c.getInputBufferSize() < appBufferSize)
599+
c.setInputBufferSize(appBufferSize);
550600
}
551601
super.setConnection(connection);
552602
}
@@ -613,12 +663,13 @@ public int fill(ByteBuffer buffer) throws IOException
613663

614664
// can we use the passed buffer if it is big enough
615665
ByteBuffer appIn;
666+
int appBufferSize = getApplicationBufferSize();
616667
if (_decryptedInput == null)
617668
{
618-
if (BufferUtil.space(buffer) > _sslEngine.getSession().getApplicationBufferSize())
669+
if (BufferUtil.space(buffer) > appBufferSize)
619670
appIn = buffer;
620671
else
621-
appIn = _decryptedInput = _bufferPool.acquire(_sslEngine.getSession().getApplicationBufferSize(), _decryptedDirectBuffers);
672+
appIn = _decryptedInput = _bufferPool.acquire(appBufferSize, _decryptedDirectBuffers);
622673
}
623674
else
624675
{
@@ -698,8 +749,21 @@ public int fill(ByteBuffer buffer) throws IOException
698749
}
699750
return filled = netFilled;
700751

752+
case BUFFER_OVERFLOW:
753+
// It's possible that SSLSession.applicationBufferSize has been expanded
754+
// by the SSLEngine implementation. Unwrapping a large encrypted buffer
755+
// causes BUFFER_OVERFLOW because the (old) applicationBufferSize is
756+
// too small. Release the decrypted input buffer so it will be re-acquired
757+
// with the larger capacity.
758+
// See also system property "jsse.SSLEngine.acceptLargeFragments".
759+
if (BufferUtil.isEmpty(_decryptedInput) && appBufferSize < getApplicationBufferSize())
760+
{
761+
releaseDecryptedInputBuffer();
762+
continue;
763+
}
764+
throw new IllegalStateException("Unexpected unwrap result " + unwrap);
765+
701766
case OK:
702-
{
703767
if (unwrapResult.getHandshakeStatus() == HandshakeStatus.FINISHED)
704768
handshakeSucceeded();
705769

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

719783
break;
720-
}
721784

722785
default:
723786
throw new IllegalStateException("Unexpected unwrap result " + unwrap);
@@ -737,17 +800,8 @@ public int fill(ByteBuffer buffer) throws IOException
737800
}
738801
finally
739802
{
740-
if (_encryptedInput != null && !_encryptedInput.hasRemaining())
741-
{
742-
_bufferPool.release(_encryptedInput);
743-
_encryptedInput = null;
744-
}
745-
746-
if (_decryptedInput != null && !_decryptedInput.hasRemaining())
747-
{
748-
_bufferPool.release(_decryptedInput);
749-
_decryptedInput = null;
750-
}
803+
releaseEncryptedInputBuffer();
804+
releaseDecryptedInputBuffer();
751805

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

977-
if (_encryptedOutput == null)
978-
_encryptedOutput = _bufferPool.acquire(_sslEngine.getSession().getPacketBufferSize(), _encryptedDirectBuffers);
1031+
int packetBufferSize = getPacketBufferSize();
1032+
acquireEncryptedOutput();
9791033

9801034
if (_handshake.compareAndSet(HandshakeState.INITIAL, HandshakeState.HANDSHAKE))
9811035
{
9821036
if (LOG.isDebugEnabled())
9831037
LOG.debug("flush starting handshake {}", SslConnection.this);
9841038
}
9851039

986-
// We call sslEngine.wrap to try to take bytes from appOut buffers and encrypt them into the _netOut buffer
1040+
// We call sslEngine.wrap to try to take bytes from appOuts
1041+
// buffers and encrypt them into the _encryptedOutput buffer.
9871042
BufferUtil.compact(_encryptedOutput);
9881043
int pos = BufferUtil.flipToFill(_encryptedOutput);
9891044
SSLEngineResult wrapResult;
@@ -1032,7 +1087,18 @@ public boolean flush(ByteBuffer... appOuts) throws IOException
10321087
case BUFFER_OVERFLOW:
10331088
if (!flushed)
10341089
return result = false;
1035-
continue;
1090+
// It's possible that SSLSession.packetBufferSize has been expanded
1091+
// by the SSLEngine implementation. Wrapping a large application buffer
1092+
// causes BUFFER_OVERFLOW because the (old) packetBufferSize is
1093+
// too small. Release the encrypted output buffer so that it will
1094+
// be re-acquired with the larger capacity.
1095+
// See also system property "jsse.SSLEngine.acceptLargeFragments".
1096+
if (packetBufferSize < getPacketBufferSize())
1097+
{
1098+
releaseEncryptedOutputBuffer();
1099+
continue;
1100+
}
1101+
throw new IllegalStateException("Unexpected wrap result " + wrap);
10361102

10371103
case OK:
10381104
if (wrapResult.getHandshakeStatus() == HandshakeStatus.FINISHED)

0 commit comments

Comments
 (0)