Re: [PATCH] PR#3323: recursive includes

[Ben Laurie <be...@algroup.co.uk>]

+1 (untested).

Marc Slemko wrote:
> 
> There are a few recursive includes still possible, including the trivial
> one of including "/" in "/" (or any directory that is handled by mod_dir).
> 
> The below patch attempts to fix it, and works for all the cases I can
> think of.  Comments are appreciated; I'm not overly familiar with this
> part (ie. all the things that impact r->filename and uri and how
> they can interact).
> 
> If any module plays with r->filename outside of the filename translation
> phase, this may not necessarily be enough to fix the problem for that
> module.  What mod_dir does is:
> 
>     /* KLUDGE --- make the sub_req lookups happen in the right directory.
>      * Fixing this in the sub_req_lookup functions themselves is difficult,
>      * and would probably break virtual includes...
>      */
> 
>     if (r->filename[strlen(r->filename) - 1] != '/') {
>         r->filename = ap_pstrcat(r->pool, r->filename, "/", NULL);
>     }
> 
> With mod_dir, you can't (well, I can't see a way) to make a request
> for a ever-changing URL that still is handled as the same filename
> by mod_dir.
> 
> If you can do an ever changing URL plus a r->filename munged outside
> of the filename translation at the same time, you can bypass
> the below checks.
> 
> The other thing that may be wise, in general for all modules, is the
> ability to put a configurable limit the depth of subrequests and/or
> internal redirects.
> 
> Also note that it is possible that there are times when you _want_
> recursive includes of the same file and your recursive includes know
> when they get to a certain depth and stop properly.  That, however,
> goes a bit beyond so I'm not sure if it is worth worrying about.
> 
> Index: modules/standard/mod_include.c
> ===================================================================
> RCS file: /export/home/cvs/apache-1.3/src/modules/standard/mod_include.c,v
> retrieving revision 1.105
> diff -u -r1.105 mod_include.c
> --- mod_include.c       1998/09/24 14:06:42     1.105
> +++ mod_include.c       1998/10/31 19:47:05
> @@ -688,13 +688,41 @@
>                      "in parsed file %s";
>              }
>              if (error_fmt == NULL) {
> +               /* try to avoid recursive includes.  We do this by walking
> +                * up the r->main list of subrequests, and at each level
> +                * walking back through any internal redirects.  At each
> +                * step, we compare the filenames and the URIs.
> +                *
> +                * The filename comparison catches a recursive include
> +                * with an ever-changing URL, eg.
> +                * <!--#include virtual=
> +                *      "$REQUEST_URI/$QUERY_STRING?$QUERY_STRING/x"-->
> +                * which, although they would eventually be caught because
> +                * we have a limit on the length of files, etc., can
> +                * recurse for a while.
> +                *
> +                * The URI comparison catches the case where the filename
> +                * is changed while processing the request, so the
> +                * current name is never the same as any previous one.
> +                * This can happen with "DocumentRoot /foo" when you
> +                * request "/" on the server and it includes "/".
> +                * This only applies to modules such as mod_dir that
> +                * (somewhat improperly) mess with r->filename outside
> +                * of a filename translation phase.
> +                */
> +               int founddupe = 0;
>                  request_rec *p;
> +                for (p = r; p != NULL && !founddupe; p = p->main) {
> +                   request_rec *q;
> +                   for (q = p; q != NULL; q = q->prev) {
> +                       if ( (strcmp(q->filename, rr->filename) == 0) ||
> +                            (strcmp(q->uri, rr->uri) == 0) ){
> +                           founddupe = 1;
> +                           break;
> +                       }
> +                   }
> +               }
> 
> -                for (p = r; p != NULL; p = p->main) {
> -                    if (strcmp(p->filename, rr->filename) == 0) {
> -                        break;
> -                    }
> -                }
>                  if (p != NULL) {
>                      error_fmt = "Recursive include of \"%s\" "
>                          "in parsed file %s";

-- 
Ben Laurie            |Phone: +44 (181) 735 0686| Apache Group member
Freelance Consultant  |Fax:   +44 (181) 735 0689|http://www.apache.org/
and Technical Director|Email: ben@algroup.co.uk |
A.L. Digital Ltd,     |Apache-SSL author     http://www.apache-ssl.org/
London, England.      |"Apache: TDG" http://www.ora.com/catalog/apache/