BUG/MAJOR: mux-h1: Prevent any UAF on H1 connection after draining a request

Since 2.9, it is possible to drain the request payload from the H1
multiplexer in case of early reply. When this happens, the upper stream is
detached but the H1 stream is not destroyed. Once the whole request is
drained, the end of the detach stage is finished. So the H1 stream is
destroyed and the H1 connection is ready to be reused, if possible,
otherwise it is released.

And here is the issue. If some data of the next request are received with
last bytes of the drained one, parsing of the next request is immediately
started. The previous H1 stream is destroyed and a new one is created to
handle the parsing.  At this stage the H1 connection may be released, for
instance because of a parsing error. This case was not properly handled.
Instead of immediately exiting the mux, it was still possible to access the
released H1 connection to refresh its timeouts, leading to a UAF issue.

Many thanks to Annika for her invaluable help on this issue.

The patch should fix the issue #2602. It must be backported as far as 2.9.

(cherry picked from commit 0e09cce)
Signed-off-by: Christopher Faulet <[email protected]>

Author: capflam

src/mux_h1.c

@@ -1018,9 +1018,10 @@ static int h1s_must_shut_conn(struct h1s *h1s)
 
 /* Really detach the H1S. Most of time of it called from h1_detach() when the
  * stream is detached from the connection. But if the request message must be
- * drained first, the detach is deferred.
+ * drained first, the detach is deferred. Returns 0 if the h1s is detached but
+ * h1c is still usable. -1 is returned if h1s was released.
  */
-static void h1s_finish_detach(struct h1s *h1s)
+static int h1s_finish_detach(struct h1s *h1s)
 {
 	struct h1c *h1c;
 	struct session *sess;
@@ -1063,7 +1064,7 @@ static void h1s_finish_detach(struct h1s *h1s)
 			if (!session_add_conn(sess, h1c->conn, h1c->conn->target)) {
 				h1c->conn->owner = NULL;
 				h1c->conn->mux->destroy(h1c);
-				goto end;
+				goto released;
 			}
 			/* Always idle at this step */
 
@@ -1074,7 +1075,7 @@ static void h1s_finish_detach(struct h1s *h1s)
 			if (session_check_idle_conn(sess, h1c->conn)) {
 				/* The connection got destroyed, let's leave */
 				TRACE_DEVEL("outgoing connection killed", H1_EV_STRM_END|H1_EV_H1C_END);
-				goto end;
+				goto released;
 			}
 		}
 		else {
@@ -1092,13 +1093,13 @@ static void h1s_finish_detach(struct h1s *h1s)
 				/* The server doesn't want it, let's kill the connection right away */
 				h1c->conn->mux->destroy(h1c);
 				TRACE_DEVEL("outgoing connection killed", H1_EV_STRM_END|H1_EV_H1C_END);
-				goto end;
+				goto released;
 			}
 			/* At this point, the connection has been added to the
 			 * server idle list, so another thread may already have
 			 * hijacked it, so we can't do anything with it.
 			 */
-			return;
+			goto end;
 		}
 	}
 
@@ -1110,15 +1111,18 @@ static void h1s_finish_detach(struct h1s *h1s)
 	    !h1c->conn->owner) {
 		TRACE_DEVEL("killing dead connection", H1_EV_STRM_END, h1c->conn);
 		h1_release(h1c);
+		goto released;
 	}
 	else {
 		if (h1c->state == H1_CS_IDLE) {
 			/* If we have a new request, process it immediately or
 			 * subscribe for reads waiting for new data
 			 */
 			if (unlikely(b_data(&h1c->ibuf))) {
-				if (h1_process(h1c) == -1)
-					goto end;
+				if (h1_process(h1c) == -1) {
+					/* h1c was released, don't reuse it anymore */
+					goto released;
+				}
 			}
 			else
 				h1c->conn->xprt->subscribe(h1c->conn, h1c->conn->xprt_ctx, SUB_RETRY_RECV, &h1c->wait_event);
@@ -1128,6 +1132,11 @@ static void h1s_finish_detach(struct h1s *h1s)
 	}
   end:
 	TRACE_LEAVE(H1_EV_STRM_END);
+	return 0;
+
+  released:
+	TRACE_DEVEL("leaving after releasing the connection", H1_EV_STRM_END);
+	return -1;
 }
 
 
@@ -4017,8 +4026,8 @@ static int h1_process(struct h1c * h1c)
 				h1_shutw_conn(conn);
 				goto release;
 			}
-			h1s_finish_detach(h1c->h1s);
-			goto end;
+			if (h1s_finish_detach(h1c->h1s) == -1)
+				goto released;
 		}
 	}
 
@@ -4088,6 +4097,7 @@ static int h1_process(struct h1c * h1c)
 		h1_release(h1c);
 		TRACE_DEVEL("leaving after releasing the connection", H1_EV_H1C_WAKE);
 	}
+  released:
 	return -1;
 }