From ba3fa296644994fc748b03cd8cb87bb282a6b5c6 Mon Sep 17 00:00:00 2001 From: Arturo Bernal Date: Thu, 15 Oct 2026 03:41:47 +0000 Subject: [PATCH] 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 | 4 ++ include/ap_mmn.h | 3 +- include/httpd.h | 12 ++++ modules/aaa/mod_auth_form.c | 2 +- modules/cache/mod_cache_disk.c | 8 +-- modules/cache/mod_cache_socache.c | 10 +-- modules/dav/fs/mod_dav_fs.c | 5 +- modules/filters/mod_buffer.c | 4 +- modules/generators/mod_cgi.c | 18 +++++- modules/generators/mod_cgid.c | 18 +++++- server/core.c | 9 ++- server/util.c | 17 +++++ test/modules/core/test_012_config_off_t.py | 65 ++++++++++++++++++++ test/modules/dav/test_003_config_quota.py | 35 +++++++++++ test/modules/filters/env.py | 2 +- test/modules/filters/test_004_buffer_size.py | 31 ++++++++++ 16 files changed, 220 insertions(+), 23 deletions(-) create mode 100644 changes-entries/size-parameter-parse.txt create mode 100644 test/modules/core/test_012_config_off_t.py create mode 100644 test/modules/dav/test_003_config_quota.py create mode 100644 test/modules/filters/test_004_buffer_size.py diff --git a/changes-entries/size-parameter-parse.txt b/changes-entries/size-parameter-parse.txt new file mode 100644 index 0000000000..51052f7800 --- /dev/null +++ b/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] diff --git a/include/ap_mmn.h b/include/ap_mmn.h index 0fa3cf233a..b69d07b74a 100644 --- a/include/ap_mmn.h +++ b/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 diff --git a/include/httpd.h b/include/httpd.h index f08c13fa1b..81ac0e80f4 100644 --- a/include/httpd.h +++ b/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. * diff --git a/modules/aaa/mod_auth_form.c b/modules/aaa/mod_auth_form.c index d443092aa7..38f30d1a3b 100644 --- a/modules/aaa/mod_auth_form.c +++ b/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."; } diff --git a/modules/cache/mod_cache_disk.c b/modules/cache/mod_cache_disk.c index ecc037867a..17e22a32f1 100644 --- a/modules/cache/mod_cache_disk.c +++ b/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."; diff --git a/modules/cache/mod_cache_socache.c b/modules/cache/mod_cache_socache.c index 38f1bfb279..78bd3e7bb4 100644 --- a/modules/cache/mod_cache_socache.c +++ b/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."; } diff --git a/modules/dav/fs/mod_dav_fs.c b/modules/dav/fs/mod_dav_fs.c index 607d45b221..7326a83080 100644 --- a/modules/dav/fs/mod_dav_fs.c +++ b/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; } diff --git a/modules/filters/mod_buffer.c b/modules/filters/mod_buffer.c index 203e672450..9f6519b0e3 100644 --- a/modules/filters/mod_buffer.c +++ b/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; diff --git a/modules/generators/mod_cgi.c b/modules/generators/mod_cgi.c index 61d888dd94..bfe45d01c1 100644 --- a/modules/generators/mod_cgi.c +++ b/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; } diff --git a/modules/generators/mod_cgid.c b/modules/generators/mod_cgid.c index 543277112d..9e92b767e9 100644 --- a/modules/generators/mod_cgid.c +++ b/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; } diff --git a/server/core.c b/server/core.c index e2a94a049e..a1a7fb5228 100644 --- a/server/core.c +++ b/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; } diff --git a/server/util.c b/server/util.c index 046a9a1a8b..ec9d33f66d 100644 --- a/server/util.c +++ b/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) diff --git a/test/modules/core/test_012_config_off_t.py b/test/modules/core/test_012_config_off_t.py new file mode 100644 index 0000000000..c7b3bd477f --- /dev/null +++ b/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}" diff --git a/test/modules/dav/test_003_config_quota.py b/test/modules/dav/test_003_config_quota.py new file mode 100644 index 0000000000..0e711591e3 --- /dev/null +++ b/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 diff --git a/test/modules/filters/env.py b/test/modules/filters/env.py index c0aba5f25c..0c2c5fbdbf 100644 --- a/test/modules/filters/env.py +++ b/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() diff --git a/test/modules/filters/test_004_buffer_size.py b/test/modules/filters/test_004_buffer_size.py new file mode 100644 index 0000000000..0d8bf39196 --- /dev/null +++ b/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(["", f" BufferSize {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", ["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