Avoid UB trimming empty response header values

include/httpd.h

@@ -2780,6 +2780,12 @@ AP_DECLARE(int) ap_array_str_index(const apr_array_header_t *array,
 AP_DECLARE(int) ap_array_str_contains(const apr_array_header_t *array, 
                                       const char *s);
 
+/**
+ * Strip trailing whitespace from a NUL-terminated string in place.
+ * @param str String to modify
+ */
+AP_DECLARE(void) ap_cstr_stripws(char *str);
+
 /**
  * Perform a case-insensitive comparison of two strings @a str1 and @a str2,
  * treating upper and lower case values of the 26 standard C/POSIX alphabetic

modules/http/byterange_filter.c

@@ -136,16 +136,14 @@ static int ap_set_byterange(request_rec *r, apr_off_t clength,
     }
     *indexes = apr_array_make(r->pool, ranges, sizeof(indexes_t));
     while ((cur = ap_getword(r->pool, &range, ','))) {
-        char *dash, *end_cur;
+        char *dash;
         apr_off_t number, start, end;
 
          /* Remove leading and trailing white spaces */
         while (apr_isspace(*cur))
             ++cur;
         /* blast trailing whitespace */
-        end_cur = &cur[strlen(cur)];
-        while (--end_cur >= cur && apr_isspace(*end_cur))
-            *end_cur = '\0';
+        ap_cstr_stripws(cur);
 
         if (!*cur)
             break;

modules/mappers/mod_negotiation.c

@@ -1022,10 +1022,8 @@ static int read_type_map(apr_file_t **map, negotiation_state *neg,
             }
             else if (!strncmp(buffer, "body:", 5)) {
                 char *tag = apr_pstrdup(neg->pool, body);
-                char *eol = strchr(tag, '\0');
                 apr_size_t len = MAX_STRING_LEN;
-                while (--eol >= tag && apr_isspace(*eol))
-                    *eol = '\0';
+                ap_cstr_stripws(tag);
                 if ((mime_info.body = get_body(buffer, &len, tag, *map)) < 0) {
                     ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(00685)
                                   "Syntax error in type map, no end tag '%s' "

modules/metadata/mod_cern_meta.c

@@ -244,9 +244,7 @@ static int scan_meta_file(request_rec *r, apr_file_t *f)
             char *tmp;
             /* Nuke trailing whitespace */
 
-            char *endp = l + strlen(l) - 1;
-            while (endp > l && apr_isspace(*endp))
-                *endp-- = '\0';
+            ap_cstr_stripws(l);
 
             tmp = apr_pstrdup(r->pool, l);
             ap_content_type_tolower(tmp);

modules/proxy/mod_proxy_hcheck.c

@@ -766,7 +766,7 @@ static int hc_read_headers(request_rec *r)
 
     /* OK, 1st line is OK... scarf in the headers */
     while ((len = ap_getline(buffer, sizeof(buffer), r, 1)) > 0) {
-        char *value, *end;
+        char *value;
         ap_log_error(APLOG_MARK, APLOG_TRACE7, 0, r->server, "%.*s",
                      len, buffer);
         if (!(value = strchr(buffer, ':'))) {
@@ -776,8 +776,7 @@ static int hc_read_headers(request_rec *r)
         ++value;
         while (apr_isspace(*value))
             ++value;            /* Skip to start of value   */
-        for (end = &value[strlen(value)-1]; end > value && apr_isspace(*end); --end)
-            *end = '\0';
+        ap_cstr_stripws(value);
         apr_table_add(r->headers_out, buffer, value);
     }
 

modules/proxy/mod_proxy_http.c

@@ -864,7 +864,7 @@ static apr_status_t ap_proxy_read_headers(request_rec *r, request_rec *rr,
                                   conn_rec *c, int *pread_len)
 {
     int len;
-    char *value, *end;
+    char *value;
     int saw_headers = 0;
     void *sconf = r->server->module_config;
     proxy_server_conf *psc;
@@ -973,8 +973,7 @@ static apr_status_t ap_proxy_read_headers(request_rec *r, request_rec *rr,
             ++value;            /* Skip to start of value   */
 
         /* should strip trailing whitespace as well */
-        for (end = &value[strlen(value)-1]; end > value && apr_isspace(*end); --end)
-            *end = '\0';
+        ap_cstr_stripws(value);
 
         /* make sure we add so as not to destroy duplicated headers
          * Modify headers requiring canonicalisation and/or affected

modules/proxy/mod_proxy_uwsgi.c

@@ -303,7 +303,7 @@ static int uwsgi_response(request_rec *r, proxy_conn_rec * backend,
 
     char buffer[HUGE_STRING_LEN];
     const char *buf;
-    char *value, *end;
+    char *value;
     char keepchar;
     int len;
     int backend_broke = 0;
@@ -386,9 +386,7 @@ static int uwsgi_response(request_rec *r, proxy_conn_rec * backend,
         }
         while (apr_isspace(*value))
             ++value;
-        for (end = &value[strlen(value) - 1];
-             end > value && apr_isspace(*end); --end)
-            *end = '\0';
+        ap_cstr_stripws(value);
         if (*ap_scan_http_field_content(value)) {
             /* invalid value */
             len = -1;

server/util.c

@@ -1183,16 +1183,15 @@ static int cfg_trim_line(char *buf)
     while (apr_isspace(*start))
         ++start;
     /* blast trailing whitespace */
-    end = &start[strlen(start)];
-    while (--end >= start && apr_isspace(*end))
-        *end = '\0';
+    ap_cstr_stripws(start);
+    end = start + strlen(start);
     /* Zap leading whitespace by shifting */
     if (start != buf)
-        memmove(buf, start, end - start + 2);
+        memmove(buf, start, end - start + 1);
 #ifdef DEBUG_CFG_LINES
     ap_log_error(APLOG_MARK, APLOG_NOTICE, 0, NULL, APLOGNO(00555) "Read config: '%s'", buf);
 #endif
-    return end - start + 1;
+    return end - start;
 }
 
 /* Read one line from open ap_configfile_t, strip LF, increase line number */
@@ -3704,6 +3703,16 @@ static const unsigned char ucharmap[256] = {
 };
 #endif
 
+AP_DECLARE(void) ap_cstr_stripws(char *str)
+{
+    char *end = str + strlen(str);
+
+    while (end > str && apr_isspace(end[-1])) {
+        --end;
+    }
+    *end = '\0';
+}
+
 AP_DECLARE(int) ap_cstr_casecmp(const char *s1, const char *s2)
 {
     const unsigned char *u1 = (const unsigned char *)s1;

server/util_script.c

@@ -649,10 +649,7 @@ AP_DECLARE(int) ap_scan_script_header_err_core_ex(request_rec *r, char *buffer,
 
             /* Nuke trailing whitespace */
 
-            char *endp = l + strlen(l) - 1;
-            while (endp > l && apr_isspace(*endp)) {
-                *endp-- = '\0';
-            }
+            ap_cstr_stripws(l);
 
             tmp = apr_pstrdup(r->pool, l);
             ap_content_type_tolower(tmp);

test/modules/proxy/test_03_response.py

@@ -16,6 +16,16 @@ def _make_response(self, data):
             return """HTTP/1.1 200 OK\r\nContent-Type: 
             text/html\x00extra\r\n\r\n""".encode()
 
+        if "/empty-header" in path:
+            body = b"Hello"
+            return (
+                b"HTTP/1.1 200 OK\r\n"
+                b"X-Empty:\r\n"
+                b"Content-Length: 5\r\n"
+                b"\r\n"
+                + body
+            )
+
         if "/forwarded" in path:
             headers = data.split(b"\r\n\r\n")[0].decode("latin-1")
             forwarded = []
@@ -89,6 +99,13 @@ def test_proxy_03_001(self, env):
             lognos=["AH01106", "AH10404"]
         )
 
+    # empty backend response header values are valid
+    def test_proxy_03_004(self, env):
+        r = env.curl_get(env.mkurl("http", "test1", "/empty-header"))
+        assert r.response["status"] == 200
+        assert "x-empty" in r.response["header"]
+        assert r.response["body"] == b"Hello"
+
     # checks X-Forwarded headers
     def test_proxy_03_002(self, env):
         if not env.httpd_is_at_least("2.4.54"):

test/modules/proxy/test_05_uwsgi.py

@@ -17,6 +17,17 @@ def hello(data):
             + body
         )
 
+    @staticmethod
+    def empty_header(data):
+        body = b"Hello"
+        return (
+            b"HTTP/1.1 200 OK\r\n"
+            b"X-Empty:\r\n"
+            b"Content-Length: 5\r\n"
+            b"\r\n"
+            + body
+        )
+
 
 class TestProxyUwsgi:
 
@@ -51,3 +62,11 @@ def test_proxy_005_01(self, env, _class_scope):
         assert data[3] == 0x00  # standard WSGI request
         assert len(data) == 4 + datasize
 
+    # empty backend response header values are valid
+    def test_proxy_005_02(self, env, _class_scope):
+        _class_scope._make_response = _UWSGIFaker.empty_header
+        r = env.curl_get(env.mkurl("http", "test1", "/"))
+        assert r.response["status"] == 200
+        assert "x-empty" in r.response["header"]
+        assert r.response["body"] == b"Hello"
+

test/unit/util.c

@@ -17,6 +17,7 @@
 #include "../httpdunit.h"
 
 #include "httpd.h"
+#include "apr_strings.h"
 
 /*
  * Test Fixture -- runs once per test
@@ -80,6 +81,39 @@ HTTPD_START_LOOP_TEST(find_token_correctly_parses_token_list, ap_test_token_case
 END_TEST
 
 
+/*
+ * ap_cstr_stripws()
+ */
+
+struct ap_cstr_stripws_case {
+    const char *input;
+    const char *expected;
+};
+
+const struct ap_cstr_stripws_case ap_cstr_stripws_cases[] = {
+    { "", "" },
+    { " ", "" },
+    { "\t\r\n", "" },
+    { "value", "value" },
+    { "value ", "value" },
+    { "value\t ", "value" },
+    { " value ", " value" },
+};
+
+const size_t ap_cstr_stripws_cases_len = sizeof(ap_cstr_stripws_cases) /
+                                         sizeof(ap_cstr_stripws_cases[0]);
+
+HTTPD_START_LOOP_TEST(cstr_stripws_strips_trailing_whitespace, ap_cstr_stripws_cases_len)
+{
+    const struct ap_cstr_stripws_case *c = &ap_cstr_stripws_cases[_i];
+    char *str = apr_pstrdup(g_pool, c->input);
+
+    ap_cstr_stripws(str);
+    ck_assert_str_eq(str, c->expected);
+}
+END_TEST
+
+
 /*
  * ap_escape_quotes()
  */