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

HTTP/2 improvements for CVE-2023-36478 #9749

Merged
merged 9 commits into from
May 27, 2023
Merged

HTTP/2 improvements for CVE-2023-36478 #9749

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
11c8ab5
HTTP/2 improvements.
sbordet May 8, 2023
0d043a8
Updates after review.
sbordet May 15, 2023
cad4417
Merged branch 'jetty-10.0.x' into 'fix/jetty-10-http2-improvements'.
sbordet May 16, 2023
4ee50ef
More configuration updates.
sbordet May 19, 2023
e27df0d
Updates from review.
sbordet May 19, 2023
3f0869a
Using a constant for the default maxTableSize.
sbordet May 23, 2023
b7bf3b9
Updates after review.
sbordet May 25, 2023
dd31c1b
Merged branch 'jetty-10.0.x' into 'fix/jetty-10-http2-improvements'.
sbordet May 25, 2023
0aa7aed
Updates after review.
sbordet May 26, 2023
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
18 changes: 18 additions & 0 deletions jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ public class HttpClient extends ContainerLifeCycle
private String defaultRequestContentType = "application/octet-stream";
private boolean useInputDirectByteBuffers = true;
private boolean useOutputDirectByteBuffers = true;
private int maxResponseHeadersSize = -1;
private Sweeper destinationSweeper;

/**
Expand Down Expand Up @@ -1181,6 +1182,23 @@ public void setUseOutputDirectByteBuffers(boolean useOutputDirectByteBuffers)
this.useOutputDirectByteBuffers = useOutputDirectByteBuffers;
}

/**
* @return the max size in bytes of the response headers
*/
@ManagedAttribute("The max size in bytes of the response headers")
public int getMaxResponseHeadersSize()
{
return maxResponseHeadersSize;
}

/**
* @param maxResponseHeadersSize the max size in bytes of the response headers
*/
public void setMaxResponseHeadersSize(int maxResponseHeadersSize)
{
this.maxResponseHeadersSize = maxResponseHeadersSize;
}

/**
* @return the forward proxy configuration
*/
Expand Down
2 changes: 1 addition & 1 deletion jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpReceiverOverHTTP.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public HttpReceiverOverHTTP(HttpChannelOverHTTP channel)
{
super(channel);
HttpClient httpClient = channel.getHttpDestination().getHttpClient();
parser = new HttpParser(this, -1, httpClient.getHttpCompliance());
parser = new HttpParser(this, httpClient.getMaxResponseHeadersSize(), httpClient.getHttpCompliance());
HttpClientTransport transport = httpClient.getTransport();
if (transport instanceof HttpClientTransportOverHTTP)
{
Expand Down
68 changes: 58 additions & 10 deletions jetty-http2/http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2Client.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,11 +106,13 @@ public class HTTP2Client extends ContainerLifeCycle
private List<String> protocols = List.of("h2");
private int initialSessionRecvWindow = 16 * 1024 * 1024;
private int initialStreamRecvWindow = 8 * 1024 * 1024;
private int maxFrameLength = Frame.DEFAULT_MAX_LENGTH;
private int maxFrameSize = Frame.DEFAULT_MAX_LENGTH;
private int maxConcurrentPushedStreams = 32;
private int maxSettingsKeys = SettingsFrame.DEFAULT_MAX_KEYS;
private int maxDynamicTableSize = 4096;
private int maxDecoderTableSize = 4096;
private int maxEncoderTableSize = 4096;
private int maxHeaderBlockFragment = 0;
private int maxResponseHeadersSize = -1;
private FlowControlStrategy.Factory flowControlStrategyFactory = () -> new BufferingFlowControlStrategy(0.5F);
private long streamIdleTimeout;
private boolean useInputDirectByteBuffers = true;
Expand Down Expand Up @@ -282,15 +284,27 @@ public void setInitialStreamRecvWindow(int initialStreamRecvWindow)
this.initialStreamRecvWindow = initialStreamRecvWindow;
}

@ManagedAttribute("The max frame length in bytes")
@Deprecated
public int getMaxFrameLength()
{
return maxFrameLength;
return getMaxFrameSize();
}

public void setMaxFrameLength(int maxFrameLength)
@Deprecated
public void setMaxFrameLength(int maxFrameSize)
{
this.maxFrameLength = maxFrameLength;
setMaxFrameSize(maxFrameSize);
}

@ManagedAttribute("The max frame size in bytes")
public int getMaxFrameSize()
{
return maxFrameSize;
}

public void setMaxFrameSize(int maxFrameSize)
{
this.maxFrameSize = maxFrameSize;
}

@ManagedAttribute("The max number of concurrent pushed streams")
Expand All @@ -315,15 +329,38 @@ public void setMaxSettingsKeys(int maxSettingsKeys)
this.maxSettingsKeys = maxSettingsKeys;
}

