Created attachment 38592 [details] fix RewriteRule with QSA flag and substitution ending with ? after update from 2.4.55 to 2.4.57 we experience a change in behavior in our regression tests: RewriteRules having the [QSA] flag set and the substitution string ends with a question mark '?' (e.g 'RewriteRule "/do.php" "/it.php?" [QSA]') do not append the query from the original URL anymore. With 2.4.55 a query from the original request URL was appended to the resulting URL. Now with 2.4.57 the query is always empty. If the substitution includes a parameter (e.g "/it.php?a=1") [QSA] flag works as expected. I would argue, a substitution with a '?' at the end represents an 'empty' query, where with the [QSA] flag set, the query from the original request must be appended to. The change was most probably introduced by this commit https://svn.apache.org/viewvc?view=revision&revision=1908097. I`ve written a small patch (attached) to revert the old behavior if the [QSA] flag is set. This maybe to primitive but would fix our regression tests.
Thanks Frank, checking it out
Committed with a minor tweak to remove the trailing ? in both cases. Will propose to backport. Added a testcase covering trailing ? and QSA together.
Hi Eric, unfortunately the revised patch still leads to a regression in our test suite. With RewriteRule: `RewriteRule ^/mod_rewrite_before(.*)$ /mod_rewrite_after$1? [QSA,QSL]` and a request with encoded ? (%3f) in the path: `curl 'http://localhost/mod_rewrite_before/%3fxxx?param=123'` leads now to splitting of the request path into the query. With 2.4.55: the the query was 'param=123' but with 2.4.57 (even with your patch) is: 'xxx¶m=123' In this case we use the '?' at the end of the substitution in combination with the [QSL] flag to secure the request from being split. with mod_rewrite:trace8 i see the following difference in the logs: 2.4.55: `mod_rewrite.c(486): ... split uri=/mod_rewrite_after/?xxx? -> uri=/mod_rewrite_after/?xxx, args=param=123` 2.4.57: `mod_rewrite.c(493): ... split uri=/mod_rewrite_after/?xxx -> uri=/mod_rewrite_after/, args=xxx¶m=123` Disclaimer: We have written our own proxy module, so I am not entirely sure if the issue does only exist for us. But I think it is a general problem.
(In reply to Frank Meier from comment #3) > Hi Eric, > unfortunately the revised patch still leads to a regression in our test > suite. > > With RewriteRule: > `RewriteRule ^/mod_rewrite_before(.*)$ /mod_rewrite_after$1? [QSA,QSL]` > > and a request with encoded ? (%3f) in the path: > `curl 'http://localhost/mod_rewrite_before/%3fxxx?param=123'` > > leads now to splitting of the request path into the query. With 2.4.55: the > the query was 'param=123' but with 2.4.57 (even with your patch) is: > 'xxx¶m=123' > > In this case we use the '?' at the end of the substitution in combination > with the [QSL] flag to secure the request from being split. > > with mod_rewrite:trace8 i see the following difference in the logs: > 2.4.55: `mod_rewrite.c(486): ... split uri=/mod_rewrite_after/?xxx? -> > uri=/mod_rewrite_after/?xxx, args=param=123` > 2.4.57: `mod_rewrite.c(493): ... split uri=/mod_rewrite_after/?xxx -> > uri=/mod_rewrite_after/, args=xxx¶m=123` Maybe we can tie this to QSL then, because that's what expects it to hang around? - *(a2_end-1) = '\0'; /* trailing ? has done its job */ + if (!(newrule->flags & RULEFLAG_QSLAST)) { + *(a2_end-1) = '\0'; /* trailing ? has done its job */ + }
I did an alternate addon fix in 1910650, making the flow look more like the the pre-CVE behavior.
(In reply to Eric Covener from comment #5) > I did an alternate addon fix in 1910650, making the flow look more like the > the pre-CVE behavior. back to your original patch now :)
Created attachment 38594 [details] current diff diff from last release
Hi Eric, I just wanted to let you know, that with your current patches from trunk (1910662 + r1910666), we do not experience further failing regression tests. Do you think this code is "final" and will be backported? TL;DR Maybe the code is not final, but there are some typos in the comments: * `/* avoid getting a a query string via inadvertent capture */` -> `..getting a query..` * mentioned function `splitoutqueryargs` should be `splitout_queryargs` (two occurrences) Regards, Frank
backported -- will be in 2.4.58
This was backported in 2.4.x branch in r1910999 and is part of version 2.4.58.