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 <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.
--
Eric Covener
cove...@gmail.com
