Apache Tomcat: DoS via malformed HTTP/2 PRIORITY_UPDATE frame

@@ -48,6 +48,7 @@
 import org.apache.coyote.http2.Http2Parser.Output;
 import org.apache.juli.logging.Log;
 import org.apache.juli.logging.LogFactory;
+import org.apache.tomcat.util.ExceptionUtils;
 import org.apache.tomcat.util.http.MimeHeaders;
 import org.apache.tomcat.util.http.parser.Priority;
 import org.apache.tomcat.util.log.UserDataHelper;
@@ -442,6 +443,13 @@ public SocketState upgradeDispatch(SocketEvent status) {
                 log.debug(sm.getString("upgradeHandler.ioerror", connectionId), ioe);
             }
             close();
+        } catch (Throwable t) {
+            ExceptionUtils.handleThrowable(t);
+            if (log.isDebugEnabled()) {
+                log.debug(sm.getString("upgradeHandler.throwable", connectionId), t);
+            }
+            // Unexpected errors close the connection.
+            close();
         }
 
         if (log.isTraceEnabled()) {

@@ -166,6 +166,7 @@ upgradeHandler.stream.error=Connection [{0}], Stream [{1}] Closed due to error
 upgradeHandler.stream.even=A new remote stream ID of [{0}] was requested but all remote streams must use odd identifiers
 upgradeHandler.stream.notWritable=Connection [{0}], Stream [{1}], This stream is in state [{2}] and is not writable
 upgradeHandler.stream.old=A new remote stream ID of [{0}] was requested but the most recent stream was [{1}]
+upgradeHandler.throwable=Connection [{0}]
 upgradeHandler.tooManyRemoteStreams=The client attempted to use more than [{0}] active streams
 upgradeHandler.tooMuchOverhead=Connection [{0}], Too much overhead so the connection will be closed
 upgradeHandler.unexpectedAck=Connection [{0}], Stream [{1}], A settings acknowledgement was received when not expected

@@ -134,6 +134,9 @@
         <bug>69614</bug>: HTTP/2 priority frames with an invalid priority field
         value should be ignored. (markt)
       </fix>
+      <fix>
+        Improve handling of unexpected errors during HTTP/2 processing. (markt)
+      </fix>
     </changelog>
   </subsection>
 </section>

@@ -48,6 +48,7 @@
 import org.apache.coyote.http2.Http2Parser.Output;
 import org.apache.juli.logging.Log;
 import org.apache.juli.logging.LogFactory;
+import org.apache.tomcat.util.ExceptionUtils;
 import org.apache.tomcat.util.http.MimeHeaders;
 import org.apache.tomcat.util.http.parser.Priority;
 import org.apache.tomcat.util.log.UserDataHelper;
@@ -441,6 +442,13 @@ public SocketState upgradeDispatch(SocketEvent status) {
                 log.debug(sm.getString("upgradeHandler.ioerror", connectionId), ioe);
             }
             close();
+        } catch (Throwable t) {
+            ExceptionUtils.handleThrowable(t);
+            if (log.isDebugEnabled()) {
+                log.debug(sm.getString("upgradeHandler.throwable", connectionId), t);
+            }
+            // Unexpected errors close the connection.
+            close();
         }
 
         if (log.isTraceEnabled()) {

@@ -166,6 +166,7 @@ upgradeHandler.stream.error=Connection [{0}], Stream [{1}] Closed due to error
 upgradeHandler.stream.even=A new remote stream ID of [{0}] was requested but all remote streams must use odd identifiers
 upgradeHandler.stream.notWritable=Connection [{0}], Stream [{1}], This stream is in state [{2}] and is not writable
 upgradeHandler.stream.old=A new remote stream ID of [{0}] was requested but the most recent stream was [{1}]
+upgradeHandler.throwable=Connection [{0}]
 upgradeHandler.tooManyRemoteStreams=The client attempted to use more than [{0}] active streams
 upgradeHandler.tooMuchOverhead=Connection [{0}], Too much overhead so the connection will be closed
 upgradeHandler.unexpectedAck=Connection [{0}], Stream [{1}], A settings acknowledgement was received when not expected

@@ -140,6 +140,9 @@
         <bug>69614</bug>: HTTP/2 priority frames with an invalid priority field
         value should be ignored. (markt)
       </fix>
+      <fix>
+        Improve handling of unexpected errors during HTTP/2 processing. (markt)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Jasper">

@@ -47,6 +47,7 @@
 import org.apache.coyote.http2.Http2Parser.Output;
 import org.apache.juli.logging.Log;
 import org.apache.juli.logging.LogFactory;
+import org.apache.tomcat.util.ExceptionUtils;
 import org.apache.tomcat.util.http.MimeHeaders;
 import org.apache.tomcat.util.http.parser.Priority;
 import org.apache.tomcat.util.log.UserDataHelper;
@@ -439,6 +440,13 @@ public SocketState upgradeDispatch(SocketEvent status) {
                 log.debug(sm.getString("upgradeHandler.ioerror", connectionId), ioe);
             }
             close();
+        } catch (Throwable t) {
+            ExceptionUtils.handleThrowable(t);
+            if (log.isDebugEnabled()) {
+                log.debug(sm.getString("upgradeHandler.throwable", connectionId), t);
+            }
+            // Unexpected errors close the connection.
+            close();
         }
 
         if (log.isTraceEnabled()) {

@@ -160,6 +160,7 @@ upgradeHandler.stream.error=Connection [{0}], Stream [{1}] Closed due to error
 upgradeHandler.stream.even=A new remote stream ID of [{0}] was requested but all remote streams must use odd identifiers
 upgradeHandler.stream.notWritable=Connection [{0}], Stream [{1}], This stream is in state [{2}] and is not writable
 upgradeHandler.stream.old=A new remote stream ID of [{0}] was requested but the most recent stream was [{1}]
+upgradeHandler.throwable=Connection [{0}]
 upgradeHandler.tooManyRemoteStreams=The client attempted to use more than [{0}] active streams
 upgradeHandler.tooMuchOverhead=Connection [{0}], Too much overhead so the connection will be closed
 upgradeHandler.unexpectedAck=Connection [{0}], Stream [{1}], A settings acknowledgement was received when not expected

@@ -134,6 +134,9 @@
         <bug>69614</bug>: HTTP/2 priority frames with an invalid priority field
         value should be ignored. (markt)
       </fix>
+      <fix>
+        Improve handling of unexpected errors during HTTP/2 processing. (markt)
+      </fix>
     </changelog>
   </subsection>
 </section>

@@ -456,6 +456,12 @@ public final void emitHeader(String name, String value) throws HpackException {
                     setIncremental(p.getIncremental());
                 } catch (IOException ioe) {
                     // Not possible with StringReader
+                } catch (IllegalArgumentException iae) {
+                    // Invalid priority header field values should be ignored
+                    if (log.isTraceEnabled()) {
+                        log.trace(sm.getString("http2Parser.processFramePriorityUpdate.invalid", getConnectionId(),
+                                getIdAsString()), iae);
+                    }
                 }
                 break;
             }

@@ -456,6 +456,12 @@ public final void emitHeader(String name, String value) throws HpackException {
                     setIncremental(p.getIncremental());
                 } catch (IOException ioe) {
                     // Not possible with StringReader
+                } catch (IllegalArgumentException iae) {
+                    // Invalid priority header field values should be ignored
+                    if (log.isTraceEnabled()) {
+                        log.trace(sm.getString("http2Parser.processFramePriorityUpdate.invalid", getConnectionId(),
+                                getIdAsString()), iae);
+                    }
                 }
                 break;
             }

@@ -450,6 +450,12 @@ public final void emitHeader(String name, String value) throws HpackException {
                     setIncremental(p.getIncremental());
                 } catch (IOException ioe) {
                     // Not possible with StringReader
+                } catch (IllegalArgumentException iae) {
+                    // Invalid priority header field values should be ignored
+                    if (log.isTraceEnabled()) {
+                        log.trace(sm.getString("http2Parser.processFramePriorityUpdate.invalid", getConnectionId(),
+                                getIdAsString()), iae);
+                    }
                 }
                 break;
             }

@@ -477,15 +477,24 @@ protected void readPriorityUpdateFrame(int payloadSize, ByteBuffer buffer) throw
 
         ByteArrayInputStream bais = new ByteArrayInputStream(payload, 4, payloadSize - 4);
         Reader r = new BufferedReader(new InputStreamReader(bais, StandardCharsets.US_ASCII));
-        Priority p = Priority.parsePriority(r);
 
