From cbf8c2e019c9280eb493f8c5d26ef4d1eb830771 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Fri, 22 Oct 2021 15:58:46 -0700 Subject: [PATCH] statschannel doesn't handle multiple reads correctly if an incoming HTTP request is incomplete, but nothing else is clearly wrong with it, the stats channel continues reading to see if there's more coming. the buffer length was not being processed correctly in this case. also, the server state was not reset correctly when the request was complete, so that subsequent requests could be appended to the first buffer instead of being treated as new. in addition fixing the above problems, this commit also increases the size of the httpd request buffer from 1024 to 4096, because some browsers send a lot of headers. --- lib/isc/httpd.c | 92 +++++++++++++++++++++++++------------------------ 1 file changed, 47 insertions(+), 45 deletions(-) diff --git a/lib/isc/httpd.c b/lib/isc/httpd.c index b71d323823..c9f78ef331 100644 --- a/lib/isc/httpd.c +++ b/lib/isc/httpd.c @@ -38,7 +38,7 @@ } \ } while (0) -#define HTTP_RECVLEN 1024 +#define HTTP_RECVLEN 4096 #define HTTP_SENDGROW 1024 #define HTTP_SEND_MAXLEN 10240 @@ -117,7 +117,6 @@ struct isc_httpd { * the backing store. * The third buffer is compbuffer, managed by us, that contains the * compressed HTTP data, if compression is used. - * */ isc_buffer_t headerbuffer; isc_buffer_t compbuffer; @@ -172,7 +171,7 @@ static isc_result_t httpd_response(isc_httpd_t *); static isc_result_t -process_request(isc_httpd_t *, isc_region_t *); +process_request(isc_httpd_t *, isc_region_t *, size_t *); static isc_result_t grow_headerspace(isc_httpd_t *); @@ -196,7 +195,7 @@ free_buffer(isc_mem_t *mctx, isc_buffer_t *buffer) { isc_region_t r; isc_buffer_region(buffer, &r); - if (r.length > 0) { + if (r.base != NULL) { isc_mem_put(mctx, r.base, r.length); } } @@ -393,13 +392,20 @@ have_header(isc_httpd_t *httpd, const char *header, const char *value, } static isc_result_t -process_request(isc_httpd_t *httpd, isc_region_t *region) { - char *s = NULL, *p = NULL; +process_request(isc_httpd_t *httpd, isc_region_t *region, size_t *buflen) { + char *s = NULL, *p = NULL, *urlend = NULL; + size_t limit = sizeof(httpd->recvbuf) - httpd->recvlen - 1; + size_t len = region->length; int delim; - memmove(httpd->recvbuf + httpd->recvlen, region->base, region->length); - httpd->recvlen += region->length; + if (len > limit) { + len = limit; + } + + memmove(httpd->recvbuf + httpd->recvlen, region->base, len); + httpd->recvlen += len; httpd->recvbuf[httpd->recvlen] = 0; + *buflen = httpd->recvlen; httpd->headers = NULL; /* @@ -417,7 +423,7 @@ process_request(isc_httpd_t *httpd, isc_region_t *region) { } /* - * NUL terminate request at the blank line. + * NULL terminate the request at the blank line. */ s[delim] = 0; @@ -454,13 +460,13 @@ process_request(isc_httpd_t *httpd, isc_region_t *region) { if (!BUFLENOK(s)) { return (ISC_R_NOMEMORY); } - *s = 0; + urlend = s; /* * Make the URL relative. */ - if ((strncmp(p, "http:/", 6) == 0) || (strncmp(p, "https:/", 7) == 0)) { - /* Skip first / */ + if (strncmp(p, "http://", 7) == 0 || strncmp(p, "https://", 8) == 0) { + /* Skip first '/' */ while (*p != '/' && *p != 0) { p++; } @@ -468,7 +474,7 @@ process_request(isc_httpd_t *httpd, isc_region_t *region) { return (ISC_R_RANGE); } p++; - /* Skip second / */ + /* Skip second '/' */ while (*p != '/' && *p != 0) { p++; } @@ -476,7 +482,7 @@ process_request(isc_httpd_t *httpd, isc_region_t *region) { return (ISC_R_RANGE); } p++; - /* Find third / */ + /* Find third '/' */ while (*p != '/' && *p != 0) { p++; } @@ -491,7 +497,7 @@ process_request(isc_httpd_t *httpd, isc_region_t *region) { s = p; /* - * Now, see if there is a ? mark in the URL. If so, this is + * Now, see if there is a question mark in the URL. If so, this is * part of the query string, and we will split it from the URL. */ httpd->querystring = strchr(httpd->url, '?'); @@ -566,6 +572,16 @@ process_request(isc_httpd_t *httpd, isc_region_t *region) { return (ISC_R_RANGE); } + /* + * Looks like a a valid request, so now we know we won't have + * to process this buffer again. We can NULL-terminate the + * URL for the caller's benefit, and set recvlen to 0 so + * the next read will overwrite this one instead of appending + * to the buffer. + */ + *urlend = 0; + httpd->recvlen = 0; + return (ISC_R_SUCCESS); } @@ -596,24 +612,24 @@ httpd_reset(void *arg) { isc_buffer_clear(&httpd->headerbuffer); isc_buffer_clear(&httpd->compbuffer); isc_buffer_invalidate(&httpd->bodybuffer); - - httpdmgr_detach(&httpdmgr); } static void httpd_put(void *arg) { isc_httpd_t *httpd = (isc_httpd_t *)arg; - isc_httpdmgr_t *httpdmgr = NULL; + isc_httpdmgr_t *mgr = NULL; REQUIRE(VALID_HTTPD(httpd)); - httpdmgr = httpd->mgr; - REQUIRE(VALID_HTTPDMGR(httpdmgr)); + mgr = httpd->mgr; + REQUIRE(VALID_HTTPDMGR(mgr)); httpd->magic = 0; + httpd->mgr = NULL; - free_buffer(httpdmgr->mctx, &httpd->headerbuffer); - free_buffer(httpdmgr->mctx, &httpd->compbuffer); + free_buffer(mgr->mctx, &httpd->headerbuffer); + free_buffer(mgr->mctx, &httpd->compbuffer); + httpdmgr_detach(&mgr); #if ENABLE_AFL if (finishhook != NULL) { @@ -633,6 +649,7 @@ new_httpd(isc_httpdmgr_t *httpdmgr, isc_nmhandle_t *handle) { if (httpd == NULL) { httpd = isc_nmhandle_getextra(handle); *httpd = (isc_httpd_t){ .handle = NULL }; + httpdmgr_attach(httpdmgr, &httpd->mgr); } if (httpd->handle == NULL) { @@ -642,8 +659,6 @@ new_httpd(isc_httpdmgr_t *httpdmgr, isc_nmhandle_t *handle) { INSIST(httpd->handle == handle); } - httpdmgr_attach(httpdmgr, &httpd->mgr); - /* * Initialize the buffer for our headers. */ @@ -749,22 +764,15 @@ render_500(const char *url, isc_httpdurl_t *urlinfo, const char *querystring, /*%< * Reallocates compbuffer to size, does nothing if compbuffer is already * larger than size. - * - * Requires: - *\li httpd a valid isc_httpd_t object - * - * Returns: - *\li #ISC_R_SUCCESS -- all is well. - *\li #ISC_R_NOMEMORY -- not enough memory to extend buffer */ -static isc_result_t +static void alloc_compspace(isc_httpd_t *httpd, unsigned int size) { - char *newspace; + char *newspace = NULL; isc_region_t r; isc_buffer_region(&httpd->compbuffer, &r); if (size < r.length) { - return (ISC_R_SUCCESS); + return; } newspace = isc_mem_get(httpd->mgr->mctx, size); @@ -773,8 +781,6 @@ alloc_compspace(isc_httpd_t *httpd, unsigned int size) { if (r.base != NULL) { isc_mem_put(httpd->mgr->mctx, r.base, r.length); } - - return (ISC_R_SUCCESS); } /*%< @@ -794,15 +800,11 @@ static isc_result_t httpd_compress(isc_httpd_t *httpd) { z_stream zstr; isc_region_t r; - isc_result_t result; int ret; int inputlen; inputlen = isc_buffer_usedlength(&httpd->bodybuffer); - result = alloc_compspace(httpd, inputlen); - if (result != ISC_R_SUCCESS) { - return (result); - } + alloc_compspace(httpd, inputlen); isc_buffer_region(&httpd->compbuffer, &r); /* @@ -842,6 +844,7 @@ httpd_request(isc_nmhandle_t *handle, isc_result_t eresult, isc_region_t r; bool is_compressed = false; char datebuf[ISC_FORMATHTTPTIMESTAMP_SIZE]; + size_t buflen = 0; httpd = isc_nmhandle_getdata(handle); @@ -851,10 +854,10 @@ httpd_request(isc_nmhandle_t *handle, isc_result_t eresult, goto cleanup_readhandle; } - result = process_request(httpd, region); + result = process_request(httpd, region, &buflen); if (result == ISC_R_NOTFOUND) { - if (httpd->recvlen < HTTP_RECVLEN - 1) { - /* don't unref, continue reading */ + if (buflen < HTTP_RECVLEN - 1) { + /* don't unref, keep reading */ return; } goto cleanup_readhandle; @@ -960,7 +963,6 @@ httpd_request(isc_nmhandle_t *handle, isc_result_t eresult, isc_nmhandle_attach(handle, &httpd->sendhandle); isc_nm_send(handle, &r, httpd_senddone, httpd); - return; cleanup_readhandle: isc_nmhandle_detach(&httpd->readhandle); }