Re: 2.0.x

Wed, 19 Jul 2006 08:39:19 -0700 

Actually, does this handle the nested case (more than one level depth?)

I'm afraid it doesn't.  Staying with the 'familiar' theme, it seems to
make sense to follow the entire code path, optimizing out the actual
operations by checking if (familiar).

William A. Rowe, Jr. wrote:
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;
         }

Reply via email to