-        if (log.isTraceEnabled()) {
-            log.trace(sm.getString("http2Parser.processFramePriorityUpdate.debug", connectionId,
-                    Integer.toString(prioritizedStreamID), Integer.toString(p.getUrgency()),
-                    Boolean.valueOf(p.getIncremental())));
-        }
+        try {
+            Priority p = Priority.parsePriority(r);
 
-        output.priorityUpdate(prioritizedStreamID, p);
+            if (log.isTraceEnabled()) {
+                log.trace(sm.getString("http2Parser.processFramePriorityUpdate.debug", connectionId,
+                        Integer.toString(prioritizedStreamID), Integer.toString(p.getUrgency()),
+                        Boolean.valueOf(p.getIncremental())));
+            }
+
+            output.priorityUpdate(prioritizedStreamID, p);
+        } catch (IllegalArgumentException iae) {
+            // Priority frames with invalid priority field values should be ignored
+            if (log.isTraceEnabled()) {
+                log.trace(sm.getString("http2Parser.processFramePriorityUpdate.invalid", connectionId,
+                        Integer.toString(prioritizedStreamID)), iae);
+            }
+        }
     }
 
 

@@ -77,6 +77,7 @@ http2Parser.processFrameHeaders.decodingDataLeft=Data left over after HPACK deco
 http2Parser.processFrameHeaders.decodingFailed=There was an error during the HPACK decoding of HTTP headers
 http2Parser.processFrameHeaders.payload=Connection [{0}], Stream [{1}], Processing headers payload of size [{2}]
 http2Parser.processFramePriorityUpdate.debug=Connection [{0}], Stream [{1}], Urgency [{2}], Incremental [{3}]
+http2Parser.processFramePriorityUpdate.invalid=Connection [{0}], Stream [{1}], Priority Update frame with invalid priority field value
 http2Parser.processFramePriorityUpdate.streamZero=Connection [{0}], Priority update frame received to prioritize stream zero
 http2Parser.processFramePushPromise=Connection [{0}], Stream [{1}], Push promise frames should not be sent by the client
 http2Parser.processFrameSettings.ackWithNonZeroPayload=Settings frame received with the ACK flag set and payload present

@@ -17,6 +17,7 @@
 package org.apache.coyote.http2;
 
 import java.io.IOException;
+import java.nio.charset.StandardCharsets;
 
 import org.junit.Assert;
 import org.junit.Test;
@@ -146,6 +147,9 @@ public void testPriority() throws Exception {
         // 19 - 7021 body left
         // 21 - 6143 body left
 
+        // BZ 69614 - invalid priority update frames should be ignored
+        sendInvalidPriorityUpdate(17);
+
         // Re-order the priorities
         sendPriorityUpdate(17, 2, true);
 
@@ -191,4 +195,25 @@ public void testPriority() throws Exception {
             ioe.printStackTrace();
         }
     }
+
+
+    private void sendInvalidPriorityUpdate(int streamId) throws IOException {
+        byte[] payload = "u=1:i".getBytes(StandardCharsets.US_ASCII);
+
+        byte[] priorityUpdateFrame = new byte[13 + payload.length];
+
+        // length
+        ByteUtil.setThreeBytes(priorityUpdateFrame, 0, 4 + payload.length);
+        // type
+        priorityUpdateFrame[3] = FrameType.PRIORITY_UPDATE.getIdByte();
+        // Stream ID
+        ByteUtil.set31Bits(priorityUpdateFrame, 5, 0);
+
+        // Payload
+        ByteUtil.set31Bits(priorityUpdateFrame, 9, streamId);
+        System.arraycopy(payload, 0, priorityUpdateFrame, 13, payload.length);
+
+        os.write(priorityUpdateFrame);
+        os.flush();
+    }
 }

@@ -130,6 +130,10 @@
         <bug>69607</bug>: Allow failed initialization of MD5. Based on code
         submitted by Shivam Verma. (remm)
       </fix>
+      <fix>
+        <bug>69614</bug>: HTTP/2 priority frames with an invalid priority field
+        value should be ignored. (markt)
+      </fix>
     </changelog>
   </subsection>
 </section>

@@ -477,15 +477,24 @@ protected void readPriorityUpdateFrame(int payloadSize, ByteBuffer buffer) throw
 
         ByteArrayInputStream bais = new ByteArrayInputStream(payload, 4, payloadSize - 4);
         Reader r = new BufferedReader(new InputStreamReader(bais, StandardCharsets.US_ASCII));
