httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <wr...@rowe-clan.net>
Subject Re: 2.0.x
Date Wed, 19 Jul 2006 15:13:46 GMT
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;
>          }
>  


Mime
View raw message