On 5/18/23 3:17 AM, Eric Covener wrote:
> bump? Just was reminded by a thread on reddit (config unclear but
> probably not non-cfgi proxy as it's a PHP app)
>
> If the proxy modules would trap it, and the encoded spaces were
> happily accepted by other modules before the fix, can we let spaces
> through mod_rewrite?
Any reason why we should not encode the spaces as proposed below?
>
> On Tue, May 9, 2023 at 6:18 PM Eric Covener <cove...@gmail.com> wrote:
>>
>> On Tue, May 9, 2023 at 3:14 PM Ruediger Pluem <rpl...@apache.org> wrote:
>>>
>>>
>>>
>>> On 5/9/23 8:01 PM, Eric Covener wrote:
>>>> On Tue, May 9, 2023 at 11:51 AM Ruediger Pluem <rpl...@apache.org> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 5/9/23 4:33 PM, Yann Ylavic wrote:
>>>>>> On Tue, May 9, 2023 at 2:10 PM Yann Ylavic <ylavic....@gmail.com> wrote:
>>>>>>>
>>>>>>> On Tue, May 9, 2023 at 12:55 PM Ruediger Pluem <rpl...@apache.org>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> On 5/9/23 12:16 PM, Eric Covener wrote:
>>>>>>>>> Still getting feedback in the PR about breakage. Any thoughts on
>>>>>>>>> options here, like allowing spaces or encoding rather than failing?
>>>>>>>>
>>>>>>>> Allowing spaces is out of question for me as it creates an invalid
>>>>>>>> request and opens the door to response splitting. At best we
>>>>>>>> could encode automatically. It is really a good question if we could
>>>>>>>> not make BCTLS the default.
>>>>>>>
>>>>>>> BCTLS by default looks fine to me, it changes the behaviour for users
>>>>>>> that (used to) expect/handle decoded spaces in the query-string in
>>>>>>> their scripts, but it's blocked now anyway..
>>>>>>
>>>>>> Hm, actually we don't really know where the backref is placed (either
>>>>>> in the uri-path or in the query-string), so escaping unconditionally
>>>
>>> Good point. Hence I think the only options left are that people fix their
>>> configurations or
>>> that we scan r->args and encode all spaces. But this leaves the question
>>> open: Encode to what? '+' or %20?
>>> Maybe we can put this job somehow in splitout_queryargs? It would have
>>> access to the RewriteRule flags and
>>> we could decide based on if RULEFLAG_ESCAPENOPLUS is set or not.
>>>
>>> Something along the following (partly pseudocode, not optimized):
>>>
>>> Index: modules/mappers/mod_rewrite.c
>>> ===================================================================
>>> --- modules/mappers/mod_rewrite.c (revision 1909373)
>>> +++ modules/mappers/mod_rewrite.c (working copy)
>>> @@ -854,6 +854,31 @@
>>> }
>>> }
>>>
>>> + if (global_option_encode_spaces) {
>>> + if (flags & RULEFLAG_ESCAPENOPLUS) {
>>> + char *p1, p2;
>>> +
>>> + p1 = apr_palloc(r->pool, strlen(r->args) * 3 + 1);
>>> + for (p2 = r->args; *p2; p2++) {
>>> + if (*p2 == ' ') {
>>> + strcpy(p1, "%20");
>>> + p1 += 3;
>>> + }
>>> + else {
>>> + *p1++ = *p2;
>>> + }
>>> + }
>>> + *p1 = '\0';
>>> + }
>>> + else {
>>> + for (char *pos = r->args; *pos; pos++) {
>>> + if (*pos == ' ') {
>>> + *pos = '+';
>>> + }
>>> + }
>>> + }
>>> + }
>>> +
>>> rewritelog(r, 3, NULL, "split uri=%s -> uri=%s, args=%s", olduri,
>>> r->filename, r->args ? r->args : "<none>");
>>> }
>>>
>>>>>> might lead to double-escaping in the uri-path. Maybe it's simpler to
>>>>>> remove the check and leave it to mod_proxy only..
>>>>>
>>>>> Do we have an example config where this currently breaks that does not
>>>>> forward it to a proxy backend?
>>>>
>>>> >From some of the GH activity, I think there is some mod_proxy_fcgi
>>>> sending to FPM and PHP. In this case the query is in an environment
>>>> variable.
>>>>
>>>> Are spaces over proxy_http really that much of a smoking gun? Wouldn't
>>>> any smuggling/splitting/desynch already be fatal if it's in the
>>>> request line? It can't be terminated early if we only allow spaces.
>>>
>>> You mean if there are just spaces and no control characters? I am not sure
>>> if you can do smuggling without \r and/or \n, but
>>> the HTTP RFC is strict about what is allowed in the request URL and literal
>>> spaces are definitely not allowed.
>>> If we allow them we send a non RFC compliant HTTP request to the backend. I
>>> think we must not do this.
>>
>> I was assuming the 403s are coming from mod_rewrite, not the proxy
>> modules, and that we'd only change the former. But it's not 100%
>> clear for the people following up in the PR.
>>
>> If it's true, the proxy modules would then still return an error
>> before putting unencoded/decoded spaces into the query.
>
Regards
Rüdiger
