[master] 4bcc1c2 Fix buffer overflow in HTC_RxInit on workspace exhaustion

Martin Blix Grydeland martin at varnish-software.com
Mon Dec 5 13:02:05 CET 2016


commit 4bcc1c213b5140c4277595bcee3c15682d9d2b96
Author: Martin Blix Grydeland <martin at varnish-software.com>
Date:   Mon Nov 28 16:41:21 2016 +0100

    Fix buffer overflow in HTC_RxInit on workspace exhaustion
    
    HTC_RxInit could write a single '\0' NUL character outside of the
    workspace when its called and there is zero bytes left in the
    workspace. This would trigger the workspace canary causing subsequent
    assertion.
    
    Fix by releaving HTC_RxInit of adding the '\0' character.
    
    HTC_RxStuff now returns HTC_S_Overflow early if the available buffer
    space is zero. The '\0' character is inserted just before calling the
    completion check function.
    
    Also fix an off-by-one error on the http_{req|resp}_size calculations,
    where the maximum number of bytes accepted was one less than the
    paramter indicated. c00039.vtc and c00040.vtc has been edited to
    reflect that and to be more expressive about the sizes they generate.
    
    Fixes: #1834

diff --git a/bin/varnishd/cache/cache_session.c b/bin/varnishd/cache/cache_session.c
index e915712..63eb665 100644
--- a/bin/varnishd/cache/cache_session.c
+++ b/bin/varnishd/cache/cache_session.c
@@ -202,7 +202,6 @@ HTC_RxInit(struct http_conn *htc, struct ws *ws)
 		htc->pipeline_b = NULL;
 		htc->pipeline_e = NULL;
 	}
-	*htc->rxbuf_e = '\0';
 }
 
 void
