Mailing List Archive

Fwd: [apache/httpd] don't forward invalid query strings (d78a166)
Still getting feedback in the PR about breakage. Any thoughts on options
here, like allowing spaces or encoding rather than failing?

---------- Forwarded message ---------
From: Ivan Zahariev <notifications@github.com>
Date: Tue, May 9, 2023, 5:25 AM
Subject: Re: [apache/httpd] don't forward invalid query strings (d78a166)
To: apache/httpd <httpd@noreply.github.com>
Cc: Eric Covener <covener@apache.org>, Mention <mention@noreply.github.com>


Hi @covener <https://github.com/covener>. This is impacting lots of
existing websites already.

What is the downside if BCTLS can be enabled by default with an Apache
config option, and there is a new flag to disable it in each RewriteRule in
the rare case where we need to forward a non-encoded URL?


Reply to this email directly, view it on GitHub
<https://github.com/apache/httpd/commit/d78a166fedd9d02c23e4b71d5f53bd9b2c4b9a51#commitcomment-112499050>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG5WPKUFT7PN7ND5BMAD5TXFIEQBANCNFSM6AAAAAAV34NVFM>
.
You are receiving this because you were mentioned.Message ID:
<apache/httpd/commit/d78a166fedd9d02c23e4b71d5f53bd9b2c4b9a51/112499050@
github.com>
Re: Fwd: [apache/httpd] don't forward invalid query strings (d78a166) [ In reply to ]
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.


>
> ---------- Forwarded message ---------
> From: *Ivan Zahariev* <notifications@github.com <mailto:notifications@github.com>>
> Date: Tue, May 9, 2023, 5:25 AM
> Subject: Re: [apache/httpd] don't forward invalid query strings (d78a166)
> To: apache/httpd <httpd@noreply.github.com <mailto:httpd@noreply.github.com>>
> Cc: Eric Covener <covener@apache.org <mailto:covener@apache.org>>, Mention <mention@noreply.github.com
> <mailto:mention@noreply.github.com>>
>
>
> Hi @covener <https://github.com/covener>. This is impacting lots of existing websites already.
>
> What is the downside if BCTLS can be enabled by default with an Apache config option, and there is a new flag to disable it in
> each RewriteRule in the rare case where we need to forward a non-encoded URL?
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/httpd/commit/d78a166fedd9d02c23e4b71d5f53bd9b2c4b9a51#commitcomment-112499050>, or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAG5WPKUFT7PN7ND5BMAD5TXFIEQBANCNFSM6AAAAAAV34NVFM>.
> You are receiving this because you were mentioned.Message ID:
> <apache/httpd/commit/d78a166fedd9d02c23e4b71d5f53bd9b2c4b9a51/112499050@github.com>
>

Regards

Rüdiger
Re: Fwd: [apache/httpd] don't forward invalid query strings (d78a166) [ In reply to ]
On Tue, May 9, 2023 at 12:55?PM Ruediger Pluem <rpluem@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..


Regards;
Yann.
Re: Fwd: [apache/httpd] don't forward invalid query strings (d78a166) [ In reply to ]
On Tue, May 9, 2023 at 2:10?PM Yann Ylavic <ylavic.dev@gmail.com> wrote:
>
> On Tue, May 9, 2023 at 12:55?PM Ruediger Pluem <rpluem@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
might lead to double-escaping in the uri-path. Maybe it's simpler to
remove the check and leave it to mod_proxy only..

>
>
> Regards;
> Yann.
Re: Fwd: [apache/httpd] don't forward invalid query strings (d78a166) [ In reply to ]
On 5/9/23 4:33 PM, Yann Ylavic wrote:
> On Tue, May 9, 2023 at 2:10?PM Yann Ylavic <ylavic.dev@gmail.com> wrote:
>>
>> On Tue, May 9, 2023 at 12:55?PM Ruediger Pluem <rpluem@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
> 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?

Regards

