From c91c37a122784de872b79ec6832fe8a9cfe675e0 Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Wed, 11 Aug 2021 11:12:46 +0200 Subject: BUG/MAJOR: h2: enforce checks on the method syntax before translating to HTX MIME-Version: 1.0 Content-Type: text/plain; charset=latin1 Content-Transfer-Encoding: 8bit The situation with message components in H2 is always troubling. They're produced by the HPACK layer which contains a dictionary of well-known hardcoded values, yet wants to remain binary transparent and protocol- agnostic with HTTP just being one user, yet at the H2 layer we're supposed to enforce some checks on some selected pseudo-headers that come from internal constants... The :method pseudo-header is no exception and is not tested when coming from the HPACK layer. This makes it possible to pass random chars into methods, that can be serialized on another H2 connection (where they would not harm), or worse, on an H1 connection where they can be used to transform the forwareded request. This is similar to the request line injection described here: https://portswigger.net/research/http2 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 be part of the ones permitted in a token, as mentioned in RFC7231#4.1. This should be backported to versions 2.0 and above, maybe even 1.8. 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 were initially thought to be safe but must be carefully rechecked since they transcode the request into H1 before processing it. Thanks to Tim Düsterhus for reporting that one. (cherry picked from commit b4be735a0a7c4a00bf3d774334763536774d7eea) Signed-off-by: Willy Tarreau (cherry picked from commit 6b827f661374704e91322a82197bbfbfbf910f70) [wt: adapted since no meth_sl in 2.3] Signed-off-by: Willy Tarreau (cherry picked from commit fbeb053d1a83faedbf3edbe04bde39bc7304cddd) 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 264848812..11e56b4ea 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. */ -- 2.28.0