BUG/MINOR: sample: reject a \0 byte in raw string fetches

These fetches return a string pointing to the raw client data, which may
contain a \0 byte, while string consumers rely on it not happening
(37204a71a). The pattern tree lookup used by case-sensitive "-m str"
matches and by maps stops at this byte, so "user\0admin" matches "user",
while the next hop sees the whole value. A rule based on these fetches
can then be bypassed.

req.ssl_sni and req.rdp_cookie now fail on such a value, and
req.ssl_alpn and req.body_param skip it and report the next one.
Legitimate values never contain a \0.

The reg-test str_embedded_nul.vtc covers it.

This should be backported along with 37204a71a.

diff --git a/reg-tests/sample_fetches/str_embedded_nul.vtc b/reg-tests/sample_fetches/str_embedded_nul.vtc
new file mode 100644 (file)
index 0000000..49b5c90
--- /dev/null
+++ b/reg-tests/sample_fetches/str_embedded_nul.vtc
@@ -0,0 +1,132 @@
+varnishtest "string samples taken from raw client bytes must not contain a NUL"
+feature ignore_unknown_macro
+
+#REGTEST_TYPE=bug
+
+# req.ssl_sni, req.ssl_alpn, req.rdp_cookie and req.body_param return client
+# bytes as a string sample. A \0 in them made "-m str" (and map_str) match on
+# the prefix only, through the pattern tree. These samples must fail instead,
+# like url_dec and json_query do since commit 37204a71a, while the following
+# occurrences remain visible to iterating ACLs.
+
+# Every accepted TCP connection reaches s1, which answers "yes". Rejected
+# ones are closed by haproxy without any data.
+server s1 -repeat 4 {
+    recv 1
+    send "yes"
+} -start
+
+haproxy h1 -conf {
+  global
+    .if feature(THREAD)
+        thread-groups 1
+    .endif
+
+  defaults
+    mode tcp
+    timeout connect "${HAPROXY_TEST_TIMEOUT-5s}"
+    timeout client  "${HAPROXY_TEST_TIMEOUT-5s}"
+    timeout server  "${HAPROXY_TEST_TIMEOUT-5s}"
+
+  frontend fe_sni
+    bind "fd@${fe_sni}"
+    tcp-request inspect-delay 5s
+    tcp-request content reject unless { req.ssl_sni -m str allowed.example.com }
+    default_backend be
+
+  frontend fe_alpn
+    bind "fd@${fe_alpn}"
+    tcp-request inspect-delay 5s
+    tcp-request content reject unless { req.ssl_alpn -m str h2 }
+    default_backend be
+
+  frontend fe_alpn_deny
+    bind "fd@${fe_alpn_deny}"
+    tcp-request inspect-delay 5s
+    tcp-request content reject if { req.ssl_alpn -m str evil }
+    tcp-request content accept if { req.ssl_alpn -m found }
+    default_backend be
+
+  frontend fe_rdp
+    bind "fd@${fe_rdp}"
+    tcp-request inspect-delay 5s
+    tcp-request content reject unless { req.rdp_cookie(mstshash) -m str alice }
+    default_backend be
+
+  backend be
+    server s1 ${s1_addr}:${s1_port}
+
+  frontend fe_body
+    mode http
+    bind "fd@${fe_body}"
+    option http-buffer-request
+    http-request deny deny_status 401 if { req.body_param(x) -m str bad }
+    http-request deny deny_status 403 unless { req.body_param(role) -m str user }
+    http-request return status 200
+} -start
+
+# ClientHello, SNI "allowed.example.com": accepted
+client c1 -connect ${h1_fe_sni_sock} {
+    sendhex "160301004b010000470303111111111111111111111111111111111111111111111111111111111111111100000213010100001c000000180016000013616c6c6f7765642e6578616d706c652e636f6d"
+    recv 3
+} -run
+
+# ClientHello, SNI "allowed.example.com\0.evil.net": rejected
+client c2 -connect ${h1_fe_sni_sock} {
+    sendhex "1603010055010000510303111111111111111111111111111111111111111111111111111111111111111100000213010100002600000022002000001d616c6c6f7765642e6578616d706c652e636f6d002e6576696c2e6e6574"
+    expect_close
+} -run
+
+# ClientHello, ALPN "h2": accepted
+client c3 -connect ${h1_fe_alpn_sock} {
+    sendhex "16030100380100003403031111111111111111111111111111111111111111111111111111111111111111000002130101000009001000050003026832"
+    recv 3
+} -run
+
+# ClientHello, ALPN "h2\0zz": rejected
+client c4 -connect ${h1_fe_alpn_sock} {
+    sendhex "160301003b010000370303111111111111111111111111111111111111111111111111111111111111111100000213010100000c001000080006056832007a7a"
+    expect_close
+} -run
+
+# ClientHello, ALPN "h2": not denied
+client c5 -connect ${h1_fe_alpn_deny_sock} {
+    sendhex "16030100380100003403031111111111111111111111111111111111111111111111111111111111111111000002130101000009001000050003026832"
+    recv 3
+} -run
+
+# ClientHello, ALPN "h2\0zz" then "evil": the name after the skipped one is
+# still seen, so the connection is denied
+client c6 -connect ${h1_fe_alpn_deny_sock} {
+    sendhex "16030100400100003c030311111111111111111111111111111111111111111111111111111111111111110000021301010000110010000d000b056832007a7a046576696c"
+    expect_close
+} -run
+
+# RDP, "Cookie: mstshash=alice": accepted
+client c7 -connect ${h1_fe_rdp_sock} {
+    sendhex "0300002b26e00000000000436f6f6b69653a206d737473686173683d616c6963650d0a0100080000000000"
+    recv 3
+} -run
+
+# RDP, "Cookie: mstshash=alice\0admin": rejected
+client c8 -connect ${h1_fe_rdp_sock} {
+    sendhex "030000312ce00000000000436f6f6b69653a206d737473686173683d616c6963650061646d696e0d0a0100080000000000"
+    expect_close
+} -run
+
+client c9 -connect ${h1_fe_body_sock} {
+    txreq -req POST -url "/" -body "role=user"
+    rxresp
+    expect resp.status == 200
+
+    # POST / with body "role=user\0admin"
+    sendhex "504f5354202f20485454502f312e310d0a686f73743a20780d0a636f6e74656e742d6c656e6774683a2031350d0a0d0a726f6c653d757365720061646d696e"
+    rxresp
+    expect resp.status == 403
+
+    # POST / with body "role=user&x=a\0b&x=bad": the value after the skipped
+    # one is still seen
+    sendhex "504f5354202f20485454502f312e310d0a686f73743a20780d0a636f6e74656e742d6c656e6774683a2032310d0a0d0a726f6c653d7573657226783d61006226783d626164"
+    rxresp
+    expect resp.status == 401
+} -run

