Re: [PATCH] BUG/MEDIUM: http-ana: fix crash or wrong header deletion by http-restrict-req-hdr-names
On Wed, Aug 17, 2022 at 01:05:55PM +, Mateusz Malek wrote: > On 17.08.2022 14:59, Mateusz Malek wrote: > > Sure - here you go: > > Sorry, wrong file. Patch in previous email had a typo (double /req9 > call instead of /req9 and /req10) in VTest test case. Perfect, thank you, now applied! I've updated the issue to remember the need to backport as far as 2.2 as you mentioned. I've just backported it to 2.6 already for the upcoming release and will handle other versions soon. Thanks again! Willy
Re: [PATCH] BUG/MEDIUM: http-ana: fix crash or wrong header deletion by http-restrict-req-hdr-names
On 17.08.2022 14:59, Mateusz Małek wrote:
> Sure - here you go:
Sorry, wrong file. Patch in previous email had a typo (double /req9
call instead of /req9 and /req10) in VTest test case.
Corrected patch below:
>From ce282735335e25ea90a88155e18d4adcb2b67e21 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mateusz=20Ma=C5=82ek?=
Date: Wed, 17 Aug 2022 14:22:09 +0200
Subject: [PATCH] BUG/MEDIUM: http-ana: fix crash or wrong header deletion by
http-restrict-req-hdr-names
When using `option http-restrict-req-hdr-names delete`, HAproxy may
crash or delete wrong header after receiving request containing multiple
forbidden characters in single header name; exact behavior depends on
number of request headers, number of forbidden characters and position
of header containing them.
This patch fixes GitHub issue #1822.
Must be backported as far as 2.2 (buggy feature got included in 2.2.25,
2.4.18 and 2.5.8).
---
.../http-rules/restrict_req_hdr_names.vtc | 62 +++
src/http_ana.c| 18 +++---
2 files changed, 73 insertions(+), 7 deletions(-)
diff --git a/reg-tests/http-rules/restrict_req_hdr_names.vtc
b/reg-tests/http-rules/restrict_req_hdr_names.vtc
index 071b9bded..4b26e33c6 100644
--- a/reg-tests/http-rules/restrict_req_hdr_names.vtc
+++ b/reg-tests/http-rules/restrict_req_hdr_names.vtc
@@ -35,6 +35,32 @@ server s5 {
txresp
} -start
+server s6 {
+rxreq
+expect req.http.x_my_hdr_with_lots_of_underscores ==
+txresp
+} -start
+
+server s7 {
+rxreq
+expect req.http.x_my_hdr-1 ==
+expect req.http.x-my-hdr-2 == on
+txresp
+} -start
+
+server s8 {
+rxreq
+expect req.http.x-my_hdr-1 ==
+expect req.http.x-my_hdr-2 ==
+txresp
+} -start
+
+server s9 {
+rxreq
+expect req.http.x-my-hdr-with-trailing-underscore_ ==
+txresp
+} -start
+
haproxy h1 -conf {
defaults
mode http
@@ -50,6 +76,10 @@ haproxy h1 -conf {
use_backend be-fcgi1 if { path /req4 }
use_backend be-fcgi2 if { path /req5 }
use_backend be-fcgi3 if { path /req6 }
+use_backend be-http4 if { path /req7 }
+use_backend be-http5 if { path /req8 }
+use_backend be-http6 if { path /req9 }
+use_backend be-http7 if { path /req10 }
backend be-http1
server s1 ${s1_addr}:${s1_port}
@@ -72,6 +102,22 @@ haproxy h1 -conf {
backend be-fcgi3
option http-restrict-req-hdr-names reject
+backend be-http4
+option http-restrict-req-hdr-names delete
+server s6 ${s6_addr}:${s6_port}
+
+backend be-http5
+option http-restrict-req-hdr-names delete
+server s7 ${s7_addr}:${s7_port}
+
+backend be-http6
+option http-restrict-req-hdr-names delete
+server s8 ${s8_addr}:${s8_port}
+
+backend be-http7
+option http-restrict-req-hdr-names delete
+server s9 ${s9_addr}:${s9_port}
+
defaults
mode http
timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
@@ -114,6 +160,22 @@ client c1 -connect ${h1_fe1_sock} {
txreq -req GET -url /req6 -hdr "X-my_hdr: on"
rxresp
expect resp.status == 403
+
+txreq -req GET -url /req7 -hdr "X_my_hdr_with_lots_of_underscores: on"
+rxresp
+expect resp.status == 200
+
+txreq -req GET -url /req8 -hdr "X_my_hdr-1: on" -hdr "X-my-hdr-2: on"
+rxresp
+expect resp.status == 200
+
+txreq -req GET -url /req9 -hdr "X-my_hdr-1: on" -hdr "X-my_hdr-2: on"
+rxresp
+expect resp.status == 200
+
+txreq -req GET -url /req10 -hdr "X-my-hdr-with-trailing-underscore_: on"
+rxresp
+expect resp.status == 200
} -run
client c2 -connect ${h1_fe2_sock} {
diff --git a/src/http_ana.c b/src/http_ana.c
index 4b74dd60d..2b2cfdc56 100644
--- a/src/http_ana.c
+++ b/src/http_ana.c
@@ -2645,17 +2645,21 @@ static enum rule_result
http_req_restrict_header_names(struct stream *s, struct
if (type == HTX_BLK_HDR) {
struct ist n = htx_get_blk_name(htx, blk);
- int i;
+ int i, end = istlen(n);
- for (i = 0; i < istlen(n); i++) {
+ for (i = 0; i < end; i++) {
if (!isalnum((unsigned char)n.ptr[i]) &&
n.ptr[i] != '-') {
- /* Block the request or remove the
header */
- if (px->options2 &
PR_O2_RSTRICT_REQ_HDR_NAMES_BLK)
- goto block;
- blk = htx_remove_blk(htx, blk);
- continue;
+ break;
}
}
+
+ if (i < end) {
+ /* Disallowed
Re: [PATCH] BUG/MEDIUM: http-ana: fix crash or wrong header deletion by http-restrict-req-hdr-names
On 17.08.2022 05:06, Willy Tarreau - w at 1wt.eu wrote:
> However here the goto at the beginning potentially maintains a
> problem: I'm wondering how we can be certain that htx_remove_blk()
> doesn't return a NULL if we remove the last block, in which case
> going back to the beginning of the loop without testing it can
> be a problem.
I guess that at least HTX_BLK_EOH should always follow headers, but
you're right it would be better not to take risk of unexpected NULL.
> Yep I agree with this. I was thinking that an even better option
> would be to remove the branches from the inner loop and use it only
> to spot unusual chars, something like: (...)
I agree with your suggestion, I've updated my patch.
> But if you can provide an updated patch for the loop, I'm fine with
> merging your patch as-is.
Sure - here you go:
>From d1129fd89e96d3d5d4abd06aff2289776883915d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mateusz=20Ma=C5=82ek?=
Date: Wed, 17 Aug 2022 14:22:09 +0200
Subject: [PATCH] BUG/MEDIUM: http-ana: fix crash or wrong header deletion by
http-restrict-req-hdr-names
When using `option http-restrict-req-hdr-names delete`, HAproxy may
crash or delete wrong header after receiving request containing multiple
forbidden characters in single header name; exact behavior depends on
number of request headers, number of forbidden characters and position
of header containing them.
This patch fixes GitHub issue #1822.
Must be backported as far as 2.2 (buggy feature got included in 2.2.25,
2.4.18 and 2.5.8).
---
.../http-rules/restrict_req_hdr_names.vtc | 62 +++
src/http_ana.c| 18 +++---
2 files changed, 73 insertions(+), 7 deletions(-)
diff --git a/reg-tests/http-rules/restrict_req_hdr_names.vtc
b/reg-tests/http-rules/restrict_req_hdr_names.vtc
index 071b9bded..713574f1c 100644
--- a/reg-tests/http-rules/restrict_req_hdr_names.vtc
+++ b/reg-tests/http-rules/restrict_req_hdr_names.vtc
@@ -35,6 +35,32 @@ server s5 {
txresp
} -start
+server s6 {
+rxreq
+expect req.http.x_my_hdr_with_lots_of_underscores ==
+txresp
+} -start
+
+server s7 {
+rxreq
+expect req.http.x_my_hdr-1 ==
+expect req.http.x-my-hdr-2 == on
+txresp
+} -start
+
+server s8 {
+rxreq
+expect req.http.x-my_hdr-1 ==
+expect req.http.x-my_hdr-2 ==
+txresp
+} -start
+
+server s9 {
+rxreq
+expect req.http.x-my-hdr-with-trailing-underscore_ ==
+txresp
+} -start
+
haproxy h1 -conf {
defaults
mode http
@@ -50,6 +76,10 @@ haproxy h1 -conf {
use_backend be-fcgi1 if { path /req4 }
use_backend be-fcgi2 if { path /req5 }
use_backend be-fcgi3 if { path /req6 }
+use_backend be-http4 if { path /req7 }
+use_backend be-http5 if { path /req8 }
+use_backend be-http6 if { path /req9 }
+use_backend be-http7 if { path /req10 }
backend be-http1
server s1 ${s1_addr}:${s1_port}
@@ -72,6 +102,22 @@ haproxy h1 -conf {
backend be-fcgi3
option http-restrict-req-hdr-names reject
+backend be-http4
+option http-restrict-req-hdr-names delete
+server s6 ${s6_addr}:${s6_port}
+
+backend be-http5
+option http-restrict-req-hdr-names delete
+server s7 ${s7_addr}:${s7_port}
+
+backend be-http6
+option http-restrict-req-hdr-names delete
+server s8 ${s8_addr}:${s8_port}
+
+backend be-http7
+option http-restrict-req-hdr-names delete
+server s9 ${s9_addr}:${s9_port}
+
defaults
mode http
timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
@@ -114,6 +160,22 @@ client c1 -connect ${h1_fe1_sock} {
txreq -req GET -url /req6 -hdr "X-my_hdr: on"
rxresp
expect resp.status == 403
+
+txreq -req GET -url /req7 -hdr "X_my_hdr_with_lots_of_underscores: on"
+rxresp
+expect resp.status == 200
+
+txreq -req GET -url /req8 -hdr "X_my_hdr-1: on" -hdr "X-my-hdr-2: on"
+rxresp
+expect resp.status == 200
+
+txreq -req GET -url /req9 -hdr "X-my_hdr-1: on" -hdr "X-my_hdr-2: on"
+rxresp
+expect resp.status == 200
+
+txreq -req GET -url /req9 -hdr "X-my-hdr-with-trailing-underscore_: on"
+rxresp
+expect resp.status == 200
} -run
client c2 -connect ${h1_fe2_sock} {
diff --git a/src/http_ana.c b/src/http_ana.c
index 4b74dd60d..2b2cfdc56 100644
--- a/src/http_ana.c
+++ b/src/http_ana.c
@@ -2645,17 +2645,21 @@ static enum rule_result
http_req_restrict_header_names(struct stream *s, struct
if (type == HTX_BLK_HDR) {
struct ist n = htx_get_blk_name(htx, blk);
- int i;
+ int i, end = istlen(n);
- for (i = 0; i < istlen(n); i++) {
+ for (i = 0; i < end; i++) {
