Compare commits

...

2 Commits

Author SHA1 Message Date
Ryan O'Hara d6eea05a77 Add prerequisite patches Related: RHEL-18169 2024-01-25 02:15:49 +00:00
Ryan O'Hara 297c7feff7 Reject special characters in URI path component
Resolves: RHEL-18169
2024-01-23 13:35:12 -06:00
8 changed files with 474 additions and 1 deletions

1
.haproxy.metadata Normal file
View File

@ -0,0 +1 @@
d0654cbab48039d998fca2459ce9251c6dbf2ae8 haproxy-2.4.22.tar.gz

View File

@ -0,0 +1,119 @@
From e5a741f94977840c58775b38f8ed830207f7e4d0 Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w@1wt.eu>
Date: Tue, 8 Aug 2023 16:17:22 +0200
Subject: [PATCH] BUG/MINOR: h1: do not accept '#' as part of the URI component
Seth Manesse and Paul Plasil reported that the "path" sample fetch
function incorrectly accepts '#' as part of the path component. This
can in some cases lead to misrouted requests for rules that would apply
on the suffix:
use_backend static if { path_end .png .jpg .gif .css .js }
Note that this behavior can be selectively configured using
"normalize-uri fragment-encode" and "normalize-uri fragment-strip".
The problem is that while the RFC says that this '#' must never be
emitted, as often it doesn't suggest how servers should handle it. A
diminishing number of servers still do accept it and trim it silently,
while others are rejecting it, as indicated in the conversation below
with other implementers:
https://lists.w3.org/Archives/Public/ietf-http-wg/2023JulSep/0070.html
Looking at logs from publicly exposed servers, such requests appear at
a rate of roughly 1 per million and only come from attacks or poorly
written web crawlers incorrectly following links found on various pages.
Thus it looks like the best solution to this problem is to simply reject
such ambiguous requests by default, and include this in the list of
controls that can be disabled using "option accept-invalid-http-request".
We're already rejecting URIs containing any control char anyway, so we
should also reject '#'.
In the H1 parser for the H1_MSG_RQURI state, there is an accelerated
parser for bytes 0x21..0x7e that has been tightened to 0x24..0x7e (it
should not impact perf since 0x21..0x23 are not supposed to appear in
a URI anyway). This way '#' falls through the fine-grained filter and
we can add the special case for it also conditionned by a check on the
proxy's option "accept-invalid-http-request", with no overhead for the
vast majority of valid URIs. Here this information is available through
h1m->err_pos that's set to -2 when the option is here (so we don't need
to change the API to expose the proxy). Example with a trivial GET
through netcat:
[08/Aug/2023:16:16:52.651] frontend layer1 (#2): invalid request
backend <NONE> (#-1), server <NONE> (#-1), event #0, src 127.0.0.1:50812
buffer starts at 0 (including 0 out), 16361 free,
len 23, wraps at 16336, error at position 7
H1 connection flags 0x00000000, H1 stream flags 0x00000810
H1 msg state MSG_RQURI(4), H1 msg flags 0x00001400
H1 chunk len 0 bytes, H1 body len 0 bytes :
00000 GET /aa#bb HTTP/1.0\r\n
00021 \r\n
This should be progressively backported to all stable versions along with
the following patch:
REGTESTS: http-rules: add accept-invalid-http-request for normalize-uri tests
Similar fixes for h2 and h3 will come in followup patches.
Thanks to Seth Manesse and Paul Plasil for reporting this problem with
detailed explanations.
(cherry picked from commit 2eab6d354322932cfec2ed54de261e4347eca9a6)
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
(cherry picked from commit 9bf75c8e22a8f2537f27c557854a8803087046d0)
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
(cherry picked from commit 9facd01c9ac85fe9bcb331594b80fa08e7406552)
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
(cherry picked from commit 832b672eee54866c7a42a1d46078cc9ae0d544d9)
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
src/h1.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/h1.c b/src/h1.c
index eeda311b7..91d3dc47a 100644
--- a/src/h1.c
+++ b/src/h1.c
@@ -480,13 +480,13 @@ int h1_headers_to_hdr_list(char *start, const char *stop,
case H1_MSG_RQURI:
http_msg_rquri:
#ifdef HA_UNALIGNED_LE
- /* speedup: skip bytes not between 0x21 and 0x7e inclusive */
+ /* speedup: skip bytes not between 0x24 and 0x7e inclusive */
while (ptr <= end - sizeof(int)) {
- int x = *(int *)ptr - 0x21212121;
+ int x = *(int *)ptr - 0x24242424;
if (x & 0x80808080)
break;
- x -= 0x5e5e5e5e;
+ x -= 0x5b5b5b5b;
if (!(x & 0x80808080))
break;
@@ -498,8 +498,15 @@ int h1_headers_to_hdr_list(char *start, const char *stop,
goto http_msg_ood;
}
http_msg_rquri2:
- if (likely((unsigned char)(*ptr - 33) <= 93)) /* 33 to 126 included */
+ if (likely((unsigned char)(*ptr - 33) <= 93)) { /* 33 to 126 included */
+ if (*ptr == '#') {
+ if (h1m->err_pos < -1) /* PR_O2_REQBUG_OK not set */
+ goto invalid_char;
+ if (h1m->err_pos == -1) /* PR_O2_REQBUG_OK set: just log */
+ h1m->err_pos = ptr - start + skip;
+ }
EAT_AND_JUMP_OR_RETURN(ptr, end, http_msg_rquri2, http_msg_ood, state, H1_MSG_RQURI);
+ }
if (likely(HTTP_IS_SPHT(*ptr))) {
sl.rq.u.len = ptr - sl.rq.u.ptr;
--
2.43.0

View File

@ -0,0 +1,76 @@
From f86e994f5fb5851cd6e4f7f6b366e37765014b9f Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w@1wt.eu>
Date: Tue, 8 Aug 2023 15:38:28 +0200
Subject: [PATCH] MINOR: h2: pass accept-invalid-http-request down the request
parser
We're adding a new argument "relaxed" to h2_make_htx_request() so that
we can control its level of acceptance of certain invalid requests at
the proxy level with "option accept-invalid-http-request". The goal
will be to add deactivable checks that are still desirable to have by
default. For now no test is subject to it.
(cherry picked from commit d93a00861d714313faa0395ff9e2acb14b0a2fca)
[ad: backported for following fix : BUG/MINOR: h2: reject more chars
from the :path pseudo header]
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
(cherry picked from commit b6be1a4f858eb6602490c192235114c1a163fef9)
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
(cherry picked from commit 26fa3a285df0748fc79e73e552161268b66fb527)
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
(cherry picked from commit 014945a1508f43e88ac4e89950fa9037e4fb0679)
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
include/haproxy/h2.h | 2 +-
src/h2.c | 6 +++++-
src/mux_h2.c | 3 ++-
3 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/include/haproxy/h2.h b/include/haproxy/h2.h
index 8d2aa9511..4f872b99d 100644
--- a/include/haproxy/h2.h
+++ b/include/haproxy/h2.h
@@ -207,7 +207,7 @@ extern struct h2_frame_definition h2_frame_definition[H2_FT_ENTRIES];
/* various protocol processing functions */
int h2_parse_cont_len_header(unsigned int *msgf, struct ist *value, unsigned long long *body_len);
-int h2_make_htx_request(struct http_hdr *list, struct htx *htx, unsigned int *msgf, unsigned long long *body_len);
+int h2_make_htx_request(struct http_hdr *list, struct htx *htx, unsigned int *msgf, unsigned long long *body_len, int relaxed);
int h2_make_htx_response(struct http_hdr *list, struct htx *htx, unsigned int *msgf, unsigned long long *body_len, char *upgrade_protocol);
int h2_make_htx_trailers(struct http_hdr *list, struct htx *htx);
diff --git a/src/h2.c b/src/h2.c
index e1554642e..94c384111 100644
--- a/src/h2.c
+++ b/src/h2.c
@@ -399,8 +399,12 @@ static struct htx_sl *h2_prepare_htx_reqline(uint32_t fields, struct ist *phdr,
*
* The Cookie header will be reassembled at the end, and for this, the <list>
* will be used to create a linked list, so its contents may be destroyed.
+ *
+ * When <relaxed> is non-nul, some non-dangerous checks will be ignored. This
+ * is in order to satisfy "option accept-invalid-http-request" for
+ * interoperability purposes.
*/
-int h2_make_htx_request(struct http_hdr *list, struct htx *htx, unsigned int *msgf, unsigned long long *body_len)
+int h2_make_htx_request(struct http_hdr *list, struct htx *htx, unsigned int *msgf, unsigned long long *body_len, int relaxed)
{
struct ist phdr_val[H2_PHDR_NUM_ENTRIES];
uint32_t fields; /* bit mask of H2_PHDR_FND_* */
diff --git a/src/mux_h2.c b/src/mux_h2.c
index 0ab86534c..61fd1a4d2 100644
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -4917,7 +4917,8 @@ static int h2c_decode_headers(struct h2c *h2c, struct buffer *rxbuf, uint32_t *f
if (h2c->flags & H2_CF_IS_BACK)
outlen = h2_make_htx_response(list, htx, &msgf, body_len, upgrade_protocol);
else
- outlen = h2_make_htx_request(list, htx, &msgf, body_len);
+ outlen = h2_make_htx_request(list, htx, &msgf, body_len,
+ !!(((const struct session *)h2c->conn->owner)->fe->options2 & PR_O2_REQBUG_OK));
if (outlen < 0 || htx_free_space(htx) < global.tune.maxrewrite) {
/* too large headers? this is a stream error only */
--
2.43.0

View File

@ -0,0 +1,71 @@
From af232e47e6264122bed3681210b054ff38ec8de8 Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w@1wt.eu>
Date: Tue, 8 Aug 2023 15:40:49 +0200
Subject: [PATCH] BUG/MINOR: h2: reject more chars from the :path pseudo header
This is the h2 version of this previous fix:
BUG/MINOR: h1: do not accept '#' as part of the URI component
In addition to the current NUL/CR/LF, this will also reject all other
control chars, the space and '#' from the :path pseudo-header, to avoid
taking the '#' for a part of the path. It's still possible to fall back
to the previous behavior using "option accept-invalid-http-request".
This patch modifies the request parser to change the ":path" pseudo header
validation function with a new one that rejects 0x00-0x1F (control chars),
space and '#'. This way such chars will be dropped early in the chain, and
the search for '#' doesn't incur a second pass over the header's value.
This should be progressively backported to stable versions, along with the
following commits it relies on:
REGTESTS: http-rules: add accept-invalid-http-request for normalize-uri tests
REORG: http: move has_forbidden_char() from h2.c to http.h
MINOR: ist: add new function ist_find_range() to find a character range
MINOR: http: add new function http_path_has_forbidden_char()
MINOR: h2: pass accept-invalid-http-request down the request parser
(cherry picked from commit b3119d4fb4588087e2483a80b01d322683719e29)
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
(cherry picked from commit 462a8600ce9e478573a957e046b446a7dcffd286)
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
(cherry picked from commit 648e59e30723b8fd4e71aab02cb679f6ea7446e7)
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
(cherry picked from commit c8e07f2fd8b5462527f102f7145d6027c0d041da)
[wt: minor ctx adjustments]
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
src/h2.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/h2.c b/src/h2.c
index 94c384111..e190c52b5 100644
--- a/src/h2.c
+++ b/src/h2.c
@@ -440,11 +440,18 @@ int h2_make_htx_request(struct http_hdr *list, struct htx *htx, unsigned int *ms
}
/* RFC7540#10.3: intermediaries forwarding to HTTP/1 must take care of
- * rejecting NUL, CR and LF characters.
+ * rejecting NUL, CR and LF characters. For :path we reject all CTL
+ * chars, spaces, and '#'.
*/
- ctl = ist_find_ctl(list[idx].v);
- if (unlikely(ctl) && has_forbidden_char(list[idx].v, ctl))
- goto fail;
+ if (phdr == H2_PHDR_IDX_PATH && !relaxed) {
+ ctl = ist_find_range(list[idx].v, 0, '#');
+ if (unlikely(ctl) && http_path_has_forbidden_char(list[idx].v, ctl))
+ goto fail;
+ } else {
+ ctl = ist_find_ctl(list[idx].v);
+ if (unlikely(ctl) && has_forbidden_char(list[idx].v, ctl))
+ goto fail;
+ }
if (phdr > 0 && phdr < H2_PHDR_NUM_ENTRIES) {
/* insert a pseudo header by its index (in phdr) and value (in value) */
--
2.43.0

View File

@ -0,0 +1,59 @@
From 0f57ac20b046b70275192651d7b6c978032e6a36 Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w@1wt.eu>
Date: Tue, 8 Aug 2023 15:24:54 +0200
Subject: [PATCH] MINOR: http: add new function http_path_has_forbidden_char()
As its name implies, this function checks if a path component has any
forbidden headers starting at the designated location. The goal is to
seek from the result of a successful ist_find_range() for more precise
chars. Here we're focusing on 0x00-0x1F, 0x20 and 0x23 to make sure
we're not too strict at this point.
(cherry picked from commit 30f58f4217d585efeac3d85cb1b695ba53b7760b)
[ad: backported for following fix : BUG/MINOR: h2: reject more chars
from the :path pseudo header]
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
(cherry picked from commit b491940181a88bb6c69ab2afc24b93a50adfa67c)
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
(cherry picked from commit f7666e5e43ce63e804ebffdf224d92cfd3367282)
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
(cherry picked from commit c699bb17b7e334c9d56e829422e29e5a204615ec)
[wt: adj minor ctx in http.h]
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
include/haproxy/http.h | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/include/haproxy/http.h b/include/haproxy/http.h
index 8a86cb6e9..e8c5b850f 100644
--- a/include/haproxy/http.h
+++ b/include/haproxy/http.h
@@ -134,6 +134,25 @@ static inline enum http_etag_type http_get_etag_type(const struct ist etag)
return ETAG_INVALID;
}
+/* Looks into <ist> for forbidden characters for :path values (0x00..0x1F,
+ * 0x20, 0x23), starting at pointer <start> which must be within <ist>.
+ * Returns non-zero if such a character is found, 0 otherwise. When run on
+ * unlikely header match, it's recommended to first check for the presence
+ * of control chars using ist_find_ctl().
+ */
+static inline int http_path_has_forbidden_char(const struct ist ist, const char *start)
+{
+ do {
+ if ((uint8_t)*start <= 0x23) {
+ if ((uint8_t)*start < 0x20)
+ return 1;
+ if ((1U << ((uint8_t)*start & 0x1F)) & ((1<<3) | (1<<0)))
+ return 1;
+ }
+ start++;
+ } while (start < istend(ist));
+ return 0;
+}
#endif /* _HAPROXY_HTTP_H */
--
2.43.0

View File

@ -0,0 +1,86 @@
From edcff741698c9519dc44f3aa13de421baad7ff43 Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w@1wt.eu>
Date: Tue, 8 Aug 2023 15:23:19 +0200
Subject: [PATCH] MINOR: ist: add new function ist_find_range() to find a
character range
This looks up the character range <min>..<max> in the input string and
returns a pointer to the first one found. It's essentially the equivalent
of ist_find_ctl() in that it searches by 32 or 64 bits at once, but deals
with a range.
(cherry picked from commit 197668de975e495f0c0f0e4ff51b96203fa9842d)
[ad: backported for following fix : BUG/MINOR: h2: reject more chars
from the :path pseudo header]
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
(cherry picked from commit 451ac6628acc4b9eed3260501a49c60d4e4d4e55)
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
(cherry picked from commit 3468f7f8e04c9c5ca5c985c7511e05e78fe1eded)
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
(cherry picked from commit b375df60341c7f7a4904c2d8041a09c66115c754)
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
include/import/ist.h | 47 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 47 insertions(+)
diff --git a/include/import/ist.h b/include/import/ist.h
index 539a27d26..31566b105 100644
--- a/include/import/ist.h
+++ b/include/import/ist.h
@@ -746,6 +746,53 @@ static inline const char *ist_find_ctl(const struct ist ist)
return NULL;
}
+/* Returns a pointer to the first character found <ist> that belongs to the
+ * range [min:max] inclusive, or NULL if none is present. The function is
+ * optimized for strings having no such chars by processing up to sizeof(long)
+ * bytes at once on architectures supporting efficient unaligned accesses.
+ * Despite this it is not very fast (~0.43 byte/cycle) and should mostly be
+ * used on low match probability when it can save a call to a much slower
+ * function. Will not work for characters 0x80 and above. It's optimized for
+ * min and max to be known at build time.
+ */
+static inline const char *ist_find_range(const struct ist ist, unsigned char min, unsigned char max)
+{
+ const union { unsigned long v; } __attribute__((packed)) *u;
+ const char *curr = (void *)ist.ptr - sizeof(long);
+ const char *last = curr + ist.len;
+ unsigned long l1, l2;
+
+ /* easier with an exclusive boundary */
+ max++;
+
+ do {
+ curr += sizeof(long);
+ if (curr > last)
+ break;
+ u = (void *)curr;
+ /* add 0x<min><min><min><min>..<min> then subtract
+ * 0x<max><max><max><max>..<max> to the value to generate a
+ * carry in the lower byte if the byte contains a lower value.
+ * If we generate a bit 7 that was not there, it means the byte
+ * was min..max.
+ */
+ l2 = u->v;
+ l1 = ~l2 & ((~0UL / 255) * 0x80); /* 0x808080...80 */
+ l2 += (~0UL / 255) * min; /* 0x<min><min>..<min> */
+ l2 -= (~0UL / 255) * max; /* 0x<max><max>..<max> */
+ } while ((l1 & l2) == 0);
+
+ last += sizeof(long);
+ if (__builtin_expect(curr < last, 0)) {
+ do {
+ if ((unsigned char)(*curr - min) < (unsigned char)(max - min))
+ return curr;
+ curr++;
+ } while (curr < last);
+ }
+ return NULL;
+}
+
/* looks for first occurrence of character <chr> in string <ist> and returns
* the tail of the string starting with this character, or (ist.end,0) if not
* found.
--
2.43.0

View File

@ -0,0 +1,46 @@
From c7492154ef07d6c08aa1eb52502697bbc3f42a69 Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w@1wt.eu>
Date: Tue, 8 Aug 2023 19:52:45 +0200
Subject: [PATCH] REGTESTS: http-rules: add accept-invalid-http-request for
normalize-uri tests
We'll soon block the '#' by default so let's prepare the test to continue
to work.
(cherry picked from commit 069d0e221e58a46119d7c049bb07fa4bcb8d0075)
[ad: backported for following fix : BUG/MINOR: h2: reject more chars
from the :path pseudo header]
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
(cherry picked from commit 1660481fab69856a39ac44cf88b76cdbcc0ea954)
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
(cherry picked from commit 90d0300cea6cda18a4e20369f4dc0b4c4783d6c9)
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
(cherry picked from commit 65849396fd6f192d9f14e81702c6c3851e580345)
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
reg-tests/http-rules/normalize_uri.vtc | 2 ++
1 file changed, 2 insertions(+)
diff --git a/reg-tests/http-rules/normalize_uri.vtc b/reg-tests/http-rules/normalize_uri.vtc
index 6a1dc31dc..56acf2cef 100644
--- a/reg-tests/http-rules/normalize_uri.vtc
+++ b/reg-tests/http-rules/normalize_uri.vtc
@@ -127,6 +127,7 @@ haproxy h1 -conf {
frontend fe_fragment_strip
bind "fd@${fe_fragment_strip}"
+ option accept-invalid-http-request
http-request set-var(txn.before) url
http-request normalize-uri fragment-strip
@@ -139,6 +140,7 @@ haproxy h1 -conf {
frontend fe_fragment_encode
bind "fd@${fe_fragment_encode}"
+ option accept-invalid-http-request
http-request set-var(txn.before) url
http-request normalize-uri fragment-encode
--
2.43.0

View File

@ -8,7 +8,7 @@
Name: haproxy
Version: 2.4.22
Release: 2%{?dist}
Release: 3%{?dist}
Summary: HAProxy reverse proxy for high availability environments
License: GPLv2+
@ -23,6 +23,12 @@ Source5: %{name}.sysusers
Source6: halog.1
Patch0: RHEL-7736_http-reject-empty-content-length-header.patch
Patch1: RHEL-18169_h1-reject-special-char-URI-path-component.patch
Patch2: RHEL-18169_h2-pass-accept-invalid-http-request-request-parser.patch
Patch3: RHEL-18169_h2-reject-special-char-from-pseudo-path-header.patch
Patch4: RHEL-18169_http-add-new-function-http_path_has_forbidden_char.patch
Patch5: RHEL-18169_ist-add-new-function-ist_find_range.patch
Patch6: RHEL-18169_regtest-add-accept-invalid-http-request.patch
BuildRequires: gcc
BuildRequires: lua-devel
@ -53,6 +59,12 @@ availability environments. Indeed, it can:
%prep
%setup -q
%patch -P0 -p1
%patch -P1 -p1
%patch -P2 -p1
%patch -P3 -p1
%patch -P4 -p1
%patch -P5 -p1
%patch -P6 -p1
%build
regparm_opts=
@ -135,6 +147,9 @@ done
%{_sysusersdir}/%{name}.conf
%changelog
* Tue Jan 23 2024 Ryan O'Hara <rohara@redhat.com> - 2.4.22-3
- Reject "#" as part of URI path component (CVE-2023-45539, RHEL-18169)
* Wed Jan 17 2024 Ryan O'Hara <rohara@redhat.com> - 2.4.22-2
- Reject any empty content-length header value (CVE-2023-40225, RHEL-7736)