BZ-69203: backport to 2.4.x #470

ylavic · 2024-08-01T15:03:17Z

r1919620
r1919621
r1919623
r1919628
r1921237

covener · 2024-08-13T14:24:10Z

modules/proxy/mod_proxy_fcgi.c

-     * If we have a raw control character or a ' ' in nocanon path,
-     * correct encoding was missed.
-     */
-    if (path == url && *ap_scan_vchar_obstext(path)) {


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

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..).

rpluem · 2024-08-15T07:44:58Z

modules/proxy/mod_proxy_fcgi.c

+    if (rconf && rconf->need_dirwalk) {
+        char *saved_filename = r->filename;
+        r->filename = r->uri;
+        ap_directory_walk(r);


See https://lists.apache.org/thread/4gw6tsp0xlpdf3l9gtz1jnr5wzycsfg8:

Why not using the result of the below Strip proxy: prefixes for r->filename? In case of a Rewriterule I guess r->uri and r->filename could be fundamentally different path wise.

Maybe it helps to say a little about this obscure option to try to calculate PATH_INFO. It's meant/doc'ed for ProxyPass/ProxyPassMatch configurations where there was no mapping to FS hence no split off for PATH_INFO.

I don't think any of these proxy-fcgi-pathinfo options are widely used. I guess if r->filename had proxy: prefix stuff in it this was maybe useless since inception hence no users?

OTOH like you say, just peeling it all off as we will already do below would seem to give a better result assuming the user doesn't intend to keep some "original" PATH_INFO.

Why not using the result of the below Strip proxy: prefixes for r->filename? In case of a Rewriterule I guess r->uri and r->filename could be fundamentally different path wise.

Done in [1] too, but this patch is not ready yet and will probably need more discussions (including with fpm maintainers..). I can take this bit from there though, will do!

[1] https://lists.apache.org/thread/vmz22tv2j8bz7vhw98hy3xwvqyp68cdc

Added 7441bb4 for this, thanks!

I'll create another PR for this because this has nothing to do with BZ-69203 afterall..

modules/proxy/mod_proxy_fcgi.c

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

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1919621 13f79535-47bb-0310-9956-ffa450edef68

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1919623 13f79535-47bb-0310-9956-ffa450edef68

…URLs. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1919628 13f79535-47bb-0310-9956-ffa450edef68

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

bastien-roucaries · 2024-10-12T16:23:52Z

What is the ETA for this pull request ?

bukka

I have done some testing with FPM (those are the test cases: https://github.com/wstool/wst-php-fpm/blob/bf390dd1dda196ea9339b4d3d26caf5fbf306226/spec/instances/httpd-proxy-fcgi-handler-basic.yaml ) and the only change required is the different status code for ? in path which is actually for better as I mentioned in the comment. So this looks like a reasonable change to me. I haven't tested UDS yet though as I need to extend my testing framework a little bit for that.

bukka · 2024-11-17T11:24:55Z

modules/proxy/mod_proxy_fcgi.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;


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.

bukka

I added similar tests for UDS and they all pass too. It seems to work fine so I think this is good to go from the PHP-FPM PoV.

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. Follow up to r1919620: CHANGES entry indent. Follow up to r1919620: init path after "proxy:" is skipped. Follow up to r1919620: Restore r->filename re-encoding for ProxyPass URLs. mod_proxy_fgci: Follow up to r1919628: Simplify. Variable from_handler is used once so axe it. Submitted by: ylavic Reviewed by: ylavic, covener, jorton Github: closes #470 git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1922080 13f79535-47bb-0310-9956-ffa450edef68

covener reviewed Aug 13, 2024

View reviewed changes

rpluem reviewed Aug 15, 2024

View reviewed changes

ylavic added 5 commits October 10, 2024 18:05

Follow up to r1919620: CHANGES entry indent.

b8b45e0

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1919621 13f79535-47bb-0310-9956-ffa450edef68

Follow up to r1919620: init path after "proxy:" is skipped.

fed3dde

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1919623 13f79535-47bb-0310-9956-ffa450edef68

Follow up to r1919620: Restore r->filename re-encoding for ProxyPass …

d4f6ad9

…URLs. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1919628 13f79535-47bb-0310-9956-ffa450edef68

mod_proxy_fgci: Follow up to r1919628: Simplify.

07279f7

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

ylavic force-pushed the bz_69203 branch from 7441bb4 to 07279f7 Compare October 10, 2024 16:08

bukka reviewed Nov 17, 2024

View reviewed changes

bukka approved these changes Nov 22, 2024

View reviewed changes

bukka mentioned this pull request Nov 27, 2024

PHP-FPM + Apache: special characters in SCRIPT_FILENAME are URL encoded php/php-src#15246

Open

bukka mentioned this pull request Dec 14, 2024

handle apache DocumentRoot cyrillic encoding php/php-src#17083

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BZ-69203: backport to 2.4.x #470

BZ-69203: backport to 2.4.x #470

ylavic commented Aug 1, 2024 •

edited

Loading

covener Aug 13, 2024

ylavic Sep 11, 2024 •

edited

Loading

rpluem Aug 15, 2024

covener Aug 15, 2024

ylavic Sep 11, 2024

ylavic Oct 10, 2024

ylavic Oct 10, 2024

ylavic Oct 10, 2024

bastien-roucaries commented Oct 12, 2024

bukka left a comment

bukka Nov 17, 2024

bukka left a comment

BZ-69203: backport to 2.4.x #470

Are you sure you want to change the base?

BZ-69203: backport to 2.4.x #470

Conversation

ylavic commented Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

ylavic Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bastien-roucaries commented Oct 12, 2024

bukka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bukka left a comment

Choose a reason for hiding this comment

ylavic commented Aug 1, 2024 •

edited

Loading

ylavic Sep 11, 2024 •

edited

Loading