Hi,
as suggested by Willy on GitHub, I'm submitting my patch for
https://github.com/haproxy/haproxy/issues/1822.
This is my first contribution, so I'm tagging it as RFC for now ;)
I'm not entirely happy with using goto (suggested by Tim) to avoid
hitting htx_get_next_blk call at the end of the loop, but I'm not
familiar with HAproxy coding standards. I think it would be nicer to:
1. Introduce flag variable preserve_blk
2. Reset it to 1 at the beginning of the while (blk) loop
3. Replace htx_remove_blk + continue with preserve_blk = 0 + break
4. At the end of the loop, call htx_get_next_blk if preserve_blk is set
or call htx_remove_blk otherwise
I have not included severity of the patch, because on GitHub issue is
still marked as `status: needs-triage`. I think MEDIUM would be
appropriate.
By the way, while writing VTest to cover this bug, I spotted something
"suspicious" about reg tests for FCGI backends - my-fcgi-app FCGI app is
defined, but it is not used anywhere? be-fcgi* backends look exactly
like be-http* to me.
Best regards
Mateusz Małek
>From 17dcd8147fb7f48b3fcce5a7a51ff921f4f69848 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mateusz=20Ma=C5=82ek?= <hapr...@sl.damisa.net>
Date: Wed, 17 Aug 2022 00:57:10 +0200
Subject: [PATCH] BUG: 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 | 47 +++++++++++++++++++
src/http_ana.c | 7 ++-
2 files changed, 53 insertions(+), 1 deletion(-)
diff --git a/reg-tests/http-rules/restrict_req_hdr_names.vtc
b/reg-tests/http-rules/restrict_req_hdr_names.vtc
index 071b9bded..eed90eab2 100644
--- a/reg-tests/http-rules/restrict_req_hdr_names.vtc
+++ b/reg-tests/http-rules/restrict_req_hdr_names.vtc
@@ -35,6 +35,26 @@ server s5 {
txresp
} -start
+server s6 {
+ rxreq
+ expect req.http.x_my_hdr_with_lots_of_underscores == <undef>
+ txresp
+} -start
+
+server s7 {
+ rxreq
+ expect req.http.x_my_hdr-1 == <undef>
+ expect req.http.x-my-hdr-2 == on
+ txresp
+} -start
+
+server s8 {
+ rxreq
+ expect req.http.x-my_hdr-1 == <undef>
+ expect req.http.x-my_hdr-2 == <undef>
+ txresp
+} -start
+
haproxy h1 -conf {
defaults
mode http
@@ -50,6 +70,9 @@ 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 }
backend be-http1
server s1 ${s1_addr}:${s1_port}
@@ -72,6 +95,18 @@ 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}
+
defaults
mode http
timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
@@ -114,6 +149,18 @@ 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
} -run
client c2 -connect ${h1_fe2_sock} {
diff --git a/src/http_ana.c b/src/http_ana.c
index 4b74dd60d..a2929cef5 100644
--- a/src/http_ana.c
+++ b/src/http_ana.c
@@ -2641,6 +2641,7 @@ static enum rule_result
http_req_restrict_header_names(struct stream *s, struct
blk = htx_get_first_blk(htx);
while (blk) {
+ next_iteration:
enum htx_blk_type type = htx_get_blk_type(blk);
if (type == HTX_BLK_HDR) {
@@ -2653,7 +2654,11 @@ static enum rule_result
http_req_restrict_header_names(struct stream *s, struct
if (px->options2 &
PR_O2_RSTRICT_REQ_HDR_NAMES_BLK)
goto block;
blk = htx_remove_blk(htx, blk);
- continue;
+ /* We must not call htx_get_next_blk at
the end of the loop,
+ * as htx_remove_blk advances us to the
next block by itself -
+ * hence, we immediately go back to the
beginning of the loop.
+ */
+ goto next_iteration;
}
}
}
--
2.37.1
