On 7/17/24 10:50 PM, yla...@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
