BUG/MEDIUM: h2/h3: reject some forbidden chars in :authority before reassembly

As discussed here:
   https://github.com/httpwg/http2-spec/pull/936
   https://github.com/haproxy/haproxy/issues/2941

It's important to take care of some special characters in the :authority
pseudo header before reassembling a complete URI, because after assembly
it's too late (e.g. the '/'). This patch does this, both for h2 and h3.

The impact on H2 was measured in the worst case at 0.3% of the request
rate, while the impact on H3 is around 1%, but H3 was about 1% faster
than H2 before and is now on par.

It may be backported after a period of observation, and in this case it
relies on this previous commit:

   MINOR: http: add a function to validate characters of :authority

Thanks to @DemiMarie for reviving this topic in issue #2941 and bringing
new potential interesting cases.

diff --git a/src/h2.c b/src/h2.c
index 3cb0af75727a64016ec028b7121787d1cd3aedeb..5a8c250688b76b61ae2bb687e851051c893ab8bc 100644 (file)
--- a/src/h2.c
+++ b/src/h2.c
@@ -203,6 +203,17 @@ static struct htx_sl *h2_prepare_htx_reqline(uint32_t fields, struct ist *phdr,
                }
        }
 
+       /* We're going to concatenate :authority with :path to form a URI. Some
+        * characters must absolutely be avoided in :authority to make sure not
+        * to result in a broken concatenation. See the following links for a
+        * discussion on this topic:
+        *   https://github.com/httpwg/http2-spec/pull/936
+        *   https://github.com/haproxy/haproxy/issues/2941
+        */
+       if ((fields & H2_PHDR_FND_AUTH) &&
+           http_authority_has_forbidden_char(phdr[H2_PHDR_IDX_AUTH]))
+               goto fail;
+
        if (!(flags & HTX_SL_F_HAS_SCHM)) {
                /* no scheme, use authority only (CONNECT) */
                uri = phdr[H2_PHDR_IDX_AUTH];

diff --git a/src/h3.c b/src/h3.c
index eb1ff3575d1903aa58a8ede7cdf9a1645523a548..680589a6548118479694fc8571b033ccf2aea0fe 100644 (file)
--- a/src/h3.c
+++ b/src/h3.c
@@ -753,6 +753,21 @@ static ssize_t h3_headers_to_htx(struct qcs *qcs, const struct buffer *buf,
                goto out;
        }
 
+       /* We're going to concatenate :authority with :path to form a URI. Some
+        * characters must absolutely be avoided in :authority to make sure not
+        * to result in a broken concatenation. See the following links for a
+        * discussion on this topic:
+        *   https://github.com/httpwg/http2-spec/pull/936
+        *   https://github.com/haproxy/haproxy/issues/2941
+        */
+       if (http_authority_has_forbidden_char(authority)) {
+               TRACE_ERROR("invalid character in authority", H3_EV_RX_FRAME|H3_EV_RX_HDR, qcs->qcc->conn, qcs);
+               h3s->err = H3_ERR_MESSAGE_ERROR;
+               qcc_report_glitch(h3c->qcc, 1);
+               len = -1;
+               goto out;
+       }
+
        if (!istlen(scheme)) {
                /* No scheme (CONNECT), use :authority only. */
                uri = authority;