CVE-2023-40225: Inconsistent Interpretation of HTTP Requests ('HTTP Request/Response Smuggling') (CWE-444)
value: HAProxy through 2.0.32, 2.1.x and 2.2.x through 2.2.30, 2.3.x and 2.4.x through 2.4.23, 2.5.x and 2.6.x before 2.6.15, 2.7.x before 2.7.10, and 2.8.x before 2.8.2 forwards empty Content-Length headers, violating RFC 9110 section 8.6. In uncommon cases, an HTTP/1 server behind HAProxy may interpret the payload as an extra request.
CWE description: The product acts as an intermediary HTTP agent
         (such as a proxy or firewall) in the data flow between two
         entities such as a client and server, but it does not
         interpret malformed HTTP requests or responses in ways that
         are consistent with how the messages will be processed by
         those entities that are at the ultimate destination.
CVSS v3 base score: 7.2
Published: 2023-08-10T21:15Z
Repository: https://github.com/haproxy/haproxy
Language: Other
Files changed: reg-tests/http-messaging/h1_to_h1.vtc, reg-tests/http-messaging/h2_to_h1.vtc, src/h1.c, src/http.c
Commit date: 2023-08-09 09:27:38 +0200
Commit message: BUG/MAJOR: http: reject any empty content-length header value

The content-length header parser has its dedicated function, in order
to take extreme care about invalid, unparsable, or conflicting values.
But there's a corner case in it, by which it stops comparing values
when reaching the end of the header. This has for a side effect that
an empty value or a value that ends with a comma does not deserve
further analysis, and it acts as if the header was absent.

While this is not necessarily a problem for the value ending with a
comma as it will be cause a header folding and will disappear, it is a
problem for the first isolated empty header because this one will not
be recontructed when next ones are seen, and will be passed as-is to the
backend server. A vulnerable HTTP/1 server hosted behind haproxy that
would just use this first value as "0" and ignore the valid one would
then not be protected by haproxy and could be attacked this way, taking
the payload for an extra request.

In field the risk depends on the server. Most commonly used servers
already have safe content-length parsers, but users relying on haproxy
to protect a known-vulnerable server might be at risk (and the risk of
a bug even in a reputable server should never be dismissed).

A configuration-based work-around consists in adding the following rule
in the frontend, to explicitly reject requests featuring an empty
content-length header that would have not be folded into an existing
one:

    http-request deny if { hdr_len(content-length) 0 }

The real fix consists in adjusting the parser so that it always expects a
value at the beginning of the header or after a comma. It will now reject
requests and responses having empty values anywhere in the C-L header.

This needs to be backported to all supported versions. Note that the
modification was made to functions h1_parse_cont_len_header() and
http_parse_cont_len_header(). Prior to 2.8 the latter was in
h2_parse_cont_len_header(). One day the two should be refused but the
former is also used by Lua.

The HTTP messaging reg-tests were completed to test these cases.

