Parse statschannel Content-Length: more carefully

A negative or excessively large Content-Length could cause a crash
by making `INSIST(httpd->consume != 0)` fail.

diff --git a/bin/tests/system/statschannel/tests.sh b/bin/tests/system/statschannel/tests.sh
index 980bdf7579444ac9f4b20f03ca31d68a3af68d81..7c71ace2dfac9d88f4522384fdf5cca241684277 100644 (file)
--- a/bin/tests/system/statschannel/tests.sh
+++ b/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

diff --git a/lib/isc/httpd.c b/lib/isc/httpd.c
index c73d7d161acad8c55ab8f6cea56702823e1853e3..6ce2ca979c9d9ca64f4f45b285e842fd51d1e5e1 100644 (file)
--- a/lib/isc/httpd.c
+++ b/lib/isc/httpd.c
@@ -420,7 +420,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;
 
@@ -431,12 +431,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")) {
@@ -472,17 +483,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: