Re: svn commit: r1919325 - in /httpd/httpd/trunk: changes-entries/pr69197.txt modules/mappers/mod_rewrite.c

[Ruediger Pluem <rp...@apache.org>]

Any comments on the below?

Regards

Rüdiger

On 8/8/24 10:18 AM, Ruediger Pluem wrote:
> 
> 
> On 7/17/24 10:50 PM, ylavic@apache.org wrote:
>> Author: ylavic
>> Date: Wed Jul 17 20:50:12 2024
>> New Revision: 1919325
>>
>> URL: http://svn.apache.org/viewvc?rev=1919325&view=rev
>> Log:
>> mod_rewrite: Better question mark tracking to avoid UnsafeAllow3F.  PR 69197.
>>
>> Track in do_expand() whether a '?' in the uri-path comes from a literal in
>> the substitution string or from an expansion (variable, lookup, ...).
>> In the former case it's safe to assume that it's the query-string separator
>> but for the other case it's not (could be a decoded %3f from r->uri).
>>
>> This allows to avoid [UnsafeAllow3F] for most cases.
>>
>>
>> Added:
>>     httpd/httpd/trunk/changes-entries/pr69197.txt
>> Modified:
>>     httpd/httpd/trunk/modules/mappers/mod_rewrite.c
>>
> 
>>
>> Modified: httpd/httpd/trunk/modules/mappers/mod_rewrite.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/mappers/mod_rewrite.c?rev=1919325&r1=1919324&r2=1919325&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/mappers/mod_rewrite.c (original)
>> +++ httpd/httpd/trunk/modules/mappers/mod_rewrite.c Wed Jul 17 20:50:12 2024
>> @@ -2402,17 +2402,45 @@ static APR_INLINE char *find_char_in_cur
>>   * of an earlier expansion to include expansion specifiers that
>>   * are interpreted by a later expansion, producing results that
>>   * were not intended by the administrator.
>> + *
>> + * unsafe_qmark if not NULL will be set to 1 or 0 if a question mark
>> + * is found respectively in a literal or in a lookup/expansion (whether
>> + * it's the first or last qmark depends on [QSL]). Should be initialized
>> + * to -1 and remains so if no qmark is found.
>>   */
>> -static char *do_expand(char *input, rewrite_ctx *ctx, rewriterule_entry *entry, apr_pool_t *pool)
>> +static char *do_expand(char *input, rewrite_ctx *ctx, rewriterule_entry *entry,
>> +                       int *unsafe_qmark, apr_pool_t *pool)
>>  {
>> +#define EXPAND_SPECIALS "\\$%"
>>      result_list *result, *current;
>>      result_list sresult[SMALL_EXPANSION];
>>      unsigned spc = 0;
>>      apr_size_t span, inputlen, outlen;
>>      char *p, *c;
>>  
>> -    span = strcspn(input, "\\$%");
>>      inputlen = strlen(input);
>> +    if (!unsafe_qmark) {
>> +        span = strcspn(input, EXPAND_SPECIALS);
>> +    }
>> +    else {
>> +        span = strcspn(input, EXPAND_SPECIALS "?");
>> +        if (input[span] == '?') {
>> +            /* this qmark is not from an expansion thus safe */
>> +            *unsafe_qmark = 0;
>> +
>> +            /* keep tracking only if interested in the last qmark */
>> +            if (entry && (entry->flags & RULEFLAG_QSLAST)) {
>> +                do {
>> +                    span++;
>> +                    span += strcspn(input + span, EXPAND_SPECIALS "?");
>> +                } while (input[span] == '?');
> 
> Why do we need this loop? Isn't that the same as
> 
> span += strcspn(input + span, EXPAND_SPECIALS);
> 
>> +            }
>> +            else {
>> +                unsafe_qmark = NULL;
>> +                span += strcspn(input + span, EXPAND_SPECIALS);
>> +            }
>> +        }
>> +    }
>>  
>>      /* fast exit */
>>      if (inputlen == span) {
> 
>> @@ -2570,8 +2603,41 @@ static char *do_expand(char *input, rewr
>>              ++outlen;
>>          }
>>  
>> +        if (unsafe_qmark && expanded && current->len
>> +            && memchr(current->string, '?', current->len)) {
>> +            /* this qmark is from an expansion thus unsafe */
>> +            *unsafe_qmark = 1;
>> +
>> +            /* keep tracking only if interested in the last qmark */
>> +            if (!entry || !(entry->flags & RULEFLAG_QSLAST)) {
>> +                unsafe_qmark = NULL;
>> +            }
>> +        }
>> +
>>          /* check the remainder */
>> -        if (*p && (span = strcspn(p, "\\$%")) > 0) {
> 
> Can't we just do a break here in the !*p case?
> 
>> +        if (!unsafe_qmark) {
>> +            span = strcspn(p, EXPAND_SPECIALS);
>> +        }
>> +        else {
>> +            span = strcspn(p, EXPAND_SPECIALS "?");
>> +            if (p[span] == '?') {
>> +                /* this qmark is not from an expansion thus safe */
>> +                *unsafe_qmark = 0;
>> +
>> +                /* keep tracking only if interested in the last qmark */
>> +                if (entry && (entry->flags & RULEFLAG_QSLAST)) {
>> +                    do {
>> +                        span++;
>> +                        span += strcspn(p + span, EXPAND_SPECIALS "?");
>> +                    } while (p[span] == '?');
> 
> Why do we need this loop? Isn't that the same as
> 
> span += strcspn(input + span, EXPAND_SPECIALS);
> 
>> +                }
>> +                else {
>> +                    unsafe_qmark = NULL;
>> +                    span += strcspn(p + span, EXPAND_SPECIALS);
>> +                }
>> +            }
>> +        }
>> +        if (span > 0) {
>>              if (current->len) {
>>                  current->next = (spc < SMALL_EXPANSION)
>>                                  ? &(sresult[spc++])
> 
> Regards
> 
> Rüdiger
>