CVE-2021-40346: Integer Overflow or Wraparound (CWE-190)
value: An integer overflow exists in HAProxy 2.0 through 2.5 in htx_add_header that can be exploited to perform an HTTP request smuggling attack, allowing an attacker to bypass all configured http-request HAProxy ACLs and possibly other ACLs.
CWE description: The product performs a calculation that can
         produce an integer overflow or wraparound when the logic
         assumes that the resulting value will always be larger than
         the original value. This occurs when an integer value is
         incremented to a value that is too large to store in the
         associated representation. When this occurs, the value may
         become a very small or negative number.
CVSS v3 base score: 7.5
Published: 2021-09-08T17:15Z
Repository: https://github.com/haproxy/haproxy
Language: C
Files changed: include/haproxy/htx.h
Commit date: 2021-09-03 16:15:29 +0200
Commit message: BUG/MAJOR: htx: fix missing header name length check in htx_add_header/trailer

Ori Hollander of JFrog Security reported that htx_add_header() and
htx_add_trailer() were missing a length check on the header name. While
this does not allow to overwrite any memory area, it results in bits of
the header name length to slip into the header value length and may
result in forging certain header names on the input. The sad thing here
is that a FIXME comment was present suggesting to add the required length
checks :-(

The injected headers are visible to the HTTP internals and to the config
rules, so haproxy will generally stay synchronized with the server. But
there is one exception which is the content-length header field, because
it is already deduplicated on the input, but before being indexed. As
such, injecting a content-length header after the deduplication stage
may be abused to present a different, shorter one on the other side and
help build a request smuggling attack, or even maybe a response splitting
attack. CVE-2021-40346 was assigned to this problem.

As a mitigation measure, it is sufficient to verify that no more than
one such header is present in any message, which is normally the case
thanks to the duplicate checks:

   http-request  deny if { req.hdr_cnt(content-length) gt 1 }
   http-response deny if { res.hdr_cnt(content-length) gt 1 }

This must be backported to all HTX-enabled versions, hence as far as 2.0.
In 2.3 and earlier, the functions are in src/htx.c instead.

Many thanks to Ori for his work and his responsible report!

Diff: diff --git a/include/haproxy/htx.h b/include/haproxy/htx.h
index b6d95274e..a6535237b 100644
--- a/include/haproxy/htx.h
+++ b/include/haproxy/htx.h
@@ -464,11 +464,13 @@ static inline struct htx_sl *htx_add_stline(struct htx *htx, enum htx_blk_type t
 static inline struct htx_blk *htx_add_header(struct htx *htx, const struct ist name,
 					     const struct ist value)
 {
 	struct htx_blk *blk;
 
-	/* FIXME: check name.len (< 256B) and value.len (< 1MB) */
+	if (name.len > 255 || value.len > 1048575)
+		return NULL;
+
 	blk = htx_add_blk(htx, HTX_BLK_HDR, name.len + value.len);
 	if (!blk)
 		return NULL;
 
 	blk->info += (value.len << 8) + name.len;
@@ -483,11 +485,13 @@ static inline struct htx_blk *htx_add_header(struct htx *htx, const struct ist n
 static inline struct htx_blk *htx_add_trailer(struct htx *htx, const struct ist name,
 					      const struct ist value)
 {
 	struct htx_blk *blk;
 
-	/* FIXME: check name.len (< 256B) and value.len (< 1MB) */
+	if (name.len > 255 || value.len > 1048575)
+		return NULL;
+
 	blk = htx_add_blk(htx, HTX_BLK_TLR, name.len + value.len);
 	if (!blk)
 		return NULL;
 
 	blk->info += (value.len << 8) + name.len;

Vulnerable code: 	/* FIXME: check name.len (< 256B) and value.len (< 1MB) */
	/* FIXME: check name.len (< 256B) and value.len (< 1MB) */
Fixed code: 	if (name.len > 255 || value.len > 1048575)
		return NULL;

	if (name.len > 255 || value.len > 1048575)
		return NULL;