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 <ma...@apache.org> 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: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org