BUG/MAJOR: htx: Check the header/trailer length limits when one is updated

The name and the value lengths of a header or a trailer are respectively
encoded on 8 and 20 bits in the block descriptor. So the name must not
exceed 255 bytes and the value must not exceed 1048575 bytes. This is
properly tested when a header or a trailer is added, since commit 3b69886f7
("BUG/MAJOR: htx: fix missing header name length check in
htx_add_header/trailer"). But no test was performed when one is updated.

htx_replace_blk_value(), used to update a part of a header or a trailer
value, may grow it above the limit. And htx_replace_header(), used to
replace a full header, does not check its arguments at all. In both cases,
the length silently overflows on the block type, changing it. For instance,
a HDR block becomes an EOH block while its value length wraps to a small
value.

Only the value length limit is reachable from the configuration, via a
"replace-header" or a "replace-value" rule, and only if "tune.bufsize" was
set above 1MB, because the block must first fit in the buffer. In that case,
a client sending a large enough header is enough to hit the issue.

So, perform the same tests than those performed when a header or a trailer
is added, instead of corrupting the message. In addition, a BUG_ON() was
added in htx_change_blk_value_len() and htx_set_blk_value_len() to catch any
other caller missing the test.

Reported by: Adam Crawford

This patch must be backported to all stable versions.

diff --git a/include/haproxy/htx.h b/include/haproxy/htx.h
index 70f5f1f513d11e749a8ddc0806b92b5f73f074b7..55b41838f5034c27468d6f696a785a9b7d752bd7 100644 (file)
--- a/include/haproxy/htx.h
+++ b/include/haproxy/htx.h
@@ -360,6 +360,7 @@ static inline void htx_change_blk_value_len(struct htx *htx, struct htx_blk *blk
        switch (type) {
                case HTX_BLK_HDR:
                case HTX_BLK_TLR:
+                       BUG_ON(newlen > 1048575);
                        oldlen = (blk->info >> 8) & 0xfffff;
                        blk->info = (type << 28) + (newlen << 8) + (blk->info & 0xff);
                        break;
@@ -392,6 +393,7 @@ static inline void htx_set_blk_value_len(struct htx_blk *blk, uint32_t vlen)
        switch (type) {
                case HTX_BLK_HDR:
                case HTX_BLK_TLR:
+                       BUG_ON(vlen > 1048575);
                        blk->info = (type << 28) + (vlen << 8) + (blk->info & 0xff);
                        break;
                case HTX_BLK_REQ_SL:

diff --git a/reg-tests/http-rules/replace_header_limits.vtc b/reg-tests/http-rules/replace_header_limits.vtc
new file mode 100644 (file)
index 0000000..5fd5a6c
--- /dev/null
+++ b/reg-tests/http-rules/replace_header_limits.vtc
@@ -0,0 +1,77 @@
+varnishtest "replace-header/replace-value length limits"
+
+# In an HTX message, the value of a header or a trailer is limited to 1048575
+# bytes, because of the way its length is encoded in the block descriptor. This
+# limit is tested when a header is added. This test checks it is also enforced
+# when a header is updated. In this case, the rewrite must fail instead of
+# silently corrupting the message.
+#
+# A "tune.bufsize" greater than 1MB is required to be able to exceed this limit.
+
+feature ignore_unknown_macro
+
+server s1 {
+    rxreq
+    expect req.url == /ok
+    expect req.http.x-test-len == 524288
+    txresp
+} -start
+
+haproxy h1 -conf {
+    global
+        tune.bufsize 2097152
+    .if feature(THREAD)
+        thread-groups 1
+    .endif
+
+    defaults
+        mode http
+        timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
+        timeout client  "${HAPROXY_TEST_TIMEOUT-5s}"
+        timeout server  "${HAPROXY_TEST_TIMEOUT-5s}"
+
+    frontend fe
+        bind "fd@${fe}"
+
+        # Each rule below doubles the length of the x-test value. The client
+        # sends a 256 bytes long value, so these 11 rules lead to a 524288
+        # bytes long value, just under the limit.
+        http-request replace-value x-test (.*) \1\1
+        http-request replace-value x-test (.*) \1\1
+        http-request replace-value x-test (.*) \1\1
+        http-request replace-value x-test (.*) \1\1
+        http-request replace-value x-test (.*) \1\1
+        http-request replace-value x-test (.*) \1\1
+        http-request replace-value x-test (.*) \1\1
+        http-request replace-value x-test (.*) \1\1
+        http-request replace-value x-test (.*) \1\1
+        http-request replace-value x-test (.*) \1\1
+        http-request replace-value x-test (.*) \1\1
+
+        # This last one would lead to a 1048576 bytes long value, just above
+        # the limit. The rewrite must fail.
+        http-request replace-value x-test (.*) \1\1 if { path /too-big }
+
+        # Don't forward the huge header, only its length
+        http-request set-header x-test-len "%[req.hdr(x-test),length]"
+        http-request del-header x-test
+
+        default_backend be
+
+    backend be
+        server s1 ${s1_addr}:${s1_port}
+} -start
+
+# 11 doublings: the value remains under the limit, the rewrites succeed
+client c1 -connect ${h1_fe_sock} {
+    txreq -url /ok -hdr "x-test: 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
+    rxresp
+    expect resp.status == 200
+} -run
+
+# 12 doublings: the value would exceed the limit, the last rewrite must fail
+client c2 -connect ${h1_fe_sock} {
+    txreq -url /too-big -hdr "x-test: 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
+    rxresp
+    expect resp.status == 500
+} -run

diff --git a/src/htx.c b/src/htx.c
index 5dcd433ad41c76d7a4954085807d0fef50b5332e..100abab325c099eadf8e729b596560ed14b6627e 100644 (file)
--- a/src/htx.c
+++ b/src/htx.c
@@ -645,15 +645,24 @@ struct htx_blk *htx_add_data_atonce(struct htx *htx, struct ist data)
 struct htx_blk *htx_replace_blk_value(struct htx *htx, struct htx_blk *blk,
                                      const struct ist old, const struct ist new)
 {
+       enum htx_blk_type type;
        struct ist n, v;
        int32_t delta;
        int ret;
 
+       type = htx_get_blk_type(blk);
        n  = htx_get_blk_name(htx, blk);
        v  = htx_get_blk_value(htx, blk);
        delta = new.len - old.len;
 
-       if (unlikely(htx_get_blk_type(blk) < HTX_BLK_EOH && htx_hdrs_too_big(htx, delta)))
+       /* The value length of a header or a trailer is encoded on 20 bits. It
+        * must not overflow on the other fields of the block descriptor.
+        */
+       if (unlikely((type == HTX_BLK_HDR || type == HTX_BLK_TLR) &&
+                    v.len + delta > 1048575))
+               return NULL;
+
+       if (unlikely(type < HTX_BLK_EOH && htx_hdrs_too_big(htx, delta)))
                return NULL;
 
        ret = htx_prepare_blk_expansion(htx, blk, delta);
@@ -742,7 +751,7 @@ struct htx_blk *htx_replace_blk_value(struct htx *htx, struct htx_blk *blk,
                free_trash_chunk(chunk);
        }
 
-       if (htx_get_blk_type(blk) < HTX_BLK_EOH)
+       if (type < HTX_BLK_EOH)
                htx->hdrs_data += delta;
 
        return blk;
@@ -920,6 +929,9 @@ struct htx_blk *htx_replace_header(struct htx *htx, struct htx_blk *blk,
        if (type != HTX_BLK_HDR)
                return NULL;
 
+       if (name.len > 255 || value.len > 1048575)
+               return NULL;
+
        delta = name.len + value.len - htx_get_blksz(blk);
        if (unlikely(htx_hdrs_too_big(htx, delta)))
                return NULL;