80 lines
2.7 KiB
Diff
80 lines
2.7 KiB
Diff
From 89265224d314a056d77d974284802c1b8a0dc97f Mon Sep 17 00:00:00 2001
|
|
From: Willy Tarreau <w@1wt.eu>
|
|
Date: Wed, 11 Aug 2021 11:12:46 +0200
|
|
Subject: [PATCH] BUG/MAJOR: h2: enforce stricter syntax checks on the :method
|
|
pseudo-header
|
|
MIME-Version: 1.0
|
|
Content-Type: text/plain; charset=utf8
|
|
Content-Transfer-Encoding: 8bit
|
|
|
|
Before HTX was introduced, all the HTTP request elements passed in
|
|
pseudo-headers fields were used to build an HTTP/1 request whose syntax
|
|
was then scrutinized by the HTTP/1 parser, leaving no room to inject
|
|
invalid characters.
|
|
|
|
While NUL, CR and LF are properly blocked, it is possible to inject
|
|
spaces in the method so that once translated to HTTP/1, fields are
|
|
shifted by one spcae, and a lenient HTTP/1 server could possibly be
|
|
fooled into using a part of the method as the URI. For example, the
|
|
following request:
|
|
|
|
H2 request
|
|
:method: "GET /admin? HTTP/1.1"
|
|
:path: "/static/images"
|
|
|
|
would become:
|
|
|
|
GET /admin? HTTP/1.1 /static/images HTTP/1.1
|
|
|
|
It's important to note that the resulting request is *not* valid, and
|
|
that in order for this to be a problem, it requires that this request
|
|
is delivered to an already vulnerable HTTP/1 server.
|
|
|
|
A workaround here is to reject malformed methods by placing this rule
|
|
in the frontend or backend, at least before leaving haproxy in H1:
|
|
|
|
http-request reject if { method -m reg [^A-Z0-9] }
|
|
|
|
Alternately H2 may be globally disabled by commenting out the "alpn"
|
|
directive on "bind" lines, and by rejecting H2 streams creation by
|
|
adding the following statement to the global section:
|
|
|
|
tune.h2.max-concurrent-streams 0
|
|
|
|
This patch adds a check for each character of the method to make sure
|
|
they belong to the ones permitted in a token, as mentioned in RFC7231#4.1.
|
|
|
|
This should be backported to versions 2.0 and above. For older versions
|
|
not having HTX_FL_PARSING_ERROR, a "goto fail" works as well as it
|
|
results in a protocol error at the stream level. Non-HTX versions are
|
|
safe because the resulting invalid request will be rejected by the
|
|
internal HTTP/1 parser.
|
|
|
|
Thanks to Tim Düsterhus for reporting that one.
|
|
---
|
|
src/h2.c | 8 ++++++++
|
|
1 file changed, 8 insertions(+)
|
|
|
|
diff --git a/src/h2.c b/src/h2.c
|
|
index b31ff93..280bbd7 100644
|
|
--- a/src/h2.c
|
|
+++ b/src/h2.c
|
|
@@ -328,6 +328,14 @@ static struct htx_sl *h2_prepare_htx_reqline(uint32_t fields, struct ist *phdr,
|
|
flags |= HTX_SL_F_HAS_AUTHORITY;
|
|
}
|
|
|
|
+ /* The method is a non-empty token (RFC7231#4.1) */
|
|
+ if (!meth_sl.len)
|
|
+ goto fail;
|
|
+ for (i = 0; i < meth_sl.len; i++) {
|
|
+ if (!HTTP_IS_TOKEN(meth_sl.ptr[i]))
|
|
+ htx->flags |= HTX_FL_PARSING_ERROR;
|
|
+ }
|
|
+
|
|
/* make sure the final URI isn't empty. Note that 7540#8.1.2.3 states
|
|
* that :path must not be empty.
|
|
*/
|
|
--
|
|
1.7.10.4
|
|
|