@@ -241,18 +240,28 @@ HTC_RxStuff(struct http_conn *htc, htc_complete_f *func,
 	int i;
 
 	CHECK_OBJ_NOTNULL(htc, HTTP_CONN_MAGIC);
-
-	if (htc->ws->r - htc->rxbuf_b < maxbytes)
-		maxbytes = (htc->ws->r - htc->rxbuf_b);
+	AN(htc->ws->r);
+	AN(htc->rxbuf_b);
+	assert(htc->rxbuf_b <= htc->rxbuf_e);
 
 	AZ(isnan(tn));
 	if (t1 != NULL)
 		assert(isnan(*t1));
 
+	if (htc->rxbuf_e == htc->ws->r) {
+		/* Can't work with a zero size buffer */
+		WS_ReleaseP(htc->ws, htc->rxbuf_b);
+		return (HTC_S_OVERFLOW);
+	}
+	if ((htc->ws->r - htc->rxbuf_b) - 1L < maxbytes)
+		maxbytes = (htc->ws->r - htc->rxbuf_b) - 1L; /* Space for NUL */
+
 	while (1) {
 		now = VTIM_real();
 		AZ(htc->pipeline_b);
 		AZ(htc->pipeline_e);
+		assert(htc->rxbuf_e < htc->ws->r);
+		*htc->rxbuf_e = '\0';
 		hs = func(htc);
 		if (hs == HTC_S_OVERFLOW || hs == HTC_S_JUNK) {
 			WS_ReleaseP(htc->ws, htc->rxbuf_b);
@@ -271,18 +280,17 @@ HTC_RxStuff(struct http_conn *htc, htc_complete_f *func,
 			/* Working on it */
 			if (t1 != NULL && isnan(*t1))
 				*t1 = now;
-		} else if (hs == HTC_S_EMPTY) {
+		} else if (hs == HTC_S_EMPTY)
 			htc->rxbuf_e = htc->rxbuf_b;
-			*htc->rxbuf_e = '\0';
-		} else
+		else
 			WRONG("htc_status_e");
 
 		tmo = tn - now;
 		if (!isnan(ti) && ti < tn)
 			tmo = ti - now;
-		i = (htc->rxbuf_e - htc->rxbuf_b) + 1L;	/* space for NUL */
-		i = maxbytes - i;
-		if (i <= 0) {
+		i = maxbytes - (htc->rxbuf_e - htc->rxbuf_b);
+		assert(i >= 0);
+		if (i == 0) {
 			WS_ReleaseP(htc->ws, htc->rxbuf_b);
 			return (HTC_S_OVERFLOW);
 		}
@@ -292,10 +300,9 @@ HTC_RxStuff(struct http_conn *htc, htc_complete_f *func,
 		if (i == 0 || i == -1) {
 			WS_ReleaseP(htc->ws, htc->rxbuf_b);
 			return (HTC_S_EOF);
-		} else if (i > 0) {
+		} else if (i > 0)
 			htc->rxbuf_e += i;
-			*htc->rxbuf_e = '\0';
-		} else if (i == -2) {
+		else if (i == -2) {
 			if (hs == HTC_S_EMPTY && ti <= now) {
 				WS_ReleaseP(htc->ws, htc->rxbuf_b);
 				return (HTC_S_IDLE);
diff --git a/bin/varnishtest/tests/c00039.vtc b/bin/varnishtest/tests/c00039.vtc
index 6afb2eb..370234f 100644
--- a/bin/varnishtest/tests/c00039.vtc
+++ b/bin/varnishtest/tests/c00039.vtc
@@ -8,6 +8,9 @@ server s1 {
 	rxreq
 	expect req.url == "/2"
 	txresp -bodylen 5
+
+	rxreq
+	txresp -bodylen 5
 } -start
 
 varnish v1 \
@@ -36,22 +39,27 @@ client c1 {
 } -run
 
 client c1 {
-	txreq -url "/1" \
-		-hdr "1...5: ..0....5\n ..0....5....0....5....0" \
-		-hdr "1...5: ..0....5\n ..0....5....0....5....0" \
-		-hdr "1...5: ..0....5\n ..0....5....0....5....0" \
-		-hdr "1...5: ..0....5\n ..0....5....0....5....0" \
-		-hdr "1...5: ..0....5\n ..0....5....0....5....0" \
-		-hdr "1...5: ..0....5\n ..0...."
+	# Each line is 32 bytes. Total: 32 * 8 == 256
+	send "GET /....0....5....0. HTTP/1.1\r\n"
+	send "1...5: ..0....5....0....5....0\r\n"
+	send "1...5: ..0....5....0....5....0\r\n"
+	send "1...5: ..0....5....0....5....0\r\n"
+	send "1...5: ..0....5....0....5....0\r\n"
+	send "1...5: ..0....5....0....5....0\r\n"
+	send "1...5: ..0....5....0....5....0\r\n"
+	send "1...5: ..0....5....0....5...\r\n\r\n"
 	rxresp
 	expect resp.status == 200
 
-	txreq -url "/1" \
-		-hdr "1...5: ..0....5\n ..0....5....0....5....0" \
-		-hdr "1...5: ..0....5\n ..0....5....0....5....0" \
-		-hdr "1...5: ..0....5\n ..0....5....0....5....0" \
-		-hdr "1...5: ..0....5\n ..0....5....0....5....0" \
-		-hdr "1...5: ..0....5\n ..0....5....0....5....0" \
-		-hdr "1...5: ..0....5\n ..0....5"
+	# Each line is 32 except last, which is 33. Total: 32 * 7 + 33 == 257
+	send "GET /....0....5....0. HTTP/1.1\r\n"
+	send "1...5: ..0....5....0....5....0\r\n"
+	send "1...5: ..0....5....0....5....0\r\n"
+	send "1...5: ..0....5....0....5....0\r\n"
+	send "1...5: ..0....5....0....5....0\r\n"
+	send "1...5: ..0....5....0....5....0\r\n"
+	send "1...5: ..0....5....0....5....0\r\n"
+	send "1...5: ..0....5....0....5....0\r\n"
+	send "1...5: ..0....5....0....5....\r\n\r\n"
 	expect_close
 } -run
diff --git a/bin/varnishtest/tests/c00040.vtc b/bin/varnishtest/tests/c00040.vtc
index f1efc96..b0511c7 100644
--- a/bin/varnishtest/tests/c00040.vtc
+++ b/bin/varnishtest/tests/c00040.vtc
@@ -26,25 +26,29 @@ server s1 {
 	accept
 	rxreq
 	expect req.url == "/5"
-	txresp \
-		-hdr "1...5: ..0....5....0....5....0....5....0" \
-		-hdr "1...5: ..0....5....0....5....0....5....0" \
-		-hdr "1...5: ..0....5....0....5....0....5....0" \
-		-hdr "1...5: ..0....5....0....5....0....5....0" \
-		-hdr "1...5: ..0....5....0....5....0....5" \
-		-hdr "1...5: ..0" \
-		-bodylen 5
+	# Each line is 32 bytes. Total: 32 * 8 == 256
+	send "HTTP/1.1 200 OK....0....5....0\r\n"
+	send "Content-Length:              4\r\n"
+	send "1...5: ..0....5....0....5....0\r\n"
+	send "1...5: ..0....5....0....5....0\r\n"
+	send "1...5: ..0....5....0....5....0\r\n"
+	send "1...5: ..0....5....0....5....0\r\n"
+	send "1...5: ..0....5....0....5....0\r\n"
+	send "1...5: ..0....5....0....5...\r\n\r\n"
+	send "asdf"
 
 	rxreq
 	expect req.url == "/6"
-	txresp \
-		-hdr "1...5: ..0....5....0....5....0....5....0" \
-		-hdr "1...5: ..0....5....0....5....0....5....0" \
-		-hdr "1...5: ..0....5....0....5....0....5....0" \
-		-hdr "1...5: ..0....5....0....5....0....5....0" \
-		-hdr "1...5: ..0....5....0....5....0....5" \
-		-hdr "1...5: ..0." \
-		-bodylen 6
+	# Each line is 32 except last, which is 33. Total: 32 * 7 + 33 == 257
+	send "HTTP/1.1 200 OK....0....5....0\r\n"
+	send "Content-Length:              4\r\n"
+	send "1...5: ..0....5....0....5....0\r\n"
+	send "1...5: ..0....5....0....5....0\r\n"
+	send "1...5: ..0....5....0....5....0\r\n"
+	send "1...5: ..0....5....0....5....0\r\n"
+	send "1...5: ..0....5....0....5....0\r\n"
+	send "1...5: ..0....5....0....5....\r\n\r\n"
+	send "asdf"
 } -start
 
 varnish v1 \
diff --git a/bin/varnishtest/tests/r01834.vtc b/bin/varnishtest/tests/r01834.vtc
new file mode 100644
index 0000000..1937da1
--- /dev/null
+++ b/bin/varnishtest/tests/r01834.vtc
@@ -0,0 +1,112 @@
+varnishtest "#1834 - Buffer overflow in backend workspace"
+
+# This test case checks fetch side for buffer overflow when there is little
+# workspace left. If failing it would be because we tripped the canary
+# at the end of the workspace.
+
+server s1 -repeat 64 {
+	rxreq
+	txresp
+} -start
+
+varnish v1 -vcl+backend {
+	import debug;
+	import std;
+
+	sub vcl_recv {
+		return (pass);
+	}
+
+	sub vcl_backend_fetch {
+		debug.workspace_allocate(backend, debug.workspace_free(backend) - std.integer(bereq.http.WS, 256));
+	}
+} -start
+
+client c1 {
+	# Start with enough workspace to receive a good result
+	txreq -hdr "WS: 320"
+	rxresp
+	expect resp.status == 200
+
+	# Continue with decreasing workspaces by decrements of 8 (sizeof void*)
+	txreq -hdr "WS: 312"
+	rxresp
+	txreq -hdr "WS: 304"
+	rxresp
+	txreq -hdr "WS: 296"
+	rxresp
+	txreq -hdr "WS: 288"
+	rxresp
+	txreq -hdr "WS: 280"
+	rxresp
+	txreq -hdr "WS: 272"
+	rxresp
+	txreq -hdr "WS: 264"
+	rxresp
+	txreq -hdr "WS: 256"
+	rxresp
+	txreq -hdr "WS: 248"
+	rxresp
+	txreq -hdr "WS: 240"
+	rxresp
+	txreq -hdr "WS: 232"
+	rxresp
+	txreq -hdr "WS: 224"
+	rxresp
+	txreq -hdr "WS: 216"
+	rxresp
+	txreq -hdr "WS: 208"
+	rxresp
+	txreq -hdr "WS: 200"
+	rxresp
+	txreq -hdr "WS: 192"
+	rxresp
+	txreq -hdr "WS: 184"
+	rxresp
+	txreq -hdr "WS: 176"
+	rxresp
+	txreq -hdr "WS: 168"
+	rxresp
+	txreq -hdr "WS: 160"
+	rxresp
+	txreq -hdr "WS: 152"
+	rxresp
+	txreq -hdr "WS: 144"
+	rxresp
+	txreq -hdr "WS: 136"
+	rxresp
+	txreq -hdr "WS: 128"
+	rxresp
+	txreq -hdr "WS: 120"
+	rxresp
+	txreq -hdr "WS: 112"
+	rxresp
+	txreq -hdr "WS: 104"
+	rxresp
+	txreq -hdr "WS: 096"
+	rxresp
+	txreq -hdr "WS: 088"
+	rxresp
+	txreq -hdr "WS: 080"
+	rxresp
+	txreq -hdr "WS: 072"
+	rxresp
+	txreq -hdr "WS: 064"
+	rxresp
+	txreq -hdr "WS: 056"
+	rxresp
+	txreq -hdr "WS: 048"
+	rxresp
+	txreq -hdr "WS: 040"
+	rxresp
+	txreq -hdr "WS: 032"
+	rxresp
+	txreq -hdr "WS: 024"
+	rxresp
+	txreq -hdr "WS: 016"
+	rxresp
+	txreq -hdr "WS: 008"
+	rxresp
+	txreq -hdr "WS: 000"
+	rxresp
+} -run



More information about the varnish-commit mailing list