Merge branch '4125-statschannel-content-length-overflow-9.18' into 'bind-9.18'

[9.18] Parse statschannel Content-Length: more carefully

See merge request isc-projects/bind9!8215

CHANGES

@@ -4,6 +4,10 @@
 6230.	[bug]		Prevent an unnecessary query restart if a synthesized
 			CNAME target points to the CNAME owner. [GL #3835]
 
+6227.	[bug]		Check the statistics-channel HTTP Content-length
+			to prevent negative or overflowing values from
+			causing a crash. [GL #4125]
+
 6224.	[bug]		Check the If-Modified-Since value length to prevent
 			out-of-bounds write. [GL #4124]
 

bin/tests/system/statschannel/tests.sh

@@ -72,9 +72,66 @@ loadkeys_on() {
     wait_for_log 20 "next key event" ns${nsidx}/named.run
 }
 
+set -x
+
+# verify that the http server dropped the connection without replying
+check_http_dropped() {
+    if [ -x "${NC}" ] ; then
+	"${NC}" 10.53.0.3 "${EXTRAPORT1}" > nc.out$n || ret=1
+	if test -s nc.out$n; then
+		ret=1
+	fi
+    else
+	echo_i "skipping test as nc not found"
+    fi
+}
+
 status=0
 n=1
 
+echo_i "check content-length parse error ($n)"
+ret=0
+check_http_dropped <<EOF
+POST /xml/v3/status HTTP/1.0
+Content-Length: nah
+
+EOF
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status + ret))
+n=$((n + 1))
+
+echo_i "check negative content-length ($n)"
+ret=0
+check_http_dropped <<EOF
+POST /xml/v3/status HTTP/1.0
+Content-Length: -50
+
+EOF
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status + ret))
+n=$((n + 1))
+
+echo_i "check content-length 32-bit overflow ($n)"
+check_http_dropped <<EOF
+POST /xml/v3/status HTTP/1.0
+Content-Length: 4294967239
+
+EOF
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status + ret))
+n=$((n + 1))
+
+echo_i "check content-length 64-bit overflow ($n)"
+check_http_dropped <<EOF
+POST /xml/v3/status HTTP/1.0
+Content-Length: 18446744073709551549
+
+EOF
+
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status + ret))
+n=$((n + 1))
+
 echo_i "Prepare for if-modified-since test ($n)"
 ret=0
 i=0

doc/notes/notes-current.rst

@@ -38,15 +38,19 @@ Feature Changes
 Bug Fixes
 ~~~~~~~~~
 
-- None.
-
 - The value of If-Modified-Since header in statistics channel was not checked
   for length leading to possible buffer overflow by an authorized user.  We
   would like to emphasize that statistics channel must be properly setup to
   allow access only from authorized users of the system. :gl:`#4124`
 
-  This was reported independently by Eric Sesterhenn of X41 D-SEC and Cameron
-  Whitehead.
+  This issue was reported independently by Eric Sesterhenn of X41 D-SEC and
+  Cameron Whitehead.
+
+- The value of Content-Length header in statistics channel was not bound checked
+  and negative or large enough value could lead to overflow and assertion failure.
+  :gl:`#4125`
+
+  This issue was reported by Eric Sesterhenn of X41 D-SEC.
 
 Known Issues
 ~~~~~~~~~~~~

lib/isc/httpd.c

@@ -427,7 +427,7 @@ process_request(isc_httpd_t *httpd, size_t last_len) {
 	 */
 	httpd->flags = 0;
 
-	ssize_t content_len = 0;
+	size_t content_len = 0;
 	bool keep_alive = false;
 	bool host_header = false;
 
@@ -438,12 +438,23 @@ process_request(isc_httpd_t *httpd, size_t last_len) {
 
 		if (name_match(header, "Content-Length")) {
 			char *endptr;
-			content_len = (size_t)strtoul(header->value, &endptr,
-						      10);
-			/* Consistency check, if we consumed all numbers */
+			long val = strtol(header->value, &endptr, 10);
+
+			errno = 0;
+
+			/* ensure we consumed all digits */
 			if ((header->value + header->value_len) != endptr) {
+				return (ISC_R_BADNUMBER);
+			}
+			/* ensure there was no minus sign */
+			if (val < 0) {
+				return (ISC_R_BADNUMBER);
+			}
+			/* ensure it did not overflow */
+			if (errno != 0) {
 				return (ISC_R_RANGE);
 			}
+			content_len = val;
 		} else if (name_match(header, "Connection")) {
 			/* multiple fields in a connection header are allowed */
 			if (value_match(header, "close")) {
@@ -479,17 +490,18 @@ process_request(isc_httpd_t *httpd, size_t last_len) {
 		return (ISC_R_BADNUMBER);
 	}
 
-	if (content_len == (ssize_t)ULONG_MAX) {
-		/* Invalid number in the header value. */
-		return (ISC_R_BADNUMBER);
+	if (content_len >= HTTP_MAX_REQUEST_LEN) {
+		return (ISC_R_RANGE);
 	}
-	if (httpd->consume + content_len > httpd->recvlen) {
+
+	size_t consume = httpd->consume + content_len;
+	if (consume > httpd->recvlen) {
 		/* The request data isn't complete yet. */
 		return (ISC_R_NOMORE);
 	}
 
 	/* Consume the request's data, which we do not use. */
-	httpd->consume += content_len;
+	httpd->consume = consume;
 
 	switch (httpd->minor_version) {
 	case 0: