Issue #2679 - HTTP/2 Spec Compliance.

Fixed sanity checks for SETTINGS values.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
This commit is contained in:
Simone Bordet 2018-07-04 16:07:31 +02:00
parent d06d5f5a71
commit 47506250c8
2 changed files with 27 additions and 25 deletions

View File

@ -306,54 +306,42 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio
case SettingsFrame.HEADER_TABLE_SIZE: case SettingsFrame.HEADER_TABLE_SIZE:
{ {
if (LOG.isDebugEnabled()) if (LOG.isDebugEnabled())
LOG.debug("Update HPACK header table size to {} for {}", value, this); LOG.debug("Updating HPACK header table size to {} for {}", value, this);
generator.setHeaderTableSize(value); generator.setHeaderTableSize(value);
break; break;
} }
case SettingsFrame.ENABLE_PUSH: case SettingsFrame.ENABLE_PUSH:
{ {
// SPEC: check the value is sane.
if (value != 0 && value != 1)
{
onConnectionFailure(ErrorCode.PROTOCOL_ERROR.code, "invalid_settings_enable_push");
return;
}
pushEnabled = value == 1;
if (LOG.isDebugEnabled()) if (LOG.isDebugEnabled())
LOG.debug("{} push for {}", pushEnabled ? "Enable" : "Disable", this); LOG.debug("{} push for {}", pushEnabled ? "Enabling" : "Disabling", this);
pushEnabled = value == 1;
break; break;
} }
case SettingsFrame.MAX_CONCURRENT_STREAMS: case SettingsFrame.MAX_CONCURRENT_STREAMS:
{ {
maxLocalStreams = value;
if (LOG.isDebugEnabled()) if (LOG.isDebugEnabled())
LOG.debug("Update max local concurrent streams to {} for {}", maxLocalStreams, this); LOG.debug("Updating max local concurrent streams to {} for {}", maxLocalStreams, this);
maxLocalStreams = value;
break; break;
} }
case SettingsFrame.INITIAL_WINDOW_SIZE: case SettingsFrame.INITIAL_WINDOW_SIZE:
{ {
if (LOG.isDebugEnabled()) if (LOG.isDebugEnabled())
LOG.debug("Update initial window size to {} for {}", value, this); LOG.debug("Updating initial window size to {} for {}", value, this);
flowControl.updateInitialStreamWindow(this, value, false); flowControl.updateInitialStreamWindow(this, value, false);
break; break;
} }
case SettingsFrame.MAX_FRAME_SIZE: case SettingsFrame.MAX_FRAME_SIZE:
{ {
if (LOG.isDebugEnabled()) if (LOG.isDebugEnabled())
LOG.debug("Update max frame size to {} for {}", value, this); LOG.debug("Updating max frame size to {} for {}", value, this);
// SPEC: check the max frame size is sane.
if (value < Frame.DEFAULT_MAX_LENGTH || value > Frame.MAX_MAX_LENGTH)
{
onConnectionFailure(ErrorCode.PROTOCOL_ERROR.code, "invalid_settings_max_frame_size");
return;
}
generator.setMaxFrameSize(value); generator.setMaxFrameSize(value);
break; break;
} }
case SettingsFrame.MAX_HEADER_LIST_SIZE: case SettingsFrame.MAX_HEADER_LIST_SIZE:
{ {
if (LOG.isDebugEnabled()) if (LOG.isDebugEnabled())
LOG.debug("Update max header list size to {} for {}", value, this); LOG.debug("Updating max header list size to {} for {}", value, this);
generator.setMaxHeaderListSize(value); generator.setMaxHeaderListSize(value);
break; break;
} }

View File

@ -25,6 +25,7 @@ import java.util.concurrent.atomic.AtomicReference;
import org.eclipse.jetty.http2.ErrorCode; import org.eclipse.jetty.http2.ErrorCode;
import org.eclipse.jetty.http2.Flags; import org.eclipse.jetty.http2.Flags;
import org.eclipse.jetty.http2.frames.Frame;
import org.eclipse.jetty.http2.frames.SettingsFrame; import org.eclipse.jetty.http2.frames.SettingsFrame;
import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Log;
import org.eclipse.jetty.util.log.Logger; import org.eclipse.jetty.util.log.Logger;
@ -57,7 +58,7 @@ public class SettingsBodyParser extends BodyParser
@Override @Override
protected void emptyBody(ByteBuffer buffer) protected void emptyBody(ByteBuffer buffer)
{ {
onSettings(new HashMap<>()); onSettings(buffer, new HashMap<>());
} }
@Override @Override
@ -120,7 +121,7 @@ public class SettingsBodyParser extends BodyParser
state = State.SETTING_ID; state = State.SETTING_ID;
length -= 4; length -= 4;
if (length == 0) if (length == 0)
return onSettings(settings); return onSettings(buffer, settings);
} }
else else
{ {
@ -145,7 +146,7 @@ public class SettingsBodyParser extends BodyParser
settings.put(settingId, settingValue); settings.put(settingId, settingValue);
state = State.SETTING_ID; state = State.SETTING_ID;
if (length == 0) if (length == 0)
return onSettings(settings); return onSettings(buffer, settings);
} }
break; break;
} }
@ -158,8 +159,21 @@ public class SettingsBodyParser extends BodyParser
return false; return false;
} }
protected boolean onSettings(Map<Integer, Integer> settings) protected boolean onSettings(ByteBuffer buffer, Map<Integer, Integer> settings)
{ {
Integer enablePush = settings.get(SettingsFrame.ENABLE_PUSH);
if (enablePush != null && enablePush != 0 && enablePush != 1)
return connectionFailure(buffer, ErrorCode.PROTOCOL_ERROR.code, "invalid_settings_enable_push");
Integer initialWindowSize = settings.get(SettingsFrame.INITIAL_WINDOW_SIZE);
// Values greater than Integer.MAX_VALUE will overflow to negative.
if (initialWindowSize != null && initialWindowSize < 0)
return connectionFailure(buffer, ErrorCode.FLOW_CONTROL_ERROR.code, "invalid_settings_initial_window_size");
Integer maxFrameLength = settings.get(SettingsFrame.MAX_FRAME_SIZE);
if (maxFrameLength != null && (maxFrameLength < Frame.DEFAULT_MAX_LENGTH || maxFrameLength > Frame.MAX_MAX_LENGTH))
return connectionFailure(buffer, ErrorCode.PROTOCOL_ERROR.code, "invalid_settings_max_frame_size");
SettingsFrame frame = new SettingsFrame(settings, hasFlag(Flags.ACK)); SettingsFrame frame = new SettingsFrame(settings, hasFlag(Flags.ACK));
reset(); reset();
notifySettings(frame); notifySettings(frame);
@ -185,7 +199,7 @@ public class SettingsBodyParser extends BodyParser
} }
@Override @Override
protected boolean onSettings(Map<Integer, Integer> settings) protected boolean onSettings(ByteBuffer buffer, Map<Integer, Integer> settings)
{ {
frameRef.set(new SettingsFrame(settings, false)); frameRef.set(new SettingsFrame(settings, false));
return true; return true;