httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Marc Slemko <>
Subject Re: Canonical filename overhaul
Date Fri, 16 Oct 1998 06:29:50 GMT
On Wed, 14 Oct 1998, Ken Parzygnat wrote:

> Time to stir up this hornets nest again.
> I've been working on Apache PR's for Win32 for
> a short time, but I've found that about 90% of
> the problems are boiling down to a bug in
> ap_os_canonical_filename.

Yes.  Two million thanks and three million free copies of Apache go to
anyone who fixes this.

> There are still problems lying around that
> relate to this, and there are still more to 
> come.  For example, the <Files> directive seems
> to be completely broken on Win32.

It does work sometimes when you have the luck to be in the right directory
using or not using the right wildcards.  But yea, it is broken.

> One thing to keep in mind is that the UNIX
> ap_os_canonical_filename is a no-op.  That is,
> it returns exactly what it gets.  This is 

You can argue that there are a couple of things that an
ap_os_canonical_filename should do on Unix if it were to do what it name
says; ie. handle "." and "..".  But, since those are resolved elsewhere
and have HTTP semantics it doesn't.

> partly why I believe the Win32 version needs
> to be very simple, and only do what it minimally
> needs to do to transform a path into a 
> well known format.

Right.  All we need is to be able to compare two names.  This seems like a
darn simple thing, but the last MS person I asked about it was astounded
by the concept and went off in wonder.

> Path: [*.perl]
> Old : [f:/work/canon/debug/*.perl]
> This really seems wrong to me.  ap_os_canonical_filename
> has defaulted the current drive and current path.  This
> is the primary reason that the <Files> directive is broken
> on Win32.  In fact, the current drive and current path
> are defaulted onto any string which is not a UNC name.  It
> seems that we should never do this, we should only be 
> getting default drive and path information from ServerRoot
> or DocumentRoot.  

Right, nothing can depend on the current directory and we can (ie. should)
never (in request handling) change directories.

> Path: [..]
> Old : [ASSERT ERROR!!!!!!!]
> These are just a few of the cases that just
> assert with the current routine.  The list

Yea, well, 90% of the asserts in the win32 code should be shot in the

> Knowing that the Unix version of 
> ap_os_canonical_filename is a no-op means that
> there is lots of code in the server to do basic
> path crunching (i.e. '..' and '.' and '///' etc).

Right, although you can think of that as not really being filesystem path
but pseudo HTTP stuff.

> 1. Change '\' to '/'
> 2. Eliminate trailing '.' characters.  I felt this was
>    important as a security issue since Win32 will ignore
>    a trailing '.'.

What about spaces?

> 3. Eliminate directories with more than 2 '.'s.  While on
>    Unix it is valid to create a directory '...', it is 
>    not on Win32.  This is a very suspect directory, and
>    therefore, as a security issue, I've opted to eliminate
>    it.  This could be up for discussion.

We already deal with that, right?  Probably the same code that is
stripping "..".  This comes up, of course, because in some situations on
Win32, "..." can actually refer to the parent or the parent of the parent.

> --- http_request.c.first	Tue Oct 06 19:12:24 1998
> +++ http_request.c	Tue Oct 13 21:57:14 1998
> @@ -359,16 +359,19 @@
>          return OK;
>      }
> -    r->filename   = ap_os_canonical_filename(r->pool, r->filename);
> -    test_filename = ap_pstrdup(r->pool, r->filename);
> -
> -    ap_no2slash(test_filename);
> -    num_dirs = ap_count_dirs(test_filename);
> +    r->filename   = ap_os_case_canonical_filename(r->pool, r->filename);
>      res = get_path_info(r);
>      if (res != OK) {
>          return res;
>      }
> +
> +    r->filename   = ap_os_canonical_filename(r->pool, r->filename);
> +
> +    test_filename = ap_pstrdup(r->pool, r->filename);
> +
> +    ap_no2slash(test_filename);
> +    num_dirs = ap_count_dirs(test_filename);
>      if ((res = check_safe_file(r))) {
>          return res;

I haven't yet gone through this, but the idea sounds good.  I will have to
look at exactly what is being done, since it is not intuitively obvious
that the required places will have the required case, but..

View raw message