From: Amaury Denoyelle <adenoyelle@haproxy.com>
Date: Thu, 1 Aug 2024 13:35:06 +0000 (+0200)
Subject: BUG/MINOR: h1: do not forward h2c upgrade header token
X-Git-Tag: v3.0.6~91
X-Git-Url: http://git.haproxy.org/?a=commitdiff_plain;h=cba44958ae00821626990add07c937e305955b95;p=haproxy-3.0.git

BUG/MINOR: h1: do not forward h2c upgrade header token

haproxy supports tunnel establishment through HTTP Upgrade mechanism.
Since the following commit, extended CONNECT is also supported for
HTTP/2 both on frontend and backend side.

  commit 9bf957335e2c385b74901481f7a89c9565dfce53
  MEDIUM: mux_h2: generate Extended CONNECT from htx upgrade

As specified by HTTP/2 rfc, "h2c" can be used by an HTTP/1.1 client to
request an upgrade to HTTP/2. In haproxy, this is not supported so it
silently ignores this. However, Connection and Upgrade headers are
forwarded as-is on the backend side.

If using HTTP/1 on the backend side and the server supports this upgrade
mechanism, haproxy won't be able to parse the HTTP response. If using
HTTP/2, mux backend tries to incorrectly convert the request to an
Extended CONNECT with h2c protocol, which may also prevent the response
to be transmitted.

To fix this, flag HTTP/1 request with "h2c" or "h2" token in an upgrade
header. On converting the header list to HTX, the upgrade header is
skipped if any of this token is present and the H1_MF_CONN_UPG flag is
removed.

This issue can easily be reproduced using curl --http2 argument to
connect to an HTTP/1 frontend.

This must be backported up to 2.4 after a period of observation.

(cherry picked from commit 7b89aa5b1943c1537a844cc375f7e8a86bac7942)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
---

diff --git a/include/haproxy/h1.h b/include/haproxy/h1.h
index 283bbad..b4e6690 100644
--- a/include/haproxy/h1.h
+++ b/include/haproxy/h1.h
@@ -98,6 +98,7 @@ enum h1m_state {
 #define H1_MF_UPG_WEBSOCKET     0x00008000 // Set for a Websocket upgrade handshake
 #define H1_MF_TE_CHUNKED        0x00010000 // T-E "chunked"
 #define H1_MF_TE_OTHER          0x00020000 // T-E other than supported ones found (only "chunked" is supported for now)
+#define H1_MF_UPG_H2C           0x00040000 // "h2c" or "h2" used as upgrade token
 
 /* Mask to use to reset H1M flags when we restart headers parsing.
  *
diff --git a/reg-tests/http-messaging/protocol_upgrade.vtc b/reg-tests/http-messaging/protocol_upgrade.vtc
index ebb6328..be291f1 100644
--- a/reg-tests/http-messaging/protocol_upgrade.vtc
+++ b/reg-tests/http-messaging/protocol_upgrade.vtc
@@ -85,6 +85,15 @@ server srv_h2_no_ws2 {
 	} -run
 } -start
 
+# http/1.1 server
+server srv_h1_h2c {
+	rxreq
+	expect req.method == "GET"
+	expect req.http.upgrade != "h2c"
+	txresp \
+	  -status 200
+} -start
+
 haproxy hap -conf {
 	defaults
 	mode http
@@ -121,6 +130,11 @@ haproxy hap -conf {
 	listen frt_h2_h1
 	bind "fd@${frt_h2_h1}" proto h2
 	server frt_h1 ${hap_frt_h1_addr}:${hap_frt_h1_port}
+
+	# h1 frontend to handle "h2c" token
+	listen frt_h1_h2c
+	bind "fd@${frt_h1_h2c}"
+	server srv_h1_h2c ${srv_h1_h2c_addr}:${srv_h1_h2c_port}
 } -start
 
 ## connect to h1 translation frontend
@@ -226,3 +240,16 @@ client c6 -connect ${hap_frt_h1_no_ws2_sock} {
 	rxresp
 	expect resp.status == 502
 } -run
+
+# connect as http/1 with invalid "h2c" protocol
+client c7_h2c -connect ${hap_frt_h1_h2c_sock} {
+	txreq \
+	  -req "GET" \
+	  -url "/" \
+	  -hdr "host: 127.0.0.1" \
+	  -hdr "connection: upgrade" \
+	  -hdr "upgrade: h2c"
+
+	rxresp
+	expect resp.status == 200
+} -run
diff --git a/src/h1.c b/src/h1.c
index 64fa470..09047fd 100644
--- a/src/h1.c
+++ b/src/h1.c
@@ -368,13 +368,14 @@ void h1_parse_connection_header(struct h1m *h1m, struct ist *value)
 
 /* Parse the Upgrade: header of an HTTP/1 request.
  * If "websocket" is found, set H1_MF_UPG_WEBSOCKET flag
+ * If "h2c" or "h2" found, set H1_MF_UPG_H2C flag.
  */
 void h1_parse_upgrade_header(struct h1m *h1m, struct ist value)
 {
 	char *e, *n;
 	struct ist word;
 
-	h1m->flags &= ~H1_MF_UPG_WEBSOCKET;
+	h1m->flags &= ~(H1_MF_UPG_WEBSOCKET|H1_MF_UPG_H2C);
 
 	word.ptr = value.ptr - 1; // -1 for next loop's pre-increment
 	e = istend(value);
@@ -393,6 +394,8 @@ void h1_parse_upgrade_header(struct h1m *h1m, struct ist value)
 
 		if (isteqi(word, ist("websocket")))
 			h1m->flags |= H1_MF_UPG_WEBSOCKET;
+		else if (isteqi(word, ist("h2c")) || isteqi(word, ist("h2")))
+			h1m->flags |= H1_MF_UPG_H2C;
 
 		word.ptr = n;
 	}
diff --git a/src/h1_htx.c b/src/h1_htx.c
index 5b8b4e2..bcde324 100644
--- a/src/h1_htx.c
+++ b/src/h1_htx.c
@@ -180,7 +180,13 @@ static int h1_postparse_req_hdrs(struct h1m *h1m, union h1_sl *h1sl, struct htx
 
 
 	flags |= h1m_htx_sl_flags(h1m);
-	if ((flags & (HTX_SL_F_CONN_UPG|HTX_SL_F_BODYLESS)) == HTX_SL_F_CONN_UPG) {
+
+	/* Remove Upgrade header in problematic cases :
+	 * - body present
+	 * - "h2c" or "h2" token specified as token
+	 */
+	if (((flags & (HTX_SL_F_CONN_UPG|HTX_SL_F_BODYLESS)) == HTX_SL_F_CONN_UPG) ||
+	    ((h1m->flags & (H1_MF_CONN_UPG|H1_MF_UPG_H2C)) == (H1_MF_CONN_UPG|H1_MF_UPG_H2C))) {
 		int i;
 
 		for (i = 0; hdrs[i].n.len; i++) {
@@ -190,6 +196,7 @@ static int h1_postparse_req_hdrs(struct h1m *h1m, union h1_sl *h1sl, struct htx
 		h1m->flags &=~ H1_MF_CONN_UPG;
 		flags &= ~HTX_SL_F_CONN_UPG;
 	}
+
 	sl = htx_add_stline(htx, HTX_BLK_REQ_SL, flags, meth, uri, vsn);
 	if (!sl || !htx_add_all_headers(htx, hdrs))
 		goto error;