From 4dbbac205187f02635a2d42c6a63dbb7e43badcb Mon Sep 17 00:00:00 2001 From: Willy Tarreau 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. (cherry picked from commit 89265224d314a056d77d974284802c1b8a0dc97f) Signed-off-by: Willy Tarreau (cherry picked from commit e9f1f1e7726084580b8eab5dfa1e07e202c376f9) [wt: adapted since no meth_sl in 2.3] Signed-off-by: Willy Tarreau --- src/h2.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/h2.c b/src/h2.c index 2648488..11e56b4 100644 --- a/src/h2.c +++ b/src/h2.c @@ -287,6 +287,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 (!phdr[H2_PHDR_IDX_METH].len) + goto fail; + for (i = 0; i < phdr[H2_PHDR_IDX_METH].len; i++) { + if (!HTTP_IS_TOKEN(phdr[H2_PHDR_IDX_METH].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