httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Marc Slemko <ma...@znep.com>
Subject RE: cvs commit: apache-1.3 STATUS
Date Wed, 07 Jun 2000 23:54:05 GMT
On Wed, 7 Jun 2000, Allan Edwards wrote:

> > -----Original Message-----
> > From: wrowe@locus.apache.org [mailto:wrowe@locus.apache.org]
> > Sent: Saturday, June 03, 2000 10:48 PM
> > To: apache-1.3-cvs@apache.org
> > Subject: cvs commit: apache-1.3 STATUS
> >
> >
> > wrowe       00/06/03 19:48:00
> >
> >   Modified:    .        STATUS
> >   Log:
> >     Add Mark Slemco's observations as a showstopper to
> > permanently close
> >     this potential security hole.
> >
> 
> >   +    * Complete the security hole in stat() by testing for anything
> >   +      other than conventional file-not-found,
> > permission-denied errors
> >   +      and rejecting the request then and there.  By rights, all of
> >   +      these cases aught to be Not Found, not Permission Denied.
> 
> I'm not sure there's anything else to do here. On NT, the stat() in get_path_info is
returning an error for strlen=MAX_PATH, we
> get errno=2 (GetLastError = PATH_NOT_FOUND). For strlen>MAX_PATH we get errno=2 (GetLastError
= FILENAME_EXCED_RANGE). The
> problem is just that ap_is_os_filename_valid in directory_walk didn't do its job of catching
the length error. As a result, the
> stat() failure made it look like the DirectoryIndex file had not been found. So we didn't
bail, but rather continued on to the
> next handler, and mod_autoindex served up the directory listing.
> 
> In Apache 2.0 the stat() call is replaced by ap_stat(), and GetFileAttributesEx returns
the same GetLastError statuses as above.
> It would be easy to add the strlen>=MAX_PATH check inside ap_stat to return ERROR_FILENAME_EXCED_LENGTH,
but I'm not sure it's
> worth doing. However, if people feel sufficiently paranoid let me know.

No, the whole point is that when something is doing a stat() to see if a
file exists or not, the _ONLY_ things that should be treated as "this
doesn't exist" are a limited set of known returns.  stat() can fail for
two zillion reasons, and most of them should not result in the request
going on as if the file doesn't exist.  This is the way things work on
Unix, and they should work the same way on NT.  It is complicated by the
fact that NT isn't POSIX and has its own random error codes, but the
concept is the same.  I am not convinced that every stat() (eg. things
like .htaccess files) is of a file that has been passed to
ap_is_os_filename_valid already.

There are also other areas, such as in mod_negotiation, that have a
similar problem, only there it is not checking the return from a
subrequest properly; same idea, not every failure is a "doesn't exist".  
This is a different issue that needs to be fixed too, since it results in
the same problem on Unix with the default MultiViews setup (which I don't
think is a good default anyway, but... that is another issue).

I really think that the check in ap_is_os_filename_valid should be
a "backup" check only.  There are a number of places where the
return from stat() simply has to be checked for a "known" doesn't
exist error.  Perhaps we need a "ap_stat_tell_me_if_it_doesnt_exist" type
function that can do whatever is necessary for the particular OS.


Mime
View raw message