This is much closer to what I'm working with, I like your patch better in fact.
Please commit to trunk; I believe we can reoptimize the familiar case using a different rule based on a cached canonical filename pattern. -but- this shouldn't hold up committing this fix; if you can save two flavors which apply clean to 2.0.58, 2.2.2, then we should make these available immediately under /dist/httpd/patches/apply_to_2.x.x/ locations. Working on a perl-framework validation that my collegue Sris started, and will commit that the moment it's ready. Bill Ruediger Pluem wrote:
I have a patch that works against trunk (checked Options None, Options FollowSymLinks, Options SymLinksifOwnerMatch). It keeps the optimization and comes at the cost of one extra stat if we have FollowSymLinks NOT set or if SymLinksifOwnerMatch is set and up to 3 further stats due to resolve_symlink if it is really a symbolic link. From my current point of view this seems to be an acceptable penalty. As I am not very familar with this code and this part of the code seems to be critical from the performance and security perspective remote eyes, reviews and comments are very welcome. Index: server/request.c =================================================================== --- server/request.c (Revision 422739) +++ server/request.c (Arbeitskopie) @@ -524,17 +524,50 @@ && (!r->path_info || !*r->path_info))) && (cache->dir_conf_tested == sec_ent) && (strcmp(entry_dir, cache->cached) == 0)) { + int familar = 0; + /* Well this looks really familiar! If our end-result (per_dir_result) * didn't change, we have absolutely nothing to do :) * Otherwise (as is the case with most dir_merged/file_merged requests) * we must merge our dir_conf_merged onto this new r->per_dir_config. */ if (r->per_dir_config == cache->per_dir_result) { - return OK; + familar = 1; } if (r->per_dir_config == cache->dir_conf_merged) { r->per_dir_config = cache->per_dir_result; + familar = 1; + } + + if (familar) { + apr_finfo_t thisinfo; + int res; + allow_options_t opts; + core_dir_config *this_dir; + + this_dir = ap_get_module_config(r->per_dir_config, &core_module); + opts = this_dir->opts; + /* + * If Symlinks are allowed in general we do not need the following + * check. + */ + if (!(opts & OPT_SYM_LINKS)) { + apr_stat(&thisinfo, r->filename, + APR_FINFO_MIN | APR_FINFO_NAME | APR_FINFO_LINK, + r->pool); + if (thisinfo.filetype == APR_LNK) { + /* Is this a possibly acceptable symlink? */ + if ((res = resolve_symlink(r->filename, &thisinfo, + opts, r->pool)) != OK) { + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, + "Symbolic link not allowed " + "or link target not accessible: %s", + r->filename); + return r->status = res; + } + } + } return OK; } Regards Rüdiger ------------------------------------------------------------------------ Index: server/request.c =================================================================== --- server/request.c (Revision 422739) +++ server/request.c (Arbeitskopie) @@ -524,17 +524,50 @@ && (!r->path_info || !*r->path_info))) && (cache->dir_conf_tested == sec_ent) && (strcmp(entry_dir, cache->cached) == 0)) { + int familar = 0; + /* Well this looks really familiar! If our end-result (per_dir_result) * didn't change, we have absolutely nothing to do :) * Otherwise (as is the case with most dir_merged/file_merged requests) * we must merge our dir_conf_merged onto this new r->per_dir_config. */ if (r->per_dir_config == cache->per_dir_result) { - return OK; + familar = 1; }if (r->per_dir_config == cache->dir_conf_merged) {r->per_dir_config = cache->per_dir_result; + familar = 1; + } + + if (familar) { + apr_finfo_t thisinfo; + int res; + allow_options_t opts; + core_dir_config *this_dir; + + this_dir = ap_get_module_config(r->per_dir_config, &core_module); + opts = this_dir->opts; + /* + * If Symlinks are allowed in general we do not need the following + * check. + */ + if (!(opts & OPT_SYM_LINKS)) { + apr_stat(&thisinfo, r->filename, + APR_FINFO_MIN | APR_FINFO_NAME | APR_FINFO_LINK, + r->pool); + if (thisinfo.filetype == APR_LNK) { + /* Is this a possibly acceptable symlink? */ + if ((res = resolve_symlink(r->filename, &thisinfo, + opts, r->pool)) != OK) { + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, + "Symbolic link not allowed " + "or link target not accessible: %s", + r->filename); + return r->status = res; + } + } + } return OK; }
