[master] 1b508d154 Add bounds-checking to vct_iscrlf and vct_skipcrlf
Martin Blix Grydeland
martin at varnish-software.com
Tue Sep 3 10:04:05 UTC 2019
commit 1b508d15475de456d77a412a54650b7259617b3b
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 ddc0f2510..2f75dfda0 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 b8b9de4a7..67d07c312 100644
--- a/bin/varnishtest/vtc_http.c
+++ b/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);
}
diff --git a/include/vct.h b/include/vct.h
index e9cf9be86..40425296a 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)
@@ -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));
+}
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