BUG/MINOR: http-htx: fix the length moved when removing a header value

http_remove_header() removes <len> bytes at the offset <off> of the header
value, <off> being "start - v.ptr" once <start> has been adjusted to also eat
the comma surrounding the removed value. The number of bytes that remain to be
moved down is therefore "v.len - off - len", but the function passes
"v.len - len" to memmove(), i.e. <off> bytes too many.

As a result, as soon as the removed value is not the first one of the
comma-delimited list (<off> != 0), memmove() reads <off> bytes past the end of
the header value and, when <off> is larger than <len>, writes (<off> - <len>)
bytes past the new end of the HTX block payload, silently corrupting whatever
follows it in the HTX buffer (typically the payload of the next blocks). For
instance with "x-test: aaaaaaaaaa,b,c", removing the second value gives
off = 10 and len = 2, hence 10 bytes read and 8 bytes written past the 20-byte
block payload. The resulting header value itself is not affected, which is
probably why this went unnoticed.

All the current callers happen to be safe: they either use <full> = 1 when
looking the header up, in which case the whole value matches and the block is
removed as a whole by the early return, or they only ever remove the first
value of the list ("Expect: 100-continue" in http_ana.c and "Age" in cache.c).
So this is only a latent out-of-bounds write for now, but the function is a
generic helper and the next caller removing a non-first value would corrupt
the message.

Let's simply compute the moved length from the end of the value. This can be
verified with the "http_htx" unit test added in the previous commit
(DEBUG_UNIT=1, then "haproxy -U http_htx").

This bug was introduced with the HTX conversion by commit 47596d378 ("MINOR:
http_htx: Add functions to manipulate HTX messages in http_htx.c") in 1.9-dev7
so it should be backported to all supported versions.

diff --git a/src/http_htx.c b/src/http_htx.c
index 3d77e3aa817df73901b11c97a97a102054890ba6..422fbe83c9642bbfff7fb0f9ff57211c93bf3ab7 100644 (file)
--- a/src/http_htx.c
+++ b/src/http_htx.c
@@ -791,8 +791,11 @@ int http_remove_header(struct htx *htx, struct http_hdr_ctx *ctx)
                start--;
                len++;
        }
-       /* Update the block content and its len */
-       memmove(start, start+len, v.len-len);
+       /* Update the block content and its len. Only the bytes located after the
+        * removed part must be moved, so the offset of <start> inside the value
+        * must be taken into account.
+        */
+       memmove(start, start+len, istend(v) - (start+len));
        htx_change_blk_value_len(htx, blk, v.len-len);
 
        /* Finally update the ctx */