From 508ad4aff91dc23653ae2cbbf6b7c6549fd982f2 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Thu, 5 Sep 2019 23:11:53 +0200 Subject: [PATCH] Issue #3978 - HTTP/2 vulnerabilities. Code cleanups and reformatting. Fixed logic for SETTINGS frame replies: they are not subject to rate control. Signed-off-by: Simone Bordet --- .../eclipse/jetty/http2/client/HTTP2Test.java | 11 ++--- .../org/eclipse/jetty/http2/HTTP2Session.java | 18 ++------ .../jetty/http2/parser/HeaderParser.java | 3 +- .../eclipse/jetty/http2/parser/Parser.java | 8 +--- .../jetty/http2/parser/RateControl.java | 2 +- .../http2/parser/SettingsBodyParser.java | 5 +- .../jetty/http2/hpack/HpackDecoder.java | 9 ++-- .../jetty/http2/hpack/MetaDataBuilder.java | 2 +- .../jetty/http2/hpack/HpackDecoderTest.java | 28 +++++------ .../HttpClientTransportOverHTTP2Test.java | 8 ++-- .../client/http/MaxConcurrentStreamsTest.java | 2 - .../org/eclipse/jetty/util/MathUtils.java | 46 +++++++++++++++++++ 12 files changed, 83 insertions(+), 59 deletions(-) create mode 100644 jetty-util/src/main/java/org/eclipse/jetty/util/MathUtils.java diff --git a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/HTTP2Test.java b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/HTTP2Test.java index 98d168f6cdb..b5be288a82b 100644 --- a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/HTTP2Test.java +++ b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/HTTP2Test.java @@ -27,7 +27,6 @@ import java.util.Map; import java.util.Random; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; -import javax.servlet.ServletException; import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -153,7 +152,7 @@ public class HTTP2Test extends AbstractTest start(new HttpServlet() { @Override - protected void service(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException + protected void service(HttpServletRequest req, HttpServletResponse resp) throws IOException { resp.getOutputStream().write(content); } @@ -201,7 +200,7 @@ public class HTTP2Test extends AbstractTest start(new EmptyHttpServlet() { @Override - protected void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException + protected void service(HttpServletRequest request, HttpServletResponse response) throws IOException { IO.copy(request.getInputStream(), response.getOutputStream()); } @@ -245,7 +244,7 @@ public class HTTP2Test extends AbstractTest start(new HttpServlet() { @Override - protected void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException + protected void service(HttpServletRequest request, HttpServletResponse response) throws IOException { int download = request.getIntHeader(downloadBytes); byte[] content = new byte[download]; @@ -288,7 +287,7 @@ public class HTTP2Test extends AbstractTest start(new HttpServlet() { @Override - protected void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException + protected void service(HttpServletRequest request, HttpServletResponse response) { response.setStatus(status); } @@ -323,7 +322,7 @@ public class HTTP2Test extends AbstractTest start(new HttpServlet() { @Override - protected void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException + protected void service(HttpServletRequest request, HttpServletResponse response) { assertEquals(host, request.getServerName()); assertEquals(port, request.getServerPort()); 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 1908884b48c..a8fb651121a 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 @@ -57,6 +57,7 @@ import org.eclipse.jetty.util.AtomicBiInteger; import org.eclipse.jetty.util.Atomics; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.CountingCallback; +import org.eclipse.jetty.util.MathUtils; import org.eclipse.jetty.util.Promise; import org.eclipse.jetty.util.Retainable; import org.eclipse.jetty.util.annotation.ManagedAttribute; @@ -464,7 +465,7 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio if (stream != null) { int streamSendWindow = stream.updateSendWindow(0); - if (sumOverflows(streamSendWindow, windowDelta)) + if (MathUtils.sumOverflows(streamSendWindow, windowDelta)) { reset(new ResetFrame(streamId, ErrorCode.FLOW_CONTROL_ERROR.code), Callback.NOOP); } @@ -483,7 +484,7 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio else { int sessionSendWindow = updateSendWindow(0); - if (sumOverflows(sessionSendWindow, windowDelta)) + if (MathUtils.sumOverflows(sessionSendWindow, windowDelta)) onConnectionFailure(ErrorCode.FLOW_CONTROL_ERROR.code, "invalid_flow_control_window"); else onWindowUpdate(null, frame); @@ -501,19 +502,6 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements ISessio callback.succeeded(); } - private boolean sumOverflows(int a, int b) - { - try - { - Math.addExact(a, b); - return false; - } - catch (ArithmeticException x) - { - return true; - } - } - @Override public void onConnectionFailure(int error, String reason) { diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/HeaderParser.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/HeaderParser.java index c973eae1cbb..e735d9ada3e 100644 --- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/HeaderParser.java +++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/HeaderParser.java @@ -33,13 +33,12 @@ public class HeaderParser private final RateControl rateControl; private State state = State.LENGTH; private int cursor; - private int length; private int type; private int flags; private int streamId; - HeaderParser(RateControl rateControl) + public HeaderParser(RateControl rateControl) { this.rateControl = rateControl; } diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/Parser.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/Parser.java index f0363ae6287..7545b6fe159 100644 --- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/Parser.java +++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/Parser.java @@ -61,7 +61,7 @@ public class Parser public Parser(ByteBufferPool byteBufferPool, Listener listener, int maxDynamicTableSize, int maxHeaderSize) { - this (byteBufferPool, listener, maxDynamicTableSize, maxHeaderSize, RateControl.NO_RATE_CONTROL); + this(byteBufferPool, listener, maxDynamicTableSize, maxHeaderSize, RateControl.NO_RATE_CONTROL); } public Parser(ByteBufferPool byteBufferPool, Listener listener, int maxDynamicTableSize, int maxHeaderSize, RateControl rateControl) @@ -76,7 +76,6 @@ public class Parser public void init(UnaryOperator wrapper) { Listener listener = wrapper.apply(this.listener); - RateControl rateControl = getRateControl(); unknownBodyParser = new UnknownBodyParser(headerParser, listener); HeaderBlockParser headerBlockParser = new HeaderBlockParser(headerParser, byteBufferPool, hpackDecoder, unknownBodyParser); HeaderBlockFragments headerBlockFragments = new HeaderBlockFragments(); @@ -240,11 +239,6 @@ public class Parser this.maxSettingsKeys = maxSettingsKeys; } - public RateControl getRateControl() - { - return headerParser.getRateControl(); - } - protected void notifyConnectionFailure(int error, String reason) { try diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/RateControl.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/RateControl.java index c811603d44f..be8f82a7cf2 100644 --- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/RateControl.java +++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/RateControl.java @@ -23,7 +23,7 @@ package org.eclipse.jetty.http2.parser; */ public interface RateControl { - RateControl NO_RATE_CONTROL = event -> true; + public static final RateControl NO_RATE_CONTROL = event -> true; /** *

