httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Francis Daly <d...@daoine.org>
Subject symlink-following bug in ap_directory_walk?
Date Sun, 08 Dec 2002 22:27:11 GMT

Hi there,

Options -FollowSymLinks (and Options -SymLinksIfOwnerMatch) do not appear
to be honoured by mod_dir, mod_autoindex, or mod_negotiation in the
current codebase. 

There is an open bug report about the mod_dir issue (bugzilla id 14206);
this mail addresses the three of them together.

The mod_negotiation and mod_dir issues mean that, with either Options
+MultiViews enabled or mod_dir loaded, if a local user can create a
symlink, a remote user can retrieve the target of it if it is readable
by the web server user. 

To demonstrate the problem, begin with a plain build (./configure,
possibly with a --prefix for tidiness). Then:

mkdir /tmp/docroot
echo 'content of /file.html' > /tmp/docroot/file.html
ln -s file.html /tmp/docroot/index.html
mkdir /tmp/docroot/dir
echo 'content of /dir/file.html' > /tmp/docroot/dir/file.html
ln -s file.html /tmp/docroot/dir/link.html

cat > conf/httpd.conf <<'EOIN'
Listen 2044
DocumentRoot /tmp/docroot
<Directory />
 Options None
</Directory>
<Directory /tmp/docroot>
 Options MultiViews
</Directory>
<Directory /tmp/docroot/dir>
 Options Indexes
</Directory>

EOIN

Now try some requests:

(1) GET http://localhost:2044/index.html
     demonstrates whether default_handler fetches links (should not)
(2) GET http://localhost:2044/
     demonstrates whether mod_dir fetches links (should not)
(3) GET http://localhost:2044/index
     demonstrates whether mod_negotiation fetches links (should not)
(4) GET http://localhost:2044/dir/
     demonstrates whether mod_autoindex lists links (arguable, but I
think should not)

Testing using STRIKER_2_0_44_PRE_1 with [error_log] : 

(1)$ GET http://localhost:2044/index.html
403 Forbidden
[Symbolic link not allowed: /tmp/docroot/index.html]

(2)$ GET http://localhost:2044/
content of /file.html

(3)$ GET http://localhost:2044/index
content of /file.html

(4)$ GET http://localhost:2044/dir/
<h1>Index of /dir</h1>
<ul><li><a href="/"> Parent Directory</a></li>
<li><a href="file.html"> file.html</a></li>
<li><a href="link.html"> link.html</a></li>
</ul>

default_handler is correct.
mod_dir fetches the link.
mod_negotiation fetches the link.
mod_autoindex lists the link.

This is the same behaviour as 2.0.43, as well as some versions tested
back to 2.0.28. More on the older versions later.

For reference, the 1.3.27 code gets the first three correct; it lists
all files in the fourth case, but its mod_autoindex model is very
different from the 2.0 model, being based on filenames rather than
uris.


The quick-to-code but slow-to-run fix is to completely disable caching
in ap_directory_walk; that is, disable the "if" part of lines 608 to 632
of server/request.c, and always run the "else" part. (That's lines 557
to 581 in the 2.0.43 code)

It works in all the tests I have done, but it's an unacceptable
performance penalty. It does indicate that bypassing the caching when we
are dealing with symlinks is probably the way to go, though.

Testing indicates that the mod_dir breakage happened between 2.0.27
and 2.0.28, and that the mod_autoindex and mod_negotiation breakages
happened between 2.0.25 and 2.0.26. (That is, in 2.0.25, the four GETs
above returned as expected.) Since they happened in two stages, it's
reasonable to expect there'll be two distinct fixes.

The problem is related to symlinks; nothing should ever do a stat()
without paying attention to OPT_SYM_*.

The first place that does this is in ap_directory_walk. Replacing the
early apr_stat() on line 580 (line 529) with apr_lstat() will cause
the problematic caching to be bypassed only when we know the filetype
is APR_LNK, which is good, but isn't actually sufficient: the "first
optimization" lower down is now too aggressive. "full_name_len" is
introduced as a way of not performing the optimization when we are
checking the file itself, as opposed to a segment of the path.