-        Priority p = Priority.parsePriority(r);
 
-        if (log.isTraceEnabled()) {
-            log.trace(sm.getString("http2Parser.processFramePriorityUpdate.debug", connectionId,
-                    Integer.toString(prioritizedStreamID), Integer.toString(p.getUrgency()),
-                    Boolean.valueOf(p.getIncremental())));
-        }
+        try {
+            Priority p = Priority.parsePriority(r);
 
-        output.priorityUpdate(prioritizedStreamID, p);
+            if (log.isTraceEnabled()) {
+                log.trace(sm.getString("http2Parser.processFramePriorityUpdate.debug", connectionId,
+                        Integer.toString(prioritizedStreamID), Integer.toString(p.getUrgency()),
+                        Boolean.valueOf(p.getIncremental())));
+            }
+
+            output.priorityUpdate(prioritizedStreamID, p);
+        } catch (IllegalArgumentException iae) {
+            // Priority frames with invalid priority field values should be ignored
+            if (log.isTraceEnabled()) {
+                log.trace(sm.getString("http2Parser.processFramePriorityUpdate.invalid", connectionId,
+                        Integer.toString(prioritizedStreamID)), iae);
+            }
+        }
     }
 
 

@@ -77,6 +77,7 @@ http2Parser.processFrameHeaders.decodingDataLeft=Data left over after HPACK deco
 http2Parser.processFrameHeaders.decodingFailed=There was an error during the HPACK decoding of HTTP headers
 http2Parser.processFrameHeaders.payload=Connection [{0}], Stream [{1}], Processing headers payload of size [{2}]
 http2Parser.processFramePriorityUpdate.debug=Connection [{0}], Stream [{1}], Urgency [{2}], Incremental [{3}]
+http2Parser.processFramePriorityUpdate.invalid=Connection [{0}], Stream [{1}], Priority Update frame with invalid priority field value
 http2Parser.processFramePriorityUpdate.streamZero=Connection [{0}], Priority update frame received to prioritize stream zero
 http2Parser.processFramePushPromise=Connection [{0}], Stream [{1}], Push promise frames should not be sent by the client
 http2Parser.processFrameSettings.ackWithNonZeroPayload=Settings frame received with the ACK flag set and payload present

@@ -17,6 +17,7 @@
 package org.apache.coyote.http2;
 
 import java.io.IOException;
+import java.nio.charset.StandardCharsets;
 
 import org.junit.Assert;
 import org.junit.Test;
@@ -146,6 +147,9 @@ public void testPriority() throws Exception {
         // 19 - 7021 body left
         // 21 - 6143 body left
 
+        // BZ 69614 - invalid priority update frames should be ignored
+        sendInvalidPriorityUpdate(17);
+
         // Re-order the priorities
         sendPriorityUpdate(17, 2, true);
 
@@ -191,4 +195,25 @@ public void testPriority() throws Exception {
             ioe.printStackTrace();
         }
     }
+
+
+    private void sendInvalidPriorityUpdate(int streamId) throws IOException {
+        byte[] payload = "u=1:i".getBytes(StandardCharsets.US_ASCII);
+
+        byte[] priorityUpdateFrame = new byte[13 + payload.length];
+
+        // length
+        ByteUtil.setThreeBytes(priorityUpdateFrame, 0, 4 + payload.length);
+        // type
+        priorityUpdateFrame[3] = FrameType.PRIORITY_UPDATE.getIdByte();
+        // Stream ID
+        ByteUtil.set31Bits(priorityUpdateFrame, 5, 0);
+
+        // Payload
+        ByteUtil.set31Bits(priorityUpdateFrame, 9, streamId);
+        System.arraycopy(payload, 0, priorityUpdateFrame, 13, payload.length);
+
+        os.write(priorityUpdateFrame);
+        os.flush();
+    }
 }

@@ -130,6 +130,10 @@
         <bug>69607</bug>: Allow failed initialization of MD5. Based on code
         submitted by Shivam Verma. (remm)
       </fix>
+      <fix>
+        <bug>69614</bug>: HTTP/2 priority frames with an invalid priority field
+        value should be ignored. (markt)
+      </fix>
     </changelog>
   </subsection>
 </section>

