From 47506250c8c66ff567f0dee81a3f8d9aaa764b77 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Wed, 4 Jul 2018 16:07:31 +0200 Subject: [PATCH] Issue #2679 - HTTP/2 Spec Compliance. Fixed sanity checks for SETTINGS values. Signed-off-by: Simone Bordet --- .../org/eclipse/jetty/http2/HTTP2Session.java | 28 ++++++------------- .../http2/parser/SettingsBodyParser.java | 24 ++++++++++++---- 2 files changed, 27 insertions(+), 25 deletions(-) diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java index 600c96b1c66..ff085915340 100644 --- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java +++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java @@ -306,54 +306,42 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio case SettingsFrame.HEADER_TABLE_SIZE: { 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); break; } 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()) - LOG.debug("{} push for {}", pushEnabled ? "Enable" : "Disable", this); + LOG.debug("{} push for {}", pushEnabled ? "Enabling" : "Disabling", this); + pushEnabled = value == 1; break; } case SettingsFrame.MAX_CONCURRENT_STREAMS: { - maxLocalStreams = value; 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; } case SettingsFrame.INITIAL_WINDOW_SIZE: { 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); break; } case SettingsFrame.MAX_FRAME_SIZE: { if (LOG.isDebugEnabled()) - LOG.debug("Update 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; - } + LOG.debug("Updating max frame size to {} for {}", value, this); generator.setMaxFrameSize(value); break; } case SettingsFrame.MAX_HEADER_LIST_SIZE: { 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); break; } diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/SettingsBodyParser.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/SettingsBodyParser.java index 7452f2bd17a..6a89ec51e32 100644 --- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/SettingsBodyParser.java +++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/SettingsBodyParser.java @@ -25,6 +25,7 @@ import java.util.concurrent.atomic.AtomicReference; import org.eclipse.jetty.http2.ErrorCode; import org.eclipse.jetty.http2.Flags; +import org.eclipse.jetty.http2.frames.Frame; import org.eclipse.jetty.http2.frames.SettingsFrame; import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; @@ -57,7 +58,7 @@ public class SettingsBodyParser extends BodyParser @Override protected void emptyBody(ByteBuffer buffer) { - onSettings(new HashMap<>()); + onSettings(buffer, new HashMap<>()); } @Override @@ -120,7 +121,7 @@ public class SettingsBodyParser extends BodyParser state = State.SETTING_ID; length -= 4; if (length == 0) - return onSettings(settings); + return onSettings(buffer, settings); } else { @@ -145,7 +146,7 @@ public class SettingsBodyParser extends BodyParser settings.put(settingId, settingValue); state = State.SETTING_ID; if (length == 0) - return onSettings(settings); + return onSettings(buffer, settings); } break; } @@ -158,8 +159,21 @@ public class SettingsBodyParser extends BodyParser return false; } - protected boolean onSettings(Map settings) + protected boolean onSettings(ByteBuffer buffer, Map 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)); reset(); notifySettings(frame); @@ -185,7 +199,7 @@ public class SettingsBodyParser extends BodyParser } @Override - protected boolean onSettings(Map settings) + protected boolean onSettings(ByteBuffer buffer, Map settings) { frameRef.set(new SettingsFrame(settings, false)); return true;