diff --git a/changes-entries/cgid-env-limit.txt b/changes-entries/cgid-env-limit.txt new file mode 100644 index 00000000000..48eca60c936 --- /dev/null +++ b/changes-entries/cgid-env-limit.txt @@ -0,0 +1,4 @@ + *) mod_cgid: Raise the limit on the number of environment variables + passed to a script from 256 to APACHE_ARG_MAX (4096 on most + platforms). PR 70270. [Emmanuel Dreyfus , + Joe Orton] diff --git a/modules/generators/mod_cgid.c b/modules/generators/mod_cgid.c index 9e92b767e96..4a4c7ad5c54 100644 --- a/modules/generators/mod_cgid.c +++ b/modules/generators/mod_cgid.c @@ -192,7 +192,9 @@ typedef struct { } cgid_rlimit_t; #endif -#define ENV_COUNT_MAX (256) +/* Sanity bound on the length of one environment variable value + * received by the daemon. */ +#define ENV_VAL_MAX (128 * 1024) typedef struct { int req_type; /* request type (CGI_REQ, SSI_REQ, etc.) */ @@ -504,7 +506,8 @@ static apr_status_t sock_writev(int fd, int auxfd, request_rec *r, int count, .. return APR_SUCCESS; } -static apr_status_t get_req(int fd, request_rec *r, char **argv0, char ***env, +static apr_status_t get_req(int fd, server_rec *main_server, request_rec *r, + char **argv0, char ***env, int *errfd, cgid_req_t *req) { int i; @@ -532,11 +535,11 @@ static apr_status_t get_req(int fd, request_rec *r, char **argv0, char ***env, } /* Sanity check the structure received. */ - if (req->env_count > ENV_COUNT_MAX + if (req->env_count > APACHE_ARG_MAX || req->filename_len == 0 || req->filename_len > APR_PATH_MAX || req->argv0_len == 0 || req->argv0_len > APR_PATH_MAX - || req->uri_len == 0 || req->uri_len > APR_PATH_MAX - || req->args_len > APR_PATH_MAX) { + || req->uri_len == 0 || req->uri_len > main_server->limit_req_line + || req->args_len > main_server->limit_req_line) { return APR_EINVAL; } @@ -573,7 +576,7 @@ static apr_status_t get_req(int fd, request_rec *r, char **argv0, char ***env, if ((stat = sock_read(fd, &curlen, sizeof(curlen))) != APR_SUCCESS) { return stat; } - if (curlen > APR_PATH_MAX) { + if (curlen > ENV_VAL_MAX) { return APR_EINVAL; } environ[i] = apr_pcalloc(r->pool, curlen + 1); @@ -614,6 +617,9 @@ static apr_status_t send_req(int fd, apr_file_t *errpipe, request_rec *r, for (req.env_count = 0; env[req.env_count]; req.env_count++) { continue; } + if (req.env_count > APACHE_ARG_MAX) { + return APR_ENOSPC; + } req.filename_len = strlen(r->filename); req.argv0_len = strlen(argv0); req.uri_len = strlen(r->uri); @@ -846,7 +852,8 @@ static int cgid_server(void *data) r = apr_pcalloc(ptrans, sizeof(request_rec)); procnew = apr_pcalloc(ptrans, sizeof(*procnew)); r->pool = ptrans; - stat = get_req(sd2, r, &argv0, &env, &errfileno, &cgid_req); + stat = get_req(sd2, main_server, r, + &argv0, &env, &errfileno, &cgid_req); if (stat != APR_SUCCESS) { ap_log_error(APLOG_MARK, APLOG_ERR, stat, main_server, APLOGNO(01248) diff --git a/test/modules/generators/htdocs/cgi/arglen.py b/test/modules/generators/htdocs/cgi/arglen.py new file mode 100644 index 00000000000..3d874332a6d --- /dev/null +++ b/test/modules/generators/htdocs/cgi/arglen.py @@ -0,0 +1,5 @@ +#!/usr/bin/env python3 +import os + +print("Content-Type: text/plain\n") +print(len(os.environ.get("QUERY_STRING", ""))) diff --git a/test/modules/generators/htdocs/cgi/envcount.py b/test/modules/generators/htdocs/cgi/envcount.py new file mode 100644 index 00000000000..11e1d2a857c --- /dev/null +++ b/test/modules/generators/htdocs/cgi/envcount.py @@ -0,0 +1,5 @@ +#!/usr/bin/env python3 +import os + +print("Content-Type: text/plain\n") +print(len(os.environ)) diff --git a/test/modules/generators/test_001_cgid.py b/test/modules/generators/test_001_cgid.py index 65bde24c481..aef9eba8c9c 100644 --- a/test/modules/generators/test_001_cgid.py +++ b/test/modules/generators/test_001_cgid.py @@ -24,6 +24,7 @@ def _class_scope(self, env): conf = HttpdConf(env, extras={ 'base': textwrap.dedent(f""" LogLevel cgid:trace1 + LimitRequestLine 16384 Options +Includes AcceptPathInfo On @@ -43,12 +44,19 @@ def _class_scope(self, env): CGIDScriptTimeout 1 - """), + """) + self.setenv_location("/envcount.py/small", 300) + + self.setenv_location("/envcount.py/large", 4200), }) conf.add_vhost_cgi() conf.install() assert env.apache_restart() == 0 + @staticmethod + def setenv_location(path, count): + setenvs = "\n".join(f"SetEnv CGID_TEST_VAR_{i:04} v" + for i in range(count)) + return f'\n{setenvs}\n\n' + def script_log(self, env): return os.path.join(env.server_logs_dir, "cgid_script.log") @@ -133,6 +141,28 @@ def test_generators_001_07_body_not_read(self, env): assert r.response["status"] == 200 assert r.response["body"].decode() == "not reading\n" + def test_generators_001_08_env_count_large(self, env): + """More than 256 environment variables reach the script.""" + r = self.get(env, "cgi", "/envcount.py/small") + assert r.response["status"] == 200 + assert int(r.response["body"]) >= 300 + + def test_generators_001_09_env_count_overflow(self, env): + """A request environment larger than APACHE_ARG_MAX is refused.""" + r = self.get(env, "cgi", "/envcount.py/large") + assert r.response["status"] == 503 + # the daemon sees the aborted connection as a read error + env.httpd_error_log.ignore_recent(lognos=["AH10245", "AH01248"]) + + def test_generators_001_15_long_query(self, env): + """A 5000-character request URI, with a query string longer + than PATH_MAX, reaches the script.""" + path = "/arglen.py" + query = "x=" + "y" * (5000 - len(path) - len("?x=")) + r = self.get(env, "cgi", f"{path}?{query}") + assert r.response["status"] == 200 + assert int(r.response["body"]) == len(query) + def test_generators_001_10_ssi_exec_cgi(self, env): """#exec cgi= runs the script with the document's query string.""" r = self.get(env, "cgi", "/ssi/exec_cgi.shtml?hello+world")