core: Add common apr_off_t config parser

Add ap_size_parameter_parse(), a helper to parse a configuration value
as an apr_off_t, which fails on trailing characters and on overflow.
The range of the value remains up to the directive.

Use it for configuration directives which parsed such values in
different ways, some of them accepting trailing characters or doing no
checks at all: LimitRequestBody (unchanged behavior), ScriptLogLength,
ScriptLogBuffer, DavQuota, BufferSize, AuthFormSize, and the
mod_cache_disk and mod_cache_socache size and time directives.

GitHub: closes #829


git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1939129 13f79535-47bb-0310-9956-ffa450edef68

changes-entries/size-parameter-parse.txt

@@ -0,0 +1,4 @@
+  *) core: Add ap_size_parameter_parse() and use it to reject malformed
+     ScriptLogLength, ScriptLogBuffer, DavQuota, BufferSize, AuthFormSize,
+     and mod_cache_disk and mod_cache_socache size and time values.
+     [Arturo Bernal]

include/ap_mmn.h

@@ -738,6 +738,7 @@
  * 20211221.30 (2.5.1-dev) Add ap_stat_check() to httpd.h
  * 20211221.31 (2.5.1-dev) Add ap_*_timingsafe() to httpd.h
  * 20211221.32 (2.5.1-dev) Add ap_cstr_stripws() to httpd.h
+ * 20211221.33 (2.5.1-dev) Add ap_size_parameter_parse() to httpd.h
  */
 
 #define MODULE_MAGIC_COOKIE 0x41503235UL /* "AP25" */
@@ -745,7 +746,7 @@
 #ifndef MODULE_MAGIC_NUMBER_MAJOR
 #define MODULE_MAGIC_NUMBER_MAJOR 20211221
 #endif