This works for me, but I don't know how cross-platform-applicable it is,
unfortunately. Hopefully someone with more knowledge can offer advice
here.

With that change (apr_stat -> apr_lstat; full_name_len declared, set,
and tested), mod_dir works correctly on my system. (GET #2 works (i.e.
correctly fails).)

Another place that stat()s without paying proper heed to OPT_SYM_*
is ap_sub_req_lookup_dirent(), lines 1731 to 1737, and 1750 to 1760
(lines 1714 to 1720 and 1733 to 1743 in the 2.0.43 code). It does
actually check ap_allow_options(rnew) before doing anything, but at
this point, that is from r->server->lookup_defaults, and corresponds to
OPT_UNSET|OPT_ALL (which includes OPT_SYM_LINKS) irrespective of any
configuration settings. Disabling the entirety of the second piece, and
the "if" part of the first (leaving the "else" to be always run) means
that what gets passed to ap_directory_walk might be APR_LNK, which will
now be handled correctly.

mod_negotiation calls ap_sub_req_lookup_dirent(), and with this change
in place, it works for me. (GET #3 works.)

mod_autoindex also calls ap_sub_req_lookup_dirent() (in fact, it's
the only other code that calls it), so it should work too. However,
it wants to handle symlinks itself between lines 2071 and 2088 (lines
2063 and 2080) and so does a stat(). Bad idea. Rip out that code, and
make_autoindex_entry will pass the APR_LNK (if that is what it is) on to
ap_sub_req_lookup_dirent, which will now do the right thing with it.

With that change, mod_autoindex now does what it should, and does not
list files it will not be able to serve. (GET #4 works.)

I offer two patches, the first against the current server/request.c
(cvs 1.121) and the second against the current mod_autoindex.c (cvs
1.112). The second is probably correct -- I don't see anything
done in the deleted section that is not now handled correctly. If
it is significantly slower this new way, then perhaps considering
OPT_SYM_LINKS before the stat() will be an adequate optimization. The
first should be considered a draft patch, for a few reasons.

Mainly, it messes with ap_directory_walk, which is an important part of
the system, and so shouldn't be messed with unless the committer is sure
it's correct.

Also, I've only tested this one one system, and this code change impacts
all systems the server runs on. Validation from someone with a different
system would be good. Particularly in relation to full_name_len.

Proper performance testing would also be a good idea.

And finally, the patch is pretty much the minimal code change that I
can find to get everything working. Any patch to apr_directory_walk has
potential security implications (in fact, this one tries to close some
potential holes), and I didn't want to clutter the main point of it with
extra changes.

Examples of some possible extra changes: 

strlen(r->filename) is called twice; they could now be replaced by the
new full_name_len.

The line

    if (!r->finfo.filetype || r->finfo.filetype == APR_LNK) {

could be replaced by

    if (!r->finfo.filetype) {

since we're now doing an apr_lstat().

Unless I'm misreading it, the line

    && ((opts.opts & (OPT_SYM_OWNER | OPT_SYM_LINKS)) == OPT_SYM_LINKS))

is exactly equivalent to

    && (opts.opts & OPT_SYM_LINKS))

and could be replaced by it.

apr_lstat() is a deprecated function, so can be multi-replaced by a
suitable apr_stat(), saving a function call each time.

And ap_sub_req_lookup_file() plays the same games with
ap_allow_options(rnew) as ap_sub_req_lookup_dirent() did; I haven't
checked, but wouldn't be surprised if it suffers the same flaw of not
being set correctly at that point.

There are probably some more bits I've missed, but I'll get these
patches out there so that people more familiar with the base code can
point out (and fix) the problems with this.

All the best,

	f
-- 
Francis Daly        deva@daoine.org

--- server/request.c	Sun Nov  3 23:12:22 2002
+++ server/request.c	Tue Dec  3 00:39:22 2002
@@ -528,6 +528,7 @@
     walk_cache_t *cache;
     char *entry_dir;
     apr_status_t rv;
+    apr_size_t full_name_len;
 
     /* XXX: Better (faster) tests needed!!!
      *
@@ -563,6 +564,7 @@
      * have significant security holes.
      */
     r->filename = entry_dir;
+    full_name_len = strlen(r->filename);
 
     cache = prep_walk_cache(AP_NOTE_DIRECTORY_WALK, r);
 
@@ -577,7 +579,7 @@
      * with APR_ENOENT, knowing that the path is good.
      */
     if (!r->finfo.filetype || r->finfo.filetype == APR_LNK) {
-        apr_stat(&r->finfo, r->filename, APR_FINFO_MIN, r->pool);
+        apr_lstat(&r->finfo, r->filename, APR_FINFO_MIN, r->pool);
 
         /* some OSs will return APR_SUCCESS/APR_REG if we stat
          * a regular file but we have '/' at the end of the name;
@@ -979,6 +981,7 @@
 #ifdef CASE_BLIND_FILESYSTEM
                 && (filename_len <= canonical_len)
 #endif
+                && (filename_len < full_name_len)
                 && ((opts.opts & (OPT_SYM_OWNER | OPT_SYM_LINKS)) == OPT_SYM_LINKS))
             {
 
@@ -1728,35 +1731,14 @@
          * this should be safe.
          */
         apr_status_t rv;
-        if (ap_allow_options(rnew) & OPT_SYM_LINKS) {
-            if (((rv = apr_stat(&rnew->finfo, rnew->filename,
-                                APR_FINFO_MIN, rnew->pool)) != APR_SUCCESS)
-                && (rv != APR_INCOMPLETE)) {
-                rnew->finfo.filetype = 0;
-            }
-        }
-        else {
-            if (((rv = apr_lstat(&rnew->finfo, rnew->filename,
-                                 APR_FINFO_MIN, rnew->pool)) != APR_SUCCESS)
-                && (rv != APR_INCOMPLETE)) {
-                rnew->finfo.filetype = 0;
-            }
+        if (((rv = apr_lstat(&rnew->finfo, rnew->filename,
+                             APR_FINFO_MIN, rnew->pool)) != APR_SUCCESS)
+            && (rv != APR_INCOMPLETE)) {
+            rnew->finfo.filetype = 0;
         }
     }
     else {
         memcpy(&rnew->finfo, dirent, sizeof(apr_finfo_t));
-    }
-
-    if (rnew->finfo.filetype == APR_LNK) {
-        /*
-         * Resolve this symlink.  We should tie this back to dir_walk's cache
-         */
-        if ((res = resolve_symlink(rnew->filename, &rnew->finfo,
-                                   ap_allow_options(rnew), rnew->pool))
-            != OK) {
-            rnew->status = res;
-            return rnew;
-        }
     }
 
     if (rnew->finfo.filetype == APR_DIR) {


--- modules/generators/mod_autoindex.c	Fri Nov 22 05:12:52 2002
+++ modules/generators/mod_autoindex.c	Tue Dec  3 00:40:41 2002
@@ -2068,24 +2068,6 @@
     dirpathlen = strlen(name);
     memcpy(fullpath, name, dirpathlen);
     while (apr_dir_read(&dirent, APR_FINFO_MIN | APR_FINFO_NAME, thedir) == APR_SUCCESS)
{
-        /* We want to explode symlinks here. */
-        if (dirent.filetype == APR_LNK) {
-            const char *savename;
-            apr_finfo_t fi;
-            /* We *must* have FNAME. */
-            savename = dirent.name;
-            apr_cpystrn(fullpath + dirpathlen, dirent.name,
-                        APR_PATH_MAX - dirpathlen);
-            status = apr_stat(&fi, fullpath, 
-                              dirent.valid & ~(APR_FINFO_NAME), r->pool);
-            if (status != APR_SUCCESS) {
-                /* Something bad happened, skip this file. */
-                continue;
-            }
-            memcpy(&dirent, &fi, sizeof(fi));
-            dirent.name = savename;
-            dirent.valid |= APR_FINFO_NAME;
-        }
         p = make_autoindex_entry(&dirent, autoindex_opts, autoindex_conf, r, 
                                  keyid, direction, pstring);
         if (p != NULL) {


Mime
View raw message