Eclipse
/
jetty.project
mirror of https://github.com/jetty/jetty.project.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Issue #4217 - SslConnection.DecryptedEndpoint.flush eternal busy loop. 

Releasing the decrypted input buffer so that it can be re-acquired
with an expanded capacity.
Looping around only if the buffer size has changed.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
This commit is contained in:
Simone Bordet 2019-10-18 19:48:14 +02:00
parent 2e633a4e86
commit 991cf20cce
2 changed files with 82 additions and 103 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

79
jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTLSTest.java
View File

@ -32,7 +32,6 @@ import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutionException;
import java.util.concurrent.Executor; import java.util.concurrent.Executor;
import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicLong;
import java.util.concurrent.atomic.AtomicReference; import java.util.concurrent.atomic.AtomicReference;
import javax.net.ssl.SSLEngine; import javax.net.ssl.SSLEngine;
@ -49,7 +48,6 @@ import org.eclipse.jetty.http.HttpStatus;
import org.eclipse.jetty.io.ByteBufferPool; import org.eclipse.jetty.io.ByteBufferPool;
import org.eclipse.jetty.io.ClientConnectionFactory; import org.eclipse.jetty.io.ClientConnectionFactory;
import org.eclipse.jetty.io.EndPoint; import org.eclipse.jetty.io.EndPoint;
import org.eclipse.jetty.io.MappedByteBufferPool;
import org.eclipse.jetty.io.ssl.SslClientConnectionFactory; import org.eclipse.jetty.io.ssl.SslClientConnectionFactory;
import org.eclipse.jetty.io.ssl.SslConnection; import org.eclipse.jetty.io.ssl.SslConnection;
import org.eclipse.jetty.io.ssl.SslHandshakeListener; import org.eclipse.jetty.io.ssl.SslHandshakeListener;
@ -75,7 +73,6 @@ import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.instanceOf;
import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertArrayEquals;
import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.assertTrue;
@ -810,80 +807,4 @@ public class HttpClientTLSTest
Throwable cause = failure.getCause(); Throwable cause = failure.getCause();
assertThat(cause, Matchers.instanceOf(SSLHandshakeException.class)); assertThat(cause, Matchers.instanceOf(SSLHandshakeException.class));
} }
@Test
public void testTLSLargeFragments() throws Exception
{
// The SSLEngine implementation may read a TLS packet that is larger than the default size,
// because the other peer uses large TLS fragments.
// When this happens, the SSLEngine implementation expands the SSLSession.packetBufferSize
// _during_ the TLS handshake, which causes a BUFFER_OVERFLOW result when trying to wrap().
SslContextFactory serverTLSFactory = createServerSslContextFactory();
startServer(serverTLSFactory, new EmptyServerHandler());
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)
{
AtomicInteger outputHash = new AtomicInteger();
byteBufferPool = new MappedByteBufferPool()
{
@Override
public void release(ByteBuffer buffer)
{
// Discard the TLS buffer so that it cannot be re-acquired.
if (outputHash.get() != System.identityHashCode(buffer))
super.release(buffer);
}
};
return new SslConnection(byteBufferPool, executor, endPoint, engine, isDirectBuffersForEncryption(), isDirectBuffersForDecryption())
{
private int wrapCount;
@Override
protected SSLEngineResult wrap(SSLEngine sslEngine, ByteBuffer[] input, ByteBuffer output) throws SSLException
{
if (++wrapCount == 1)
{
outputHash.set(System.identityHashCode(output));
// Replace the output buffer with one that will cause BUFFER_OVERFLOW.
output = ByteBuffer.allocate(1);
SSLEngineResult result = super.wrap(sslEngine, input, output);
assertEquals(SSLEngineResult.Status.BUFFER_OVERFLOW, result.getStatus());
return result;
}
else
{
// Make sure that if there was a BUFFER_OVERFLOW, we re-acquire
// the output buffer with potentially a different capacity due
// to the buffer expansion performed by the TLS implementation.
assertNotEquals(outputHash, System.identityHashCode(output));
return super.wrap(sslEngine, input, output);
}
}
};
}
};
}
};
client.setExecutor(clientThreads);
client.start();
ContentResponse response = client.newRequest("localhost", connector.getLocalPort())
.scheme(HttpScheme.HTTPS.asString())
.send();
assertEquals(HttpStatus.OK_200, response.getStatus());
}
} }

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

