From 177e17c8f129c58daeeb98055761ee65ab5c3dfc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alf-Andr=C3=A9=20Walla?= <fwsgonzo@hotmail.com>
Date: Tue, 13 Aug 2019 12:52:39 +0200
Subject: [PATCH] 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.
---
 bin/varnishd/http1/cache_http1_proto.c | 16 +++++++++-------
 bin/varnishtest/vtc_http.c             | 26 ++++++++++++++------------
 include/vct.h                          | 19 +++++++++++++++++--
 bin/varnishtest/vtc_subr.c             | 3  ++-
 4 files changed, 42 insertions(+), 22 deletions(-)

diff --git a/bin/varnishd/http1/cache_http1_proto.c b/bin/varnishd/http1/cache_http1_proto.c
index dd81863d32..5d99da47a8 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 616cb459e1..e17643f8eb 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 24143a3322..1b7ffbd4f5 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));
+}
diff --git a/bin/varnishtest/vtc_subr.c b/bin/varnishtest/vtc_subr.c
index 2c1439a..f200981 100644
--- a/bin/varnishtest/vtc_subr.c
+++ b/bin/varnishtest/vtc_subr.c
@@ -33,10 +33,11 @@
 #include <string.h>
 #include <stdint.h>
 
+#include "vtc.h"
+
 #include "vct.h"
 #include "vnum.h"
 #include "vre.h"
-#include "vtc.h"
 
 struct vsb *
 vtc_hex_to_bin(struct vtclog *vl, const char *arg)
