Merge pull request #42 from cesarhernandezgt/8.5.x-TT.x-patch

Backporte 9fec9a8 - to address CVE-2024-34750
tomitribe · Jul 12, 2024 · 8fb1b69 · 8fb1b69
2 parents 6d8b8f1 + b9396f6
commit 8fb1b69
Show file tree

Hide file tree

Showing 5 changed files with 32 additions and 12 deletions.
diff --git a/build.properties.default b/build.properties.default
@@ -33,7 +33,7 @@ version.major=8
 version.minor=5
 version.build=100
 version.patch=0
-version.suffix=-TT.1
+version.suffix=-TT.2
 version.dev=
 
 # ----- Build tools -----

diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -283,8 +283,8 @@ private void processStreamOnContainerThread(Stream stream) {
     }
 
 
-    protected void decrementActiveRemoteStreamCount() {
-        setConnectionTimeoutForStreamCount(activeRemoteStreamCount.decrementAndGet());
+    protected void decrementActiveRemoteStreamCount(Stream stream) {
+        setConnectionTimeoutForStreamCount(stream.decrementAndGetActiveRemoteStreamCount());
     }
 
 
@@ -591,7 +591,7 @@ void sendStreamReset(StreamStateMachine state, StreamException se) throws IOExce
                 boolean active = state.isActive();
                 state.sendReset();
                 if (active) {
-                    decrementActiveRemoteStreamCount();
+                    decrementActiveRemoteStreamCount(getStream(se.getStreamId()));
                 }
             }
             socketWrapper.write(true, rstFrame, 0, rstFrame.length);
@@ -835,7 +835,7 @@ void writeBody(Stream stream, ByteBuffer data, int len, boolean finished) throws
     protected void sentEndOfStream(Stream stream) {
         stream.sentEndOfStream();
         if (!stream.isActive()) {
-            decrementActiveRemoteStreamCount();
+            decrementActiveRemoteStreamCount(stream);
         }
     }
 
@@ -1216,7 +1216,7 @@ private int allocate(AbstractStream stream, int allocation) {
     }
 
 
-    private Stream getStream(int streamId) {
+    Stream getStream(int streamId) {
         Integer key = Integer.valueOf(streamId);
         AbstractStream result = streams.get(key);
         if (result instanceof Stream) {
@@ -1696,6 +1696,7 @@ public HeaderEmitter headersStart(int streamId, boolean headersEndStream) throws
             Stream stream = getStream(streamId, false);
             if (stream == null) {
                 stream = createRemoteStream(streamId);
+                activeRemoteStreamCount.incrementAndGet();
             }
             if (streamId < maxActiveRemoteStreamId) {
                 throw new ConnectionException(sm.getString("upgradeHandler.stream.old", Integer.valueOf(streamId),
@@ -1774,9 +1775,8 @@ public void headersEnd(int streamId, boolean endOfStream) throws Http2Exception
             Stream stream = (Stream) abstractNonZeroStream;
             if (stream.isActive()) {
                 if (stream.receivedEndOfHeaders()) {
-
-                    if (localSettings.getMaxConcurrentStreams() < activeRemoteStreamCount.incrementAndGet()) {
-                        decrementActiveRemoteStreamCount();
+                    if (localSettings.getMaxConcurrentStreams() < activeRemoteStreamCount.get()) {
+                        decrementActiveRemoteStreamCount(stream);
                         // Ignoring maxConcurrentStreams increases the overhead count
                         increaseOverheadCount(FrameType.HEADERS);
                         throw new StreamException(
@@ -1820,7 +1820,7 @@ public void receivedEndOfStream(int streamId) throws ConnectionException {
     private void receivedEndOfStream(Stream stream) throws ConnectionException {
         stream.receivedEndOfStream();
         if (!stream.isActive()) {
-            decrementActiveRemoteStreamCount();
+            decrementActiveRemoteStreamCount(stream);
         }
     }
 
@@ -1846,7 +1846,7 @@ public void reset(int streamId, long errorCode) throws Http2Exception {
             boolean active = stream.isActive();
             stream.receiveReset(errorCode);
             if (active) {
-                decrementActiveRemoteStreamCount();
+                decrementActiveRemoteStreamCount(stream);
             }
         }
     }

diff --git a/java/org/apache/coyote/http2/Stream.java b/java/org/apache/coyote/http2/Stream.java
@@ -26,6 +26,7 @@
 import java.util.HashSet;
 import java.util.Locale;
 import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
 
@@ -89,6 +90,7 @@ class Stream extends AbstractNonZeroStream implements HeaderEmitter {
     private final StreamInputBuffer inputBuffer;
     private final StreamOutputBuffer streamOutputBuffer = new StreamOutputBuffer();
     private final Http2OutputBuffer http2OutputBuffer = new Http2OutputBuffer(coyoteResponse, streamOutputBuffer);
+    private final AtomicBoolean removedFromActiveCount = new AtomicBoolean(false);
 
     // State machine would be too much overhead
     private int headerState = HEADER_STATE_START;
@@ -838,6 +840,20 @@ public void setIncremental(boolean incremental) {
     }
 
 
+    int decrementAndGetActiveRemoteStreamCount() {
+        /*
+         * Protect against mis-counting of active streams. This method should only be called once per stream but since
+         * the count of active streams is used to enforce the maximum concurrent streams limit, make sure each stream is
+         * only removed from the active count exactly once.
+         */
+        if (removedFromActiveCount.compareAndSet(false, true)) {
+            return handler.activeRemoteStreamCount.decrementAndGet();
+        } else {
+            return handler.activeRemoteStreamCount.get();
+        }
+    }
+
+
     private static void push(final Http2UpgradeHandler handler, final Request request, final Stream stream)
             throws IOException {
         if (org.apache.coyote.Constants.IS_SECURITY_ENABLED) {

diff --git a/res/maven/mvn.properties.default b/res/maven/mvn.properties.default
@@ -39,7 +39,7 @@ maven.asf.release.repo.url=https://repository.apache.org/service/local/staging/d
 maven.asf.release.repo.repositoryId=apache.releases.https
 
 # Release version info
-maven.asf.release.deploy.version=8.5.101
+maven.asf.release.deploy.version=8.5.100-TT.2
 
 #Where do we load the libraries from
 tomcat.lib.path=../../output/build/lib

diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
@@ -161,6 +161,10 @@
         <code>org.apache.catalina.security.TLSCertificateReloadListener</code>.
         (markt)
       </fix>
+      <fix>
+        Make counting of active HTTP/2 streams per connection more robust.
+        (markt)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Jasper">