@ -31,6 +31,7 @@ import javax.net.ssl.SSLEngineResult.HandshakeStatus;
import javax.net.ssl.SSLEngineResult.Status; import javax.net.ssl.SSLEngineResult.Status;
import javax.net.ssl.SSLException; import javax.net.ssl.SSLException;
import javax.net.ssl.SSLHandshakeException; import javax.net.ssl.SSLHandshakeException;
import javax.net.ssl.SSLSession;
import org.eclipse.jetty.io.AbstractConnection; import org.eclipse.jetty.io.AbstractConnection;
import org.eclipse.jetty.io.AbstractEndPoint; import org.eclipse.jetty.io.AbstractEndPoint;
@ -308,10 +309,38 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
return state == HandshakeState.SUCCEEDED || state == HandshakeState.FAILED; return state == HandshakeState.SUCCEEDED || state == HandshakeState.FAILED;
} }
private int getApplicationBufferSize()
{
SSLSession hsSession = _sslEngine.getHandshakeSession();
SSLSession session = _sslEngine.getSession();
int size = session.getApplicationBufferSize();
if (hsSession == null)
return size;
int hsSize = hsSession.getApplicationBufferSize();
return Math.max(hsSize, size);
}
private int getPacketBufferSize()
{
SSLSession hsSession = _sslEngine.getHandshakeSession();
SSLSession session = _sslEngine.getSession();
int size = session.getPacketBufferSize();
if (hsSession == null)
return size;
int hsSize = hsSession.getPacketBufferSize();
return Math.max(hsSize, size);
}
private void acquireEncryptedInput() private void acquireEncryptedInput()
{ {
if (_encryptedInput == null) 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 @Override
@ -409,6 +438,24 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
connection instanceof AbstractConnection ? ((AbstractConnection)connection).toConnectionString() : connection); 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() private void releaseEncryptedOutputBuffer()
{ {
if (!Thread.holdsLock(_decryptedEndPoint)) if (!Thread.holdsLock(_decryptedEndPoint))
@ -544,9 +591,12 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
{ {
if (connection instanceof 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); super.setConnection(connection);
} }
@ -613,12 +663,13 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
// can we use the passed buffer if it is big enough // can we use the passed buffer if it is big enough
ByteBuffer appIn; ByteBuffer appIn;
int appBufferSize = getApplicationBufferSize();
if (_decryptedInput == null) if (_decryptedInput == null)
{ {
if (BufferUtil.space(buffer) > _sslEngine.getSession().getApplicationBufferSize()) if (BufferUtil.space(buffer) > appBufferSize)
appIn = buffer; appIn = buffer;
else else
appIn = _decryptedInput = _bufferPool.acquire(_sslEngine.getSession().getApplicationBufferSize(), _decryptedDirectBuffers); appIn = _decryptedInput = _bufferPool.acquire(appBufferSize, _decryptedDirectBuffers);
} }
else else
{ {
@ -698,8 +749,21 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
} }
return filled = netFilled; 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: case OK:
{
if (unwrapResult.getHandshakeStatus() == HandshakeStatus.FINISHED) if (unwrapResult.getHandshakeStatus() == HandshakeStatus.FINISHED)
handshakeSucceeded(); handshakeSucceeded();
@ -717,7 +781,6 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
} }
break; break;
}
default: default:
throw new IllegalStateException("Unexpected unwrap result " + unwrap); throw new IllegalStateException("Unexpected unwrap result " + unwrap);
@ -737,17 +800,8 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
} }
finally finally
{ {
if (_encryptedInput != null && !_encryptedInput.hasRemaining()) releaseEncryptedInputBuffer();
{ releaseDecryptedInputBuffer();
_bufferPool.release(_encryptedInput);
_encryptedInput = null;
}
if (_decryptedInput != null && !_decryptedInput.hasRemaining())
{
_bufferPool.release(_decryptedInput);
_decryptedInput = null;
}
if (_flushState == FlushState.WAIT_FOR_FILL) if (_flushState == FlushState.WAIT_FOR_FILL)
{ {
@ -974,8 +1028,8 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
throw new IllegalStateException("Unexpected HandshakeStatus " + status); throw new IllegalStateException("Unexpected HandshakeStatus " + status);
} }
if (_encryptedOutput == null) int packetBufferSize = getPacketBufferSize();
_encryptedOutput = _bufferPool.acquire(_sslEngine.getSession().getPacketBufferSize(), _encryptedDirectBuffers); acquireEncryptedOutput();
if (_handshake.compareAndSet(HandshakeState.INITIAL, HandshakeState.HANDSHAKE)) if (_handshake.compareAndSet(HandshakeState.INITIAL, HandshakeState.HANDSHAKE))
{ {
@ -1034,13 +1088,17 @@ public class SslConnection extends AbstractConnection implements Connection.Upgr
if (!flushed) if (!flushed)
return result = false; return result = false;
// It's possible that SSLSession.packetBufferSize has been expanded // It's possible that SSLSession.packetBufferSize has been expanded
// during the TLS handshake. Wrapping a large application buffer // by the SSLEngine implementation. Wrapping a large application buffer
// causes BUFFER_OVERFLOW because the (old) packetBufferSize is // causes BUFFER_OVERFLOW because the (old) packetBufferSize is
// too small. Release the encrypted output buffer so that it will // too small. Release the encrypted output buffer so that it will
// be re-acquired with the larger capacity. // be re-acquired with the larger capacity.
// See also system property "jsse.SSLEngine.acceptLargeFragments". // See also system property "jsse.SSLEngine.acceptLargeFragments".
releaseEncryptedOutputBuffer(); if (packetBufferSize < getPacketBufferSize())
continue; {
releaseEncryptedOutputBuffer();
continue;
}
throw new IllegalStateException("Unexpected wrap result " + wrap);
case OK: case OK:
if (wrapResult.getHandshakeStatus() == HandshakeStatus.FINISHED) if (wrapResult.getHandshakeStatus() == HandshakeStatus.FINISHED)