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/cache/cache_vrt_filter.c

@@ -34,12 +34,12 @@
 #include <stdint.h>
 #include <stdlib.h>
 
-#include "vct.h"
-
 #include "cache_varnishd.h"
 #include "cache_vcl.h"
 #include "vrt_obj.h"
 
+#include "vct.h"
+
 #include "cache_filter.h"
 
 /*--------------------------------------------------------------------

bin/varnishd/http1/cache_http1_proto.c

@@ -120,31 +120,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++ = ' ';
 		}
 
@@ -269,7 +269,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);
@@ -278,7 +278,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;
 

bin/varnishtest/vtc_http.c

@@ -428,20 +428,20 @@ http_splitheader(struct http *hp, int req)
 	hh[n++] = p;
 	while (!vct_islws(*p))
 		p++;
-	AZ(vct_iscrlf(p));
+	AZ(vct_iscrlf(p, hp->rx_e));
 	*p++ = '\0';
 
 	/* URL/STATUS */
 	while (vct_issp(*p))		/* XXX: H space only */
 		p++;
-	AZ(vct_iscrlf(p));
+	AZ(vct_iscrlf(p, hp->rx_e));
 	hh[n++] = p;
 	while (!vct_islws(*p))
 		p++;
-	if (vct_iscrlf(p)) {
+	if (vct_iscrlf(p, hp->rx_e)) {
 		hh[n++] = NULL;
 		q = p;
-		p += vct_skipcrlf(p);
+		p = vct_skipcrlf(p, hp->rx_e);
 		*q = '\0';
 	} else {
 		*p++ = '\0';
@@ -449,30 +449,29 @@ 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, hp->rx_e))
 			p++;
 		q = p;
-		p += vct_skipcrlf(p);
+		p = vct_skipcrlf(p, hp->rx_e);
 		*q = '\0';
 	}
 	assert(n == 3);
 
 	while (*p != '\0') {
 		assert(n < MAX_HDR);
-		if (vct_iscrlf(p))
+		if (vct_iscrlf(p, hp->rx_e))
 			break;
 		hh[n++] = p++;
-		while (*p != '\0' && !vct_iscrlf(p))
+		while (*p != '\0' && !vct_iscrlf(p, hp->rx_e))
 			p++;
 		if (*p == '\0') {
 			break;
 		}
 		q = p;
-		p += vct_skipcrlf(p);
+		p = vct_skipcrlf(p, hp->rx_e);
 		*q = '\0';
 	}
-	if (*p != '\0')
-		p += vct_skipcrlf(p);
+	p = vct_skipcrlf(p, hp->rx_e);
 	assert(*p == '\0');
 
 	for (n = 0; n < 3 || hh[n] != NULL; n++) {
@@ -568,7 +567,7 @@ http_rxchunk(struct http *hp)
 	old = hp->rx_p;
 	if (http_rxchar(hp, 2, 0) < 0)
 		return (-1);
-	if (!vct_iscrlf(old)) {
+	if (!vct_iscrlf(old, hp->rx_e)) {
 		vtc_log(hp->vl, hp->fatal, "Chunklen without CRLF");
 		return (-1);
 	}

include/vct.h

@@ -30,6 +30,8 @@
 
 /* from libvarnish/vct.c */
 
+#include "vas.h"
+
 #define VCT_SP			(1<<0)
 #define VCT_CRLF		(1<<1)
 #define VCT_LWS			(VCT_CRLF | VCT_SP)
@@ -78,7 +80,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));
+}

lib/libvarnish/vnum.c

@@ -38,9 +38,9 @@
 
 #include "vdef.h"
 
-#include "vct.h"
 #include "vnum.h"
 #include "vas.h"
+#include "vct.h"
 
 static const char err_miss_num[] = "Missing number";
 static const char err_invalid_num[] = "Invalid number";