From 690d14bcad1df3f08152cb9ea41c21ae5d85aa1b Mon Sep 17 00:00:00 2001 From: Yann Ylavic Date: Thu, 1 Aug 2024 14:43:58 +0000 Subject: [PATCH 1/5] mod_proxy_fcgi: Don't re-encode SCRIPT_FILENAME. PR 69203 Before r1918550 (r1918559 in 2.4.60), "SetHandler proxy:..." configurations did not pass through proxy_fixup() hence the proxy_canon_handler hooks, leaving fcgi's SCRIPT_FILENAME environment variable (from r->filename) decoded, or more exactly not re-encoded. We still want to call ap_proxy_canon_url() for "fcgi:" to handle/strip the UDS "unix:" case and check that r->filename is valid and contains no controls, but proxy_fcgi_canon() will not ap_proxy_canonenc_ex() thus re-encode anymore. Note that this will do the same for "ProxyPass fcgi:...", there is no reason that using SetHandler or ProxyPass don't result in the same thing. If an opt in/out makes sense we should probably look at ProxyFCGIBackendType. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1919620 13f79535-47bb-0310-9956-ffa450edef68 --- changes-entries/bz69203.txt | 1 + modules/proxy/mod_proxy_fcgi.c | 38 +++++++++++++--------------------- 2 files changed, 15 insertions(+), 24 deletions(-) create mode 100644 changes-entries/bz69203.txt diff --git a/changes-entries/bz69203.txt b/changes-entries/bz69203.txt new file mode 100644 index 00000000000..3613dad7636 --- /dev/null +++ b/changes-entries/bz69203.txt @@ -0,0 +1 @@ +mod_proxy_fcgi: Don't re-encode SCRIPT_FILENAME. PR 69203. [Yann Ylavic] \ No newline at end of file diff --git a/modules/proxy/mod_proxy_fcgi.c b/modules/proxy/mod_proxy_fcgi.c index d420df6a77a..b5e7c01abdb 100644 --- a/modules/proxy/mod_proxy_fcgi.c +++ b/modules/proxy/mod_proxy_fcgi.c @@ -59,7 +59,7 @@ static int proxy_fcgi_canon(request_rec *r, char *url) { char *host, sport[7]; const char *err; - char *path; + char *path = url, *pc; apr_port_t port, def_port; fcgi_req_config_t *rconf = NULL; const char *pathinfo_type = NULL; @@ -75,7 +75,7 @@ static int proxy_fcgi_canon(request_rec *r, char *url) ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, "canonicalising URL %s", url); - err = ap_proxy_canon_netloc(r->pool, &url, NULL, NULL, &host, &port); + err = ap_proxy_canon_netloc(r->pool, &path, NULL, NULL, &host, &port); if (err) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01059) "error parsing URL %s: %s", url, err); @@ -92,29 +92,19 @@ static int proxy_fcgi_canon(request_rec *r, char *url) host = apr_pstrcat(r->pool, "[", host, "]", NULL); } - if (apr_table_get(r->notes, "proxy-nocanon") - || apr_table_get(r->notes, "proxy-noencode")) { - path = url; /* this is the raw/encoded path */ - } - else { - core_dir_config *d = ap_get_core_module_config(r->per_dir_config); - int flags = d->allow_encoded_slashes && !d->decode_encoded_slashes ? PROXY_CANONENC_NOENCODEDSLASHENCODING : 0; - - path = ap_proxy_canonenc_ex(r->pool, url, strlen(url), enc_path, flags, - r->proxyreq); - if (!path) { - return HTTP_BAD_REQUEST; - } - } - /* - * If we have a raw control character or a ' ' in nocanon path, - * correct encoding was missed. + /* We do not call ap_proxy_canonenc_ex() on the path here because the CGI + * environment variable SCRIPT_FILENAME based on it want the decoded/local + * path, don't let control characters pass still. + * + * XXX: should we encode based on dconf->backend_type though? */ - if (path == url && *ap_scan_vchar_obstext(path)) { - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10414) - "To be forwarded path contains control " - "characters or spaces"); - return HTTP_FORBIDDEN; + for (pc = path; *pc; pc++) { + if (apr_iscntrl(*pc)) { + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10414) + "To be forwarded path contains control " + "characters"); + return HTTP_FORBIDDEN; + } } r->filename = apr_pstrcat(r->pool, "proxy:fcgi://", host, sport, "/", From b8b45e0a83d78b40c5378e46d467c8cc09a77a8c Mon Sep 17 00:00:00 2001 From: Yann Ylavic Date: Thu, 1 Aug 2024 15:20:16 +0000 Subject: [PATCH 2/5] Follow up to r1919620: CHANGES entry indent. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1919621 13f79535-47bb-0310-9956-ffa450edef68 --- changes-entries/bz69203.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changes-entries/bz69203.txt b/changes-entries/bz69203.txt index 3613dad7636..4d9080ba37c 100644 --- a/changes-entries/bz69203.txt +++ b/changes-entries/bz69203.txt @@ -1 +1 @@ -mod_proxy_fcgi: Don't re-encode SCRIPT_FILENAME. PR 69203. [Yann Ylavic] \ No newline at end of file + *) mod_proxy_fcgi: Don't re-encode SCRIPT_FILENAME. PR 69203. [Yann Ylavic] \ No newline at end of file From fed3dde1989f164766ac7bd120ff83a3d4dce056 Mon Sep 17 00:00:00 2001 From: Yann Ylavic Date: Thu, 1 Aug 2024 16:09:14 +0000 Subject: [PATCH 3/5] Follow up to r1919620: init path after "proxy:" is skipped. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1919623 13f79535-47bb-0310-9956-ffa450edef68 --- modules/proxy/mod_proxy_fcgi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/modules/proxy/mod_proxy_fcgi.c b/modules/proxy/mod_proxy_fcgi.c index b5e7c01abdb..5535ed464fc 100644 --- a/modules/proxy/mod_proxy_fcgi.c +++ b/modules/proxy/mod_proxy_fcgi.c @@ -59,7 +59,7 @@ static int proxy_fcgi_canon(request_rec *r, char *url) { char *host, sport[7]; const char *err; - char *path = url, *pc; + char *path, *pc; apr_port_t port, def_port; fcgi_req_config_t *rconf = NULL; const char *pathinfo_type = NULL; @@ -71,6 +71,7 @@ static int proxy_fcgi_canon(request_rec *r, char *url) return DECLINED; } + path = url; port = def_port = ap_proxy_port_of_scheme("fcgi"); ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, From d4f6ad9ec6e8f23d85784aec7474f4dca78348db Mon Sep 17 00:00:00 2001 From: Yann Ylavic Date: Fri, 2 Aug 2024 00:53:53 +0000 Subject: [PATCH 4/5] Follow up to r1919620: Restore r->filename re-encoding for ProxyPass URLs. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1919628 13f79535-47bb-0310-9956-ffa450edef68 --- changes-entries/bz69203.txt | 3 ++- modules/proxy/mod_proxy.c | 2 ++ modules/proxy/mod_proxy_fcgi.c | 48 +++++++++++++++++++++++++--------- 3 files changed, 39 insertions(+), 14 deletions(-) diff --git a/changes-entries/bz69203.txt b/changes-entries/bz69203.txt index 4d9080ba37c..2408352a3bd 100644 --- a/changes-entries/bz69203.txt +++ b/changes-entries/bz69203.txt @@ -1 +1,2 @@ - *) mod_proxy_fcgi: Don't re-encode SCRIPT_FILENAME. PR 69203. [Yann Ylavic] \ No newline at end of file + *) mod_proxy_fcgi: Don't re-encode SCRIPT_FILENAME when set via SetHandler. + PR 69203. [Yann Ylavic] \ No newline at end of file diff --git a/modules/proxy/mod_proxy.c b/modules/proxy/mod_proxy.c index 756c41c4a1d..16cd5aaef2d 100644 --- a/modules/proxy/mod_proxy.c +++ b/modules/proxy/mod_proxy.c @@ -1240,6 +1240,7 @@ static int proxy_handler(request_rec *r) r->proxyreq = PROXYREQ_REVERSE; r->filename = apr_pstrcat(r->pool, r->handler, r->filename, NULL); + apr_table_setn(r->notes, "proxy-sethandler", "1"); /* Still need to canonicalize r->filename */ rc = ap_proxy_canon_url(r); @@ -1249,6 +1250,7 @@ static int proxy_handler(request_rec *r) } } else if (r->proxyreq && strncmp(r->filename, "proxy:", 6) == 0) { + apr_table_unset(r->notes, "proxy-sethandler"); rc = OK; } if (rc != OK) { diff --git a/modules/proxy/mod_proxy_fcgi.c b/modules/proxy/mod_proxy_fcgi.c index 5535ed464fc..5575dca7b2e 100644 --- a/modules/proxy/mod_proxy_fcgi.c +++ b/modules/proxy/mod_proxy_fcgi.c @@ -59,10 +59,13 @@ static int proxy_fcgi_canon(request_rec *r, char *url) { char *host, sport[7]; const char *err; - char *path, *pc; + char *path; apr_port_t port, def_port; fcgi_req_config_t *rconf = NULL; const char *pathinfo_type = NULL; + fcgi_dirconf_t *dconf = ap_get_module_config(r->per_dir_config, + &proxy_fcgi_module); + int from_handler; if (ap_cstr_casecmpn(url, "fcgi:", 5) == 0) { url += 5; @@ -71,12 +74,11 @@ static int proxy_fcgi_canon(request_rec *r, char *url) return DECLINED; } - path = url; port = def_port = ap_proxy_port_of_scheme("fcgi"); ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, "canonicalising URL %s", url); - err = ap_proxy_canon_netloc(r->pool, &path, NULL, NULL, &host, &port); + err = ap_proxy_canon_netloc(r->pool, &url, NULL, NULL, &host, &port); if (err) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01059) "error parsing URL %s: %s", url, err); @@ -93,20 +95,40 @@ static int proxy_fcgi_canon(request_rec *r, char *url) host = apr_pstrcat(r->pool, "[", host, "]", NULL); } - /* We do not call ap_proxy_canonenc_ex() on the path here because the CGI - * environment variable SCRIPT_FILENAME based on it want the decoded/local - * path, don't let control characters pass still. - * - * XXX: should we encode based on dconf->backend_type though? - */ - for (pc = path; *pc; pc++) { - if (apr_iscntrl(*pc)) { + from_handler = apr_table_get(r->notes, "proxy-sethandler") != NULL; + if (from_handler + || apr_table_get(r->notes, "proxy-nocanon") + || apr_table_get(r->notes, "proxy-noencode")) { + char *c = path = url; /* this is the raw path */ + + /* We do not call ap_proxy_canonenc_ex() on the path here, don't + * let control characters pass still, and for php-fpm no '?' either. + */ + if (FCGI_MAY_BE_FPM(dconf)) { + while (!apr_iscntrl(*c) && *c != '?') + c++; + } + else { + while (!apr_iscntrl(*c)) + c++; + } + if (*c) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10414) - "To be forwarded path contains control " - "characters"); + "To be forwarded path contains control characters%s", + FCGI_MAY_BE_FPM(dconf) ? " or '?'" : ""); return HTTP_FORBIDDEN; } } + else { + core_dir_config *d = ap_get_core_module_config(r->per_dir_config); + int flags = d->allow_encoded_slashes && !d->decode_encoded_slashes ? PROXY_CANONENC_NOENCODEDSLASHENCODING : 0; + + path = ap_proxy_canonenc_ex(r->pool, url, strlen(url), enc_path, flags, + r->proxyreq); + if (!path) { + return HTTP_BAD_REQUEST; + } + } r->filename = apr_pstrcat(r->pool, "proxy:fcgi://", host, sport, "/", path, NULL); From 07279f7c0ced31e2b18f17c0375217df427b2aa9 Mon Sep 17 00:00:00 2001 From: Yann Ylavic Date: Thu, 10 Oct 2024 15:36:48 +0000 Subject: [PATCH 5/5] mod_proxy_fgci: Follow up to r1919628: Simplify. Variable from_handler is used once so axe it. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1921237 13f79535-47bb-0310-9956-ffa450edef68 --- modules/proxy/mod_proxy_fcgi.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/modules/proxy/mod_proxy_fcgi.c b/modules/proxy/mod_proxy_fcgi.c index 5575dca7b2e..50f443e50d9 100644 --- a/modules/proxy/mod_proxy_fcgi.c +++ b/modules/proxy/mod_proxy_fcgi.c @@ -65,7 +65,6 @@ static int proxy_fcgi_canon(request_rec *r, char *url) const char *pathinfo_type = NULL; fcgi_dirconf_t *dconf = ap_get_module_config(r->per_dir_config, &proxy_fcgi_module); - int from_handler; if (ap_cstr_casecmpn(url, "fcgi:", 5) == 0) { url += 5; @@ -95,11 +94,10 @@ static int proxy_fcgi_canon(request_rec *r, char *url) host = apr_pstrcat(r->pool, "[", host, "]", NULL); } - from_handler = apr_table_get(r->notes, "proxy-sethandler") != NULL; - if (from_handler + if (apr_table_get(r->notes, "proxy-sethandler") || apr_table_get(r->notes, "proxy-nocanon") || apr_table_get(r->notes, "proxy-noencode")) { - char *c = path = url; /* this is the raw path */ + char *c = url; /* We do not call ap_proxy_canonenc_ex() on the path here, don't * let control characters pass still, and for php-fpm no '?' either. @@ -114,10 +112,12 @@ static int proxy_fcgi_canon(request_rec *r, char *url) } if (*c) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10414) - "To be forwarded path contains control characters%s", - FCGI_MAY_BE_FPM(dconf) ? " or '?'" : ""); + "To be forwarded path contains control characters%s (%s)", + FCGI_MAY_BE_FPM(dconf) ? " or '?'" : "", url); return HTTP_FORBIDDEN; } + + path = url; /* this is the raw path */ } else { core_dir_config *d = ap_get_core_module_config(r->per_dir_config);