@@ -460,15 +460,24 @@ protected void readPriorityUpdateFrame(int payloadSize, ByteBuffer buffer) throw
 
         ByteArrayInputStream bais = new ByteArrayInputStream(payload, 4, payloadSize - 4);
         Reader r = new BufferedReader(new InputStreamReader(bais, StandardCharsets.US_ASCII));
-        Priority p = Priority.parsePriority(r);
 
-        if (log.isTraceEnabled()) {
-            log.trace(sm.getString("http2Parser.processFramePriorityUpdate.debug", connectionId,
-                    Integer.toString(prioritizedStreamID), Integer.toString(p.getUrgency()),
-                    Boolean.valueOf(p.getIncremental())));
-        }
+        try {
+            Priority p = Priority.parsePriority(r);
 
-        output.priorityUpdate(prioritizedStreamID, p);
+            if (log.isTraceEnabled()) {
+                log.trace(sm.getString("http2Parser.processFramePriorityUpdate.debug", connectionId,
+                        Integer.toString(prioritizedStreamID), Integer.toString(p.getUrgency()),
+                        Boolean.valueOf(p.getIncremental())));
+            }
+
+            output.priorityUpdate(prioritizedStreamID, p);
+        } catch (IllegalArgumentException iae) {
+            // Priority frames with invalid priority field values should be ignored
+            if (log.isTraceEnabled()) {
+                log.trace(sm.getString("http2Parser.processFramePriorityUpdate.invalid", connectionId,
+                        Integer.toString(prioritizedStreamID)), iae);
+            }
+        }
     }
 
 

@@ -77,6 +77,7 @@ http2Parser.processFrameHeaders.decodingDataLeft=Data left over after HPACK deco
 http2Parser.processFrameHeaders.decodingFailed=There was an error during the HPACK decoding of HTTP headers
 http2Parser.processFrameHeaders.payload=Connection [{0}], Stream [{1}], Processing headers payload of size [{2}]
 http2Parser.processFramePriorityUpdate.debug=Connection [{0}], Stream [{1}], Urgency [{2}], Incremental [{3}]
+http2Parser.processFramePriorityUpdate.invalid=Connection [{0}], Stream [{1}], Priority Update frame with invalid priority field value
 http2Parser.processFramePriorityUpdate.streamZero=Connection [{0}], Priority update frame received to prioritize stream zero
 http2Parser.processFramePushPromise=Connection [{0}], Stream [{1}], Push promise frames should not be sent by the client
 http2Parser.processFrameSettings.ackWithNonZeroPayload=Settings frame received with the ACK flag set and payload present

@@ -17,6 +17,7 @@
 package org.apache.coyote.http2;
 
 import java.io.IOException;
+import java.nio.charset.StandardCharsets;
 
 import org.junit.Assert;
 import org.junit.Test;
@@ -146,6 +147,9 @@ public void testPriority() throws Exception {
         // 19 - 7021 body left
         // 21 - 6143 body left
 
+        // BZ 69614 - invalid priority update frames should be ignored
+        sendInvalidPriorityUpdate(17);
+
         // Re-order the priorities
         sendPriorityUpdate(17, 2, true);
 
@@ -191,4 +195,25 @@ public void testPriority() throws Exception {
             ioe.printStackTrace();
         }
     }
+
+
+    private void sendInvalidPriorityUpdate(int streamId) throws IOException {
+        byte[] payload = "u=1:i".getBytes(StandardCharsets.US_ASCII);
+
+        byte[] priorityUpdateFrame = new byte[13 + payload.length];
+
+        // length
+        ByteUtil.setThreeBytes(priorityUpdateFrame, 0, 4 + payload.length);
+        // type
+        priorityUpdateFrame[3] = FrameType.PRIORITY_UPDATE.getIdByte();
+        // Stream ID
+        ByteUtil.set31Bits(priorityUpdateFrame, 5, 0);
+
+        // Payload
+        ByteUtil.set31Bits(priorityUpdateFrame, 9, streamId);
+        System.arraycopy(payload, 0, priorityUpdateFrame, 13, payload.length);
+
+        os.write(priorityUpdateFrame);
+        os.flush();
+    }
 }

@@ -136,6 +136,10 @@
         <bug>69607</bug>: Allow failed initialization of MD5. Based on code
         submitted by Shivam Verma. (remm)
       </fix>
+      <fix>
+        <bug>69614</bug>: HTTP/2 priority frames with an invalid priority field
+        value should be ignored. (markt)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Jasper">