@ManagedAttribute("The HPACK dynamic table maximum size")
@ManagedAttribute("The HPACK encoder dynamic table maximum size")
public int getMaxEncoderTableSize()
{
return maxEncoderTableSize;
}

public void setMaxEncoderTableSize(int maxEncoderTableSize)
{
this.maxEncoderTableSize = maxEncoderTableSize;
}

@ManagedAttribute("The HPACK decoder dynamic table maximum size")
public int getMaxDecoderTableSize()
{
return maxDecoderTableSize;
}

public void setMaxDecoderTableSize(int maxDecoderTableSize)
{
this.maxDecoderTableSize = maxDecoderTableSize;
}

@Deprecated
public int getMaxDynamicTableSize()
{
return maxDynamicTableSize;
return getMaxDecoderTableSize();
}

public void setMaxDynamicTableSize(int maxDynamicTableSize)
@Deprecated
public void setMaxDynamicTableSize(int maxTableSize)
{
this.maxDynamicTableSize = maxDynamicTableSize;
setMaxDecoderTableSize(maxTableSize);
}

@ManagedAttribute("The max size of header block fragments")
Expand All @@ -337,6 +374,17 @@ public void setMaxHeaderBlockFragment(int maxHeaderBlockFragment)
this.maxHeaderBlockFragment = maxHeaderBlockFragment;
}

@ManagedAttribute("The max size of response headers")
public int getMaxResponseHeadersSize()
{
return maxResponseHeadersSize;
}

public void setMaxResponseHeadersSize(int maxResponseHeadersSize)
{
this.maxResponseHeadersSize = maxResponseHeadersSize;
}

@ManagedAttribute("Whether to use direct ByteBuffers for reading")
public boolean isUseInputDirectByteBuffers()
{
Expand Down
77 changes: 60 additions & 17 deletions ...tp2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.eclipse.jetty.http2.HTTP2Connection;
import org.eclipse.jetty.http2.ISession;
import org.eclipse.jetty.http2.api.Session;
import org.eclipse.jetty.http2.frames.Frame;
import org.eclipse.jetty.http2.frames.PrefaceFrame;
import org.eclipse.jetty.http2.frames.SettingsFrame;
import org.eclipse.jetty.http2.frames.WindowUpdateFrame;
Expand Down Expand Up @@ -54,27 +55,29 @@ public Connection newConnection(EndPoint endPoint, Map<String, Object> context)
Scheduler scheduler = client.getScheduler();
Session.Listener listener = (Session.Listener)context.get(SESSION_LISTENER_CONTEXT_KEY);
@SuppressWarnings("unchecked")
Promise<Session> promise = (Promise<Session>)context.get(SESSION_PROMISE_CONTEXT_KEY);
Promise<Session> sessionPromise = (Promise<Session>)context.get(SESSION_PROMISE_CONTEXT_KEY);

Generator generator = new Generator(byteBufferPool, client.getMaxDynamicTableSize(), client.getMaxHeaderBlockFragment());
Generator generator = new Generator(byteBufferPool, client.getMaxEncoderTableSize(), client.getMaxHeaderBlockFragment());
FlowControlStrategy flowControl = client.getFlowControlStrategyFactory().newFlowControlStrategy();
HTTP2ClientSession session = new HTTP2ClientSession(scheduler, endPoint, generator, listener, flowControl);

Parser parser = new Parser(byteBufferPool, client.getMaxDecoderTableSize(), client.getMaxResponseHeadersSize());
parser.setMaxFrameSize(client.getMaxFrameSize());
parser.setMaxSettingsKeys(client.getMaxSettingsKeys());

HTTP2ClientSession session = new HTTP2ClientSession(scheduler, endPoint, parser, generator, listener, flowControl);
gregw marked this conversation as resolved.
Show resolved Hide resolved
session.setMaxRemoteStreams(client.getMaxConcurrentPushedStreams());
long streamIdleTimeout = client.getStreamIdleTimeout();
if (streamIdleTimeout > 0)
session.setStreamIdleTimeout(streamIdleTimeout);

Parser parser = new Parser(byteBufferPool, session, 4096, 8192);
parser.setMaxFrameLength(client.getMaxFrameLength());
parser.setMaxSettingsKeys(client.getMaxSettingsKeys());

RetainableByteBufferPool retainableByteBufferPool = byteBufferPool.asRetainableByteBufferPool();

HTTP2ClientConnection connection = new HTTP2ClientConnection(client, retainableByteBufferPool, executor, endPoint,
parser, session, client.getInputBufferSize(), promise, listener);
session, client.getInputBufferSize(), sessionPromise, listener);
connection.setUseInputDirectByteBuffers(client.isUseInputDirectByteBuffers());
connection.setUseOutputDirectByteBuffers(client.isUseOutputDirectByteBuffers());
connection.addEventListener(connectionListener);
parser.init(connection);

