From cdab051dec6a09c8d07e9d8b52dbc8aae0f9400b Mon Sep 17 00:00:00 2001 From: Ryan O'Hara Date: Wed, 17 Jan 2024 22:59:37 -0600 Subject: [PATCH] Reject any empty content-length header value Resolves: RHEL-7736 --- ...p-reject-empty-content-length-header.patch | 275 ++++++++++++++++++ haproxy.spec | 8 +- 2 files changed, 282 insertions(+), 1 deletion(-) create mode 100644 RHEL-7736_http-reject-empty-content-length-header.patch diff --git a/RHEL-7736_http-reject-empty-content-length-header.patch b/RHEL-7736_http-reject-empty-content-length-header.patch new file mode 100644 index 0000000..e30c9f3 --- /dev/null +++ b/RHEL-7736_http-reject-empty-content-length-header.patch @@ -0,0 +1,275 @@ +From ba9afd2774c03e434165475b537d0462801f49bb Mon Sep 17 00:00:00 2001 +From: Willy Tarreau +Date: Wed, 9 Aug 2023 08:32:48 +0200 +Subject: [PATCH] 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). + +(cherry picked from commit 6492f1f29d738457ea9f382aca54537f35f9d856) +Signed-off-by: Amaury Denoyelle +(cherry picked from commit a32f99f6f991d123ea3e307bf8aa63220836d365) +Signed-off-by: Amaury Denoyelle +(cherry picked from commit 65921ee12d88e9fb1fa9f6cd8198fd64b3a3f37f) +Signed-off-by: Amaury Denoyelle +(cherry picked from commit d17c50010d591d1c070e1cb0567a06032d8869e9) +[wt: applied to h2_parse_cont_len_header() in src/h2.c instead] +Signed-off-by: Willy Tarreau +--- + reg-tests/http-messaging/h1_to_h1.vtc | 26 ++++++++++++ + reg-tests/http-messaging/h2_to_h1.vtc | 60 +++++++++++++++++++++++++++ + src/h1.c | 20 +++++++-- + src/h2.c | 20 +++++++-- + 4 files changed, 120 insertions(+), 6 deletions(-) + +diff --git a/reg-tests/http-messaging/h1_to_h1.vtc b/reg-tests/http-messaging/h1_to_h1.vtc +index c7d00858e..603c03210 100644 +--- a/reg-tests/http-messaging/h1_to_h1.vtc ++++ b/reg-tests/http-messaging/h1_to_h1.vtc +@@ -275,3 +275,29 @@ client c3h1 -connect ${h1_feh1_sock} { + # 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 0d2b1e5f2..ec7a7c123 100644 +--- a/reg-tests/http-messaging/h2_to_h1.vtc ++++ b/reg-tests/http-messaging/h2_to_h1.vtc +@@ -10,6 +10,8 @@ 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 +@@ -31,6 +33,12 @@ server s1 { + + 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 { +@@ -121,6 +129,32 @@ client c1h2 -connect ${h1_feh2_sock} { + 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 +@@ -263,4 +297,30 @@ client c3h2 -connect ${h1_feh2_sock} { + 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 73de48be0..eeda311b7 100644 +--- a/src/h1.c ++++ b/src/h1.c +@@ -34,13 +34,20 @@ int h1_parse_cont_len_header(struct h1m *h1m, struct ist *value) + 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++) { +@@ -79,6 +86,13 @@ int h1_parse_cont_len_header(struct h1m *h1m, struct ist *value) + 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 +diff --git a/src/h2.c b/src/h2.c +index dd1f7d9b6..e1554642e 100644 +--- a/src/h2.c ++++ b/src/h2.c +@@ -80,13 +80,20 @@ int h2_parse_cont_len_header(unsigned int *msgf, struct ist *value, unsigned lon + int not_first = !!(*msgf & H2_MSGF_BODY_CL); + 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++) { +@@ -125,6 +132,13 @@ int h2_parse_cont_len_header(unsigned int *msgf, struct ist *value, unsigned lon + *msgf |= H2_MSGF_BODY_CL; + *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 +-- +2.43.0 + diff --git a/haproxy.spec b/haproxy.spec index fca9d6b..dbb7e40 100644 --- a/haproxy.spec +++ b/haproxy.spec @@ -8,7 +8,7 @@ Name: haproxy Version: 2.4.22 -Release: 1%{?dist} +Release: 2%{?dist} Summary: HAProxy reverse proxy for high availability environments License: GPLv2+ @@ -22,6 +22,8 @@ Source4: %{name}.sysconfig Source5: %{name}.sysusers Source6: halog.1 +Patch0: RHEL-7736_http-reject-empty-content-length-header.patch + BuildRequires: gcc BuildRequires: lua-devel BuildRequires: pcre2-devel @@ -50,6 +52,7 @@ availability environments. Indeed, it can: %prep %setup -q +%patch 0 -p1 %build regparm_opts= @@ -132,6 +135,9 @@ done %{_sysusersdir}/%{name}.conf %changelog +* Wed Jan 17 2024 Ryan O'Hara - 2.4.22-2 +- Reject any empty content-length header value (CVE-2023-40225, RHEL-7736) + * Tue Jun 06 2023 Ryan O'Hara - 2.4.22-1 - Update to 2.4.22 (#2196530)