[master] 319497d Inspired by yesterdays code reading, polish up the HTTP protocol parser a little bit, and nail a couple of corner cases.
Poul-Henning Kamp
phk at varnish-cache.org
Thu Oct 31 10:19:23 CET 2013
commit 319497d64c9c5e75bd8c1c3bef4c2ebffe8b02bd
Author: Poul-Henning Kamp <phk at FreeBSD.org>
Date: Thu Oct 31 09:18:49 2013 +0000
Inspired by yesterdays code reading, polish up the HTTP protocol
parser a little bit, and nail a couple of corner cases.
diff --git a/bin/varnishd/cache/cache_http1_proto.c b/bin/varnishd/cache/cache_http1_proto.c
index 583d2e1..db0a1c5 100644
--- a/bin/varnishd/cache/cache_http1_proto.c
+++ b/bin/varnishd/cache/cache_http1_proto.c
@@ -37,6 +37,9 @@
* and stops when we see the magic marker (double [CR]NL), and if we overshoot,
* it keeps track of the "pipelined" data.
*
+ * Until we see the magic marker, we have to keep the rxbuf NUL terminated
+ * because we use strchr(3) on it.
+ *
* We use this both for client and backend connections.
*/
@@ -97,17 +100,17 @@ HTTP1_Reinit(struct http_conn *htc)
/*--------------------------------------------------------------------
* Check if we have a complete HTTP request or response yet
- *
*/
enum htc_status_e
HTTP1_Complete(struct http_conn *htc)
{
- int i;
- const char *p;
+ char *p;
txt *t;
CHECK_OBJ_NOTNULL(htc, HTTP_CONN_MAGIC);
+ AZ(htc->pipeline.b);
+ AZ(htc->pipeline.e);
t = &htc->rxbuf;
Tcheck(*t);
@@ -133,14 +136,11 @@ HTTP1_Complete(struct http_conn *htc)
break;
}
p++;
- i = p - t->b;
- WS_ReleaseP(htc->ws, htc->rxbuf.e);
- AZ(htc->pipeline.b);
- AZ(htc->pipeline.e);
- if (htc->rxbuf.b + i < htc->rxbuf.e) {
- htc->pipeline.b = htc->rxbuf.b + i;
- htc->pipeline.e = htc->rxbuf.e;
- htc->rxbuf.e = htc->pipeline.b;
+ WS_ReleaseP(htc->ws, t->e);
+ if (p < t->e) {
+ htc->pipeline.b = p;
+ htc->pipeline.e = t->e;
+ t->e = p;
}
return (HTTP1_COMPLETE);
}
@@ -156,6 +156,8 @@ HTTP1_Rx(struct http_conn *htc)
CHECK_OBJ_NOTNULL(htc, HTTP_CONN_MAGIC);
AN(htc->ws->r);
+ AZ(htc->pipeline.b);
+ AZ(htc->pipeline.e);
i = (htc->ws->r - htc->rxbuf.e) - 1; /* space for NUL */
if (i <= 0) {
WS_ReleaseP(htc->ws, htc->rxbuf.b);
@@ -221,9 +223,6 @@ htc_dissect_hdrs(struct http *hp, char *p, const struct http_conn *htc)
char *q, *r;
txt t = htc->rxbuf;
- if (*p == '\r')
- p++;
-
hp->nhd = HTTP_HDR_FIRST;
hp->conds = 0;
r = NULL; /* For FlexeLint */
@@ -232,7 +231,7 @@ htc_dissect_hdrs(struct http *hp, char *p, const struct http_conn *htc)
/* Find end of next header */
q = r = p;
while (r < t.e) {
- if (!vct_iscrlf(*r)) {
+ if (!vct_iscrlf(r)) {
r++;
continue;
}
@@ -260,6 +259,13 @@ htc_dissect_hdrs(struct http *hp, char *p, const struct http_conn *htc)
if (p == q)
break;
+ if (vct_islws(*p)) {
+ VSLb(hp->vsl, SLT_BogoHeader,
+ "1st header has white space: %.*s",
+ (int)(q - p > 20 ? 20 : q - p), p);
+ return (400);
+ }
+
if ((p[0] == 'i' || p[0] == 'I') &&
(p[1] == 'f' || p[1] == 'F') &&
p[2] == '-')
@@ -291,9 +297,13 @@ htc_dissect_hdrs(struct http *hp, char *p, const struct http_conn *htc)
static uint16_t
htc_splitline(struct http *hp, const struct http_conn *htc, int req)
{
- char *p, *q;
+ char *p;
int h1, h2, h3;
+ CHECK_OBJ_NOTNULL(htc, HTTP_CONN_MAGIC);
+ CHECK_OBJ_NOTNULL(hp, HTTP_MAGIC);
+ Tcheck(htc->rxbuf);
+
if (req) {
h1 = HTTP_HDR_METHOD;
h2 = HTTP_HDR_URL;
@@ -304,38 +314,31 @@ htc_splitline(struct http *hp, const struct http_conn *htc, int req)
h3 = HTTP_HDR_RESPONSE;
}
- CHECK_OBJ_NOTNULL(htc, HTTP_CONN_MAGIC);
- CHECK_OBJ_NOTNULL(hp, HTTP_MAGIC);
-
- /* XXX: Assert a NUL at rx.e ? */
- Tcheck(htc->rxbuf);
-
/* Skip leading LWS */
for (p = htc->rxbuf.b ; vct_islws(*p); p++)
continue;
+ hp->hd[h1].b = p;
- /* First field cannot contain SP, CRLF or CTL */
- q = p;
+ /* First field cannot contain SP or CTL */
for (; !vct_issp(*p); p++) {
if (vct_isctl(*p))
return (400);
}
- hp->hd[h1].b = q;
hp->hd[h1].e = p;
+ assert(Tlen(hp->hd[h1]));
/* Skip SP */
for (; vct_issp(*p); p++) {
if (vct_isctl(*p))
return (400);
}
+ hp->hd[h2].b = p;
/* Second field cannot contain LWS or CTL */
- q = p;
for (; !vct_islws(*p); p++) {
if (vct_isctl(*p))
return (400);
}
- hp->hd[h2].b = q;
hp->hd[h2].e = p;
if (!Tlen(hp->hd[h2]))
@@ -346,15 +349,15 @@ htc_splitline(struct http *hp, const struct http_conn *htc, int req)
if (vct_isctl(*p))
return (400);
}
+ hp->hd[h3].b = p;
- /* Third field is optional and cannot contain CTL */
- q = p;
- if (!vct_iscrlf(*p)) {
- for (; !vct_iscrlf(*p); p++)
- if (!vct_issep(*p) && vct_isctl(*p))
- return (400);
+ /* Third field is optional and cannot contain CTL except TAB */
+ for (; !vct_iscrlf(p); p++) {
+ if (vct_isctl(*p) && !vct_issp(*p)) {
+ hp->hd[h3].b = NULL;
+ return (400);
+ }
}
- hp->hd[h3].b = q;
hp->hd[h3].e = p;
/* Skip CRLF */
diff --git a/bin/varnishtest/tests/b00040.vtc b/bin/varnishtest/tests/b00040.vtc
new file mode 100644
index 0000000..7ce7615
--- /dev/null
+++ b/bin/varnishtest/tests/b00040.vtc
@@ -0,0 +1,43 @@
+varnishtest "test certain mailformed reqests"
+
+server s1 {
+ rxreq
+ # expect req.url == /3
+ txresp
+} -start
+
+varnish v1 -vcl+backend { } -start
+
+client c1 {
+ send "GET /1 HTTP/1.1\r\n"
+ send " Host: foo\r\n"
+ send "\r\n"
+ rxresp
+ expect resp.status == 400
+} -run
+delay .1
+
+client c1 {
+ send "GET /2 HTTP/1.1\r\n"
+ send " Host: foo\r\n"
+ send "\r\n"
+ rxresp
+ expect resp.status == 400
+} -run
+delay .1
+
+client c1 {
+ send "GET /3 HTTP/1.1\r\n"
+ send "\rHost: foo\r\n"
+ send "\r\n"
+ rxresp
+ expect resp.status == 400
+} -run
+delay .1
+
+client c1 {
+ send "GET /4 HTTP/1.1\r\n"
+ send "\r\n"
+ rxresp
+ expect resp.status == 200
+} -run
diff --git a/bin/varnishtest/vtc_http.c b/bin/varnishtest/vtc_http.c
index dcfe023..5686ff0 100644
--- a/bin/varnishtest/vtc_http.c
+++ b/bin/varnishtest/vtc_http.c
@@ -294,17 +294,17 @@ http_splitheader(struct http *hp, int req)
hh[n++] = p;
while (!vct_islws(*p))
p++;
- assert(!vct_iscrlf(*p));
+ assert(!vct_iscrlf(p));
*p++ = '\0';
/* URL/STATUS */
while (vct_issp(*p)) /* XXX: H space only */
p++;
- assert(!vct_iscrlf(*p));
+ assert(!vct_iscrlf(p));
hh[n++] = p;
while (!vct_islws(*p))
p++;
- if (vct_iscrlf(*p)) {
+ if (vct_iscrlf(p)) {
hh[n++] = NULL;
q = p;
p += vct_skipcrlf(p);
@@ -315,7 +315,7 @@ 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))
p++;
q = p;
p += vct_skipcrlf(p);
@@ -325,10 +325,10 @@ http_splitheader(struct http *hp, int req)
while (*p != '\0') {
assert(n < MAX_HDR);
- if (vct_iscrlf(*p))
+ if (vct_iscrlf(p))
break;
hh[n++] = p++;
- while (*p != '\0' && !vct_iscrlf(*p))
+ while (*p != '\0' && !vct_iscrlf(p))
p++;
q = p;
p += vct_skipcrlf(p);
@@ -419,11 +419,11 @@ http_rxchunk(struct http *hp)
}
l = hp->prxbuf;
(void)http_rxchar(hp, 2, 0);
- if(!vct_iscrlf(hp->rxbuf[l]))
+ if(!vct_iscrlf(hp->rxbuf + l))
vtc_log(hp->vl, hp->fatal,
"Wrong chunk tail[0] = %02x",
hp->rxbuf[l] & 0xff);
- if(!vct_iscrlf(hp->rxbuf[l + 1]))
+ if(!vct_iscrlf(hp->rxbuf + l + 1))
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 4f9e2d4..3c41dd7 100644
--- a/include/vct.h
+++ b/include/vct.h
@@ -53,7 +53,6 @@ vct_is(int x, uint16_t y)
#define vct_issp(x) vct_is(x, VCT_SP)
#define vct_ishex(x) vct_is(x, VCT_HEX)
-#define vct_iscrlf(x) vct_is(x, VCT_CRLF)
#define vct_islws(x) vct_is(x, VCT_LWS)
#define vct_isctl(x) vct_is(x, VCT_CTL)
#define vct_isdigit(x) vct_is(x, VCT_DIGIT)
@@ -63,5 +62,7 @@ vct_is(int x, uint16_t y)
#define vct_isxmlnamestart(x) vct_is(x, VCT_XMLNAMESTART)
#define vct_isxmlname(x) vct_is(x, VCT_XMLNAMESTART | VCT_XMLNAME)
+#define vct_iscrlf(p) (((p)[0] == '\r' && (p)[1] == '\n') || (p)[0] == '\n')
+
/* 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)
+#define vct_skipcrlf(p) ((p)[0] == 0x0d && (p)[1] == 0x0a ? 2 : 1)
More information about the varnish-commit
mailing list