Rüdiger
Re: Fwd: [apache/httpd] don't forward invalid query strings (d78a166) [ In reply to ]
On Tue, May 9, 2023 at 11:51?AM Ruediger Pluem <rpluem@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.dev@gmail.com> wrote:
> >>
> >> On Tue, May 9, 2023 at 12:55?PM Ruediger Pluem <rpluem@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
> > 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.
Re: Fwd: [apache/httpd] don't forward invalid query strings (d78a166) [ In reply to ]
On 5/9/23 8:01 PM, Eric Covener wrote:
> On Tue, May 9, 2023 at 11:51?AM Ruediger Pluem <rpluem@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.dev@gmail.com> wrote:
>>>>
>>>> On Tue, May 9, 2023 at 12:55?PM Ruediger Pluem <rpluem@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.

Regards

Rüdiger
Re: Fwd: [apache/httpd] don't forward invalid query strings (d78a166) [ In reply to ]
On Tue, May 9, 2023 at 3:14?PM Ruediger Pluem <rpluem@apache.org> wrote:
>
>
>
> On 5/9/23 8:01 PM, Eric Covener wrote:
> > On Tue, May 9, 2023 at 11:51?AM Ruediger Pluem <rpluem@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.dev@gmail.com> wrote:
> >>>>
> >>>> On Tue, May 9, 2023 at 12:55?PM Ruediger Pluem <rpluem@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.
Re: Fwd: [apache/httpd] don't forward invalid query strings (d78a166) [ In reply to ]
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?

On Tue, May 9, 2023 at 6:18?PM Eric Covener <covener@gmail.com> wrote:
>
> On Tue, May 9, 2023 at 3:14?PM Ruediger Pluem <rpluem@apache.org> wrote:
> >
> >
> >
> > On 5/9/23 8:01 PM, Eric Covener wrote:
> > > On Tue, May 9, 2023 at 11:51?AM Ruediger Pluem <rpluem@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.dev@gmail.com> wrote:
> > >>>>
> > >>>> On Tue, May 9, 2023 at 12:55?PM Ruediger Pluem <rpluem@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.



--
Eric Covener
covener@gmail.com
Re: Fwd: [apache/httpd] don't forward invalid query strings (d78a166) [ In reply to ]
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 <covener@gmail.com> wrote:
>>
>> On Tue, May 9, 2023 at 3:14?PM Ruediger Pluem <rpluem@apache.org> wrote:
>>>
>>>
>>>
>>> On 5/9/23 8:01 PM, Eric Covener wrote:
>>>> On Tue, May 9, 2023 at 11:51?AM Ruediger Pluem <rpluem@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.dev@gmail.com> wrote:
>>>>>>>
>>>>>>> On Tue, May 9, 2023 at 12:55?PM Ruediger Pluem <rpluem@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
Re: Fwd: [apache/httpd] don't forward invalid query strings (d78a166) [ In reply to ]
On Thu, May 18, 2023 at 6:40?AM Ruediger Pluem <rpluem@apache.org> wrote:
>
>
>
> 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?

I am just afraid it breaks the applications that were getting them
decoded forever.
Re: Fwd: [apache/httpd] don't forward invalid query strings (d78a166) [ In reply to ]
On 5/18/23 1:55 PM, Eric Covener wrote:
> On Thu, May 18, 2023 at 6:40?AM Ruediger Pluem <rpluem@apache.org> wrote:
>>
>>
>>
>> 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?
>
> I am just afraid it breaks the applications that were getting them
> decoded forever.
>

But r->args is encoded. Hence they need to decode anyway as there could be other
encoded stuff in it or spaces that have not been taken decoded from the path.

Regards

Rüdiger
Re: Fwd: [apache/httpd] don't forward invalid query strings (d78a166) [ In reply to ]
> But r->args is encoded. Hence they need to decode anyway as there could be other
> encoded stuff in it or spaces that have not been taken decoded from the path.

These are applications/configurations that were functional prior to
the change though.
I don't think the risk of differing spaces in the input is something
we should break people for (in a slightly different way than failing
the request).