From: Christopher Faulet Date: Wed, 7 Oct 2026 08:32:17 +0000 (+0200) Subject: BUG/MEDIUM: h1: make the Connection/Upgrade headers cleanup idempotent X-Git-Tag: v3.5-dev10~160 X-Git-Url: https://git.kaiwu.me/$%7BGITURL%7D$%7Bcid%7D?a=commitdiff_plain;h=a71005572b6cdfbb972fd5df554cff1c8718bf6a;p=haproxy.git BUG/MEDIUM: h1: make the Connection/Upgrade headers cleanup idempotent The H1 parser rewrites the Connection header value in place to remove the "close" and "keep-alive" tokens (H1_MF_CLEAN_CONN_HDR, always set by the H1 mux), and the Upgrade header value to remove the "h2c" and "h2" tokens. The kept tokens are moved to the beginning of the value and the value's length is reduced, but the original bytes beyond the new length are left untouched in the input buffer. However the parser is designed to restart from the beginning of the message once an incomplete one is completed, and it relies on in-place modifications being idempotent. When the headers block is received in several parts and one of these headers was already processed in a first pass, the second pass parses the rewritten value followed by the remains of the original one. To fix the issue, the bytes freed at the end of the rewritten value are now replaced with spaces so that parsing the same area again gives the same value (trailing spaces are trimmed). And since the removed "close" and "keep-alive" tokens are not there anymore on the second pass, the H1_MF_CONN_CLO and H1_MF_CONN_KAL flags are now preserved when the parsing is restarted, and when the parser is reset after an output full condition. The Connection header issue exists since commit a51ebb7f5 ("MEDIUM: h1: Add an option to sanitize connection headers during parsing") in 2.0, the Upgrade one since commit 8dd49dfab ("BUG/MEDIUM: h1: Skip all h2c values from Upgrade headers during parsing") in 3.4 (and its backports). This must be backported to all stable versions. --- diff --git a/include/haproxy/h1.h b/include/haproxy/h1.h index 84c9d9217..caf9a8e42 100644 --- a/include/haproxy/h1.h +++ b/include/haproxy/h1.h @@ -104,9 +104,14 @@ enum h1m_state { * * WARNING: Don't forget to update it if a new flag must be preserved when * headers parsing is restarted. + * + * Note: H1_MF_CONN_CLO and H1_MF_CONN_KAL must be preserved because the + * corresponding values may have been removed from the connection + * headers before the restart (see h1_parse_connection_header()). */ #define H1_MF_RESTART_MASK (H1_MF_RESP|H1_MF_TOLOWER|H1_MF_NO_PHDR|H1_MF_HDRS_ONLY| \ - H1_MF_CLEAN_CONN_HDR|H1_MF_METH_CONNECT|H1_MF_METH_HEAD) + H1_MF_CLEAN_CONN_HDR|H1_MF_METH_CONNECT|H1_MF_METH_HEAD| \ + H1_MF_CONN_CLO|H1_MF_CONN_KAL) /* Note: for a connection to be persistent, we need this for the request : * - one of CLEN or CHNK diff --git a/src/h1.c b/src/h1.c index 6190f4798..8d23cf9a5 100644 --- a/src/h1.c +++ b/src/h1.c @@ -217,7 +217,12 @@ static int h1_validate_mismatch_authority(struct ist scheme, struct ist authorit * function is safe for being called multiple times if multiple occurrences * are found. If the flag H1_MF_CLEAN_CONN_HDR, the header value is cleaned * up from "keep-alive" and "close" values. To do so, the header value is - * rewritten in place and its length is updated. + * rewritten in place and its length is updated. The bytes freed at the end of + * the original value are replaced with spaces so that parsing the same area + * again (e.g. when the parser restarts on an incomplete message) gives the + * same value. However the removed values are not there anymore, so the caller + * must preserve the H1_MF_CONN_CLO and H1_MF_CONN_KAL flags across such + * restarts. */ void h1_parse_connection_header(struct h1m *h1m, struct ist *value) { @@ -270,11 +275,17 @@ void h1_parse_connection_header(struct h1m *h1m, struct ist *value) skip_val: word.ptr = p = n; } + + /* blank the freed area to make the operation idempotent */ + if ((h1m->flags & H1_MF_CLEAN_CONN_HDR) && value->ptr + value->len < e) + memset(value->ptr + value->len, ' ', e - (value->ptr + value->len)); } /* Parse the Upgrade: header of an HTTP/1 request. * If "websocket" is found, set H1_MF_UPG_WEBSOCKET flag - * If "h2c" or "h2" found, the value is skipped. + * If "h2c" or "h2" found, the value is skipped. The header value is rewritten + * in place and the bytes freed at the end of the original value are replaced + * with spaces so that parsing the same area again gives the same value. */ void h1_parse_upgrade_header(struct h1m *h1m, struct ist *value) { @@ -317,6 +328,10 @@ void h1_parse_upgrade_header(struct h1m *h1m, struct ist *value) word.ptr = p = n; } + /* blank the freed area to make the operation idempotent */ + if (value->ptr + value->len < e) + memset(value->ptr + value->len, ' ', e - (value->ptr + value->len)); + if (istlen(*value)) h1m->flags |= H1_MF_UPG_HDR; } diff --git a/src/h1_htx.c b/src/h1_htx.c index f5ddd2495..60cee11ce 100644 --- a/src/h1_htx.c +++ b/src/h1_htx.c @@ -264,8 +264,12 @@ static int h1_postparse_req_hdrs(struct h1m *h1m, union h1_sl *h1sl, struct htx return 1; output_full: + /* The connection header values may have been cleaned up, so the + * corresponding flags must be preserved for the next parsing. + */ + flags = h1m->flags & (H1_MF_CONN_CLO|H1_MF_CONN_KAL); h1m_init_req(h1m); - h1m->flags |= (H1_MF_NO_PHDR|H1_MF_CLEAN_CONN_HDR); + h1m->flags |= (H1_MF_NO_PHDR|H1_MF_CLEAN_CONN_HDR|flags); return -2; error: h1m->err_pos = h1m->next; @@ -385,8 +389,12 @@ static int h1_postparse_res_hdrs(struct h1m *h1m, union h1_sl *h1sl, struct htx return 1; output_full: + /* The connection header values may have been cleaned up, so the + * corresponding flags must be preserved for the next parsing. + */ + flags = h1m->flags & (H1_MF_CONN_CLO|H1_MF_CONN_KAL); h1m_init_res(h1m); - h1m->flags |= (H1_MF_NO_PHDR|H1_MF_CLEAN_CONN_HDR); + h1m->flags |= (H1_MF_NO_PHDR|H1_MF_CLEAN_CONN_HDR|flags); return -2; error: h1m->err_pos = h1m->next;