Thanks to Ben Kallus of Dartmouth College and Narf Industries for
reporting this! (this is in GH #2237).

Diff: diff --git a/reg-tests/http-messaging/h1_to_h1.vtc b/reg-tests/http-messaging/h1_to_h1.vtc
index 0d65366986..67aba14409 100644
--- a/reg-tests/http-messaging/h1_to_h1.vtc
+++ b/reg-tests/http-messaging/h1_to_h1.vtc
@@ -271,5 +271,31 @@ client c3h1 -connect ${h1_feh1_sock} {
 
 	# the connection is expected to be closed and no more response must
 	# arrive here.
 	expect_close
 } -run
+
+client c4h1 -connect ${h1_feh1_sock} {
+	# this request is invalid and advertises an invalid C-L ending with an
+        # empty value, which results in a stream error.
+	txreq \
+	  -req "GET" \
+	  -url "/test31.html" \
+          -hdr "content-length: 0," \
+          -hdr "connection: close"
+	rxresp
+	expect resp.status == 400
+	expect_close
+} -run
+
+client c5h1 -connect ${h1_feh1_sock} {
+	# this request is invalid and advertises an empty C-L, which results
+	# in a stream error.
+	txreq \
+	  -req "GET" \
+	  -url "/test41.html" \
+          -hdr "content-length:" \
+          -hdr "connection: close"
+	rxresp
+	expect resp.status == 400
+	expect_close
+} -run
diff --git a/reg-tests/http-messaging/h2_to_h1.vtc b/reg-tests/http-messaging/h2_to_h1.vtc
index 91d1056d86..637b664866 100644
--- a/reg-tests/http-messaging/h2_to_h1.vtc
+++ b/reg-tests/http-messaging/h2_to_h1.vtc
@@ -7,10 +7,12 @@ feature ignore_unknown_macro
 # synchronize requests between streams
 barrier b1 cond 2 -cyclic
 barrier b2 cond 2 -cyclic
 barrier b3 cond 2 -cyclic
 barrier b4 cond 2 -cyclic
+barrier b5 cond 2 -cyclic
+barrier b6 cond 2 -cyclic
 
 server s1 {
 	rxreq
 	txresp \
 	  -status 200 \
@@ -28,10 +30,16 @@ server s1 {
 	  -status 200 \
 	  -body "response 3"
 
 	barrier b4 sync
 	# the next request is never received
+
+	barrier b5 sync
+	# the next request is never received
+
+	barrier b6 sync
+	# the next request is never received
 } -repeat 2 -start
 
 haproxy h1 -conf {
     global
         # WT: limit false-positives causing "HTTP header incomplete" due to
@@ -117,10 +125,36 @@ client c1h2 -connect ${h1_feh2_sock} {
 		  -hdr "content-length" "2" \
 		  -nostrend
 		txdata -data "this is sent and ignored"
 		rxrst
 	} -run
+
+	# fifth request is invalid and advertises an invalid C-L ending with an
+        # empty value, which results in a stream error.
+	stream 9 {
+		barrier b5 sync
+		txreq \
+		  -req "GET" \
+		  -scheme "https" \
+		  -url "/test5.html" \
+		  -hdr "content-length" "0," \
+		  -nostrend
+		rxrst
+	} -run
+
+	# sixth request is invalid and advertises an empty C-L, which results
+	# in a stream error.
+	stream 11 {
+		barrier b6 sync
+		txreq \
+		  -req "GET" \
+		  -scheme "https" \
+		  -url "/test6.html" \
+		  -hdr "content-length" "" \
+		  -nostrend
+		rxrst
+	} -run
 } -run
 
 # HEAD requests : don't work well yet
 #client c2h2 -connect ${h1_feh2_sock} {
 #	txpri
@@ -259,6 +293,32 @@ client c3h2 -connect ${h1_feh2_sock} {
 		  -hdr "content-length" "2" \
 		  -nostrend
 		txdata -data "this is sent and ignored"
 		rxrst
 	} -run
+
+	# fifth request is invalid and advertises invalid C-L ending with an
+        # empty value, which results in a stream error.
+	stream 9 {
+		barrier b5 sync
+		txreq \
+		  -req "POST" \
+		  -scheme "https" \
+		  -url "/test25.html" \
+		  -hdr "content-length" "0," \
+		  -nostrend
+		rxrst
+	} -run
+
+	# sixth request is invalid and advertises an empty C-L, which results
+	# in a stream error.
+	stream 11 {
+		barrier b6 sync
+		txreq \
+		  -req "POST" \
+		  -scheme "https" \
+		  -url "/test26.html" \
+		  -hdr "content-length" "" \
+		  -nostrend
+		rxrst
+	} -run
 } -run
diff --git a/src/h1.c b/src/h1.c
index 4e224f8954..92ec96bfe1 100644
--- a/src/h1.c
+++ b/src/h1.c
@@ -32,17 +32,24 @@ int h1_parse_cont_len_header(struct h1m *h1m, struct ist *value)
 	char *e, *n;
 	long long cl;
 	int not_first = !!(h1m->flags & H1_MF_CLEN);
 	struct ist word;
 
-	word.ptr = value->ptr - 1; // -1 for next loop's pre-increment
+	word.ptr = value->ptr;
 	e = value->ptr + value->len;
 
-	while (++word.ptr < e) {
+	while (1) {
+		if (word.ptr >= e) {
+			/* empty header or empty value */
+			goto fail;
+		}
+
 		/* skip leading delimiter and blanks */
-		if (unlikely(HTTP_IS_LWS(*word.ptr)))
+		if (unlikely(HTTP_IS_LWS(*word.ptr))) {
+			word.ptr++;
 			continue;
+		}
 
 		/* digits only now */
 		for (cl = 0, n = word.ptr; n < e; n++) {
 			unsigned int c = *n - '0';
 			if (unlikely(c > 9)) {
@@ -77,10 +84,17 @@ int h1_parse_cont_len_header(struct h1m *h1m, struct ist *value)
 
 		/* OK, store this result as the one to be indexed */
 		h1m->flags |= H1_MF_CLEN;
 		h1m->curr_len = h1m->body_len = cl;
 		*value = word;
+
+		/* Now either n==e and we're done, or n points to the comma,
+		 * and we skip it and continue.
+		 */
+		if (n++ == e)
+			break;
+
 		word.ptr = n;
 	}
 	/* here we've reached the end with a single value or a series of
 	 * identical values, all matching previous series if any. The last
 	 * parsed value was sent back into <value>. We just have to decide
diff --git a/src/http.c b/src/http.c
index 18f111c955..85cd2f2e1d 100644
--- a/src/http.c
+++ b/src/http.c
@@ -705,17 +705,24 @@ int http_parse_cont_len_header(struct ist *value, unsigned long long *body_len,
 	char *e, *n;
 	unsigned long long cl;
 	struct ist word;
 	int check_prev = not_first;
 
-	word.ptr = value->ptr - 1; // -1 for next loop's pre-increment
+	word.ptr = value->ptr;
 	e = value->ptr + value->len;
 
-	while (++word.ptr < e) {
+	while (1) {
+		if (word.ptr >= e) {
+			/* empty header or empty value */
+			goto fail;
+		}
+
 		/* skip leading delimiter and blanks */
-		if (unlikely(HTTP_IS_LWS(*word.ptr)))
+		if (unlikely(HTTP_IS_LWS(*word.ptr))) {
+			word.ptr++;
 			continue;
+		}
 
 		/* digits only now */
 		for (cl = 0, n = word.ptr; n < e; n++) {
 			unsigned int c = *n - '0';
 			if (unlikely(c > 9)) {
@@ -749,10 +756,17 @@ int http_parse_cont_len_header(struct ist *value, unsigned long long *body_len,
 			goto fail;
 
 		/* OK, store this result as the one to be indexed */
 		*body_len = cl;
 		*value = word;
+
+		/* Now either n==e and we're done, or n points to the comma,
+		 * and we skip it and continue.
+		 */
+		if (n++ == e)
+			break;
+
 		word.ptr = n;
 		check_prev = 1;
 	}
 
 	/* here we've reached the end with a single value or a series of

Vulnerable code: 	word.ptr = value->ptr - 1; // -1 for next loop's pre-increment
	while (++word.ptr < e) {
		if (unlikely(HTTP_IS_LWS(*word.ptr)))
	word.ptr = value->ptr - 1; // -1 for next loop's pre-increment
	while (++word.ptr < e) {
		if (unlikely(HTTP_IS_LWS(*word.ptr)))
Fixed code: 
client c4h1 -connect ${h1_feh1_sock} {
	# this request is invalid and advertises an invalid C-L ending with an
        # empty value, which results in a stream error.
	txreq \
	  -req "GET" \
	  -url "/test31.html" \
          -hdr "content-length: 0," \
          -hdr "connection: close"
	rxresp
	expect resp.status == 400
	expect_close
} -run

client c5h1 -connect ${h1_feh1_sock} {
	# this request is invalid and advertises an empty C-L, which results
	# in a stream error.
	txreq \
	  -req "GET" \
	  -url "/test41.html" \
          -hdr "content-length:" \
          -hdr "connection: close"
	rxresp
	expect resp.status == 400
	expect_close
} -run
barrier b5 cond 2 -cyclic
barrier b6 cond 2 -cyclic

	barrier b5 sync
	# the next request is never received

	barrier b6 sync
	# the next request is never received

	# fifth request is invalid and advertises an invalid C-L ending with an
        # empty value, which results in a stream error.
	stream 9 {
		barrier b5 sync
		txreq \
		  -req "GET" \
		  -scheme "https" \
		  -url "/test5.html" \
		  -hdr "content-length" "0," \
		  -nostrend
		rxrst
	} -run

	# sixth request is invalid and advertises an empty C-L, which results
	# in a stream error.
	stream 11 {
		barrier b6 sync
		txreq \
		  -req "GET" \
		  -scheme "https" \
		  -url "/test6.html" \
		  -hdr "content-length" "" \
		  -nostrend
		rxrst
	} -run

	# fifth request is invalid and advertises invalid C-L ending with an
        # empty value, which results in a stream error.
	stream 9 {
		barrier b5 sync
		txreq \
		  -req "POST" \
		  -scheme "https" \
		  -url "/test25.html" \
		  -hdr "content-length" "0," \
		  -nostrend
		rxrst
	} -run

	# sixth request is invalid and advertises an empty C-L, which results
	# in a stream error.
	stream 11 {
		barrier b6 sync
		txreq \
		  -req "POST" \
		  -scheme "https" \
		  -url "/test26.html" \
		  -hdr "content-length" "" \
		  -nostrend
		rxrst
	} -run
	word.ptr = value->ptr;
	while (1) {
		if (word.ptr >= e) {
			/* empty header or empty value */
			goto fail;
		}

		if (unlikely(HTTP_IS_LWS(*word.ptr))) {
			word.ptr++;
		}

		/* Now either n==e and we're done, or n points to the comma,
		 * and we skip it and continue.
		 */
		if (n++ == e)
			break;

	word.ptr = value->ptr;
	while (1) {
		if (word.ptr >= e) {
			/* empty header or empty value */
			goto fail;
		}

		if (unlikely(HTTP_IS_LWS(*word.ptr))) {
			word.ptr++;
		}

		/* Now either n==e and we're done, or n points to the comma,
		 * and we skip it and continue.
		 */
		if (n++ == e)
			break;