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