mod_http3 lifetime security fixes

changes-entries/h3_connect_crash.txt

@@ -0,0 +1,2 @@
+  *) mod_http3: A CONNECT request with no :path no longer crashes the child
+     process. [Alexander Gerasimov <repo codeit.guru>]

changes-entries/h3_lifetime_fixes.txt

@@ -0,0 +1,4 @@
+  *) mod_http3: Fix a write to a freed session when a request completes on
+     a closed connection, lost entries in the QUIC accept queues, and new
+     connections that stay allocated during a graceful stop.
+     [Alexander Gerasimov <repo codeit.guru>]

modules/http3/h3_request.c

@@ -290,11 +290,9 @@ static void* APR_THREAD_FUNC stream_worker(apr_thread_t* thd, void* data)
     r->header_only = (r->method_number == M_GET && r->method[0] == 'H');
     h3s->r = r;
 
-    if (h3s->path)
-    {
-        ap_parse_uri(r, h3s->path);
-        r->the_request = apr_pstrcat(r->pool, r->method, " ", r->unparsed_uri, " ", r->protocol, NULL);
-    }
+    /* CONNECT has no :path; its target is the authority, as in HTTP/1.1. */
+    ap_parse_uri(r, h3s->path ? h3s->path : h3s->authority ? h3s->authority : "");
+    r->the_request = apr_pstrcat(r->pool, r->method, " ", r->unparsed_uri, " ", r->protocol, NULL);
     if (h3s->authority)
     {
         apr_table_setn(r->headers_in, "Host", apr_pstrdup(r->pool, h3s->authority));
@@ -421,8 +419,8 @@ static void* APR_THREAD_FUNC stream_worker(apr_thread_t* thd, void* data)
     apr_thread_mutex_lock(session->lock);
     h3s->worker_done = 1;
     apr_thread_mutex_unlock(session->lock);
-    apr_atomic_dec32(&session->active_tasks);
     h3_session_wake(session); /* the event thread reclaims the stream */
+    apr_atomic_dec32(&session->active_tasks); /* last: at zero the event thread can free the session */
     return NULL;
 }
 

modules/http3/h3_threads.c

@@ -103,7 +103,7 @@ void* APR_THREAD_FUNC h3_event_thread(apr_thread_t* thread, void* data)
             } while (pumped > 0 && ++pump_rounds < H3_PUMP_BUDGET);
         }
 
-        if (io->thread_running && !io->draining)
+        if (io->thread_running)
         {
             for (;;)
             {
@@ -112,6 +112,11 @@ void* APR_THREAD_FUNC h3_event_thread(apr_thread_t* thread, void* data)
                 {
                     break;
                 }
+                if (io->draining)
+                {
+                    h3q_conn_free(conn); /* no new connections during a graceful stop */
+                    continue;
+                }
                 ap_log_error(APLOG_MARK, APLOG_INFO, 0, io->server, "accepted new QUIC connection");
                 if (h3_io_at_connection_limit(io))
                 {

modules/http3/quic/h3q_conn.c

@@ -590,14 +590,14 @@ void h3q_conn_free(h3q_conn* conn)
             break;
         }
     }
-    for (h3q_conn** slot = &engine->accept_head; *slot; slot = &(*slot)->next_accept)
+    for (h3q_conn *prev = NULL, **slot = &engine->accept_head; *slot; prev = *slot, slot = &(*slot)->next_accept)
     {
         if (*slot == conn)
         {
             *slot = conn->next_accept;
-            if (!*slot)
+            if (engine->accept_tail == conn)
             {
-                engine->accept_tail = NULL;
+                engine->accept_tail = prev;
             }
             break;
         }

modules/http3/quic/h3q_stream.c

@@ -241,14 +241,14 @@ void h3q_stream_free(h3q_stream* st)
             break;
         }
     }
-    for (h3q_stream** slot = &conn->accept_head; *slot; slot = &(*slot)->next_accept)
+    for (h3q_stream *prev = NULL, **slot = &conn->accept_head; *slot; prev = *slot, slot = &(*slot)->next_accept)
     {
         if (*slot == st)
         {
             *slot = st->next_accept;
-            if (!*slot)
+            if (conn->accept_tail == st)
             {
-                conn->accept_tail = NULL;
+                conn->accept_tail = prev;
             }
             break;
         }

test/modules/http3/test_108_misc.py

@@ -24,6 +24,8 @@ def _class_scope(cls, env):
             "  SetOutputFilter INCLUDES",
             "</Files>",
             f"ProxyPass /proxy/ http://127.0.0.1:{env.http_port}/",
+            "<Location /h3-location>",
+            "</Location>",
         ]).install()
         assert env.apache_restart() == 0
 
@@ -59,3 +61,10 @@ def test_h3_108_06_proxy_post(self, env):
                                               body=body)
         assert r.status == 200, f"{r}"
         assert r.body == body
+
+    def test_h3_108_07_connect_no_path(self, env):
+        # A CONNECT has no :path. The location walk must not get a NULL URI.
+        r1, r2 = env.h3_client().requests([dict(method="CONNECT"), dict(path="/")],
+                                          sequential=True)
+        assert 400 <= r1.status < 500, f"{r1}"
+        assert r2.status == 200, f"{r2}"

test/pyhttpd/h3client.py

@@ -118,6 +118,9 @@ def send_request(self, method, path, authority, headers, body,
             self.http.head_streams.add(sid)
         hdrs = [(b":method", method.encode()), (b":scheme", b"https"),
                 (b":authority", authority.encode()), (b":path", path.encode())]
+        if method == "CONNECT":
+            # RFC 9114 4.4: no :scheme and no :path
+            hdrs = [hdrs[0], hdrs[2]]
         hdrs.extend((k.lower().encode(), v.encode()) for k, v in headers)
         self.http.send_headers(sid, hdrs, end_stream=body is None)
         if body is not None: