From 86b8e621061bbea9d6854df6624f4436e40f6751 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Wed, 20 Jul 2022 10:06:56 +0000 Subject: [PATCH 1/5] Enhance the have_header() function to find the HTTP header's value Add a new `const char **fvalue` parameter to the httpd.c:have_header() function which, when set, will point to the found header's value. --- lib/isc/httpd.c | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/lib/isc/httpd.c b/lib/isc/httpd.c index 278683121f..368b9cd2e1 100644 --- a/lib/isc/httpd.c +++ b/lib/isc/httpd.c @@ -319,10 +319,12 @@ destroy_httpdmgr(isc_httpdmgr_t *httpdmgr) { /* * Look for the given header in headers. * If value is specified look for it terminated with a character in eov. + * If fvalue is specified and the header was found, then *fvalue will point to + * the found header's value. */ static bool have_header(isc_httpd_t *httpd, const char *header, const char *value, - const char *eov) { + const char *eov, const char **fvalue) { char *cr, *nl, *h; size_t hlen, vlen = 0; @@ -356,10 +358,6 @@ have_header(isc_httpd_t *httpd, const char *header, const char *value, continue; } - if (value == NULL) { - return (true); - } - /* * Skip optional leading white space. */ @@ -367,6 +365,18 @@ have_header(isc_httpd_t *httpd, const char *header, const char *value, while (*h == ' ' || *h == '\t') { h++; } + + /* + * Set the found value. + */ + if (fvalue != NULL) { + *fvalue = h; + } + + if (value == NULL) { + return (true); + } + /* * Terminate token search on NULL or EOL. */ @@ -556,17 +566,17 @@ process_request(isc_httpd_t *httpd, isc_region_t *region, size_t *buflen) { httpd->headers = s; - if (have_header(httpd, "Connection:", "close", ", \t\r\n")) { + if (have_header(httpd, "Connection:", "close", ", \t\r\n", NULL)) { httpd->flags |= HTTPD_CLOSE; } - if (have_header(httpd, "Host:", NULL, NULL)) { + if (have_header(httpd, "Host:", NULL, NULL, NULL)) { httpd->flags |= HTTPD_FOUNDHOST; } if (strncmp(httpd->protocol, "HTTP/1.0", 8) == 0) { - if (have_header(httpd, "Connection:", "Keep-Alive", ", \t\r\n")) - { + if (have_header(httpd, "Connection:", "Keep-Alive", ", \t\r\n", + NULL)) { httpd->flags |= HTTPD_KEEPALIVE; } else { httpd->flags |= HTTPD_CLOSE; @@ -577,7 +587,8 @@ process_request(isc_httpd_t *httpd, isc_region_t *region, size_t *buflen) { * Check for Accept-Encoding: */ #ifdef HAVE_ZLIB - if (have_header(httpd, "Accept-Encoding:", "deflate", ";, \t\r\n")) { + if (have_header(httpd, "Accept-Encoding:", "deflate", ";, \t\r\n", + NULL)) { httpd->flags |= HTTPD_ACCEPT_DEFLATE; } #endif /* ifdef HAVE_ZLIB */ From 8c4cdd9b21e07f17d6200f0bc883c29cc39be333 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Wed, 20 Jul 2022 10:18:56 +0000 Subject: [PATCH 2/5] Fix statistics channel multiple request processing with non-empty bodies When the HTTP request has a body part after the HTTP headers, it is not getting processed and is being prepended to the next request's data, which results in an error when trying to parse it. Improve the httpd.c:process_request() function with the following additions: 1. Require that HTTP POST requests must have Content-Length header. 2. When Content-Length header is set, extract its value, and make sure that it is valid and that the whole request's body is received before processing the request. 3. Discard the request's body by consuming Content-Length worth of data in the buffer. --- lib/isc/httpd.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/lib/isc/httpd.c b/lib/isc/httpd.c index 368b9cd2e1..b154b3aa6c 100644 --- a/lib/isc/httpd.c +++ b/lib/isc/httpd.c @@ -408,8 +408,10 @@ 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, size_t *buflen) { char *s = NULL, *p = NULL, *urlend = NULL; + const char *content_length = NULL; size_t limit = sizeof(httpd->recvbuf) - httpd->recvlen - 1; size_t len = region->length; + size_t clen = 0; int delim; bool truncated = false; @@ -566,6 +568,29 @@ process_request(isc_httpd_t *httpd, isc_region_t *region, size_t *buflen) { httpd->headers = s; + if (!have_header(httpd, "Content-Length:", NULL, NULL, &content_length)) + { + /* Require a Content-Length header for POST requests. */ + if (httpd->method == METHOD_POST) { + return (ISC_R_BADNUMBER); + } + } else { + INSIST(content_length != NULL); + + clen = (size_t)strtoul(content_length, NULL, 10); + if (clen == ULONG_MAX) { + /* Invalid number in the header value. */ + return (ISC_R_BADNUMBER); + } + if (httpd->recvlen < httpd->consume + clen) { + /* The request data isn't complete yet. */ + return (ISC_R_NOTFOUND); + } + + /* Consume the request's data, which we do not use. */ + httpd->consume += clen; + } + if (have_header(httpd, "Connection:", "close", ", \t\r\n", NULL)) { httpd->flags |= HTTPD_CLOSE; } From 9885306d9ad62069440b29eda68ba7bc29e7c9e9 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Wed, 20 Jul 2022 10:27:29 +0000 Subject: [PATCH 3/5] Add CHANGES not for [GL #3463] --- CHANGES | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGES b/CHANGES index 615f80a34c..726e201b06 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +5946. [bug] Fix statistics channel's handling of multiple HTTP + requests in a single connection which have non-empty + request bodies. [GL #3463] + 5945. [bug] If parsing /etc/bind.key failed, delv could assert when trying to parse the built in trust anchors as the parser hadn't been reset. [GL !6468] From 3e28d364f6d463a19f770397d7904d086af824b4 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Wed, 20 Jul 2022 13:21:27 +0000 Subject: [PATCH 4/5] Replace expr commands with $((expression)) shell constucts Update the "statschannel" system test to use the $((expression)) shell constucts instead of executing the `expr` program. --- bin/tests/system/statschannel/tests.sh | 50 +++++++++++++------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/bin/tests/system/statschannel/tests.sh b/bin/tests/system/statschannel/tests.sh index a247ef4882..5485a5cbbf 100644 --- a/bin/tests/system/statschannel/tests.sh +++ b/bin/tests/system/statschannel/tests.sh @@ -110,8 +110,8 @@ if [ $PERL_JSON ]; then [ "$noerror_count" -eq "$json_noerror_count" ] || ret=1 fi if [ $ret != 0 ]; then echo_i "failed"; fi -status=`expr $status + $ret` -n=`expr $n + 1` +status=$((status + ret)) +n=$((n + 1)) ret=0 echo_i "checking malloced memory statistics xml/json ($n)" @@ -131,8 +131,8 @@ if [ $PERL_JSON ]; then grep '"Malloced":[0-9][0-9]*,' json.mem > /dev/null || ret=1 fi if [ $ret != 0 ]; then echo_i "failed"; fi -status=`expr $status + $ret` -n=`expr $n + 1` +status=$((status + ret)) +n=$((n + 1)) echo_i "checking consistency between regular and compressed output ($n)" for i in 1 2 3 4 5; do @@ -158,8 +158,8 @@ for i in 1 2 3 4 5; do fi done -status=`expr $status + $ret` -n=`expr $n + 1` +status=$((status + ret)) +n=$((n + 1)) ret=0 echo_i "checking if compressed output is really compressed ($n)" @@ -169,15 +169,15 @@ then grep -i Content-Length | sed -e "s/.*: \([0-9]*\).*/\1/"` COMPSIZE=`cat compressed.headers | \ grep -i Content-Length | sed -e "s/.*: \([0-9]*\).*/\1/"` - if [ ! `expr $REGSIZE / $COMPSIZE` -gt 2 ]; then + if [ ! $((REGSIZE / COMPSIZE)) -gt 2 ]; then ret=1 fi else echo_i "skipped" fi if [ $ret != 0 ]; then echo_i "failed"; fi -status=`expr $status + $ret` -n=`expr $n + 1` +status=$((status + ret)) +n=$((n + 1)) # Test dnssec sign statistics. zone="dnssec" @@ -208,8 +208,8 @@ if [ $PERL_JSON ]; then cmp zones.out.j$n zones.expect.$n || ret=1 fi if [ $ret != 0 ]; then echo_i "failed"; fi -status=`expr $status + $ret` -n=`expr $n + 1` +status=$((status + ret)) +n=$((n + 1)) # Test sign operations after dynamic update. ret=0 @@ -238,8 +238,8 @@ if [ $PERL_JSON ]; then cmp zones.out.j$n zones.expect.$n || ret=1 fi if [ $ret != 0 ]; then echo_i "failed"; fi -status=`expr $status + $ret` -n=`expr $n + 1` +status=$((status + ret)) +n=$((n + 1)) # Test sign operations of KSK. ret=0 @@ -265,8 +265,8 @@ if [ $PERL_JSON ]; then cmp zones.out.j$n zones.expect.$n || ret=1 fi if [ $ret != 0 ]; then echo_i "failed"; fi -status=`expr $status + $ret` -n=`expr $n + 1` +status=$((status + ret)) +n=$((n + 1)) # Test sign operations for scheduled resigning (many keys). ret=0 @@ -306,8 +306,8 @@ if [ $PERL_JSON ]; then cmp zones.out.j$n zones.expect.$n || ret=1 fi if [ $ret != 0 ]; then echo_i "failed"; fi -status=`expr $status + $ret` -n=`expr $n + 1` +status=$((status + ret)) +n=$((n + 1)) # Test sign operations after dynamic update (many keys). ret=0 @@ -344,8 +344,8 @@ if [ $PERL_JSON ]; then cmp zones.out.j$n zones.expect.$n || ret=1 fi if [ $ret != 0 ]; then echo_i "failed"; fi -status=`expr $status + $ret` -n=`expr $n + 1` +status=$((status + ret)) +n=$((n + 1)) # Test sign operations after dnssec-policy change (removing keys). ret=0 @@ -373,8 +373,8 @@ if [ $PERL_JSON ]; then cmp zones.out.j$n zones.expect.$n || ret=1 fi if [ $ret != 0 ]; then echo_i "failed"; fi -status=`expr $status + $ret` -n=`expr $n + 1` +status=$((status + ret)) +n=$((n + 1)) if [ -x "${NC}" ] ; then echo_i "Check HTTP/1.1 pipelined requests are handled ($n)" @@ -391,8 +391,8 @@ EOF lines=$(grep "^HTTP/1.1" nc.out$n | wc -l) test $lines = 2 || ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi - status=`expr $status + $ret` - n=`expr $n + 1` + status=$((status + ret)) + n=$((n + 1)) else echo_i "skipping test as nc not found" fi @@ -421,8 +421,8 @@ test $((time2 - time1)) -lt 5 || ret=1 lines=$(grep "^HTTP/1.1" send.out$n | wc -l) test $lines = 91 || ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi -status=`expr $status + $ret` -n=`expr $n + 1` +status=$((status + ret)) +n=$((n + 1)) echo_i "exit status: $status" [ $status -eq 0 ] || exit 1 From dc9b0ea6a6ba7104528ac6a64ac58139246c6896 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Wed, 20 Jul 2022 13:33:40 +0000 Subject: [PATCH 5/5] Add pipelined POST requests check in the statschannel system test Use `nc` to check that multiple POST requests with non-empty HTTP body are serviced normally by the statistics channel. --- bin/tests/system/statschannel/tests.sh | 29 +++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/bin/tests/system/statschannel/tests.sh b/bin/tests/system/statschannel/tests.sh index 5485a5cbbf..1d0cff0589 100644 --- a/bin/tests/system/statschannel/tests.sh +++ b/bin/tests/system/statschannel/tests.sh @@ -377,7 +377,7 @@ status=$((status + ret)) n=$((n + 1)) if [ -x "${NC}" ] ; then - echo_i "Check HTTP/1.1 pipelined requests are handled ($n)" + echo_i "Check HTTP/1.1 pipelined requests are handled (GET) ($n)" ret=0 ${NC} 10.53.0.3 ${EXTRAPORT1} << EOF > nc.out$n || ret=1 GET /xml/v3/status HTTP/1.1 @@ -397,6 +397,33 @@ else echo_i "skipping test as nc not found" fi +if [ -x "${NC}" ] ; then + echo_i "Check HTTP/1.1 pipelined requests are handled (POST) ($n)" + ret=0 + ${NC} 10.53.0.3 ${EXTRAPORT1} << EOF > nc.out$n || ret=1 +POST /xml/v3/status HTTP/1.1 +Host: 10.53.0.3:${EXTRAPORT1} +Content-Type: application/json +Content-Length: 3 + +{} +POST /xml/v3/status HTTP/1.1 +Host: 10.53.0.3:${EXTRAPORT1} +Content-Type: application/json +Content-Length: 3 +Connection: close + +{} +EOF + lines=$(grep "^HTTP/1.1" nc.out$n | wc -l) + test $lines = 2 || ret=1 + if [ $ret != 0 ]; then echo_i "failed"; fi + status=$((status + ret)) + n=$((n + 1)) +else + echo_i "skipping test as nc not found" +fi + echo_i "Check HTTP/1.1 pipelined with truncated stream ($n)" ret=0 i=0