Skip to content

Commit

Permalink
Fix buffer overflow in HTC_RxInit on workspace exhaustion
Browse files Browse the repository at this point in the history
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
  • Loading branch information
mbgrydeland committed Dec 5, 2016
1 parent 23ca54d commit 4bcc1c2
Show file tree
Hide file tree
Showing 4 changed files with 174 additions and 43 deletions.
33 changes: 20 additions & 13 deletions bin/varnishd/cache/cache_session.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand All @@ -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);
}
Expand All @@ -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);
Expand Down
36 changes: 22 additions & 14 deletions bin/varnishtest/tests/c00039.vtc
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ server s1 {
rxreq
expect req.url == "/2"
txresp -bodylen 5

rxreq
txresp -bodylen 5
} -start

varnish v1 \
Expand Down Expand Up @@ -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
36 changes: 20 additions & 16 deletions bin/varnishtest/tests/c00040.vtc
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Expand Down
112 changes: 112 additions & 0 deletions bin/varnishtest/tests/r01834.vtc
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 4bcc1c2

Please sign in to comment.