Return-Path: Delivered-To: apmail-httpd-dev-archive@httpd.apache.org Received: (qmail 88460 invoked by uid 500); 8 Dec 2002 22:27:07 -0000 Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm Precedence: bulk Reply-To: dev@httpd.apache.org list-help: list-unsubscribe: list-post: Delivered-To: mailing list dev@httpd.apache.org Received: (qmail 88447 invoked from network); 8 Dec 2002 22:27:07 -0000 Date: Sun, 8 Dec 2002 22:27:11 +0000 From: Francis Daly To: dev@httpd.apache.org Subject: symlink-following bug in ap_directory_walk? Message-ID: <20021208222711.GA2626@kerna.ie> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.4i X-Spam-Rating: daedalus.apache.org 1.6.2 0/1000/N 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 Options None Options MultiViews Options Indexes 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/

Index of /dir

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) {