From 1005dd74d9ea0c39a24c71e90aef7ad16b3d3107 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. (cherry picked from commit 376e698dc21f4117d6461101c4cfbaef2b724592) --- 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 7a1512b6f7..a2bff7bc17 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 d4c5d1c650ae0e97a083b0ce7a705c20fc001f07 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. (cherry picked from commit c2bbdc8a648c9630b2c9cea5227ad5c309c2ade5) --- lib/isc/httpd.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/lib/isc/httpd.c b/lib/isc/httpd.c index a2bff7bc17..a701fb2a84 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 95452d9a47ecae747b60e1bcd6f5c7016b402a45 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] (cherry picked from commit a00d787f2cf909fe0d8dce016488916997e8d67e) --- CHANGES | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGES b/CHANGES index 801bc38604..1dade4b8ca 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 8e37e5f27f57339282017e948ca51139b69d81c5 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. (cherry picked from commit 8034819b53789b52dd1c80b0256880b506a3f31b) --- 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 cbb5d4f08b781103c2a236db11198fbe4b737b40 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. (cherry picked from commit bc32885ba981cab6308981936f49ab625af84bf2) --- 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