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 84c9d921779c8cfc88b53077129db3a1ea92d836..caf9a8e42d5347702a4818680fda09cf516fd8a5 100644 (file)
--- 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 6190f479895ae80a1f49c8a61d546d5d3f440438..8d23cf9a5234f3d13f47ff4b35813211d4342770 100644 (file)
--- 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 f5ddd24954bc3e3ae48c612287a28657b31032fe..60cee11ce395217e32175f1d4a5ffa7e8328acb8 100644 (file)
--- 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;