[6.0] 177e17c8f Add bounds-checking to vct_iscrlf and vct_skipcrlf

Martin Blix Grydeland martin at varnish-software.com
Tue Sep 3 10:05:06 UTC 2019


commit 177e17c8f129c58daeeb98055761ee65ab5c3dfc
Author: Alf-André Walla <fwsgonzo at hotmail.com>
Date:   Tue Aug 13 12:52:39 2019 +0200

    Add bounds-checking to vct_iscrlf and vct_skipcrlf
    
    The macros vct_iscrlf() and vct_skipcrlf() may look at one or two bytes
    after its pointer value, causing OOB reads. This would allow
    http1_dissect_hdrs to wrongly see a CRLF when one wasn't there (the last
    LF left over in the bufer from the previous request).
    
    Change the macros to inline functions, and harden them by always sending
    the end pointer so that they can't overflow.
    
    vct_iscrlf() will return an int value of 0 for no [CR]LF, 1 for LF and 2
    for CRLF.
    
    vct_skipcrlf() will return the pointer having been skipped 0, 1 or 2
    bytes.

diff --git a/bin/varnishd/http1/cache_http1_proto.c b/bin/varnishd/http1/cache_http1_proto.c
index dd81863d3..5d99da47a 100644
--- a/bin/varnishd/http1/cache_http1_proto.c
+++ b/bin/varnishd/http1/cache_http1_proto.c
@@ -121,31 +121,31 @@ http1_dissect_hdrs(struct http *hp, char *p, struct http_conn *htc,
 
 		/* Find end of next header */
 		q = r = p;
-		if (vct_iscrlf(p))
+		if (vct_iscrlf(p, htc->rxbuf_e))
 			break;
 		while (r < htc->rxbuf_e) {
 			if (!vct_isctl(*r) || vct_issp(*r)) {
 				r++;
 				continue;
 			}
-			if (!vct_iscrlf(r)) {
+			if (!vct_iscrlf(r, htc->rxbuf_e)) {
 				VSLb(hp->vsl, SLT_BogoHeader,
 				    "Header has ctrl char 0x%02x", *r);
 				return (400);
 			}
 			q = r;
 			assert(r < htc->rxbuf_e);
-			r += vct_skipcrlf(r);
+			r = vct_skipcrlf(r, htc->rxbuf_e);
 			if (r >= htc->rxbuf_e)
 				break;
-			if (vct_iscrlf(r))
+			if (vct_iscrlf(r, htc->rxbuf_e))
 				break;
 			/* If line does not continue: got it. */
 			if (!vct_issp(*r))
 				break;
 
 			/* Clear line continuation LWS to spaces */
-			while (vct_islws(*q))
+			while (q < htc->rxbuf_e && vct_islws(*q))
 				*q++ = ' ';
 		}
 
@@ -275,7 +275,7 @@ http1_splitline(struct http *hp, struct http_conn *htc, const int *hf,
 	hp->hd[hf[2]].b = p;
 
 	/* Third field is optional and cannot contain CTL except TAB */
-	for (; !vct_iscrlf(p); p++) {
+	for (; p < htc->rxbuf_e && !vct_iscrlf(p, htc->rxbuf_e); p++) {
 		if (vct_isctl(*p) && !vct_issp(*p)) {
 			hp->hd[hf[2]].b = NULL;
 			return (400);
@@ -284,7 +284,9 @@ http1_splitline(struct http *hp, struct http_conn *htc, const int *hf,
 	hp->hd[hf[2]].e = p;
 
 	/* Skip CRLF */
-	i = vct_skipcrlf(p);
+	i = vct_iscrlf(p, htc->rxbuf_e);
+	if (!i)
+		return (400);
 	*p = '\0';
 	p += i;
 
diff --git a/bin/varnishtest/vtc_http.c b/bin/varnishtest/vtc_http.c
index 616cb459e..e17643f8e 100644
--- a/bin/varnishtest/vtc_http.c
+++ b/bin/varnishtest/vtc_http.c
@@ -409,6 +409,7 @@ http_splitheader(struct http *hp, int req)
 	char *p, *q, **hh;
 	int n;
 	char buf[20];
+	const char* rxbuf_e = &hp->rxbuf[hp->prxbuf];
 
 	CHECK_OBJ_NOTNULL(hp, HTTP_MAGIC);
 	if (req) {
@@ -428,20 +429,20 @@ http_splitheader(struct http *hp, int req)
 	hh[n++] = p;
 	while (!vct_islws(*p))
 		p++;
-	AZ(vct_iscrlf(p));
+	AZ(vct_iscrlf(p, rxbuf_e));
 	*p++ = '\0';
 
 	/* URL/STATUS */
 	while (vct_issp(*p))		/* XXX: H space only */
 		p++;
-	AZ(vct_iscrlf(p));
+	AZ(vct_iscrlf(p, rxbuf_e));
 	hh[n++] = p;
 	while (!vct_islws(*p))
 		p++;
-	if (vct_iscrlf(p)) {
+	if (vct_iscrlf(p, rxbuf_e)) {
 		hh[n++] = NULL;
 		q = p;
-		p += vct_skipcrlf(p);
+		p = vct_skipcrlf(p, rxbuf_e);
 		*q = '\0';
 	} else {
 		*p++ = '\0';
@@ -449,26 +450,26 @@ http_splitheader(struct http *hp, int req)
 		while (vct_issp(*p))		/* XXX: H space only */
 			p++;
 		hh[n++] = p;
-		while (!vct_iscrlf(p))
+		while (!vct_iscrlf(p, rxbuf_e))
 			p++;
 		q = p;
-		p += vct_skipcrlf(p);
+		p = vct_skipcrlf(p, rxbuf_e);
 		*q = '\0';
 	}
 	assert(n == 3);
 
 	while (*p != '\0') {
 		assert(n < MAX_HDR);
-		if (vct_iscrlf(p))
+		if (vct_iscrlf(p, rxbuf_e))
 			break;
 		hh[n++] = p++;
-		while (*p != '\0' && !vct_iscrlf(p))
+		while (*p != '\0' && !vct_iscrlf(p, rxbuf_e))
 			p++;
 		q = p;
-		p += vct_skipcrlf(p);
+		p = vct_skipcrlf(p, rxbuf_e);
 		*q = '\0';
 	}
-	p += vct_skipcrlf(p);
+	p = vct_skipcrlf(p, rxbuf_e);
 	assert(*p == '\0');
 
 	for (n = 0; n < 3 || hh[n] != NULL; n++) {
@@ -564,15 +565,16 @@ http_rxchunk(struct http *hp)
 		vtc_dump(hp->vl, 4, "chunk", hp->rxbuf + l, i);
 	}
 	l = hp->prxbuf;
+
 	if (http_rxchar(hp, 2, 0) < 0)
 		return (-1);
-	if (!vct_iscrlf(hp->rxbuf + l)) {
+	if (!vct_iscrlf(&hp->rxbuf[l], &hp->rxbuf[hp->prxbuf])) {
 		vtc_log(hp->vl, hp->fatal,
 		    "Wrong chunk tail[0] = %02x",
 		    hp->rxbuf[l] & 0xff);
 		return (-1);
 	}
-	if (!vct_iscrlf(hp->rxbuf + l + 1)) {
+	if (!vct_iscrlf(&hp->rxbuf[l + 1], &hp->rxbuf[hp->prxbuf])) {
 		vtc_log(hp->vl, hp->fatal,
 		    "Wrong chunk tail[1] = %02x",
 		    hp->rxbuf[l + 1] & 0xff);
diff --git a/include/vct.h b/include/vct.h
index 24143a332..1b7ffbd4f 100644
--- a/include/vct.h
+++ b/include/vct.h
@@ -76,7 +76,22 @@ vct_is(int x, uint16_t y)
 #define vct_isxmlname(x) vct_is(x, VCT_XMLNAMESTART | VCT_XMLNAME)
 #define vct_istchar(x) vct_is(x, VCT_ALPHA | VCT_DIGIT | VCT_TCHAR)
 
-#define vct_iscrlf(p) (((p)[0] == 0x0d && (p)[1] == 0x0a) || (p)[0] == 0x0a)
+static inline int
+vct_iscrlf(const char* p, const char* end)
+{
+	assert(p <= end);
+	if (p == end)
+		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);
+}
 
 /* NB: VCT always operate in ASCII, don't replace 0x0d with \r etc. */
-#define vct_skipcrlf(p) ((p)[0] == 0x0d && (p)[1] == 0x0a ? 2 : 1)
+static inline char*
+vct_skipcrlf(char* p, const char* end)
+{
+	return (p + vct_iscrlf(p, end));
+}


More information about the varnish-commit mailing list