From 9d61ea4d722549a984d912603902fccfac473824 Mon Sep 17 00:00:00 2001 From: Martin Blix Grydeland Date: Fri, 13 Mar 2015 15:23:15 +0100 Subject: [PATCH] Fail fetch on malformed Content-Length header Add a common content length parser that is being used by both client and backend side. Original patch by: fgs Fixes: #1691 --- bin/varnishd/cache/cache.h | 7 ++++--- bin/varnishd/cache/cache_http.c | 29 +++++++++++++++++++++++++++++ bin/varnishd/cache/cache_http1_fetch.c | 32 +++++--------------------------- bin/varnishd/cache/cache_http1_fsm.c | 20 ++++++++++---------- bin/varnishd/cache/cache_http1_proto.c | 5 +++-- bin/varnishd/cache/cache_rfc2616.c | 18 +++++++++++++++--- bin/varnishtest/tests/r01691.vtc | 21 +++++++++++++++++++++ 7 files changed, 87 insertions(+), 45 deletions(-) create mode 100644 bin/varnishtest/tests/r01691.vtc Index: varnish-4.0.3/bin/varnishd/cache/cache.h =================================================================== --- varnish-4.0.3.orig/bin/varnishd/cache/cache.h +++ varnish-4.0.3/bin/varnishd/cache/cache.h @@ -208,7 +208,7 @@ struct http { * */ -typedef ssize_t htc_read(struct http_conn *, void *, size_t); +typedef ssize_t htc_read(struct http_conn *, void *, ssize_t); struct http_conn { unsigned magic; @@ -560,7 +560,7 @@ struct busyobj { struct pool_task fetch_task; - char *h_content_length; + ssize_t content_length; #define BO_FLAG(l, r, w, d) unsigned l:1; #include "tbl/bo_flags.h" @@ -1014,6 +1014,7 @@ int http_GetHdrData(const struct http *h int http_GetHdrField(const struct http *hp, const char *hdr, const char *field, char **ptr); double http_GetHdrQ(const struct http *hp, const char *hdr, const char *field); +ssize_t http_GetContentLength(const struct http *hp); uint16_t http_GetStatus(const struct http *hp); void http_SetStatus(struct http *to, uint16_t status); const char *http_GetReq(const struct http *hp); @@ -1040,7 +1041,7 @@ void HTTP1_Init(struct http_conn *htc, s unsigned maxbytes, unsigned maxhdr); enum htc_status_e HTTP1_Reinit(struct http_conn *htc); enum htc_status_e HTTP1_Rx(struct http_conn *htc); -ssize_t HTTP1_Read(struct http_conn *htc, void *d, size_t len); +ssize_t HTTP1_Read(struct http_conn *htc, void *d, ssize_t len); enum htc_status_e HTTP1_Complete(struct http_conn *htc); uint16_t HTTP1_DissectRequest(struct req *); uint16_t HTTP1_DissectResponse(struct http *sp, const struct http_conn *htc); Index: varnish-4.0.3/bin/varnishd/cache/cache_http.c =================================================================== --- varnish-4.0.3.orig/bin/varnishd/cache/cache_http.c +++ varnish-4.0.3/bin/varnishd/cache/cache_http.c @@ -488,6 +488,35 @@ http_GetHdrField(const struct http *hp, return (i); } +/*--------------------------------------------------------------------*/ + +ssize_t +http_GetContentLength(const struct http *hp) +{ + ssize_t cl, cll; + char *b; + + CHECK_OBJ_NOTNULL(hp, HTTP_MAGIC); + + if (!http_GetHdr(hp, H_Content_Length, &b)) + return (-1); + cl = 0; + if (!vct_isdigit(*b)) + return (-2); + for (;vct_isdigit(*b); b++) { + cll = cl; + cl *= 10; + cl += *b - '0'; + if (cll != cl / 10) + return (-2); + } + while (vct_islws(*b)) + b++; + if (*b != '\0') + return (-2); + return (cl); +} + /*-------------------------------------------------------------------- * XXX: redo with http_GetHdrField() ? */ Index: varnish-4.0.3/bin/varnishd/cache/cache_http1_fetch.c =================================================================== --- varnish-4.0.3.orig/bin/varnishd/cache/cache_http1_fetch.c +++ varnish-4.0.3/bin/varnishd/cache/cache_http1_fetch.c @@ -43,29 +43,6 @@ #include "vtcp.h" #include "vtim.h" -/*-------------------------------------------------------------------- - * Convert a string to a size_t safely - */ - -static ssize_t -vbf_fetch_number(const char *nbr, int radix) -{ - uintmax_t cll; - ssize_t cl; - char *q; - - if (*nbr == '\0') - return (-1); - cll = strtoumax(nbr, &q, radix); - if (q == NULL || *q != '\0') - return (-1); - - cl = (ssize_t)cll; - if((uintmax_t)cl != cll) /* Protect against bogusly large values */ - return (-1); - return (cl); -} - /*--------------------------------------------------------------------*/ static enum vfp_status __match_proto__(vfp_pull_f) @@ -167,7 +144,6 @@ ssize_t V1F_Setup_Fetch(struct busyobj *bo) { struct http_conn *htc; - ssize_t cl; CHECK_OBJ_NOTNULL(bo, BUSYOBJ_MAGIC); htc = &bo->htc; @@ -176,13 +152,15 @@ V1F_Setup_Fetch(struct busyobj *bo) switch(htc->body_status) { case BS_EOF: + assert(bo->content_length == -1); VFP_Push(bo, v1f_pull_eof, 0); return(-1); case BS_LENGTH: - cl = vbf_fetch_number(bo->h_content_length, 10); - VFP_Push(bo, v1f_pull_straight, cl); - return (cl); + assert(bo->content_length > 0); + VFP_Push(bo, v1f_pull_straight, bo->content_length); + return (bo->content_length); case BS_CHUNKED: + assert(bo->content_length == -1); VFP_Push(bo, v1f_pull_chunked, -1); return (-1); default: Index: varnish-4.0.3/bin/varnishd/cache/cache_http1_fsm.c =================================================================== --- varnish-4.0.3.orig/bin/varnishd/cache/cache_http1_fsm.c +++ varnish-4.0.3/bin/varnishd/cache/cache_http1_fsm.c @@ -262,22 +262,22 @@ http1_cleanup(struct sess *sp, struct wo static enum req_body_state_e http1_req_body_status(struct req *req) { - char *ptr, *endp; + ssize_t cl; CHECK_OBJ_NOTNULL(req, REQ_MAGIC); - if (http_GetHdr(req->http, H_Content_Length, &ptr)) { - AN(ptr); - if (*ptr == '\0') - return (REQ_BODY_FAIL); - req->req_bodybytes = strtoul(ptr, &endp, 10); - if (*endp != '\0' && !vct_islws(*endp)) - return (REQ_BODY_FAIL); - if (req->req_bodybytes == 0) - return (REQ_BODY_NONE); + req->req_bodybytes = 0; + cl = http_GetContentLength(req->http); + if (cl == -2) + return (REQ_BODY_FAIL); + else if (cl == 0) + return (REQ_BODY_NONE); + else if (cl > 0) { + req->req_bodybytes = cl; req->h1.bytes_yet = req->req_bodybytes - req->h1.bytes_done; return (REQ_BODY_PRESENT); } + assert(cl == -1); /* No Content-Length header */ if (http_HdrIs(req->http, H_Transfer_Encoding, "chunked")) { req->chunk_ctr = -1; return (REQ_BODY_CHUNKED); Index: varnish-4.0.3/bin/varnishd/cache/cache_http1_proto.c =================================================================== --- varnish-4.0.3.orig/bin/varnishd/cache/cache_http1_proto.c +++ varnish-4.0.3/bin/varnishd/cache/cache_http1_proto.c @@ -191,14 +191,15 @@ HTTP1_Rx(struct http_conn *htc) * Read up to len bytes, returning pipelined data first. */ -ssize_t -HTTP1_Read(struct http_conn *htc, void *d, size_t len) +ssize_t __match_proto__(htc_read) +HTTP1_Read(struct http_conn *htc, void *d, ssize_t len) { size_t l; unsigned char *p; ssize_t i = 0; CHECK_OBJ_NOTNULL(htc, HTTP_CONN_MAGIC); + assert(len > 0); l = 0; p = d; if (htc->pipeline.b) { Index: varnish-4.0.3/bin/varnishd/cache/cache_rfc2616.c =================================================================== --- varnish-4.0.3.orig/bin/varnishd/cache/cache_rfc2616.c +++ varnish-4.0.3/bin/varnishd/cache/cache_rfc2616.c @@ -188,6 +188,7 @@ enum body_status RFC2616_Body(struct busyobj *bo, struct dstat *stats) { struct http *hp; + ssize_t cl; char *b; hp = bo->beresp; @@ -199,6 +200,8 @@ RFC2616_Body(struct busyobj *bo, struct else bo->should_close = 0; + bo->content_length = -1; + if (!strcasecmp(http_GetReq(bo->bereq), "head")) { /* * A HEAD request can never have a body in the reply, @@ -246,9 +249,18 @@ RFC2616_Body(struct busyobj *bo, struct return (BS_ERROR); } - if (http_GetHdr(hp, H_Content_Length, &bo->h_content_length)) { - stats->fetch_length++; - return (BS_LENGTH); + cl = http_GetContentLength(hp); + if (cl == -2) + return (BS_ERROR); + if (cl >= 0) { + bo->content_length = cl; + if (cl == 0) { + stats->fetch_zero++; + return (BS_NONE); + } else { + stats->fetch_length++; + return (BS_LENGTH); + } } if (http_HdrIs(hp, H_Connection, "keep-alive")) { Index: varnish-4.0.3/bin/varnishtest/tests/r01691.vtc =================================================================== --- /dev/null +++ varnish-4.0.3/bin/varnishtest/tests/r01691.vtc @@ -0,0 +1,21 @@ +varnishtest "Test bogus Content-Length header" + +server s1 { + rxreq + txresp -nolen -hdr "Content-Length: bogus" +} -start + +varnish v1 -vcl+backend { + +} -start + +logexpect l1 -v v1 { + expect * 1002 VCL_Error "Body cannot be fetched" +} -start + +client c1 { + txreq + rxresp +} -run + +logexpect l1 -wait