Applications should call this method when they want to signal an 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 97e3e28c65b..b58fa8a8b51 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 @@ -73,8 +73,9 @@ public class SettingsBodyParser extends BodyParser @Override protected void emptyBody(ByteBuffer buffer) { - SettingsFrame frame = new SettingsFrame(Collections.emptyMap(), hasFlag(Flags.ACK)); - if (!rateControlOnEvent(frame)) + boolean isReply = hasFlag(Flags.ACK); + SettingsFrame frame = new SettingsFrame(Collections.emptyMap(), isReply); + if (!isReply && !rateControlOnEvent(frame)) connectionFailure(buffer, ErrorCode.ENHANCE_YOUR_CALM_ERROR.code, "invalid_settings_frame_rate"); else onSettings(frame); diff --git a/jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/HpackDecoder.java b/jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/HpackDecoder.java index 1021bc98273..bb1d3ede5fb 100644 --- a/jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/HpackDecoder.java +++ b/jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/HpackDecoder.java @@ -176,16 +176,17 @@ public class HpackDecoder name = Huffman.decode(buffer, length); else name = toASCIIString(buffer, length); - check: for (int i = name.length(); i-- > 0;) + check: + for (int i = name.length(); i-- > 0;) { char c = name.charAt(i); - if (c>0xff) + if (c > 0xff) { _builder.streamException("Illegal header name %s", name); - break check; + break; } HttpTokens.Token token = HttpTokens.TOKENS[0xFF & c]; - switch(token.getType()) + switch (token.getType()) { case ALPHA: if (c >= 'A' && c <= 'Z') diff --git a/jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/MetaDataBuilder.java b/jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/MetaDataBuilder.java index 41f02c3df3f..ce1f8b36cbe 100644 --- a/jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/MetaDataBuilder.java +++ b/jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/MetaDataBuilder.java @@ -115,7 +115,7 @@ public class MetaDataBuilder { case C_STATUS: if (checkPseudoHeader(header, _status)) - _status = Integer.valueOf(field.getIntValue()); + _status = field.getIntValue(); _response = true; break; diff --git a/jetty-http2/http2-hpack/src/test/java/org/eclipse/jetty/http2/hpack/HpackDecoderTest.java b/jetty-http2/http2-hpack/src/test/java/org/eclipse/jetty/http2/hpack/HpackDecoderTest.java index f4488c72459..791ecd3d060 100644 --- a/jetty-http2/http2-hpack/src/test/java/org/eclipse/jetty/http2/hpack/HpackDecoderTest.java +++ b/jetty-http2/http2-hpack/src/test/java/org/eclipse/jetty/http2/hpack/HpackDecoderTest.java @@ -384,7 +384,7 @@ public class HpackDecoderTest mdb.emit(new HttpField(HttpHeader.C_SCHEME, "http")); mdb.emit(new HttpField(HttpHeader.C_AUTHORITY, "localhost:8080")); mdb.emit(new HttpField(HttpHeader.C_PATH, "")); - StreamException ex = assertThrows(StreamException.class, () -> mdb.build()); + StreamException ex = assertThrows(StreamException.class, mdb::build); assertThat(ex.getMessage(), Matchers.containsString("No Path")); } @@ -394,7 +394,7 @@ public class HpackDecoderTest mdb.emit(new HttpField(HttpHeader.C_SCHEME, "http")); mdb.emit(new HttpField(HttpHeader.C_AUTHORITY, "localhost:8080")); mdb.emit(new HttpField(HttpHeader.C_PATH, "/")); - StreamException ex = assertThrows(StreamException.class, () -> mdb.build()); + StreamException ex = assertThrows(StreamException.class, mdb::build); assertThat(ex.getMessage(), Matchers.containsString("No Method")); } @@ -404,7 +404,7 @@ public class HpackDecoderTest mdb.emit(new HttpField(HttpHeader.C_METHOD, "GET")); mdb.emit(new HttpField(HttpHeader.C_AUTHORITY, "localhost:8080")); mdb.emit(new HttpField(HttpHeader.C_PATH, "/")); - StreamException ex = assertThrows(StreamException.class, () -> mdb.build()); + StreamException ex = assertThrows(StreamException.class, mdb::build); assertThat(ex.getMessage(), Matchers.containsString("No Scheme")); } @@ -414,7 +414,7 @@ public class HpackDecoderTest mdb.emit(new HttpField(HttpHeader.C_METHOD, "GET")); mdb.emit(new HttpField(HttpHeader.C_SCHEME, "http")); mdb.emit(new HttpField(HttpHeader.C_AUTHORITY, "localhost:8080")); - StreamException ex = assertThrows(StreamException.class, () -> mdb.build()); + StreamException ex = assertThrows(StreamException.class, mdb::build); assertThat(ex.getMessage(), Matchers.containsString("No Path")); } @@ -426,7 +426,7 @@ public class HpackDecoderTest mdb.emit(new HttpField(HttpHeader.C_SCHEME, "http")); mdb.emit(new HttpField(HttpHeader.C_AUTHORITY, "localhost:8080")); mdb.emit(new HttpField(HttpHeader.C_PATH, "/")); - StreamException ex = assertThrows(StreamException.class, () -> mdb.build()); + StreamException ex = assertThrows(StreamException.class, mdb::build); assertThat(ex.getMessage(), Matchers.containsString("Duplicate")); } @@ -439,7 +439,7 @@ public class HpackDecoderTest mdb.emit(new HttpField(HttpHeader.C_AUTHORITY, "localhost:8080")); mdb.emit(new HttpField(HttpHeader.C_PATH, "/")); - StreamException ex = assertThrows(StreamException.class, () -> mdb.build()); + StreamException ex = assertThrows(StreamException.class, mdb::build); assertThat(ex.getMessage(), Matchers.containsString("Duplicate")); } } @@ -463,7 +463,7 @@ public class HpackDecoderTest /* 5.2.1: Sends a Huffman-encoded string literal representation with padding longer than 7 bits */ @Test - public void testHuffmanEncodedExtraPadding() throws Exception + public void testHuffmanEncodedExtraPadding() { HpackDecoder decoder = new HpackDecoder(4096, 8192); @@ -475,7 +475,7 @@ public class HpackDecoderTest /* 5.2.2: Sends a Huffman-encoded string literal representation padded by zero */ @Test - public void testHuffmanEncodedZeroPadding() throws Exception + public void testHuffmanEncodedZeroPadding() { HpackDecoder decoder = new HpackDecoder(4096, 8192); @@ -488,7 +488,7 @@ public class HpackDecoderTest /* 5.2.3: Sends a Huffman-encoded string literal representation containing the EOS symbol */ @Test - public void testHuffmanEncodedWithEOS() throws Exception + public void testHuffmanEncodedWithEOS() { HpackDecoder decoder = new HpackDecoder(4096, 8192); @@ -500,7 +500,7 @@ public class HpackDecoderTest } @Test - public void testHuffmanEncodedOneIncompleteOctet() throws Exception + public void testHuffmanEncodedOneIncompleteOctet() { HpackDecoder decoder = new HpackDecoder(4096, 8192); @@ -512,7 +512,7 @@ public class HpackDecoderTest } @Test - public void testHuffmanEncodedTwoIncompleteOctet() throws Exception + public void testHuffmanEncodedTwoIncompleteOctet() { HpackDecoder decoder = new HpackDecoder(4096, 8192); @@ -524,7 +524,7 @@ public class HpackDecoderTest } @Test - public void testZeroLengthName() throws Exception + public void testZeroLengthName() { HpackDecoder decoder = new HpackDecoder(4096, 8192); @@ -547,7 +547,7 @@ public class HpackDecoderTest } @Test - public void testUpperCaseName() throws Exception + public void testUpperCaseName() { HpackDecoder decoder = new HpackDecoder(4096, 8192); @@ -558,7 +558,7 @@ public class HpackDecoderTest } @Test - public void testWhiteSpaceName() throws Exception + public void testWhiteSpaceName() { HpackDecoder decoder = new HpackDecoder(4096, 8192); diff --git a/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2Test.java b/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2Test.java index 3a7739f42d3..883078555dc 100644 --- a/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2Test.java +++ b/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2Test.java @@ -18,7 +18,6 @@ package org.eclipse.jetty.http2.client.http; -import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.net.ServerSocket; @@ -38,7 +37,6 @@ import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import java.util.function.UnaryOperator; -import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -188,7 +186,7 @@ public class HttpClientTransportOverHTTP2Test extends AbstractTest start(new AbstractHandler() { @Override - public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) { baseRequest.setHandled(true); HttpVersion version = HttpVersion.fromString(request.getProtocol()); @@ -314,7 +312,7 @@ public class HttpClientTransportOverHTTP2Test extends AbstractTest start(new AbstractHandler() { @Override - public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) { baseRequest.setHandled(true); assertEquals(path, request.getRequestURI()); @@ -338,7 +336,7 @@ public class HttpClientTransportOverHTTP2Test extends AbstractTest start(new AbstractHandler() { @Override - public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) { baseRequest.setHandled(true); assertEquals(path, request.getRequestURI()); diff --git a/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/MaxConcurrentStreamsTest.java b/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/MaxConcurrentStreamsTest.java index 1fc482a1a97..f05b0da7ae2 100644 --- a/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/MaxConcurrentStreamsTest.java +++ b/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/MaxConcurrentStreamsTest.java @@ -49,7 +49,6 @@ import org.eclipse.jetty.http2.frames.HeadersFrame; import org.eclipse.jetty.http2.frames.PingFrame; import org.eclipse.jetty.http2.frames.ResetFrame; import org.eclipse.jetty.http2.frames.SettingsFrame; -import org.eclipse.jetty.http2.parser.RateControl; import org.eclipse.jetty.http2.server.HTTP2ServerConnectionFactory; import org.eclipse.jetty.server.Handler; import org.eclipse.jetty.server.HttpConfiguration; @@ -75,7 +74,6 @@ public class MaxConcurrentStreamsTest extends AbstractTest { HTTP2ServerConnectionFactory http2 = new HTTP2ServerConnectionFactory(new HttpConfiguration()); http2.setMaxConcurrentStreams(maxConcurrentStreams); - http2.setRateControl(RateControl.NO_RATE_CONTROL); prepareServer(http2); server.setHandler(handler); server.start(); diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/MathUtils.java b/jetty-util/src/main/java/org/eclipse/jetty/util/MathUtils.java new file mode 100644 index 00000000000..6c522299b21 --- /dev/null +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/MathUtils.java @@ -0,0 +1,46 @@ +// +// ======================================================================== +// Copyright (c) 1995-2019 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.util; + +public class MathUtils +{ + private MathUtils() + { + } + + /** + * Returns whether the sum of the arguments overflows an {@code int}. + * + * @param a the first value + * @param b the second value + * @return whether the sum of the arguments overflows an {@code int} + */ + public static boolean sumOverflows(int a, int b) + { + try + { + Math.addExact(a, b); + return false; + } + catch (ArithmeticException x) + { + return true; + } + } +}