Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes #6072 - jetty server high CPU when client sends TLS record length > 17408 (jetty-10.0.x) #6074

Merged
merged 2 commits into from Mar 22, 2021
Merged

Fixes #6072 - jetty server high CPU when client sends TLS record length > 17408 (jetty-10.0.x) #6074

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5925b48
Fixes #6072 - jetty server high CPU when client send data length > 17…
joakime Mar 18, 2021
040124b
Fixes #6072 - jetty server high CPU when client send data length > 17…
sbordet Mar 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 9 additions & 5 deletions jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java
View file
Open in desktop
Expand Up @@ -712,8 +712,15 @@ public int fill(ByteBuffer buffer) throws IOException
return filled = -1;

case BUFFER_UNDERFLOW:
if (BufferUtil.space(_encryptedInput) == 0)
{
BufferUtil.clear(_encryptedInput);
throw new SSLHandshakeException("Encrypted buffer max length exceeded");
}

if (netFilled > 0)
continue; // try filling some more

_underflown = true;
if (netFilled < 0 && _sslEngine.getUseClientMode())
{
Expand All @@ -722,7 +729,7 @@ public int fill(ByteBuffer buffer) throws IOException
{
Throwable handshakeFailure = new SSLHandshakeException("Abruptly closed by peer");
if (closeFailure != null)
handshakeFailure.initCause(closeFailure);
handshakeFailure.addSuppressed(closeFailure);
throw handshakeFailure;
}
return filled = -1;
Expand Down Expand Up @@ -1418,9 +1425,7 @@ private boolean isRenegotiating()
return false;
if (isTLS13())
return false;
if (_sslEngine.getHandshakeStatus() == HandshakeStatus.NOT_HANDSHAKING)
return false;
return true;
return _sslEngine.getHandshakeStatus() != HandshakeStatus.NOT_HANDSHAKING;
}

private boolean allowRenegotiate()
Expand Down Expand Up @@ -1554,6 +1559,5 @@ public String toString()
return String.format("SSL@%h.DEP.writeCallback", SslConnection.this);
}
}

}
}
71 changes: 70 additions & 1 deletion jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SSLEngineTest.java
View file
Open in desktop
Expand Up @@ -29,19 +29,31 @@
import java.net.SocketException;
import java.net.SocketTimeoutException;
import java.net.URL;
import java.nio.ByteBuffer;
import java.util.Arrays;
import java.util.concurrent.atomic.AtomicLong;
import javax.net.SocketFactory;
import javax.net.ssl.HostnameVerifier;
import javax.net.ssl.HttpsURLConnection;
import javax.net.ssl.SSLContext;
import javax.net.ssl.SSLEngine;
import javax.net.ssl.SSLEngineResult;
import javax.net.ssl.SSLException;
import javax.net.ssl.SSLSession;
import javax.servlet.ServletException;
import javax.servlet.ServletOutputStream;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import org.eclipse.jetty.io.EndPoint;
import org.eclipse.jetty.io.ssl.SslConnection;
import org.eclipse.jetty.server.ConnectionFactory;
import org.eclipse.jetty.server.Connector;
import org.eclipse.jetty.server.HttpConnectionFactory;
import org.eclipse.jetty.server.Request;
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.ServerConnector;
import org.eclipse.jetty.server.SslConnectionFactory;
import org.eclipse.jetty.server.handler.AbstractHandler;
import org.eclipse.jetty.toolchain.test.MavenTestingUtils;
import org.eclipse.jetty.util.IO;
Expand All @@ -54,6 +66,7 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.greaterThan;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.lessThan;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;

Expand Down Expand Up @@ -101,12 +114,13 @@ public class SSLEngineTest

private Server server;
private ServerConnector connector;
private SslContextFactory.Server sslContextFactory;

@BeforeEach
public void startServer() throws Exception
{
String keystore = MavenTestingUtils.getTestResourceFile("keystore.p12").getAbsolutePath();
SslContextFactory.Server sslContextFactory = new SslContextFactory.Server();
sslContextFactory = new SslContextFactory.Server();
sslContextFactory.setKeyStorePath(keystore);
sslContextFactory.setKeyStorePassword("storepwd");

Expand Down Expand Up @@ -185,6 +199,61 @@ public void testBigResponse() throws Exception
assertThat(response.length(), greaterThan(102400));
}

@Test
public void testInvalidLargeTLSFrame() throws Exception
{
AtomicLong unwraps = new AtomicLong();
ConnectionFactory http = connector.getConnectionFactory(HttpConnectionFactory.class);
ConnectionFactory ssl = new SslConnectionFactory(sslContextFactory, 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
{
unwraps.incrementAndGet();
return super.unwrap(sslEngine, input, output);
}
};
}
};
ServerConnector tlsConnector = new ServerConnector(server, 1, 1, ssl, http);
server.addConnector(tlsConnector);
server.setHandler(new HelloWorldHandler());
server.start();

// Create raw TLS record.
byte[] bytes = new byte[20005];
Arrays.fill(bytes, (byte)1);

bytes[0] = 22; // record type
bytes[1] = 3; // major version
bytes[2] = 3; // minor version
bytes[3] = 78; // record length 2 bytes / 0x4E20 / decimal 20,000
bytes[4] = 32; // record length
bytes[5] = 1; // message type
bytes[6] = 0; // message length 3 bytes / 0x004E17 / decimal 19,991
bytes[7] = 78;
bytes[8] = 23;

SocketFactory socketFactory = SocketFactory.getDefault();
try (Socket client = socketFactory.createSocket("localhost", tlsConnector.getLocalPort()))
{
client.getOutputStream().write(bytes);

// Sleep to see if the server spins.
Thread.sleep(1000);
assertThat(unwraps.get(), lessThan(128L));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move the assertion out of the try-with-resource block as the IO.readBytes call already would wait 1s if this bug is triggered.


// Read until -1 or read timeout.
client.setSoTimeout(1000);
IO.readBytes(client.getInputStream());
}
}

@Test
public void testRequestJettyHttps() throws Exception
{
Expand Down