vct: Strict HTTP/1 CRLF parsing

We should avoid loose parsing of HTTP/1 header line delimiters. This could potentially create a smuggling vector if Varnish was stacked with a different server that would interpret the delimiters differently. Refs varnishcache#3246
dridi · Aug 13, 2021 · fbaa0ee · fbaa0ee
1 parent f3f0116
commit fbaa0ee
Show file tree

Hide file tree

Showing 17 changed files with 42 additions and 45 deletions.
diff --git a/bin/varnishtest/tests/b00011.vtc b/bin/varnishtest/tests/b00011.vtc
@@ -2,9 +2,9 @@ varnishtest "Check HTTP/1.0 EOF transmission"
 
 server s1 {
 	rxreq
-	send "HTTP/1.0 200 OK\n"
-	send "Connection: close\n"
-	send "\n"
+	send "HTTP/1.0 200 OK\r\n"
+	send "Connection: close\r\n"
+	send "\r\n"
 	send "Body line 1\n"
 	send "Body line 2\n"
 	send "Body line 3\n"

diff --git a/bin/varnishtest/tests/b00012.vtc b/bin/varnishtest/tests/b00012.vtc
@@ -12,7 +12,7 @@ server s1 {
 varnish v1 -vcl+backend {} -start
 
 client c1 {
-	send "GET /foo HTTP/1.1\nHost: foo\n\nGET /bar HTTP/1.1\nHost: foo\n\nGET /bar HTTP/1.1\nHost: foo\n\n"
+	send "GET /foo HTTP/1.1\r\nHost: foo\r\n\r\nGET /bar HTTP/1.1\r\nHost: foo\r\n\r\nGET /bar HTTP/1.1\r\nHost: foo\r\n\r\n"
 	rxresp
 	expect resp.status == 200
 	expect resp.bodylen == 3

diff --git a/bin/varnishtest/tests/b00013.vtc b/bin/varnishtest/tests/b00013.vtc
@@ -21,12 +21,12 @@ client c1 {
 	expect resp.status == 200
 	expect resp.bodylen == 3
 	expect resp.http.x-varnish == "1001"
-	send "/bar HTTP/1.1\nHost: foo\n\nGET /bar "
+	send "/bar HTTP/1.1\r\nHost: foo\r\n\r\nGET /bar "
 	rxresp
 	expect resp.status == 200
 	expect resp.bodylen == 6
 	expect resp.http.x-varnish == "1003"
-	send "HTTP/1.1\nHost: foo\n\n"
+	send "HTTP/1.1\r\nHost: foo\r\n\r\n"
 	rxresp
 	expect resp.status == 200
 	expect resp.bodylen == 6

diff --git a/bin/varnishtest/tests/b00055.vtc b/bin/varnishtest/tests/b00055.vtc
@@ -12,7 +12,7 @@ server s1 {
 varnish v1 -arg "-a ${tmpdir}/v1.sock" -vcl+backend {} -start
 
 client c1 -connect "${tmpdir}/v1.sock" {
-	send "GET /foo HTTP/1.1\nHost: foo\n\nGET /bar HTTP/1.1\nHost: foo\n\nGET /bar HTTP/1.1\nHost: foo\n\n"
+	send "GET /foo HTTP/1.1\r\nHost: foo\r\n\r\nGET /bar HTTP/1.1\r\nHost: foo\r\n\r\nGET /bar HTTP/1.1\r\nHost: foo\r\n\r\n"
 	rxresp
 	expect resp.status == 200
 	expect resp.bodylen == 3

diff --git a/bin/varnishtest/tests/b00056.vtc b/bin/varnishtest/tests/b00056.vtc
@@ -21,12 +21,12 @@ client c1 -connect "${tmpdir}/v1.sock" {
 	expect resp.status == 200
 	expect resp.bodylen == 3
 	expect resp.http.x-varnish == "1001"
-	send "/bar HTTP/1.1\nHost: foo\n\nGET /bar "
+	send "/bar HTTP/1.1\r\nHost: foo\r\n\r\nGET /bar "
 	rxresp
 	expect resp.status == 200
 	expect resp.bodylen == 6
 	expect resp.http.x-varnish == "1003"
-	send "HTTP/1.1\nHost: foo\n\n"
+	send "HTTP/1.1\r\nHost: foo\r\n\r\n"
 	rxresp
 	expect resp.status == 200
 	expect resp.bodylen == 6

diff --git a/bin/varnishtest/tests/b00069.vtc b/bin/varnishtest/tests/b00069.vtc
@@ -15,15 +15,15 @@ client c1 {
 	send "POST / HTTP/1.1\r\nHost: asdf.com\r\nFoo: baar\r\n\r\n\r\n\r\n\r\n"
 	rxresp
 	expect resp.status == 200
-	send "POST / HTTP/1.1\r\nHost: asdf.com\r\nAsdf: b\n \r\n\r\nSj\r"
+	send "POST / HTTP/1.1\r\nHost: asdf.com\r\nAsdf: b\r\n \r\n\r\nSj\r"
 	rxresp
 	expect resp.status == 200
 } -run
 
 # This tests that the line continuation handling doesn't clear out the end of headers
-# [CR]LF
+# CRLF
 client c1 {
-	send "POST / HTTP/1.1\r\nHost: asdf.com\r\nAsdf: b\n \r\n\r\nSj"
+	send "POST / HTTP/1.1\r\nHost: asdf.com\r\nAsdf: b\r\n \r\n\r\nSj"
 	rxresp
 	expect resp.status == 200
 } -run
diff --git a/bin/varnishtest/tests/b00075.vtc b/bin/varnishtest/tests/b00075.vtc
@@ -16,7 +16,7 @@ server s1 {
 varnish v1 -vcl {
 	backend s1 {
 		.host = "${s1_sock}";
-		.preamble = :UE9TVCAvcHJlYW1ibGUgSFRUUC83LjMKSGVhZGVyOiA0MgoKCg==: ;
+		.preamble = :UE9TVCAvcHJlYW1ibGUgSFRUUC83LjMNCkhlYWRlcjogNDINCg0K:;
 	}
 } -start
 

diff --git a/bin/varnishtest/tests/c00039.vtc b/bin/varnishtest/tests/c00039.vtc
@@ -30,10 +30,10 @@ client c1 {
 } -run
 
 client c1 {
-	txreq -url "/2" -hdr "host: 127.0.0.1" -hdr "1...5: ..0....5\n ..0....5....0....5....0"
+	txreq -url "/2" -hdr "host: 127.0.0.1" -hdr "1...5: ..0....5\r\n .0....5....0....5....0"
 	rxresp
 	expect resp.status == 200
-	txreq -url "/2" -hdr "host: 127.0.0.1" -hdr "1...5....0....5\n ..0....5....0....5....0."
+	txreq -url "/2" -hdr "host: 127.0.0.1" -hdr "1...5....0....5\r\n .0....5....0....5....0."
 	rxresp
 	expect resp.status == 400
 } -run

diff --git a/bin/varnishtest/tests/c00040.vtc b/bin/varnishtest/tests/c00040.vtc
@@ -15,12 +15,12 @@ server s1 {
 	rxreq
 	expect req.url == "/3"
 	txresp \
-		-hdr "1...5: ..0....5....0\n ..5....0....5....0" \
+		-hdr "1...5: ..0....5....0\r\n .5....0....5....0" \
 		-bodylen 3
 	rxreq
 	expect req.url == "/4"
 	txresp \
-		-hdr "1...5: ..0....5....0\n ..5....0....5....0." \
+		-hdr "1...5: ..0....5....0\r\n .5....0....5....0." \
 		-bodylen 4
 
 	accept

diff --git a/bin/varnishtest/tests/e00007.vtc b/bin/varnishtest/tests/e00007.vtc
@@ -17,9 +17,9 @@ varnishtest "ESI spanning storage bits"
 server s1 {
 	rxreq
 	expect req.url == "/foo/bar"
-	send "HTTP/1.0 200 OK\n"
-	send "Connection: close\n"
-	send "\n"
+	send "HTTP/1.0 200 OK\r\n"
+	send "Connection: close\r\n"
+	send "\r\n"
 	send {
 		<html>filler
 		This is before the test

diff --git a/bin/varnishtest/tests/e00014.vtc b/bin/varnishtest/tests/e00014.vtc
@@ -3,9 +3,9 @@ varnishtest "Check <esi: detector"
 server s1 {
 	rxreq
 	expect req.url == "/foo"
-	send "HTTP/1.0 200 OK\n"
-	send "Connection: close\n"
-	send "\n"
+	send "HTTP/1.0 200 OK\r\n"
+	send "Connection: close\r\n"
+	send "\r\n"
 	send {           <a>   <esi/>                          }
 } -start
 

diff --git a/bin/varnishtest/tests/p00007.vtc b/bin/varnishtest/tests/p00007.vtc
@@ -8,9 +8,9 @@ barrier b2 cond 2
 server s1 {
 	rxreq
 	expect req.url == "/1"
-	send "HTTP/1.1 200 OK\n"
-	send "Transfer-encoding: chunked\n"
-	send "\n"
+	send "HTTP/1.1 200 OK\r\n"
+	send "Transfer-encoding: chunked\r\n"
+	send "\r\n"
 	chunkedlen 32
 	# Tell top-level that it can sync the stevedore
 	barrier b1 sync

diff --git a/bin/varnishtest/tests/r00494.vtc b/bin/varnishtest/tests/r00494.vtc
@@ -2,10 +2,10 @@ varnishtest "HTTP continuation lines"
 
 server s1 {
 	rxreq
-	txresp -hdr "Foo: bar,\n\tbarf: fail" -body "xxx"
+	txresp -hdr "Foo: bar,\r\n\tbarf: fail" -body "xxx"
 
 	rxreq
-	txresp -hdr "Foo: bar,\n \t\n\tbarf: fail" -body "xxx"
+	txresp -hdr "Foo: bar,\r\n \t\r\n\tbarf: fail" -body "xxx"
 } -start
 
 varnish v1 -vcl+backend {
@@ -18,13 +18,13 @@ varnish v1 -vcl+backend {
 client c1 {
 	txreq
 	rxresp
-	expect resp.http.bar == "bar,  barf: fail"
+	expect resp.http.bar == "bar,   barf: fail"
 	expect resp.http.barf == <undef>
 	expect resp.http.foo == <undef>
 
 	txreq -url /2
 	rxresp
-	expect resp.http.bar == "bar,     barf: fail"
+	expect resp.http.bar == "bar,       barf: fail"
 	expect resp.http.barf == <undef>
 	expect resp.http.foo == <undef>
 } -run
diff --git a/bin/varnishtest/tests/r00700.vtc b/bin/varnishtest/tests/r00700.vtc
@@ -2,7 +2,7 @@ varnishtest "check TAB in 3 header field"
 
 server s1 {
 	rxreq
-	send "HTTP/1.1 666 foo\tbar\n\nFOO"
+	send "HTTP/1.1 666 foo\tbar\r\n\r\nFOO"
 } -start
 
 varnish v1 -vcl+backend {} -start

diff --git a/bin/varnishtest/tests/r00733.vtc b/bin/varnishtest/tests/r00733.vtc
@@ -2,8 +2,8 @@ varnishtest "HTTP/1.1 Backend sends no length hint"
 
 server s1 {
 	rxreq
-	send "HTTP/1.0 200 OK\n"
-	send "\n"
+	send "HTTP/1.0 200 OK\r\n"
+	send "\r\n"
 	send "12345"
 } -start
 

diff --git a/bin/varnishtest/tests/r01612.vtc b/bin/varnishtest/tests/r01612.vtc
@@ -9,18 +9,18 @@ server s1 {
 	# Nonzero chunked response is OK.
 	rxreq
 	expect req.url == "/1"
-	send "HTTP/1.1 200 OK\n"
-	send "Transfer-encoding: chunked\n"
-	send "\n"
+	send "HTTP/1.1 200 OK\r\n"
+	send "Transfer-encoding: chunked\r\n"
+	send "\r\n"
 	chunkedlen 20
 	chunkedlen 0
 
 	# Empty chunked response is not.
 	rxreq
 	expect req.url == "/2"
-	send "HTTP/1.1 200 OK\n"
-	send "Transfer-encoding: chunked\n"
-	send "\n"
+	send "HTTP/1.1 200 OK\r\n"
+	send "Transfer-encoding: chunked\r\n"
+	send "\r\n"
 	chunkedlen 0
 } -start
 

diff --git a/include/vct.h b/include/vct.h
@@ -90,14 +90,11 @@ vct_is(int x, uint16_t y)
 static inline int
 vct_iscrlf(const char* p, const char* end)
 {
+
 	assert(p <= end);
-	if (p == end)
+	if (p + 2 > end || p[0] != 0x0d || p[1] != 0x0a)
 		return (0);
-	if ((p[0] == 0x0d && (p+1 < end) && p[1] == 0x0a)) // CR LF
-		return (2);
-	if (p[0] == 0x0a) // LF
-		return (1);
-	return (0);
+	return (2);
 }
 
 /* NB: VCT always operate in ASCII, don't replace 0x0d with \r etc. */