diff --git a/src/http_fetch.c b/src/http_fetch.c
index ff50504ae976d68f05f0cfeb78f29ac9bf6dc8c1..af9250978c82d484a4aa7d129a7e4afca33c0c99 100644 (file)
--- a/src/http_fetch.c
+++ b/src/http_fetch.c
@@ -1994,6 +1994,7 @@ static int smp_fetch_param(char delim, const char *name, int name_len, const str
        struct buffer *temp;
        const char **chunks = (const char **)smp->ctx.a;
 
+  next_param:
        if (!http_find_next_url_param(chunks, name, name_len,
                                 &vstart, &vend, delim, insensitive))
                return 0;
@@ -2030,6 +2031,10 @@ static int smp_fetch_param(char delim, const char *name, int name_len, const str
                chunks[2] = NULL;
        }
 
+       /* a string sample cannot contain a \0 */
+       if (memchr(smp->data.u.str.area, 0, smp->data.u.str.data))
+               goto next_param;
+
        if (chunks[0] < chunks[1])
                smp->flags |= SMP_F_NOT_LAST;
 

diff --git a/src/payload.c b/src/payload.c
index dbc4a69cadd06a614f46ed35e2c40e2573b72bb9..2a080fc424d5efac4c2fda33010d6e5ca961fbf0 100644 (file)
--- a/src/payload.c
+++ b/src/payload.c
@@ -806,6 +806,10 @@ smp_fetch_ssl_hello_sni(const struct arg *args, struct sample *smp, const char *
                                goto not_ssl_hello;
 
                        if (name_type == 0) { /* hostname */
+                               /* a host name never contains a \0 */
+                               if (memchr(data + 9, 0, name_len))
+                                       goto not_ssl_hello;
+
                                smp->data.type = SMP_T_STR;
                                smp->data.u.str.area = (char *)data + 9;
                                smp->data.u.str.data = name_len;
@@ -890,11 +894,21 @@ smp_fetch_ssl_hello_alpn(const struct arg *args, struct sample *smp, const char
 
                        /* Name cursor in ctx, must begin after protocol_names_len */
                        name_offset = smp->ctx.i < 6 ? 6 : smp->ctx.i;
+               next_name:
                        name_len = data[name_offset];
 
                        if (name_len + name_offset - 3 > ext_len)
                                goto not_ssl_hello;
 
+                       /* skip the protocol names containing a \0 */
+                       if (memchr(data + name_offset + 1, 0, name_len)) {
+                               if (name_len + name_offset - 3 < ext_len) {
+                                       name_offset += name_len + 1;
+                                       goto next_name;
+                               }
+                               goto not_ssl_hello;
+                       }
+
                        smp->data.type = SMP_T_STR;
                        smp->data.u.str.area = (char *)data + name_offset + 1; /* +1 to skip name_len */
                        smp->data.u.str.data = name_len;
@@ -1001,6 +1015,11 @@ fetch_rdp_cookie_name(struct stream *s, struct sample *smp, const char *cname, i
                goto not_cookie;
 
        smp->data.u.str.data = (char *)data - smp->data.u.str.area;
+
+       /* a string sample cannot contain a \0 */
+       if (memchr(smp->data.u.str.area, 0, smp->data.u.str.data))
+               goto not_cookie;
+
        smp->flags = SMP_F_VOLATILE | SMP_F_CONST;
        return 1;