[6.2] 1cb778f6f Add bounds-checking to vct_iscrlf and vct_skipcrlf

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


commit 1cb778f6f69737109e8c070a74b8e95b78f46d13
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/cache/cache_vrt_filter.c b/bin/varnishd/cache/cache_vrt_filter.c
index 5e83a89a7..271be6ccd 100644
--- a/bin/varnishd/cache/cache_vrt_filter.c
+++ b/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"
 
 /*--------------------------------------------------------------------
diff --git a/bin/varnishd/http1/cache_http1_proto.c b/bin/varnishd/http1/cache_http1_proto.c
index 15090bdcf..c64a56853 100644
--- a/bin/varnishd/http1/cache_http1_proto.c
+++ b/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;
 
diff --git a/bin/varnishtest/vtc_http.c b/bin/varnishtest/vtc_http.c
index 0c772f50d..36c8964c6 100644
--- a/bin/varnishtest/vtc_http.c
+++ b/bin/varnishtest/vtc_http.c
@@ -431,20 +431,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';
@@ -452,26 +452,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, 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++;
 		q = p;
-		p += vct_skipcrlf(p);
+		p = vct_skipcrlf(p, hp->rx_e);
 		*q = '\0';
 	}
-	p += vct_skipcrlf(p);
+	p = vct_skipcrlf(p, hp->rx_e);
 	assert(*p == '\0');
 
 	for (n = 0; n < 3 || hh[n] != NULL; n++) {
@@ -567,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);
 	}
diff --git a/include/vct.h b/include/vct.h
index 24143a332..5a09020a0 100644
--- a/include/vct.h
+++ b/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)
@@ -76,7 +78,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/lib/libvarnish/vnum.c b/lib/libvarnish/vnum.c
index 13bad74d1..7533158ac 100644
--- a/lib/libvarnish/vnum.c
+++ b/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";


More information about the varnish-commit mailing list