[tomcat] branch 8.5.x updated: Expand the HTTP/2 excessive overhead protection

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

[tomcat] branch 8.5.x updated: Expand the HTTP/2 excessive overhead protection

markt
This is an automated email from the ASF dual-hosted git repository.

markt pushed a commit to branch 8.5.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git


The following commit(s) were added to refs/heads/8.5.x by this push:
     new d5bd2de5 Expand the HTTP/2 excessive overhead protection
d5bd2de5 is described below

commit d5bd2de5faca467d0f4279934ae4755c01978746
Author: Mark Thomas <[hidden email]>
AuthorDate: Tue Aug 13 19:23:03 2019 +0100

    Expand the HTTP/2 excessive overhead protection
   
    Various abusive behaviours described in
    https://github.com/Netflix/security-bulletins/blob/master/advisories/third-party/2019-002.md
    that don't trigger a DoS in Tomcat but never the less should be blocked.
    Close the connection if detected.
    Detection thresholds are configurable.
---
 java/org/apache/coyote/http2/Http2Parser.java      | 18 ++++--
 java/org/apache/coyote/http2/Http2Protocol.java    | 36 +++++++++++
 .../apache/coyote/http2/Http2UpgradeHandler.java   | 70 +++++++++++++++++++++-
 test/org/apache/coyote/http2/Http2TestBase.java    |  8 ++-
 .../apache/coyote/http2/TestHttp2Section_5_2.java  |  4 ++
 .../apache/coyote/http2/TestHttp2Section_5_3.java  |  4 ++
 webapps/docs/changelog.xml                         |  5 ++
 webapps/docs/config/http2.xml                      | 34 +++++++++++
 8 files changed, 171 insertions(+), 8 deletions(-)

diff --git a/java/org/apache/coyote/http2/Http2Parser.java b/java/org/apache/coyote/http2/Http2Parser.java
index 3980094..bf01b33 100644
--- a/java/org/apache/coyote/http2/Http2Parser.java
+++ b/java/org/apache/coyote/http2/Http2Parser.java
@@ -164,7 +164,7 @@ class Http2Parser {
                     Integer.toString(streamId), Integer.toString(dataLength), padding));
         }
 
-        ByteBuffer dest = output.startRequestBodyFrame(streamId, payloadSize);
+        ByteBuffer dest = output.startRequestBodyFrame(streamId, payloadSize, endOfStream);
         if (dest == null) {
             swallow(streamId, dataLength, false);
             // Process padding before sending any notifications in case padding
@@ -298,7 +298,10 @@ class Http2Parser {
                     Http2Error.FRAME_SIZE_ERROR);
         }
 
