[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