return customize(connection, context);
}

Expand All @@ -84,11 +87,11 @@ private static class HTTP2ClientConnection extends HTTP2Connection implements Ca
private final Promise<Session> promise;
private final Session.Listener listener;

private HTTP2ClientConnection(HTTP2Client client, RetainableByteBufferPool retainableByteBufferPool, Executor executor, EndPoint endpoint, Parser parser, ISession session, int bufferSize, Promise<Session> promise, Session.Listener listener)
private HTTP2ClientConnection(HTTP2Client client, RetainableByteBufferPool retainableByteBufferPool, Executor executor, EndPoint endpoint, HTTP2ClientSession session, int bufferSize, Promise<Session> sessionPromise, Session.Listener listener)
{
super(retainableByteBufferPool, executor, endpoint, parser, session, bufferSize);
super(retainableByteBufferPool, executor, endpoint, session, bufferSize);
this.client = client;
this.promise = promise;
this.promise = sessionPromise;
this.listener = listener;
}

Expand All @@ -98,12 +101,52 @@ public void onOpen()
Map<Integer, Integer> settings = listener.onPreface(getSession());
if (settings == null)
settings = new HashMap<>();
settings.computeIfAbsent(SettingsFrame.INITIAL_WINDOW_SIZE, k -> client.getInitialStreamRecvWindow());
settings.computeIfAbsent(SettingsFrame.MAX_CONCURRENT_STREAMS, k -> client.getMaxConcurrentPushedStreams());

Integer maxFrameLength = settings.get(SettingsFrame.MAX_FRAME_SIZE);
if (maxFrameLength != null)
getParser().setMaxFrameLength(maxFrameLength);
// Below we want to populate any settings to send to the server
// that have a different default than what prescribed by the RFC.
// Changing the configuration is done when the SETTINGS is sent.

lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
settings.compute(SettingsFrame.HEADER_TABLE_SIZE, (k, v) ->
{
if (v == null)
{
v = client.getMaxDecoderTableSize();
if (v == 4096)
v = null;
}
return v;
});
settings.computeIfAbsent(SettingsFrame.MAX_CONCURRENT_STREAMS, k -> client.getMaxConcurrentPushedStreams());
settings.compute(SettingsFrame.INITIAL_WINDOW_SIZE, (k, v) ->
{
if (v == null)
{
v = client.getInitialStreamRecvWindow();
if (v == FlowControlStrategy.DEFAULT_WINDOW_SIZE)
v = null;
}
return v;
});
settings.compute(SettingsFrame.MAX_FRAME_SIZE, (k, v) ->
gregw marked this conversation as resolved.
Show resolved Hide resolved
{
if (v == null)
{
v = client.getMaxFrameSize();
if (v == Frame.DEFAULT_MAX_LENGTH)
v = null;
}
return v;
});
settings.compute(SettingsFrame.MAX_HEADER_LIST_SIZE, (k, v) ->
{
if (v == null)
{
v = client.getMaxResponseHeadersSize();
if (v <= 0)
v = null;
}
return v;
});
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved

PrefaceFrame prefaceFrame = new PrefaceFrame();
SettingsFrame settingsFrame = new SettingsFrame(settings, false);
Expand Down
26 changes: 24 additions & 2 deletions ...y-http2/http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientSession.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@

package org.eclipse.jetty.http2.client;

import java.util.Map;

import org.eclipse.jetty.http.MetaData;
import org.eclipse.jetty.http2.CloseState;
import org.eclipse.jetty.http2.ErrorCode;
Expand All @@ -23,7 +25,9 @@
import org.eclipse.jetty.http2.api.Stream;
import org.eclipse.jetty.http2.frames.HeadersFrame;
import org.eclipse.jetty.http2.frames.PushPromiseFrame;
import org.eclipse.jetty.http2.frames.SettingsFrame;
import org.eclipse.jetty.http2.generator.Generator;
import org.eclipse.jetty.http2.parser.Parser;
import org.eclipse.jetty.io.EndPoint;
import org.eclipse.jetty.util.Callback;
import org.eclipse.jetty.util.thread.Scheduler;
Expand All @@ -34,9 +38,9 @@ public class HTTP2ClientSession extends HTTP2Session
{
private static final Logger LOG = LoggerFactory.getLogger(HTTP2ClientSession.class);

public HTTP2ClientSession(Scheduler scheduler, EndPoint endPoint, Generator generator, Session.Listener listener, FlowControlStrategy flowControl)
public HTTP2ClientSession(Scheduler scheduler, EndPoint endPoint, Parser parser, Generator generator, Session.Listener listener, FlowControlStrategy flowControl)
{
super(scheduler, endPoint, generator, listener, flowControl, 1);
super(scheduler, endPoint, parser, generator, listener, flowControl, 1);
}

@Override
Expand Down Expand Up @@ -87,12 +91,30 @@ public void onHeaders(HeadersFrame frame)
}
}

@Override
public void onSettings(SettingsFrame frame)
{
Map<Integer, Integer> settings = frame.getSettings();
Integer value = settings.get(SettingsFrame.ENABLE_PUSH);
// SPEC: servers can only send ENABLE_PUSH=0.
if (value != null && value != 0)
onConnectionFailure(ErrorCode.PROTOCOL_ERROR.code, "invalid_settings_frame");
else
super.onSettings(frame);
}

@Override
public void onPushPromise(PushPromiseFrame frame)
{
if (LOG.isDebugEnabled())
LOG.debug("Received {}", frame);

if (!isPushEnabled())
{
onConnectionFailure(ErrorCode.PROTOCOL_ERROR.code, "unexpected_push_promise_frame");
return;
}

int streamId = frame.getStreamId();
int pushStreamId = frame.getPromisedStreamId();
IStream stream = getStream(streamId);
Expand Down
1 change: 1 addition & 0 deletions ...tp2-client/src/test/java/org/eclipse/jetty/http2/client/ConcurrentStreamCreationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ public void onHeaders(Stream stream, HeadersFrame frame)
x.printStackTrace();
}
}).start());

assertTrue(clientLatch.await(total, TimeUnit.MILLISECONDS), String.format("Missing streams on client: %d/%d", clientLatch.getCount(), total));
assertTrue(serverLatch.await(total, TimeUnit.MILLISECONDS), String.format("Missing streams on server: %d/%d", serverLatch.getCount(), total));
assertTrue(responseLatch.await(total, TimeUnit.MILLISECONDS), String.format("Missing response on client: %d/%d", clientLatch.getCount(), total));
Expand Down