BUG/MAJOR: h2: enforce checks on the method syntax before translating to HTX

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 <w@1wt.eu>
(cherry picked from commit 6b827f661374704e91322a82197bbfbfbf910f70)
[wt: adapted since no meth_sl in 2.3]
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit fbeb053d1a83faedbf3edbe04bde39bc7304cddd)
Signed-off-by: Willy Tarreau <w@1wt.eu>
Gbp-Pq: Name 2.2-0004-BUG-MAJOR-h2-enforce-checks-on-the-method-syntax-bef.patch

diff --git a/src/h2.c b/src/h2.c
index 2648488120769b0344249cac8f94ae0000322ac9..11e56b4eaaca95702e678f7469876ef0aaab8e6c 100644 (file)
--- 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.
         */