BUG/MEDIUM: h1: reject absolute http(s) URIs with an empty host

An absolute-form request target with the http or https scheme and an empty
host was accepted and forwarded verbatim to H1 servers, for instance:

    GET http:///admin.internal/x HTTP/1.1\r\n\r\n
    GET http://:8080/x HTTP/1.1\r\n\r\n
    GET http://@/x HTTP/1.1\r\n\r\n

RFC9110#4.2.1 says that a recipient must reject such URIs as invalid. Here
the rules see an empty host (or ":8080") and the path "/admin.internal/x",
while servers relying on WHATWG-style URL parsers (e.g. NodeJS) take
"admin.internal" as the host and "/x" as the path. It's a parser
differential that may be used to bypass host or path based rules, depending
on the server.

Let's reject these requests, unless unsafe violations are accepted (then the
error is only captured). A reg-test was added for this.

Note that the "strange cases" C32 to C35 of the h1_host_normalization
reg-test explicitly verified that "GET http://:" with "host: :80" (or
"https://:" and "host: :443" / "host: :") were accepted and normalized to
"http:///" with an empty host. These cases are now rejected with a 400, so
the test was updated accordingly. Developers may want to double-check that
there was no other reason to accept them.

The validation of the request target was introduced by commit 25bcdb1d9
("BUG/MAJOR: h1: Be stricter on request target validation during message
parsing") in 3.0-dev12, which didn't cover this case. This should be
backported as far as 3.0.

diff --git a/doc/configuration.txt b/doc/configuration.txt
index d62059922c472c600f72c5b4185b1fd1e9e4cb9b..2f047285dbd7b3fa85a7ad9d57f551c4438e25ae 100644 (file)
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -10445,7 +10445,13 @@ no option accept-unsafe-violations-in-http-request
       requests;
 
     * In H1 only, no check will be performed against the authority and the Host
-      header value.
+      header value;
+
+    * In H1 only, when a request has no Host header, a Host header is built
+      from the authority of its target (the request-target itself for CONNECT
+      requests). But it is not added when the authority contains characters
+      forbidden in the Host header or, for CONNECT requests, when the
+      authority is invalid.
 
     * In H1 only, tests on the HTTP version will be relaxed. It will allow
       HTTP/0.9 GET requests to pass through (no version specified), as well as

diff --git a/reg-tests/http-messaging/h1_absolute_uri_empty_host.vtc b/reg-tests/http-messaging/h1_absolute_uri_empty_host.vtc
new file mode 100644 (file)
index 0000000..1841476
--- /dev/null
+++ b/reg-tests/http-messaging/h1_absolute_uri_empty_host.vtc
@@ -0,0 +1,43 @@
+varnishtest "H1 absolute-form http(s) URIs with an empty host"
+
+# http and https URIs with an empty host are invalid (RFC9110#4.2.1) and
+# must be rejected, otherwise rules see no host while some servers would
+# find one in the forwarded URI.
+
+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}"
+
+    frontend fe1
+        bind "fd@${fe1}"
+        http-request return status 200
+} -start
+
+client c1 -connect ${h1_fe1_sock} {
+       txreq -url "http:///admin.internal/x" -nohost
+       rxresp
+       expect resp.status == 400
+} -run
+
+client c2 -connect ${h1_fe1_sock} {
+       txreq -url "http://:8080/x" -nohost
+       rxresp
+       expect resp.status == 400
+} -run
+
+client c3 -connect ${h1_fe1_sock} {
+       txreq -url "https://@/x" -nohost
+       rxresp
+       expect resp.status == 400
+} -run
+
+client c4 -connect ${h1_fe1_sock} {
+       txreq -url "http://www.example.com:8080/x" -nohost
+       rxresp
+       expect resp.status == 200
+} -run

