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.

diff --git a/include/haproxy/h1.h b/include/haproxy/h1.h
index a9d01d0477282022620a1cfd198b00a03fbc5150..25d3e5fd028c8053650bc438653ba201914c734b 100644 (file)
--- 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 7fb78804f2fb3d45659946bfa782a453841bb439..792737c9e3f5ae2bed155545293b71c4ff0ce614 100644 (file)
--- 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 9e2996e19bebb29bd68f1e5beec13769e19ce179..d0837abbcc7e2bfb1bbf32ee249c4ef8b30a1ba9 100644 (file)
--- 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 982d267b797713d943dc1163c5ea99879184559b..cb95556c7bb87e0903a79dd442e6aebfca833942 100644 (file)
--- 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))) {