BUG/MEDIUM: h1: reject CONNECT request targets other than "host:port"

For CONNECT requests, the H1 parser validates the authority extracted by
http_parse_authority() (with userinfo removed and stopping at the first '/',
'?' or '#') against the Host header and checks it for forbidden
characters. But the request target itself is kept and forwarded as-is.  So
the following targets were accepted with "Host: allowed.com:443":

    CONNECT internal:22@allowed.com:443 HTTP/1.1
    CONNECT allowed.com:443/x HTTP/1.1
    CONNECT allowed.com:443?x HTTP/1.1

and forwarded verbatim to the server (or as the :authority of an H2
request). RFC9112#3.2.3 says the authority-form only contains
"uri-host:port". The next hop (typically a forward proxy) may parse such
invalid targets differently than haproxy (e.g. "internal" port 22), and
connect to a destination that was not checked by the host-based rules.

Now, a CONNECT request is rejected (or the error is captured when unsafe
violations are accepted, like for other authority errors) if the
validated authority does not span the whole request target.

A reg-test was added to verify this.

This was introduced by commit 3f5fbe940 ("BUG/MEDIUM: h1: Improve
authority validation for CONNCET request") in 2.7 which validated the
stripped authority. It should be backported as far as 2.8.

diff --git a/reg-tests/http-messaging/h1_connect_target_validation.vtc b/reg-tests/http-messaging/h1_connect_target_validation.vtc
new file mode 100644 (file)
index 0000000..0736843
--- /dev/null
+++ b/reg-tests/http-messaging/h1_connect_target_validation.vtc
@@ -0,0 +1,50 @@
+varnishtest "H1 CONNECT request target validation"
+
+# RFC9112#3.2.3: the request target of a CONNECT request is in authority-form
+# ("uri-host:port"). Since it is forwarded as-is, it must not contain any
+# userinfo, path or query that would make haproxy and the next hop disagree
+# on the destination.
+
+feature ignore_unknown_macro
+
+haproxy h1 -conf {
+    defaults
+        mode http
+        timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
+        timeout client  "${HAPROXY_TEST_TIMEOUT-5s}"
+        timeout server  "${HAPROXY_TEST_TIMEOUT-5s}"
+
+    listen li1
+        bind "fd@${li1}"
+        http-request return status 200
+} -start
+
+client c1 -connect ${h1_li1_sock} {
+       txreq -req "CONNECT" -url "allowed.com:443" -hdr "Host: allowed.com:443"
+       rxresp
+       expect resp.status == 200
+} -run
+
+client c2 -connect ${h1_li1_sock} {
+       txreq -req "CONNECT" -url "internal:22@allowed.com:443" -hdr "Host: allowed.com:443"
+       rxresp
+       expect resp.status == 400
+} -run
+
+client c3 -connect ${h1_li1_sock} {
+       txreq -req "CONNECT" -url "allowed.com:443/x" -hdr "Host: allowed.com:443"
+       rxresp
+       expect resp.status == 400
+} -run
+
+client c4 -connect ${h1_li1_sock} {
+       txreq -req "CONNECT" -url "allowed.com:443?x" -hdr "Host: allowed.com:443"
+       rxresp
+       expect resp.status == 400
+} -run
+
+client c5 -connect ${h1_li1_sock} {
+       txreq -req "CONNECT" -url "internal:22@allowed.com:443"
+       rxresp
+       expect resp.status == 400
+} -run

diff --git a/src/h1.c b/src/h1.c
index 814a5ea4d0044055c77a36f7239add31280b6d3c..2a91407abac149b2be7e88185ee876394369ea61 100644 (file)
--- a/src/h1.c
+++ b/src/h1.c
@@ -1166,6 +1166,17 @@ int h1_headers_to_hdr_list(char *start, const char *stop,
                                        if (host_idx != -1)
                                                host = &hdr[host_idx].v;
                                        ret = h1_validate_connect_authority(scheme, authority, host);
+
+                                       /* RFC9112#3.2.3: the authority-form only contains
+                                        * "uri-host:port". The authority was validated
+                                        * without any userinfo, path or query, which must
+                                        * then not be present since the request target is
+                                        * forwarded as-is.
+                                        */
+                                       if (!ret && (istptr(authority) != istptr(sl.rq.u) ||
+                                                    istlen(authority) != istlen(sl.rq.u)))
+                                               ret = -1;
+
                                        if (ret < 0) {
                                                if (h1m->err_pos < -1) {
                                                        state = H1_MSG_LAST_LF;