diff --git a/reg-tests/http-messaging/h1_host_normalization.vtc b/reg-tests/http-messaging/h1_host_normalization.vtc
index 048ad95bdebb9e9e2e7ebb6a0a84671f8391e55d..70225998ca1862929c9e63a06bafb7922f3fe7b8 100644 (file)
--- a/reg-tests/http-messaging/h1_host_normalization.vtc
+++ b/reg-tests/http-messaging/h1_host_normalization.vtc
@@ -175,22 +175,22 @@ syslog S1 -level info {
 
     # C32
     recv
-    expect ~ "^.* uri: GET http:/// HTTP/1.1; host: {}$"
+    expect ~ "^.* uri: <BADREQ>; host: $"
     barrier b1 sync
 
     # C33
     recv
-    expect ~ "^.* uri: GET https:/// HTTP/1.1; host: {}$"
+    expect ~ "^.* uri: <BADREQ>; host: $"
     barrier b1 sync
 
     # C34
     recv
-    expect ~ "^.* uri: GET http:/// HTTP/1.1; host: {}$"
+    expect ~ "^.* uri: <BADREQ>; host: $"
     barrier b1 sync
 
     # C35
     recv
-    expect ~ "^.* uri: GET https:/// HTTP/1.1; host: {}$"
+    expect ~ "^.* uri: <BADREQ>; host: $"
     barrier b1 sync
 
     # C36
@@ -769,7 +769,7 @@ client c32 -connect ${h1_fe_sock} {
       -hdr "host: :80"
 
     rxresp
-    expect resp.status == 200
+    expect resp.status == 400
 } -run
 
 # Wait matching on log message
@@ -783,7 +783,7 @@ client c33 -connect ${h1_fe_sock} {
       -hdr "host: :443"
 
     rxresp
-    expect resp.status == 200
+    expect resp.status == 400
 } -run
 
 # Wait matching on log message
@@ -797,7 +797,7 @@ client c34 -connect ${h1_fe_sock} {
       -hdr "host: :"
 
     rxresp
-    expect resp.status == 200
+    expect resp.status == 400
 } -run
 
 # Wait matching on log message
@@ -811,7 +811,7 @@ client c35 -connect ${h1_fe_sock} {
       -hdr "host: :"
 
     rxresp
-    expect resp.status == 200
+    expect resp.status == 400
 } -run
 
 # Wait matching on log message

diff --git a/src/h1.c b/src/h1.c
index 2cef677ac8bbf4d2fb5dbfea18e0232d8be9af7e..39a298790671b39d8e4397a25ae834f9a914d3f7 100644 (file)
--- a/src/h1.c
+++ b/src/h1.c
@@ -1199,12 +1199,32 @@ int h1_headers_to_hdr_list(char *start, const char *stop,
                                }
                                else { /* Scheme found:  MUST be an absolute-URI */
                                        struct ist host = IST_NULL;
+                                       struct ist uri_host = authority;
+                                       struct ist uri_port = http_get_host_port(authority);
 
                                        if (sl.rq.meth == HTTP_METH_CONNECT) {
                                                ptr = sl.rq.u.ptr; /* Set ptr on the error */
                                                goto http_msg_invalid;
                                        }
 
+                                       /* RFC9110#4.2.1/4.2.2: an http(s) URI with an
+                                        * empty host is invalid. Otherwise the target
+                                        * seen by the rules (no host) would differ from
+                                        * the one some servers would find in the URI.
+                                        */
+                                       if (isttest(uri_port))
+                                               uri_host.len -= istlen(uri_port) + 1;
+                                       if (!istlen(uri_host) &&
+                                           (isteqi(scheme, ist("http://")) || isteqi(scheme, ist("https://")))) {
+                                               if (h1m->err_pos < -1) {
+                                                       state = H1_MSG_LAST_LF;
+                                                       ptr = sl.rq.u.ptr; /* Set ptr on the error */
+                                                       goto http_msg_invalid;
+                                               }
+                                               if (h1m->err_pos == -1) /* capture the error pointer */
+                                                       h1m->err_pos = sl.rq.u.ptr - start + skip; /* >= 0 now */
+                                       }
+
                                        if (host_idx != -1)
                                                host = hdr[host_idx].v;
                                        /* For non-CONNECT method, the authority must match the host header value */