-        if (payloadSize != 0) {
+        if (payloadSize == 0 && !ack) {
+            // Ensure empty SETTINGS frame increments the overhead count
+            output.setting(null, 0);
+        } else {
             // Process the settings
             byte[] setting = new byte[6];
             for (int i = 0; i < payloadSize / 6; i++) {
@@ -376,9 +379,15 @@ class Http2Parser {
                     Integer.toString(streamId)), Http2Error.PROTOCOL_ERROR);
         }
 
+        boolean endOfHeaders = Flags.isEndOfHeaders(flags);
+
+        // Used to detect abusive clients sending large numbers of small
+        // continuation frames
+        output.headersContinue(payloadSize, endOfHeaders);
+
         readHeaderPayload(streamId, payloadSize);
 
-        if (Flags.isEndOfHeaders(flags)) {
+        if (endOfHeaders) {
             headersCurrentStream = -1;
             onHeadersComplete(streamId);
         }
@@ -629,7 +638,7 @@ class Http2Parser {
         HpackDecoder getHpackDecoder();
 
         // Data frames
-        ByteBuffer startRequestBodyFrame(int streamId, int payloadSize) throws Http2Exception;
+        ByteBuffer startRequestBodyFrame(int streamId, int payloadSize, boolean endOfStream) throws Http2Exception;
         void endRequestBodyFrame(int streamId) throws Http2Exception;
         void receivedEndOfStream(int streamId) throws ConnectionException;
         void swallowedPadding(int streamId, int paddingLength) throws ConnectionException, IOException;
@@ -637,6 +646,7 @@ class Http2Parser {
         // Header frames
         HeaderEmitter headersStart(int streamId, boolean headersEndStream)
                 throws Http2Exception, IOException;
+        void headersContinue(int payloadSize, boolean endOfHeaders);
         void headersEnd(int streamId) throws ConnectionException;
 
         // Priority frames (also headers)
diff --git a/java/org/apache/coyote/http2/Http2Protocol.java b/java/org/apache/coyote/http2/Http2Protocol.java
index 27449d4..a8556f6 100644
--- a/java/org/apache/coyote/http2/Http2Protocol.java
+++ b/java/org/apache/coyote/http2/Http2Protocol.java
@@ -55,6 +55,9 @@ public class Http2Protocol implements UpgradeProtocol {
     static final int DEFAULT_INITIAL_WINDOW_SIZE = (1 << 16) - 1;
 
     static final int DEFAULT_OVERHEAD_COUNT_FACTOR = 1;
+    static final int DEFAULT_OVERHEAD_CONTINUATION_THRESHOLD = 1024;
+    static final int DEFAULT_OVERHEAD_DATA_THRESHOLD = 1024;
+    static final int DEFAULT_OVERHEAD_WINDOW_UPDATE_THRESHOLD = 1024;
 
     private static final String HTTP_UPGRADE_NAME = "h2c";
     private static final String ALPN_NAME = "h2";
@@ -82,6 +85,9 @@ public class Http2Protocol implements UpgradeProtocol {
     private int maxTrailerCount = Constants.DEFAULT_MAX_TRAILER_COUNT;
     private int maxTrailerSize = Constants.DEFAULT_MAX_TRAILER_SIZE;
     private int overheadCountFactor = DEFAULT_OVERHEAD_COUNT_FACTOR;
+    private int overheadContinuationThreshold = DEFAULT_OVERHEAD_CONTINUATION_THRESHOLD;
+    private int overheadDataThreadhold = DEFAULT_OVERHEAD_DATA_THRESHOLD;
+    private int overheadWindowUpdateThreadhold = DEFAULT_OVERHEAD_WINDOW_UPDATE_THRESHOLD;
 
     private boolean initiatePingDisabled = false;
     // Compression
@@ -316,6 +322,36 @@ public class Http2Protocol implements UpgradeProtocol {
     }
 
 
+    public int getOverheadContinuationThreshhold() {
+        return overheadContinuationThreshold;
+    }
+
+
+    public void setOverheadContinuationThreshhold(int overheadContinuationThreshold) {
+        this.overheadContinuationThreshold = overheadContinuationThreshold;
+    }
+
+
+    public int getOverheadDataThreadhold() {
+        return overheadDataThreadhold;
+    }
+
+
+    public void setOverheadDataThreadhold(int overheadDataThreadhold) {
+        this.overheadDataThreadhold = overheadDataThreadhold;
+    }
+
+
+    public int getOverheadWindowUpdateThreadhold() {
+        return overheadWindowUpdateThreadhold;
+    }
+
+
+    public void setOverheadWindowUpdateThreadhold(int overheadWindowUpdateThreadhold) {
+        this.overheadWindowUpdateThreadhold = overheadWindowUpdateThreadhold;
+    }
+
+
     public void setInitiatePingDisabled(boolean initiatePingDisabled) {
         this.initiatePingDisabled = initiatePingDisabled;
     }
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index 50365a9..383417b 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -1452,8 +1452,25 @@ public class Http2UpgradeHandler extends AbstractStream implements InternalHttpU
 
 
     @Override
-    public ByteBuffer startRequestBodyFrame(int streamId, int payloadSize) throws Http2Exception {
+    public ByteBuffer startRequestBodyFrame(int streamId, int payloadSize, boolean endOfStream) throws Http2Exception {
+        // DATA frames reduce the overhead count ...
         reduceOverheadCount();
+
+        // .. but lots of small payloads are inefficient so that will increase
+        // the overhead count unless it is the final DATA frame where small
+        // payloads are expected.
+        if (!endOfStream) {
+            int overheadThreshold = protocol.getOverheadDataThreadhold();
+            if (payloadSize < overheadThreshold) {
+                if (payloadSize == 0) {
+                    // Avoid division by zero
+                    overheadCount.addAndGet(overheadThreshold);
+                } else {
+                    overheadCount.addAndGet(overheadThreshold / payloadSize);
+                }
+            }
+        }
+
         Stream stream = getStream(streamId, true);
         stream.checkState(FrameType.DATA);
         stream.receivedData(payloadSize);
@@ -1497,8 +1514,6 @@ public class Http2UpgradeHandler extends AbstractStream implements InternalHttpU
         // determines if a new stream is created or if this stream is ignored.
         checkPauseState();
 
-        reduceOverheadCount();
-
         if (connectionState.get().isNewStreamAllowed()) {
             Stream stream = getStream(streamId, false);
             if (stream == null) {
@@ -1514,16 +1529,21 @@ public class Http2UpgradeHandler extends AbstractStream implements InternalHttpU
             closeIdleStreams(streamId);
             if (localSettings.getMaxConcurrentStreams() < activeRemoteStreamCount.incrementAndGet()) {
                 setConnectionTimeoutForStreamCount(activeRemoteStreamCount.decrementAndGet());
+                // Ignoring maxConcurrentStreams increases the overhead count
+                increaseOverheadCount();
                 throw new StreamException(sm.getString("upgradeHandler.tooManyRemoteStreams",
                         Long.toString(localSettings.getMaxConcurrentStreams())),
                         Http2Error.REFUSED_STREAM, streamId);
             }
+            // Valid new stream reduces the overhead count
+            reduceOverheadCount();
             return stream;
         } else {
             if (log.isDebugEnabled()) {
                 log.debug(sm.getString("upgradeHandler.noNewStreams",
                         connectionId, Integer.toString(streamId)));
             }
+            reduceOverheadCount();
             // Stateless so a static can be used to save on GC
             return HEADER_SINK;
         }
@@ -1565,6 +1585,25 @@ public class Http2UpgradeHandler extends AbstractStream implements InternalHttpU
 
 
     @Override
+    public void headersContinue(int payloadSize, boolean endOfHeaders) {
+        // Generally, continuation frames don't impact the overhead count but if
+        // they are small and the frame isn't the final header frame then that
+        // is indicative of an abusive client
+        if (!endOfHeaders) {
+            int overheadThreshold = getProtocol().getOverheadContinuationThreshhold();
+            if (payloadSize < overheadThreshold) {
+                if (payloadSize == 0) {
+                    // Avoid division by zero
+                    overheadCount.addAndGet(overheadThreshold);
+                } else {
+                    overheadCount.addAndGet(overheadThreshold / payloadSize);
+                }
+            }
+        }
+    }
+
+
+    @Override
     public void headersEnd(int streamId) throws ConnectionException {
         Stream stream = getStream(streamId, connectionState.get().isNewStreamAllowed());
         if (stream != null) {
@@ -1598,6 +1637,11 @@ public class Http2UpgradeHandler extends AbstractStream implements InternalHttpU
 
         increaseOverheadCount();
 
+        // Possible with empty settings frame
+        if (setting == null) {
+            return;
+        }
+
         // Special handling required
         if (setting == Setting.INITIAL_WINDOW_SIZE) {
             long oldValue = remoteSettings.getInitialWindowSize();
@@ -1658,10 +1702,30 @@ public class Http2UpgradeHandler extends AbstractStream implements InternalHttpU
 
     @Override
     public void incrementWindowSize(int streamId, int increment) throws Http2Exception {
+        int overheadThreshold = protocol.getOverheadWindowUpdateThreadhold();
+
         if (streamId == 0) {
+            // Check for small increments which are inefficient
+            if (increment < overheadThreshold) {
+                // The smaller the increment, the larger the overhead
+                overheadCount.addAndGet(overheadThreshold / increment);
+            }
+
             incrementWindowSize(increment);
         } else {
             Stream stream = getStream(streamId, true);
+
+            // Check for small increments which are inefficient
+            if (increment < overheadThreshold) {
+                // For Streams, client might only release the minimum so check
+                // against current demand
+                BacklogTracker tracker = backLogStreams.get(stream);
+                if (tracker == null || increment < tracker.getRemainingReservation()) {
+                    // The smaller the increment, the larger the overhead
+                    overheadCount.addAndGet(overheadThreshold / increment);
+                }
+            }
+
             stream.checkState(FrameType.WINDOW_UPDATE);
             stream.incrementWindowSize(increment);
         }
diff --git a/test/org/apache/coyote/http2/Http2TestBase.java b/test/org/apache/coyote/http2/Http2TestBase.java
index 0361296..4630804 100644
--- a/test/org/apache/coyote/http2/Http2TestBase.java
+++ b/test/org/apache/coyote/http2/Http2TestBase.java
@@ -972,7 +972,7 @@ public abstract class Http2TestBase extends TomcatBaseTest {
 
 
         @Override
-        public ByteBuffer startRequestBodyFrame(int streamId, int payloadSize) {
+        public ByteBuffer startRequestBodyFrame(int streamId, int payloadSize, boolean endOfStream) {
             lastStreamId = Integer.toString(streamId);
             bytesRead += payloadSize;
             if (traceBody) {
@@ -1051,6 +1051,12 @@ public abstract class Http2TestBase extends TomcatBaseTest {
 
 
         @Override
+        public void headersContinue(int payloadSize, boolean endOfHeaders) {
+            // NO-OP: Logging occurs per header
+        }
+
+
+        @Override
         public void headersEnd(int streamId) {
             trace.append(streamId + "-HeadersEnd\n");
         }
diff --git a/test/org/apache/coyote/http2/TestHttp2Section_5_2.java b/test/org/apache/coyote/http2/TestHttp2Section_5_2.java
index 545825f..ad559807 100644
--- a/test/org/apache/coyote/http2/TestHttp2Section_5_2.java
+++ b/test/org/apache/coyote/http2/TestHttp2Section_5_2.java
@@ -40,6 +40,10 @@ public class TestHttp2Section_5_2 extends Http2TestBase {
 
         http2Connect();
 
+        // This test uses small window updates that will trigger the excessive
+        // overhead protection so disable it.
+        http2Protocol.setOverheadWindowUpdateThreadhold(0);
+
         // Set the default window size to 1024 bytes
         sendSettings(0, false, new SettingValue(4, 1024));
         // Wait for the ack
diff --git a/test/org/apache/coyote/http2/TestHttp2Section_5_3.java b/test/org/apache/coyote/http2/TestHttp2Section_5_3.java
index a0bcbaf..3b14890 100644
--- a/test/org/apache/coyote/http2/TestHttp2Section_5_3.java
+++ b/test/org/apache/coyote/http2/TestHttp2Section_5_3.java
@@ -52,6 +52,10 @@ public class TestHttp2Section_5_3 extends Http2TestBase {
 
         http2Connect();
 
+        // This test uses small window updates that will trigger the excessive
+        // overhead protection so disable it.
+        http2Protocol.setOverheadWindowUpdateThreadhold(0);
+
         // Default connection window size is 64k - 1. Initial request will have
         // used 8k (56k -1). Increase it to 57k
         sendWindowUpdate(0, 1 + 1024);
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index bfcdbab..8f8e6a5 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -151,6 +151,11 @@
         Timeouts for HTTP/2 connections were not always correctly handled
         leaving some connections open for longer than expected. (markt)
       </fix>
+      <add>
+        Expand the HTTP/2 excessive overhead protection to cover various forms
+        of abusive client behaviour and close the connection if any such
+        behaviour is detected. (markt)
+      </add>
     </changelog>
   </subsection>
   <subsection name="Web applications">
diff --git a/webapps/docs/config/http2.xml b/webapps/docs/config/http2.xml
index 28579f0..8a8ae70 100644
--- a/webapps/docs/config/http2.xml
+++ b/webapps/docs/config/http2.xml
@@ -188,6 +188,17 @@
       The default value is an empty String (regexp matching disabled).</p>
     </attribute>
 
+    <attribute name="overheadContinuationThreshold" required="false">
+      <p>The threshold below which the payload size of a non-final
+      <code>CONTINUATION</code> frame will trigger an increase in the overhead
+      count (see <strong>overheadCountFactor</strong>). The overhead count will
+      be increased by <code>overheadContinuationThreshold/payloadSize</code> so
+      that the smaller the <code>CONTINUATION</code> frame, the greater the
+      increase in the overhead count. A value of zero or less disables the
+      checking of non-final <code>CONTINUATION</code> frames. If not specified,
+      a default value of <code>1024</code> will be used.</p>
+    </attribute>
+
     <attribute name="overheadCountFactor" required="false">
       <p>The factor to apply when counting overhead frames to determine if a
       connection has too high an overhead and should be closed. The overhead
@@ -202,6 +213,29 @@
       used.</p>
     </attribute>
 
+    <attribute name="overheadDataThreadhold" required="false">
+      <p>The threshold below which the payload size of a non-final
+      <code>DATA</code> frame will trigger an increase in the overhead count
+      (see <strong>overheadCountFactor</strong>). The overhead count will be
+      increased by <code>overheadDataThreadhold/payloadSize</code> so that the
+      smaller the <code>DATA</code> frame, the greater the increase in the
+      overhead count. A value of zero or less disables the checking of non-final
+      <code>DATA</code> frames. If not specified, a default value of
+      <code>1024</code> will be used.</p>
+    </attribute>
+
+    <attribute name="overheadWindowUpdateThreadhold" required="false">
+      <p>The threshold below which the size of a <code>WINDOW_UPDATE</code>
+      frame will trigger an increase in the overhead count (see
+      <strong>overheadCountFactor</strong>). The overhead count will be
+      increased by <code>overheadWindowUpdateThreadhold/updateSize</code> so
+      that the smaller the <code>WINDOW_UPDATE</code>, the greater the increase
+      in the overhead count. A value of zero or less disables the checking of
+      <code>WINDOW_UPDATE</code> frames. If not specified, a default value of
+      <code>1024</code> will be used.</p>
+    </attribute>
+
+
     <attribute name="readTimeout" required="false">
       <p>The time, in milliseconds, that Tomcat will wait for additional data
       when a partial HTTP/2 frame has been received. Negative values will be


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]