[master] e082a30c8 validate_headers feature

[Nils Goroll]

commit e082a30c83657004727b94df2444b366d550a888
Author: Nils Goroll <nils.goroll at uplex.de>
Date:   Fri Oct 9 13:45:41 2020 +0200

    validate_headers feature
    
    We now validate all header set operations to conform with the allowed
    characters by RFC7230:
    
    * HTAB 0x09
    * VCHAR 0x20 to 0x7e
    * obs-text 0x80 to 0xff
    
    Ref https://httpwg.org/specs/rfc7230.html#header.fields
    
    See #3407

diff --git a/bin/varnishd/cache/cache_vrt.c b/bin/varnishd/cache/cache_vrt.c
index 68597ad84..25fb07fd8 100644
--- a/bin/varnishd/cache/cache_vrt.c
+++ b/bin/varnishd/cache/cache_vrt.c
@@ -633,19 +633,23 @@ VRT_SetHdr(VRT_CTX , VCL_HEADER hs, const char *p, ...)
 	AN(hs->what);
 	hp = VRT_selecthttp(ctx, hs->where);
 	CHECK_OBJ_NOTNULL(hp, HTTP_MAGIC);
-	va_start(ap, p);
 	if (p == vrt_magic_string_unset) {
 		http_Unset(hp, hs->what);
 	} else {
+		va_start(ap, p);
 		b = VRT_String(hp->ws, hs->what + 1, p, ap);
+		va_end(ap);
 		if (b == NULL) {
 			VSLb(ctx->vsl, SLT_LostHeader, "%s", hs->what + 1);
-		} else {
-			http_Unset(hp, hs->what);
-			http_SetHeader(hp, b);
+			return;
+		}
+		if (FEATURE(FEATURE_VALIDATE_HEADERS) && ! validhdr(b)) {
+			VRT_fail(ctx, "Bad header %s", b);
+			return;
 		}
+		http_Unset(hp, hs->what);
+		http_SetHeader(hp, b);
 	}
-	va_end(ap);
 }
 
 /*--------------------------------------------------------------------*/
diff --git a/bin/varnishd/mgt/mgt_param_bits.c b/bin/varnishd/mgt/mgt_param_bits.c
index cc0812624..6a113e72e 100644
--- a/bin/varnishd/mgt/mgt_param_bits.c
+++ b/bin/varnishd/mgt/mgt_param_bits.c
@@ -222,7 +222,12 @@ tweak_feature(struct vsb *vsb, const struct parspec *par, const char *arg)
 	(void)par;
 
 	if (arg != NULL && arg != JSON_FMT) {
-		if (!strcmp(arg, "none")) {
+		if (!strcmp(arg, "default")) {
+			memset(mgt_param.feature_bits,
+			    0, sizeof mgt_param.feature_bits);
+			(void)bit(mgt_param.feature_bits,
+			    FEATURE_VALIDATE_HEADERS, BSET);
+		} else if (!strcmp(arg, "none")) {
 			memset(mgt_param.feature_bits,
 			    0, sizeof mgt_param.feature_bits);
 		} else {
@@ -271,9 +276,10 @@ struct parspec VSL_parspec[] = {
 #undef DEBUG_BIT
 		},
 	{ "feature", tweak_feature, NULL,
-		NULL, NULL, "none",
+		NULL, NULL, "default",
 		NULL,
 		"Enable/Disable various minor features.\n"
+		"\tdefault\tSet default value\n"
 		"\tnone\tDisable all features.\n\n"
 		"Use +/- prefix to enable/disable individual feature:"
 #define FEATURE_BIT(U, l, d) "\n\t" #l "\t" d
diff --git a/bin/varnishtest/tests/b00040.vtc b/bin/varnishtest/tests/b00040.vtc
index cc7479d05..5e396b386 100644
--- a/bin/varnishtest/tests/b00040.vtc
+++ b/bin/varnishtest/tests/b00040.vtc
@@ -1,12 +1,22 @@
-varnishtest "test certain mailformed requests"
+varnishtest "test certain malformed requests and validate_headers"
 
 server s1 {
 	rxreq
 	expect req.url == /4
 	txresp
+	rxreq
+	expect req.url == /9
+	txresp
 } -start
 
-varnish v1 -vcl+backend { } -start
+varnish v1 -vcl+backend {
+	sub vcl_recv {
+		if (req.url == "/9") {
+			set req.http.foo = {"
+			"};
+		}
+	}
+} -start
 
 logexpect l1 -v v1 -g raw {
 	expect * 1001 BogoHeader {1st header has white space:.*}
@@ -16,6 +26,7 @@ logexpect l1 -v v1 -g raw {
 	expect * 1012 BogoHeader {Header has ctrl char 0x0d}
 	expect * 1014 BogoHeader {Header has ctrl char 0x0d}
 	expect * 1016 BogoHeader {Missing header name:.*}
+	expect * 1018 VCL_Error  {Bad header foo:}
 } -start
 
 client c1 {
@@ -80,5 +91,18 @@ client c1 {
 	expect resp.status == 400
 } -run
 
+client c1 {
+	txreq -url /9
+	rxresp
+	expect resp.status == 503
+} -run
+
 logexpect l1 -wait
 
+varnish v1 -cliok "param.set feature -validate_headers"
+
+client c1 {
+	txreq -url /9
+	rxresp
+	expect resp.status == 200
+} -run
diff --git a/doc/changes.rst b/doc/changes.rst
index 5a81f289e..b2c192e43 100644
--- a/doc/changes.rst
+++ b/doc/changes.rst
@@ -33,6 +33,10 @@ Varnish Cache Next (2021-03-15)
 * counters MAIN.s_req_bodybytes and VBE.*.tools.beresp_bodybytes
   are now always the number of bodybytes moved on the wire.
 
+* Unless the new ``validate_headers`` feature is disabled, all newly
+  set headers are now validated to contain only characters allowed by
+  RFC7230. A (runtime) VCL failure is triggered if not.
+
 ================================
 Varnish Cache 6.5.1 (2020-09-25)
 ================================
diff --git a/include/tbl/feature_bits.h b/include/tbl/feature_bits.h
index 2f314cfb2..207e18e11 100644
--- a/include/tbl/feature_bits.h
+++ b/include/tbl/feature_bits.h
@@ -74,6 +74,10 @@ FEATURE_BIT(WAIT_SILO,			wait_silo,
     "Wait for persistent silos to completely load before serving requests."
 )
 
+FEATURE_BIT(VALIDATE_HEADERS,		validate_headers,
+    "Validate all header set operations to conform to RFC7230."
+)
+
 #undef FEATURE_BIT
 
 /*lint -restore */