Mailing List Archive

On Fri, Mar 10, 2023 at 11:57?AM Yann Ylavic <ylavic.dev@gmail.com> wrote:
>
> On Fri, Mar 10, 2023 at 4:34?PM Eric Covener <covener@gmail.com> wrote:
> >
> > Saw another report on users@
> >
> > Any thoughts on something like this to just allow spaces?
> > http://people.apache.org/~covener/patches/rewrite-lax.diff
>
> What about:
>
> Index: modules/mappers/mod_rewrite.c
> ===================================================================
> --- modules/mappers/mod_rewrite.c (revision 1908254)
> +++ modules/mappers/mod_rewrite.c (working copy)
> @@ -4814,7 +4814,8 @@ static int hook_uri2file(request_rec *r)
> apr_size_t flen;
> int to_proxyreq;
>
> - if (r->args && *(ap_scan_vchar_obstext(r->args))) {
> + if (rulestatus == ACTION_NOESCAPE
> + && r->args && *(ap_scan_vchar_obstext(r->args))) {
> /*
> * We have a raw control character or a ' ' in r->args.
> * Correct encoding was missed.

I think it helps for the users@ redirect case, but I think we still
have a concern with non-redirect (where IIUC there is not any escaping
even w/o the flag/status, but I am not 100% sure on this)

On 3/10/23 16:33, Eric Covener wrote:
> Saw another report on users@
>
> Any thoughts on something like this to just allow spaces?
> http://people.apache.org/~covener/patches/rewrite-lax.diff

that makes sense, any other possible char that we should allow other then spaces ?
Giovanni


>
> (this is off my $bigco fork so may not actually apply)
>
> On Thu, Mar 9, 2023 at 3:08?PM BUSH Steve <Steven.BUSH@3ds.com> wrote:
>>
>>>> Maybe we can slip an additional entry into the changelog.
>>
>>>> I think in this case, for now at least, we'd primarily rely on the error_log entry. Did this produce the new AH10410?
>>
>>
>>
>> Yes, the error log did include the AH10410 message.
>>
>>
>>
>> URL encoding the spaces either as \%20 (path or query string) or + (query string) does eliminate the problem for our mappings.
>>
>>
>>
>> From: Eric Covener <covener@gmail.com>
>> Sent: Wednesday, March 8, 2023 8:31 PM
>> To: dev@httpd.apache.org
>> Subject: Re: [VOTE] [VOTE] Release httpd-2.4.56-rc1 as httpd-2.4.56
>>
>>
>>
>> On Wed, Mar 8, 2023 at 11:?02 PM BUSH Steve <Steven.?BUSH@?3ds.?com> wrote: Correction! I used our test template for the rule when I e-mailed just now, but once it is converted to the apache httpd.?conf format, the actual rule appears in the
>>
>> ZjQcmQR
>>
>> YFpfptBannerEnd
>>
>>
>>
>> On Wed, Mar 8, 2023 at 11:02?PM BUSH Steve <Steven.BUSH@3ds.com> wrote:
>>
>> Correction!
>>
>> I used our test template for the rule when I e-mailed just now, but once it is converted to the apache httpd.conf format, the actual rule appears in the httpd.conf as:
>>
>> RewriteRule ^/zoology/animals/reset/(\d+)$ "/auth/launchjob?Number of Records=$1&__poolid=animal-magic" [B,PT,L,QSA]
>>
>>
>>
>> Thanks for the report. Time will tell, but I think this is a very fringe case. The space isn't a backreference (where `B` would have fixed it) and a literal with a space in the substitution has to be quite rare (famous last words)
>>
>> I just looked at the mod_rewrite.c source differences from 2.4.55 to 2.4.56 and it’s clear that the use of spaces in the query string of the mapped URL are the cause of the 403 forbidden messages.
>>
>>
>>
>> We can update our httpd.conf mapping code, so it won’t be a problem for us, but it might be worth updating the mod_rewrite documentation on this?
>>
>>
>>
>>
>>
>> Maybe we can slip an additional entry into the changelog.
>>
>> I think in this case, for now at least, we'd primarily rely on the error_log entry. Did this produce the new AH10410?
>>
>>
>>
>>
>>
>> This email and any attachments are intended solely for the use of the individual or entity to whom it is addressed and may be confidential and/or privileged.
>>
>> If you are not one of the named recipients or have received this email in error,
>>
>> (i) you should not read, disclose, or copy it,
>>
>> (ii) please notify sender of your receipt by reply email and delete this email and all attachments,
>>
>> (iii) Dassault Systèmes does not accept or assume any liability or responsibility for any use of or reliance on this email.
>>
>>
>> Please be informed that your personal data are processed according to our data privacy policy as described on our website. Should you have any questions related to personal data protection, please contact 3DS Data Protection Officer https://www.3ds.com/privacy-policy/contact/
>>
>>
>
>

On Mar 10, 2023, at 8:56 AM, Yann Ylavic <ylavic.dev@gmail.com> wrote:

> On Fri, Mar 10, 2023 at 4:34?PM Eric Covener <covener@gmail.com> wrote:
>>
>> Saw another report on users@
>>
>> Any thoughts on something like this to just allow spaces?
>> http://people.apache.org/~covener/patches/rewrite-lax.diff
>
> What about:
>
> Index: modules/mappers/mod_rewrite.c
> ===================================================================
> --- modules/mappers/mod_rewrite.c (revision 1908254)
> +++ modules/mappers/mod_rewrite.c (working copy)
> @@ -4814,7 +4814,8 @@ static int hook_uri2file(request_rec *r)
> apr_size_t flen;
> int to_proxyreq;
>
> - if (r->args && *(ap_scan_vchar_obstext(r->args))) {
> + if (rulestatus == ACTION_NOESCAPE
> + && r->args && *(ap_scan_vchar_obstext(r->args))) {
> /*
> * We have a raw control character or a ' ' in r->args.
> * Correct encoding was missed.
> ?
>
> Regards;
> Yann.

Allowing a space to be sent within the proxied request target is not an option,
regardless of how the user has configured the server. The CVE fix was just to
prevent an invalid target sent from us.

Why don't we fix the source of the spaces? The place where the variable is decoding
the matched string being inserted. I find that bit surprising, since it doesn't behave
like a proper regex.

Likewise, the rewrite mapper should always pct-encode or reject embedded spaces
long before we get to the proxy (or internal redirect) request.

....Roy

> Allowing a space to be sent within the proxied request target is not an option,
> regardless of how the user has configured the server. The CVE fix was just to
> prevent an invalid target sent from us.

This context in mod_rewrite is not specific to proxying. The CVE is
addressed in a similar snippet in the proxy modules.

> Why don't we fix the source of the spaces? The place where the variable is decoding
> the matched string being inserted. I find that bit surprising, since it doesn't behave
> like a proper regex.

The input here is the decoded URL-path. rewrite can explicitly look
at the original request verbatim, but it's a rare thing to be used.

> Likewise, the rewrite mapper should always pct-encode or reject embedded spaces
> long before we get to the proxy (or internal redirect) request.

In the non-proxy case, the backreference may be in a local filename or
the query string. I guess the latter is still bogus in CGI-like cases,
but it's been tolerated forever and being passed onto CGI-like things
without automatic encoding.

Pulling up some of the checks so we can consider the flag:
http://people.apache.org/~covener/patches/rewrite-escaping.diff

(needs to be duplicated in fixups hook)

On Fri, Mar 10, 2023 at 11:57?AM Yann Ylavic <ylavic.dev@gmail.com> wrote:
>
> On Fri, Mar 10, 2023 at 4:34?PM Eric Covener <covener@gmail.com> wrote:
> >
> > Saw another report on users@
> >
> > Any thoughts on something like this to just allow spaces?
> > http://people.apache.org/~covener/patches/rewrite-lax.diff
>
> What about:
>
> Index: modules/mappers/mod_rewrite.c
> ===================================================================
> --- modules/mappers/mod_rewrite.c (revision 1908254)
> +++ modules/mappers/mod_rewrite.c (working copy)
> @@ -4814,7 +4814,8 @@ static int hook_uri2file(request_rec *r)
> apr_size_t flen;
> int to_proxyreq;
>
> - if (r->args && *(ap_scan_vchar_obstext(r->args))) {
> + if (rulestatus == ACTION_NOESCAPE
> + && r->args && *(ap_scan_vchar_obstext(r->args))) {
> /*
> * We have a raw control character or a ' ' in r->args.
> * Correct encoding was missed.
> ?
>
> Regards;
> Yann.



--
Eric Covener
covener@gmail.com

committed two related things to trunk this afternoon:

- allow anything if redirecting and no [NE] flag
- add another [B] like flag that escapes only controls and spaces.


On Sat, Mar 11, 2023 at 2:30?PM Eric Covener <covener@gmail.com> wrote:
>
> Pulling up some of the checks so we can consider the flag:
> http://people.apache.org/~covener/patches/rewrite-escaping.diff
>
> (needs to be duplicated in fixups hook)
>
> On Fri, Mar 10, 2023 at 11:57?AM Yann Ylavic <ylavic.dev@gmail.com> wrote:
> >
> > On Fri, Mar 10, 2023 at 4:34?PM Eric Covener <covener@gmail.com> wrote:
> > >
> > > Saw another report on users@
> > >
> > > Any thoughts on something like this to just allow spaces?
> > > http://people.apache.org/~covener/patches/rewrite-lax.diff
> >
> > What about:
> >
> > Index: modules/mappers/mod_rewrite.c
> > ===================================================================
> > --- modules/mappers/mod_rewrite.c (revision 1908254)
> > +++ modules/mappers/mod_rewrite.c (working copy)
> > @@ -4814,7 +4814,8 @@ static int hook_uri2file(request_rec *r)
> > apr_size_t flen;
> > int to_proxyreq;
> >
> > - if (r->args && *(ap_scan_vchar_obstext(r->args))) {
> > + if (rulestatus == ACTION_NOESCAPE
> > + && r->args && *(ap_scan_vchar_obstext(r->args))) {
> > /*
> > * We have a raw control character or a ' ' in r->args.
> > * Correct encoding was missed.
> > ?
> >
> > Regards;
> > Yann.
>
>
>
> --
> Eric Covener
> covener@gmail.com



--
Eric Covener
covener@gmail.com