Date: Tue, 5 Aug 1997 17:03:36 -0700 (PDT)
From: Dean Gaudet <dgaudet@arctic.org>
To: new-httpd@apache.org
Subject: Re: [PATCH] (1.2 and 1.3) sub_req_lookup_file revisited
In-Reply-To: 
 <Pine.LNX.3.95dg3.970803202332.21920A-100000@benchlark.arctic.org>
Message-ID: <Pine.LNX.3.95dg3.970805170229.6308G-100000@twinlark.arctic.org>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: new-httpd-owner@apache.org
Precedence: bulk
Reply-To: new-httpd@apache.org

I have this non-committed bug fix patch, which I'll commit to 1.3 along
with the directory_walk optimization tomorrow likely.  1.2.2 shouldn't
wait for this either. 

Dean

On Sun, 3 Aug 1997, Dean Gaudet wrote:

> Ok here we go, this should fix my concerns regarding <Directory> regexs
> and sub_req_lookup_file.  In fact, this is a security hole that's been
> around since before 1.2.  I think the impact is about as severe as the
> security problems we fixed in 1.2.1... that is to say, not very severe.  A
> refresher:
> 
> sub_req_lookup_file is most frequently used by modules when they want to
> get the content_type of a file, or other similar metadata.  In particular
> it's used by mod_autoindex to decide on the icon, and used by
> mod_negotiation to do negotiation.  Both of those cases are the important
> ones -- because they're potential speed problems.  So ages ago (before my
> time) an optimization was added: sub_req_lookup_simple.
> 
> sub_req_lookup_simple handled the case where the filename looked up is
> relative, and contains no slashes.  When this was added, it was true that
> once you'd passed access checks on a particular directory you would also
> pass access checks on any of the files in that directory.  So the
> optimization was to avoid the extra directory_walk() and associated access
> checks. 
> 
> But when <Files> was added, the assumptions for the optimization were no
> longer true.  This is what I attempted to fix in 1.2b8.  I actually
> rewrote sub_req_lookup_file, and eliminated sub_req_lookup_simple.  Now it
> does part of the full lookup -- it performs the file_walk.  If the
> file_walk doesn't have any matches then the assumptions for the
> optimization are still valid and we proceed with the optimization. 
> 
> While doing the directory_walk optimization I noticed that a <Directory>
> regex could possibly match the file being looked up, and so the
> sub_req_lookup_file optimization was broken.  But what didn't occur to me
> until today was that the sub_req_lookup_file optimization applies only to
> files, and shouldn't ever be applied to directories -- directories need a
> full directory_walk performed on them.  This bug existed prior to my work
> in 1.2b8.
> 
> The patch below fixes this by testing if the file is really a directory
> and if so, bailing out of the optimization. 
> 
> Now, as to ways to abuse this.  mod_include uses sub_req_lookup_file when
> you do a #include file.  Conceivably you could #include the index.html
> from a directory that you don't pass access checks for.  I don't think
> it's a huge problem at all though... it's just more of a bug.
> 
> Dean
> 
> Index: http_request.c
> ===================================================================
> RCS file: /export/home/cvs/apache/src/http_request.c,v
> retrieving revision 1.67
> diff -u -r1.67 http_request.c
> --- http_request.c	1997/08/01 08:01:21	1.67
> +++ http_request.c	1997/08/04 03:23:19
> @@ -716,22 +716,31 @@
>  
>  	rnew->per_dir_config = r->per_dir_config;
>  
> -	if ((res = check_symlinks (rnew->filename, allow_options (rnew)))) {
> -	    log_reason ("Symbolic link not allowed", rnew->filename, rnew);
> -	    rnew->status = res;
> -	    return rnew;
> -	}
> -	/* do a file_walk, if it doesn't change the per_dir_config then
> -	 * we know that we don't have to redo all the access checks */
> -	if ((res = file_walk (rnew))) {
> -	    rnew->status = res;
> -	    return rnew;
> -	}
> -	if (rnew->per_dir_config == r->per_dir_config) {
> -	    if ((res = find_types (rnew)) || (res = run_fixups (rnew))) {
> +	/* no matter what, if it's a subdirectory, we need to re-run
> +	 * directory_walk */
> +	if (S_ISDIR (rnew->finfo.st_mode)) {
> +	    res = directory_walk (rnew);
> +	    if (!res) {
> +		res = file_walk (rnew);
> +	    }
> +	} else {
> +	    if ((res = check_symlinks (rnew->filename, allow_options (rnew)))) {
> +		log_reason ("Symbolic link not allowed", rnew->filename, rnew);
> +		rnew->status = res;
> +		return rnew;
> +	    }
> +	    /* do a file_walk, if it doesn't change the per_dir_config then
> +	     * we know that we don't have to redo all the access checks */
> +	    if ((res = file_walk (rnew))) {
>  		rnew->status = res;
> +		return rnew;
> +	    }
> +	    if (rnew->per_dir_config == r->per_dir_config) {
> +		if ((res = find_types (rnew)) || (res = run_fixups (rnew))) {
> +		    rnew->status = res;
> +		}
> +		return rnew;
>  	    }
> -	    return rnew;
>  	}
>      } else {
>  	/* XXX: this should be set properly like it is in the same-dir case
> 
>