From 8dd49dfabaf60f95da0387d0c0b5d4060dc28a6b Mon Sep 17 00:00:00 2001 From: Christopher Faulet Date: Mon, 18 May 2026 16:09:30 +0200 Subject: [PATCH] BUG/MEDIUM: h1: Skip all h2c values from Upgrade headers during parsing During the H1 message parsing, the Upgrade header values are checked to detect "h2c" and "h2" tokens and skip them. To do so, we rely on H1_MF_UPG_H2C flag, set during the parsing. And during the request post-parsing, if this flag was set, all Upgrade headers are removed. This was fixed by the commit 7b89aa5b1 ("BUG/MINOR: h1: do not forward h2c upgrade header token"). However, there are two issues here and the commit above must be refined. First, the flag is reset for each new Upgrade header. So "h2c" or "h2" tokens will be properly detected if all tokens are set on the same Upgrade header. But if splitted on several headers, previously detected tokens will be hidden by a next ones. Concretly, the following will be properly caught Connection: upgrade Upgrade: foo, h2c, bar But then following not: Connection: upgrade: Upgrade: foo, h2c Upgrade: bar Then, when a "h2c" or "h2" token is finally reported, all Upgrade headers are removed, regardless other tokens. So, to fix the both issues, everything is now handled during the message parsing by skipping "h2c" and "h2" tokens, rebuilding the Upgrade header value without then offending tokens. The same was already performed for the Connection header, to skip "keep-alive" and "close" value. So it is not a so fancy change. Thanks to this change, it is no longer necessary to handle H1_MF_UPG_H2C during the request post-parsing. And in fact, this flag is no longer necessary. So let's remove it too. Thanks to Vincent55 for finding and reporting this. This patch must be backported as far as 2.4. --- include/haproxy/h1.h | 4 ++-- src/h1.c | 35 ++++++++++++++++++++++++++--------- src/h1_htx.c | 14 -------------- src/mux_h1.c | 4 +++- 4 files changed, 31 insertions(+), 26 deletions(-) diff --git a/include/haproxy/h1.h b/include/haproxy/h1.h index a9d01d047..25d3e5fd0 100644 --- a/include/haproxy/h1.h +++ b/include/haproxy/h1.h @@ -98,7 +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 +/* unused: 0x00040000 */ #define H1_MF_NOT_HTTP 0x00080000 // Not an HTTP message (e.g "RTSP", only possible if invalid message are accepted) /* Mask to use to reset H1M flags when we restart headers parsing. * @@ -160,7 +160,7 @@ int h1_headers_to_hdr_list(char *start, const char *stop, int h1_parse_xfer_enc_header(struct h1m *h1m, struct ist value); void h1_parse_connection_header(struct h1m *h1m, struct ist *value); -void h1_parse_upgrade_header(struct h1m *h1m, struct ist value); +void h1_parse_upgrade_header(struct h1m *h1m, struct ist *value); void h1_generate_random_ws_input_key(char key_out[25]); void h1_calculate_ws_output_key(const char *key, char *result); diff --git a/src/h1.c b/src/h1.c index 7fb78804f..792737c9e 100644 --- a/src/h1.c +++ b/src/h1.c @@ -274,17 +274,19 @@ 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. + * If "h2c" or "h2" found, the value is skipped. */ -void h1_parse_upgrade_header(struct h1m *h1m, struct ist value) +void h1_parse_upgrade_header(struct h1m *h1m, struct ist *value) { - char *e, *n; + char *e, *n, *p; struct ist word; - h1m->flags &= ~(H1_MF_UPG_WEBSOCKET|H1_MF_UPG_H2C); + h1m->flags &= ~H1_MF_UPG_WEBSOCKET; - word.ptr = value.ptr - 1; // -1 for next loop's pre-increment - e = istend(value); + word.ptr = value->ptr - 1; // -1 for next loop's pre-increment + p = value->ptr; + e = value->ptr + value->len; + value->len = 0; while (++word.ptr < e) { /* skip leading delimiter and blanks */ @@ -301,9 +303,20 @@ 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; + goto skip_val; - word.ptr = n; + if (value->ptr + value->len == p) { + /* no rewrite done till now */ + value->len = n - value->ptr; + } + else { + if (value->len) + value->ptr[value->len++] = ','; + istcat(value, word, e - value->ptr); + } + + skip_val: + word.ptr = p = n; } } @@ -983,7 +996,11 @@ int h1_headers_to_hdr_list(char *start, const char *stop, } } else if (isteqi(n, ist("upgrade"))) { - h1_parse_upgrade_header(h1m, v); + h1_parse_upgrade_header(h1m, &v); + if (!v.len) { + /* skip it */ + break; + } } else if (!(h1m->flags & H1_MF_RESP) && isteqi(n, ist("host"))) { if (host_idx == -1) { diff --git a/src/h1_htx.c b/src/h1_htx.c index 9e2996e19..d0837abbc 100644 --- a/src/h1_htx.c +++ b/src/h1_htx.c @@ -215,20 +215,6 @@ static int h1_postparse_req_hdrs(struct h1m *h1m, union h1_sl *h1sl, struct htx flags |= h1m_htx_sl_flags(h1m); - /* Remove Upgrade header in problematic cases : - * - "h2c" or "h2" token specified as token - */ - if ((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++) { - if (isteqi(hdrs[i].n, ist("upgrade"))) - hdrs[i].v = IST_NULL; - } - 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; diff --git a/src/mux_h1.c b/src/mux_h1.c index 982d267b7..cb95556c7 100644 --- a/src/mux_h1.c +++ b/src/mux_h1.c @@ -2713,7 +2713,9 @@ static size_t h1_make_headers(struct h1s *h1s, struct h1m *h1m, struct htx *htx, goto nextblk; } else if (isteq(n, ist("upgrade"))) { - h1_parse_upgrade_header(h1m, v); + h1_parse_upgrade_header(h1m, &v); + if (!v.len) + goto nextblk; } else if ((isteq(n, ist("sec-websocket-accept")) && h1m->flags & H1_MF_RESP) || (isteq(n, ist("sec-websocket-key")) && !(h1m->flags & H1_MF_RESP))) {