You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by rp...@apache.org on 2006/07/20 13:01:08 UTC
svn commit: r423886 - in /httpd/httpd/trunk: CHANGES server/request.c
Author: rpluem
Date: Thu Jul 20 04:01:07 2006
New Revision: 423886
URL: http://svn.apache.org/viewvc?rev=423886&view=rev
Log:
* Check for symbolic links of the target file in the optimized case that we
had already done this specific directory walk for this request. This can
happen when we have an internal redirect, like the ones caused by mod_dir
(/ -> index.html). See also
http://mail-archives.apache.org/mod_mbox/httpd-dev/200607.mbox/%3c44B5521F.8050906@globalvanet.com%3e
If we do not do this we have a security hole as the FollowSymLinks and
SymLinksIfOwnerMatch settings can circumvented this way.
Reviewed by: wrowe
Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/server/request.c
Modified: httpd/httpd/trunk/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=423886&r1=423885&r2=423886&view=diff
==============================================================================
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Thu Jul 20 04:01:07 2006
@@ -2,6 +2,11 @@
Changes with Apache 2.3.0
[Remove entries to the current 2.0 and 2.2 section below, when backported]
+ *) SECURITY:
+ core: Do not allow internal redirects like the DirectoryIndex of mod_dir
+ to circumvent the symbolic link checks imposed by FollowSymLinks and
+ SymLinksIfOwnerMatch. [Nick Kew, Ruediger Pluem, William Rowe]
+
*) mod_proxy: Support environment variable interpolation in reverse
proxying directives [Nick Kew]
Modified: httpd/httpd/trunk/server/request.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/request.c?rev=423886&r1=423885&r2=423886&view=diff
==============================================================================
--- httpd/httpd/trunk/server/request.c (original)
+++ httpd/httpd/trunk/server/request.c Thu Jul 20 04:01:07 2006
@@ -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;
}
Re: svn commit: r423886 - in /httpd/httpd/trunk: CHANGES server/request.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Joe Orton wrote:
>
> I think it's a *very* bad idea to imply that SymLinksIfOwnerMatch is a
> security feature.
>
> If you did want to call this a "security feature" then you also need to
> fix the big fat race condition inbetween all those nice careful stat()
> calls and the default handler going to open the file. Which I doubt
> would be simple to say the least.
>
> I'd stay well clear of the word "security" here.
+1. I simply don't see how we can permanently solve every case where users
are permitted to modify the server. And in fact; I'd like us to finally
divorce all of the "foolish/nefarious web author has done X to administrator's
server", into their own class of bugs. Let's give this a name other than
'security vulnerability'.
There are a bazillion other things nefarious users, who an administrator has
put faith in, can do to a server. Let's try to narrow this down to "Untrusted
User" and "Untrusted Author" categories (1. has a shell for various operations
to perms, symlinks, run scripts, etc etc), 2. can only place 'files' into the
web space).
The "security" rule of apache is simple, anything user "nobody" can see, apache
is capable of serving, and it's up to the administrator to configure such that
1. user "nobody" has no access to the files, or
2. configure apache in such as was as to "avoid" serving those files.
Re: svn commit: r423886 - in /httpd/httpd/trunk: CHANGES server/request.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 07/20/2006 02:04 PM, Joe Orton wrote:
>
> I think it's a *very* bad idea to imply that SymLinksIfOwnerMatch is a
> security feature.
>
> If you did want to call this a "security feature" then you also need to
> fix the big fat race condition inbetween all those nice careful stat()
> calls and the default handler going to open the file. Which I doubt
> would be simple to say the least.
>
> I'd stay well clear of the word "security" here.
I adjusted the svn log message (http://svn.apache.org/viewvc?view=rev&revision=423886)
and removed the word SECURITY from the CHANGES file (http://svn.apache.org/viewvc?view=rev&revision=424084).
I hope this addresses your concerns.
Regards
Rüdiger
Re: svn commit: r423886 - in /httpd/httpd/trunk: CHANGES server/request.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Ruediger Pluem wrote:
>
> I guess I can't change the log entry anymore. All I can do is adjust the CHANGES
> entry. Would that address your concerns?
You can. Syntax escapes me at the moment.
Re: svn commit: r423886 - in /httpd/httpd/trunk: CHANGES server/request.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 07/20/2006 06:11 PM, Garrett Rooney wrote:
>
>
> Actually you can change the log entry. Try 'svn pedit --revprop -r
> REVISION'
>
> -garrett
Thanks for your help Garrett and Justin. I mixed both of your proposals
and it worked just fine :-)
Regards
Rüdiger
Re: svn commit: r423886 - in /httpd/httpd/trunk: CHANGES server/request.c
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 7/20/06, Ruediger Pluem <rp...@apache.org> wrote:
> I guess I can't change the log entry anymore. All I can do is adjust the CHANGES
> entry. Would that address your concerns?
Actually you can change the log entry. Try 'svn pedit --revprop -r REVISION'
-garrett
Re: svn commit: r423886 - in /httpd/httpd/trunk: CHANGES server/request.c
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 7/20/06, Ruediger Pluem <rp...@apache.org> wrote:
> I guess I can't change the log entry anymore. All I can do is adjust the CHANGES
> entry. Would that address your concerns?
svn propchange --revprop -r423886 svn:log https://svn.apache.org/repos/asf/
HTH. -- justin
Re: svn commit: r423886 - in /httpd/httpd/trunk: CHANGES server/request.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 20.07.2006 14:04, Joe Orton wrote:
>
> I think it's a *very* bad idea to imply that SymLinksIfOwnerMatch is a
> security feature.
>
> If you did want to call this a "security feature" then you also need to
> fix the big fat race condition inbetween all those nice careful stat()
> calls and the default handler going to open the file. Which I doubt
> would be simple to say the least.
This is true.
>
> I'd stay well clear of the word "security" here.
I guess I can't change the log entry anymore. All I can do is adjust the CHANGES
entry. Would that address your concerns?
Regards
Rüdiger
Re: svn commit: r423886 - in /httpd/httpd/trunk: CHANGES server/request.c
Posted by Joe Orton <jo...@redhat.com>.
On Thu, Jul 20, 2006 at 11:01:08AM -0000, rpluem@apache.org wrote:
> Author: rpluem
> Date: Thu Jul 20 04:01:07 2006
> New Revision: 423886
>
> URL: http://svn.apache.org/viewvc?rev=423886&view=rev
> Log:
> * Check for symbolic links of the target file in the optimized case that we
> had already done this specific directory walk for this request. This can
> happen when we have an internal redirect, like the ones caused by mod_dir
> (/ -> index.html). See also
>
> http://mail-archives.apache.org/mod_mbox/httpd-dev/200607.mbox/%3c44B5521F.8050906@globalvanet.com%3e
>
> If we do not do this we have a security hole as the FollowSymLinks and
> SymLinksIfOwnerMatch settings can circumvented this way.
I think it's a *very* bad idea to imply that SymLinksIfOwnerMatch is a
security feature.
If you did want to call this a "security feature" then you also need to
fix the big fat race condition inbetween all those nice careful stat()
calls and the default handler going to open the file. Which I doubt
would be simple to say the least.
I'd stay well clear of the word "security" here.
joe