BUG/MINOR: h2: always trim leading and trailing LWS in header values

Annika Wickert reported some occasional disconnections between haproxy
and varnish when communicating over HTTP/2, with varnish complaining
about protocol errors while captures looked apparently normal. Nils
Goroll managed to reproduce this on varnish by injecting the capture of
the outgoing haproxy traffic and noticed that haproxy was forwarding a
header value containing a trailing space, which is now explicitly
forbidden since RFC9113.

It turns out that the only way for such a header to pass through haproxy
is to arrive in h2 and not be edited, in which case it will arrive in
HTX with its undesired spaces. Since the code dealing with HTX headers
always trims spaces around them, these are not observable in dumps, but
only when started in debug mode (-d). Conversions to/from h1 also drop
the spaces.

With this patch we trim LWS both on input and on output. This way we
always present clean headers in the whole stack, and even if some are
manually crafted by the configuration or Lua, they will be trimmed on
the output.

This must be backported to all stable versions.

Thanks to Annika for the helpful capture and Nils for the help with
the analysis on the varnish side!

diff --git a/src/h2.c b/src/h2.c
index 536240a6dd797d711a57df6cc7e40134ab55a189..fa929aece202bf6c9e46f784fafea9a4ce051ea3 100644 (file)
--- a/src/h2.c
+++ b/src/h2.c
@@ -318,6 +318,7 @@ int h2_make_htx_request(struct http_hdr *list, struct htx *htx, unsigned int *ms
        struct htx_sl *sl = NULL;
        unsigned int sl_flags = 0;
        const char *ctl;
+       struct ist v;
 
        lck = ck = -1; // no cookie for now
        fields = 0;
@@ -434,7 +435,15 @@ int h2_make_htx_request(struct http_hdr *list, struct htx *htx, unsigned int *ms
                        continue;
                }
 
-               if (!htx_add_header(htx, list[idx].n, list[idx].v))
+               /* trim leading/trailing LWS as per RC9113#8.2.1 */
+               for (v = list[idx].v; v.len; v.len--) {
+                       if (unlikely(HTTP_IS_LWS(*v.ptr)))
+                               v.ptr++;
+                       else if (!unlikely(HTTP_IS_LWS(v.ptr[v.len - 1])))
+                               break;
+               }
+
+               if (!htx_add_header(htx, list[idx].n, v))
                        goto fail;
        }
 
@@ -623,6 +632,7 @@ int h2_make_htx_response(struct http_hdr *list, struct htx *htx, unsigned int *m
        struct htx_sl *sl = NULL;
        unsigned int sl_flags = 0;
        const char *ctl;
+       struct ist v;
 
        fields = 0;
        for (idx = 0; list[idx].n.len != 0; idx++) {
@@ -702,7 +712,15 @@ int h2_make_htx_response(struct http_hdr *list, struct htx *htx, unsigned int *m
                    isteq(list[idx].n, ist("transfer-encoding")))
                        goto fail;
 
-               if (!htx_add_header(htx, list[idx].n, list[idx].v))
+               /* trim leading/trailing LWS as per RC9113#8.2.1 */
+               for (v = list[idx].v; v.len; v.len--) {
+                       if (unlikely(HTTP_IS_LWS(*v.ptr)))
+                               v.ptr++;
+                       else if (!unlikely(HTTP_IS_LWS(v.ptr[v.len - 1])))
+                               break;
+               }
+
+               if (!htx_add_header(htx, list[idx].n, v))
                        goto fail;
        }
 
@@ -781,6 +799,7 @@ int h2_make_htx_response(struct http_hdr *list, struct htx *htx, unsigned int *m
 int h2_make_htx_trailers(struct http_hdr *list, struct htx *htx)
 {
        const char *ctl;
+       struct ist v;
        uint32_t idx;
        int i;
 
@@ -816,7 +835,15 @@ int h2_make_htx_trailers(struct http_hdr *list, struct htx *htx)
                if (unlikely(ctl) && http_header_has_forbidden_char(list[idx].v, ctl))
                        goto fail;
 
-               if (!htx_add_trailer(htx, list[idx].n, list[idx].v))
+               /* trim leading/trailing LWS as per RC9113#8.2.1 */
+               for (v = list[idx].v; v.len; v.len--) {
+                       if (unlikely(HTTP_IS_LWS(*v.ptr)))
+                               v.ptr++;
+                       else if (!unlikely(HTTP_IS_LWS(v.ptr[v.len - 1])))
+                               break;
+               }
+
+               if (!htx_add_trailer(htx, list[idx].n, v))
                        goto fail;
        }
 

diff --git a/src/mux_h2.c b/src/mux_h2.c
index 0370d6a13ce6337fca963c14e038667e30d32e63..68ad3eb3d79880390d6b82427726c9bd20650c4a 100644 (file)
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -19,6 +19,7 @@
 #include <haproxy/hpack-dec.h>
 #include <haproxy/hpack-enc.h>
 #include <haproxy/hpack-tbl.h>
+#include <haproxy/http.h>
 #include <haproxy/http_htx.h>
 #include <haproxy/htx.h>
 #include <haproxy/istbuf.h>
@@ -1209,11 +1210,20 @@ static inline int h2_encode_header(struct buffer *buf, const struct ist hn, cons
                                   uint64_t mask, const struct ist trc_loc, const char *func,
                                   const struct h2c *h2c, const struct h2s *h2s)
 {
+       struct ist v;
        int ret;
 
-       ret = hpack_encode_header(buf, hn, hv);
+       /* trim leading/trailing LWS as per RC9113#8.2.1 */
+       for (v = hv; v.len; v.len--) {
+               if (unlikely(HTTP_IS_LWS(*v.ptr)))
+                       v.ptr++;
+               else if (!unlikely(HTTP_IS_LWS(v.ptr[v.len - 1])))
+                       break;
+       }
+
+       ret = hpack_encode_header(buf, hn, v);
        if (ret)
-               h2_trace_header(hn, hv, mask, trc_loc, func, h2c, h2s);
+               h2_trace_header(hn, v, mask, trc_loc, func, h2c, h2s);
 
        return ret;
 }