From 525afc666a3040330f786634c990bd595c1de83b Mon Sep 17 00:00:00 2001 From: Tony Finch Date: Fri, 9 Jun 2023 09:33:57 +0100 Subject: [PATCH 1/3] Parse statschannel Content-Length: more carefully A negative or excessively large Content-Length could cause a crash by making `INSIST(httpd->consume != 0)` fail. (cherry picked from commit 26e10e8fb51d5f101cfdc319794f2650084153c3) --- bin/tests/system/statschannel/tests.sh | 57 ++++++++++++++++++++++++++ lib/isc/httpd.c | 30 ++++++++++---- 2 files changed, 78 insertions(+), 9 deletions(-) diff --git a/bin/tests/system/statschannel/tests.sh b/bin/tests/system/statschannel/tests.sh index 3318f45b0c..25122e1eca 100644 --- 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 <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: From 3f4d9f739c1b3e0de2a7e0214f04a05c4b3f9069 Mon Sep 17 00:00:00 2001 From: Tony Finch Date: Fri, 9 Jun 2023 09:33:57 +0100 Subject: [PATCH 2/3] Add CHANGES entry for [GL #4125] (cherry picked from commit 1ba538f32f597a4c357c7e65b19b4d3ecb0814e5) --- CHANGES | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGES b/CHANGES index bc76b1cf6d..1cfe48002f 100644 --- a/CHANGES +++ b/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] From 034a63c1f3e33778f1ccf5b985d5a12056747036 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Mon, 21 Aug 2023 15:15:26 +0200 Subject: [PATCH 3/3] Add release note for [GL #4125] (cherry picked from commit 58a8de5246d9126f96d1aabaa324baf51a977926) --- doc/notes/notes-current.rst | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index 79dcce4a0b..c09e915ff5 100644 --- a/doc/notes/notes-current.rst +++ b/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 ~~~~~~~~~~~~