-#define MODULE_MAGIC_NUMBER_MINOR 32             /* 0...n */
+#define MODULE_MAGIC_NUMBER_MINOR 33             /* 0...n */
 
 /**
  * Determine if the server's current MODULE_MAGIC_NUMBER is at least a

include/httpd.h

@@ -2363,6 +2363,18 @@ AP_DECLARE(apr_status_t) ap_timeout_parameter_parse(
                                                apr_interval_time_t *timeout,
                                                const char *default_time_unit);
 
+/**
+ * Parse a configuration parameter as a base 10 apr_off_t. The whole parameter
+ * has to be a number, there must not be any other character after it. What
+ * values are acceptable is up to the caller.
+ * @param parameter The string containing the number.
+ * @param value The parsed value, only set on success.
+ * @return APR_SUCCESS, APR_EINVAL if the parameter is not a number, or
+ *         APR_ERANGE if the number does not fit in an apr_off_t.
+ */
+AP_DECLARE(apr_status_t) ap_size_parameter_parse(const char *parameter,
+                                                 apr_off_t *value);
+
 /**
  * Determine if a request has a request body or not.
  *

modules/aaa/mod_auth_form.c

@@ -256,7 +256,7 @@ static const char *set_cookie_form_size(cmd_parms * cmd, void *config,
     auth_form_config_rec *conf = config;
     apr_off_t size;
 
-    if (APR_SUCCESS != apr_strtoff(&size, arg, NULL, 10)
+    if (APR_SUCCESS != ap_size_parameter_parse(arg, &size)
         || size < 0 || size > APR_SIZE_MAX) {
         return "AuthCookieFormSize must be a size in bytes, or zero.";
     }

modules/cache/mod_cache_disk.c

@@ -1488,7 +1488,7 @@ static const char
 {
     disk_cache_dir_conf *dconf = (disk_cache_dir_conf *)in_struct_ptr;
 
-    if (apr_strtoff(&dconf->minfs, arg, NULL, 10) != APR_SUCCESS ||
+    if (ap_size_parameter_parse(arg, &dconf->minfs) != APR_SUCCESS ||
             dconf->minfs < 0)
     {
         return "CacheMinFileSize argument must be a non-negative integer representing the min size of a file to cache in bytes.";
@@ -1502,7 +1502,7 @@ static const char
 {
     disk_cache_dir_conf *dconf = (disk_cache_dir_conf *)in_struct_ptr;
 
-    if (apr_strtoff(&dconf->maxfs, arg, NULL, 10) != APR_SUCCESS ||
+    if (ap_size_parameter_parse(arg, &dconf->maxfs) != APR_SUCCESS ||
             dconf->maxfs < 0)
     {
         return "CacheMaxFileSize argument must be a non-negative integer representing the max size of a file to cache in bytes.";
@@ -1516,7 +1516,7 @@ static const char
 {
     disk_cache_dir_conf *dconf = (disk_cache_dir_conf *)in_struct_ptr;
 
-    if (apr_strtoff(&dconf->readsize, arg, NULL, 10) != APR_SUCCESS ||
+    if (ap_size_parameter_parse(arg, &dconf->readsize) != APR_SUCCESS ||
             dconf->readsize < 0)
     {
         return "CacheReadSize argument must be a non-negative integer representing the max amount of data to cache in go.";
@@ -1531,7 +1531,7 @@ static const char
     disk_cache_dir_conf *dconf = (disk_cache_dir_conf *)in_struct_ptr;
     apr_off_t milliseconds;
 
-    if (apr_strtoff(&milliseconds, arg, NULL, 10) != APR_SUCCESS ||
+    if (ap_size_parameter_parse(arg, &milliseconds) != APR_SUCCESS ||
             milliseconds < 0)
     {
         return "CacheReadTime argument must be a non-negative integer representing the max amount of time taken to cache in go.";

modules/cache/mod_cache_socache.c

@@ -1264,7 +1264,7 @@ static const char *set_cache_max(cmd_parms *parms, void *in_struct_ptr,
 {
     cache_socache_dir_conf *dconf = (cache_socache_dir_conf *) in_struct_ptr;
 
-    if (apr_strtoff(&dconf->max, arg, NULL, 10) != APR_SUCCESS
+    if (ap_size_parameter_parse(arg, &dconf->max) != APR_SUCCESS
             || dconf->max < 1024 || dconf->max > APR_UINT32_MAX) {
         return "CacheSocacheMaxSize argument must be a integer representing "
                "the max size of a cached entry (headers and body), at least 1024 "
@@ -1280,7 +1280,7 @@ static const char *set_cache_maxtime(cmd_parms *parms, void *in_struct_ptr,
     cache_socache_dir_conf *dconf = (cache_socache_dir_conf *) in_struct_ptr;
     apr_off_t seconds;
 
-    if (apr_strtoff(&seconds, arg, NULL, 10) != APR_SUCCESS || seconds < 0) {
+    if (ap_size_parameter_parse(arg, &seconds) != APR_SUCCESS || seconds < 0) {
         return "CacheSocacheMaxTime argument must be the maximum amount of time in seconds to cache an entry.";
     }
     dconf->maxtime = apr_time_from_sec(seconds);
@@ -1294,7 +1294,7 @@ static const char *set_cache_mintime(cmd_parms *parms, void *in_struct_ptr,
     cache_socache_dir_conf *dconf = (cache_socache_dir_conf *) in_struct_ptr;
     apr_off_t seconds;
 
-    if (apr_strtoff(&seconds, arg, NULL, 10) != APR_SUCCESS || seconds < 0) {
+    if (ap_size_parameter_parse(arg, &seconds) != APR_SUCCESS || seconds < 0) {
         return "CacheSocacheMinTime argument must be the minimum amount of time in seconds to cache an entry.";
     }
     dconf->mintime = apr_time_from_sec(seconds);
@@ -1307,7 +1307,7 @@ static const char *set_cache_readsize(cmd_parms *parms, void *in_struct_ptr,
 {
     cache_socache_dir_conf *dconf = (cache_socache_dir_conf *) in_struct_ptr;
 
-    if (apr_strtoff(&dconf->readsize, arg, NULL, 10) != APR_SUCCESS
+    if (ap_size_parameter_parse(arg, &dconf->readsize) != APR_SUCCESS
             || dconf->readsize < 0) {
         return "CacheSocacheReadSize argument must be a non-negative integer representing the max amount of data to cache in go.";
     }
@@ -1321,7 +1321,7 @@ static const char *set_cache_readtime(cmd_parms *parms, void *in_struct_ptr,
     cache_socache_dir_conf *dconf = (cache_socache_dir_conf *) in_struct_ptr;
     apr_off_t milliseconds;
 
-    if (apr_strtoff(&milliseconds, arg, NULL, 10) != APR_SUCCESS
+    if (ap_size_parameter_parse(arg, &milliseconds) != APR_SUCCESS
             || milliseconds < 0) {
         return "CacheSocacheReadTime argument must be a non-negative integer representing the max amount of time taken to cache in go.";
     }

modules/dav/fs/mod_dav_fs.c

@@ -262,8 +262,9 @@ static const char *dav_fs_cmd_quota(cmd_parms *cmd, void *config,
         conf->quota = DAV_FS_QUOTA_OFF;
     else if (!strcasecmp(bytes, "None"))
         conf->quota = DAV_FS_QUOTA_NONE;
-    else
-        conf->quota = atol(bytes);
+    else if (ap_size_parameter_parse(bytes, &conf->quota) != APR_SUCCESS
+             || conf->quota < 0)
+        return "DAVquota must be Off, None, or a size in bytes.";
 
     return NULL;
 }

modules/filters/mod_buffer.c

@@ -321,8 +321,8 @@ static const char *set_buffer_size(cmd_parms *cmd, void *dconf, const char *arg)
 {
     buffer_conf *conf = dconf;
 
-    if (APR_SUCCESS != apr_strtoff(&(conf->size), arg, NULL, 10) || conf->size
-            <= 0) {
+    if (APR_SUCCESS != ap_size_parameter_parse(arg, &(conf->size))
+            || conf->size <= 0) {
         return "BufferSize must be a size in bytes, and greater than zero";
     }
     conf->size_set = 1;

modules/generators/mod_cgi.c

@@ -146,7 +146,14 @@ static const char *set_scriptlog_length(cmd_parms *cmd, void *dummy,
     cgi_server_conf *conf = ap_get_module_config(s->module_config,
                                                  &cgi_module);
 
-    conf->logbytes = atol(arg);
+    apr_off_t bytes;
+
+    if (ap_size_parameter_parse(arg, &bytes) != APR_SUCCESS
+            || bytes < 0 || bytes > LONG_MAX) {
+        return "ScriptLogLength must be a non-negative integer.";
+    }
+
+    conf->logbytes = (long)bytes;
     return NULL;
 }
 
@@ -157,7 +164,14 @@ static const char *set_scriptlog_buffer(cmd_parms *cmd, void *dummy,
     cgi_server_conf *conf = ap_get_module_config(s->module_config,
                                                  &cgi_module);
 
-    conf->bufbytes = atoi(arg);
+    apr_off_t bytes;
+
+    if (ap_size_parameter_parse(arg, &bytes) != APR_SUCCESS
+            || bytes < 0 || (apr_uint64_t)bytes > APR_SIZE_MAX) {
+        return "ScriptLogBuffer must be a non-negative integer.";
+    }
+
+    conf->bufbytes = (apr_size_t)bytes;
     return NULL;
 }
 

modules/generators/mod_cgid.c

@@ -1153,7 +1153,14 @@ static const char *set_scriptlog_length(cmd_parms *cmd, void *dummy, const char
     cgid_server_conf *conf = ap_get_module_config(s->module_config,
                                                   &cgid_module);
 
-    conf->logbytes = atol(arg);
+    apr_off_t bytes;
+
+    if (ap_size_parameter_parse(arg, &bytes) != APR_SUCCESS
+            || bytes < 0 || bytes > LONG_MAX) {
+        return "ScriptLogLength must be a non-negative integer.";
+    }
+
+    conf->logbytes = (long)bytes;
     return NULL;
 }
 
@@ -1163,7 +1170,14 @@ static const char *set_scriptlog_buffer(cmd_parms *cmd, void *dummy, const char
     cgid_server_conf *conf = ap_get_module_config(s->module_config,
                                                   &cgid_module);
 
-    conf->bufbytes = atoi(arg);
+    apr_off_t bytes;
+
+    if (ap_size_parameter_parse(arg, &bytes) != APR_SUCCESS
+            || bytes < 0 || bytes > INT_MAX) {
+        return "ScriptLogBuffer must be a non-negative integer.";
+    }
+
+    conf->bufbytes = (int)bytes;
     return NULL;
 }
 

server/core.c

@@ -3900,14 +3900,17 @@ static const char *set_limit_req_body(cmd_parms *cmd, void *conf_,
                                       const char *arg)
 {
     core_dir_config *conf = conf_;
-    char *errp;
+    apr_off_t limit;
+    apr_status_t rv;
 
-    if (APR_SUCCESS != apr_strtoff(&conf->limit_req_body, arg, &errp, 10)) {
+    rv = ap_size_parameter_parse(arg, &limit);
+    if (rv == APR_ERANGE) {
         return "LimitRequestBody argument is not parsable.";
     }
-    if (*errp || conf->limit_req_body < 0) {
+    if (rv != APR_SUCCESS || limit < 0) {
         return "LimitRequestBody requires a non-negative integer.";
     }
+    conf->limit_req_body = limit;
 
     return NULL;
 }

server/util.c

@@ -2773,6 +2773,23 @@ AP_DECLARE(apr_status_t) ap_timeout_parameter_parse(
     *timeout = (apr_interval_time_t)check;
     return APR_SUCCESS;
 }
+
+AP_DECLARE(apr_status_t) ap_size_parameter_parse(const char *parameter,
+                                                 apr_off_t *value)
+{
+    apr_off_t parsed;
+    char *end;
+
+    if (apr_strtoff(&parsed, parameter, &end, 10) != APR_SUCCESS) {
+        return APR_ERANGE;
+    }
+    if (end == parameter || *end != '\0') {
+        return APR_EINVAL;
+    }
+
+    *value = parsed;
+    return APR_SUCCESS;
+}
 #undef CHECK_OVERFLOW
 
 AP_DECLARE(int) ap_parse_strict_length(apr_off_t *len, const char *str)

test/modules/core/test_012_config_off_t.py

@@ -0,0 +1,65 @@
+import os
+
+import pytest
+
+from pyhttpd.conf import HttpdConf
+
+# size and time directives parsed with ap_size_parameter_parse(), which can
+# not be negative
+DIRECTIVES = [
+    "LimitRequestBody",
+    "ScriptLogLength", "ScriptLogBuffer",
+    "CacheMinFileSize", "CacheMaxFileSize", "CacheReadSize", "CacheReadTime",
+    "CacheSocacheMaxTime", "CacheSocacheMinTime", "CacheSocacheReadSize",
+    "CacheSocacheReadTime",
+]
+
+
+class TestConfigOffT:
+
+    # have httpd parse the configuration, like "httpd -t" does
+    def check_config(self, env, directive, value):
+        conf = HttpdConf(env)
+        conf.add(f"{directive} {value}")
+        conf.add_vhost_test1()
+        conf.install()
+        return env.run([os.path.join(env.bin_dir, 'httpd'),
+                        "-t", "-d", env.server_dir,
+                        "-f", os.path.join(env.server_dir, 'conf', 'httpd.conf')])
+
+    @pytest.mark.parametrize("directive", DIRECTIVES)
+    @pytest.mark.parametrize("value", ["0", "1", "123", "1000000", "00123"])
+    def test_core_012_001(self, env, directive, value):
+        r = self.check_config(env, directive, value)
+        assert r.exit_code == 0, f"{r.stderr}"
+
+    # only a whole decimal number is valid
+    @pytest.mark.parametrize("directive", DIRECTIVES)
+    @pytest.mark.parametrize("value", [
+        "abc", "abc123", "123abc", "123foo456", "12.3", "0x10", "1K", "+", "-",
+    ])
+    def test_core_012_002(self, env, directive, value):
+        r = self.check_config(env, directive, value)
+        assert r.exit_code != 0, f"{value} was accepted"
+        assert directive in r.stderr, f"{r.stderr}"
+
+    # a number the directive does not accept, or too large to be stored
+    @pytest.mark.parametrize("directive", DIRECTIVES)
+    @pytest.mark.parametrize("value", ["-1", "9223372036854775808", "9" * 40])
+    def test_core_012_003(self, env, directive, value):
+        r = self.check_config(env, directive, value)
+        assert r.exit_code != 0, f"{value} was accepted"
+        assert directive in r.stderr, f"{r.stderr}"
+
+    # this one has a lower limit as well
+    @pytest.mark.parametrize("value, valid", [
+        ["1024", True], ["102400", True], ["1023", False], ["0", False],
+        ["1024abc", False], ["abc", False], ["9" * 40, False],
+    ])
+    def test_core_012_004(self, env, value, valid):
+        r = self.check_config(env, "CacheSocacheMaxSize", value)
+        if valid:
+            assert r.exit_code == 0, f"{r.stderr}"
+        else:
+            assert r.exit_code != 0, f"{value} was accepted"
+            assert "CacheSocacheMaxSize" in r.stderr, f"{r.stderr}"

test/modules/dav/test_003_config_quota.py

@@ -0,0 +1,35 @@
+import os
+
+import pytest
+
+from pyhttpd.conf import HttpdConf
+
+
+class TestConfigQuota:
+
+    # have httpd parse the configuration, like "httpd -t" does
+    def check_config(self, env, value):
+        conf = HttpdConf(env)
+        conf.add(f"DavQuota {value}")
+        conf.add_vhost_test1()
+        conf.install()
+        return env.run([os.path.join(env.bin_dir, 'httpd'),
+                        "-t", "-d", env.server_dir,
+                        "-f", os.path.join(env.server_dir, 'conf', 'httpd.conf')])
+
+    @pytest.mark.parametrize("value", [
+        "Off", "None", "0", "1", "123", "1000000", "00123",
+    ])
+    def test_dav_003_01(self, env, value):
+        r = self.check_config(env, value)
+        assert r.exit_code == 0, f"{r.stderr}"
+
+    # not Off, None or a size, in particular no negative number, which would
+    # be taken for one of the first two
+    @pytest.mark.parametrize("value", [
+        "abc", "123abc", "12.3", "0x10", "-1", "-2", "9" * 40,
+    ])
+    def test_dav_003_02(self, env, value):
+        r = self.check_config(env, value)
+        assert r.exit_code != 0, f"{value} was accepted"
+        assert "DAVquota must be Off, None, or a size in bytes" in r.stderr

test/modules/filters/env.py

@@ -12,7 +12,7 @@ class FiltersTestSetup(HttpdTestSetup):
     def __init__(self, env: 'HttpdTestEnv'):
         super().__init__(env=env)
         self.add_source_dir(os.path.dirname(inspect.getfile(FiltersTestSetup)))
-        self.add_modules(["substitute", "sed"])
+        self.add_modules(["substitute", "sed", "buffer"])
         self.add_cgi_module()
 
 

test/modules/filters/test_004_buffer_size.py

@@ -0,0 +1,31 @@
+import os
+
+import pytest
+
+from pyhttpd.conf import HttpdConf
+
+
+class TestBufferSize:
+
+    # have httpd parse the configuration, like "httpd -t" does
+    def check_config(self, env, value):
+        conf = HttpdConf(env)
+        conf.add(["<Location \"/buffer\">", f"    BufferSize {value}", "</Location>"])
+        conf.add_vhost_test1()
+        conf.install()
+        return env.run([os.path.join(env.bin_dir, 'httpd'),
+                        "-t", "-d", env.server_dir,
+                        "-f", os.path.join(env.server_dir, 'conf', 'httpd.conf')])
+
+    @pytest.mark.parametrize("value", ["1", "123", "131072", "00123"])
+    def test_filters_004_01(self, env, value):
+        r = self.check_config(env, value)
+        assert r.exit_code == 0, f"{r.stderr}"
+
+    @pytest.mark.parametrize("value", [
+        "abc", "123abc", "12.3", "0x10", "0", "-1", "9" * 40,
+    ])
+    def test_filters_004_02(self, env, value):
+        r = self.check_config(env, value)
+        assert r.exit_code != 0, f"{value} was accepted"
+        assert "BufferSize must be a size in bytes" in r.stderr