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 <simone.bordet@gmail.com>

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());

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)
     {

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;
     }

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<Listener> 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

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;
 
     /**
      * <p>Applications should call this method when they want to signal an

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));
+        boolean isReply = hasFlag(Flags.ACK);
-        if (!rateControlOnEvent(frame))
+        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);

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')

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;
 

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);
 

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());

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();

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;
+        }
+    }
+}