Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BZ-69203: backport to 2.4.x #470

Open
wants to merge 5 commits into
base: 2.4.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changes-entries/bz69203.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
*) mod_proxy_fcgi: Don't re-encode SCRIPT_FILENAME when set via SetHandler.
PR 69203. [Yann Ylavic]
2 changes: 2 additions & 0 deletions modules/proxy/mod_proxy.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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) {
Expand Down
37 changes: 25 additions & 12 deletions modules/proxy/mod_proxy_fcgi.c
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ static int proxy_fcgi_canon(request_rec *r, char *url)
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);

if (ap_cstr_casecmpn(url, "fcgi:", 5) == 0) {
url += 5;
Expand Down Expand Up @@ -92,9 +94,30 @@ 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")
if (apr_table_get(r->notes, "proxy-sethandler")
|| apr_table_get(r->notes, "proxy-nocanon")
|| apr_table_get(r->notes, "proxy-noencode")) {
path = url; /* this is the raw/encoded 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.
*/
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%s (%s)",
FCGI_MAY_BE_FPM(dconf) ? " or '?'" : "", url);
return HTTP_FORBIDDEN;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note that this changed from 404 to 403 (just tested the question mark case) compare to 2.4.59. But I think it's actually better error code now so should be fine.

}

path = url; /* this is the raw path */
}
else {
core_dir_config *d = ap_get_core_module_config(r->per_dir_config);
Expand All @@ -106,16 +129,6 @@ static int proxy_fcgi_canon(request_rec *r, char *url)
return HTTP_BAD_REQUEST;
}
}
/*
* If we have a raw control character or a ' ' in nocanon path,
* correct encoding was missed.
*/
if (path == url && *ap_scan_vchar_obstext(path)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have we intentionally lost failing on spaces here? I guess the original intent was about splitting with CRLF?

Copy link
Member Author

@ylavic ylavic Sep 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, IIUC that's the issue reported in BZ-69203.
Before 2.4.60+ we didn't call proxy_fcgi_canon() for SetHandler URLs, so SCRIPT_FILENAME was never encoded. Now that we do, this PR applies "nocanon" for SetHandler URLs, and lets spaces through because supposedly they are not harmful in the SCRIPT_FILENAME env var (unlike in the uri-path for an HTTP request..).

ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10414)
"To be forwarded path contains control "
"characters or spaces");
return HTTP_FORBIDDEN;
}

r->filename = apr_pstrcat(r->pool, "proxy:fcgi://", host, sport, "/",
path, NULL);
Expand Down
Loading