You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ka...@apache.org on 2010/04/12 13:26:29 UTC
svn commit: r933194 -
/subversion/trunk/subversion/mod_authz_svn/mod_authz_svn.c
Author: kameshj
Date: Mon Apr 12 11:26:28 2010
New Revision: 933194
URL: http://svn.apache.org/viewvc?rev=933194&view=rev
Log:
[issue2753] Fix issue 2753.
Relax requests aimed at the repo Parent path from authz control.
* subversion/mod_authz_svn/mod_authz_svn.c
(create_authz_svn_dir_config): Canonicalize conf->base_path.
(req_check_access): When canonicalized 'uri' and 'conf->base_path' are same
allow the request.
(access_checker, check_user_id, auth_checker):
Initialize repos_path to 'NULL' otherwise it can point
to stray values when req_check_access relaxes certain requests without
initialising the out parameters.
Modified:
subversion/trunk/subversion/mod_authz_svn/mod_authz_svn.c
Modified: subversion/trunk/subversion/mod_authz_svn/mod_authz_svn.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_authz_svn/mod_authz_svn.c?rev=933194&r1=933193&r2=933194&view=diff
==============================================================================
--- subversion/trunk/subversion/mod_authz_svn/mod_authz_svn.c (original)
+++ subversion/trunk/subversion/mod_authz_svn/mod_authz_svn.c Mon Apr 12 11:26:28 2010
@@ -66,6 +66,9 @@ create_authz_svn_dir_config(apr_pool_t *
authz_svn_config_rec *conf = apr_pcalloc(p, sizeof(*conf));
conf->base_path = d;
+ if (d)
+ conf->base_path = svn_uri_canonicalize(d, p);
+
/* By default keep the fortress secure */
conf->authoritative = 1;
conf->anonymous = 1;
@@ -210,6 +213,7 @@ req_check_access(request_rec *r,
svn_authz_t *access_conf = NULL;
svn_error_t *svn_err;
char errbuf[256];
+ const char *canonicalized_uri;
const char *username_to_authorize = get_username_to_authorize(r, conf);
switch (r->method_number)
@@ -249,6 +253,22 @@ req_check_access(request_rec *r,
break;
}
+ canonicalized_uri = svn_uri_canonicalize(r->uri, r->pool);
+ if (strcmp(canonicalized_uri, conf->base_path) == 0)
+ {
+ /* Do no access control when conf->base_path(as configured in <Location>)
+ * and given uri are same. The reason for such relaxation of access
+ * control is "This module is meant to control access inside the
+ * repository path, in this case inside PATH is empty and hence
+ * dav_svn_split_uri fails saying no repository name present".
+ * One may ask it will allow access to '/' inside the repository if
+ * repository is served via SVNPath instead of SVNParentPath.
+ * It does not, The other methods(PROPFIND, MKACTIVITY) for
+ * accomplishing the operation takes care of making a request to
+ * proper URL */
+ return OK;
+ }
+
dav_err = dav_svn_split_uri(r,
r->uri,
conf->base_path,
@@ -554,7 +574,7 @@ access_checker(request_rec *r)
{
authz_svn_config_rec *conf = ap_get_module_config(r->per_dir_config,
&authz_svn_module);
- const char *repos_path;
+ const char *repos_path = NULL;
const char *dest_repos_path = NULL;
int status;
@@ -611,7 +631,7 @@ check_user_id(request_rec *r)
{
authz_svn_config_rec *conf = ap_get_module_config(r->per_dir_config,
&authz_svn_module);
- const char *repos_path;
+ const char *repos_path = NULL;
const char *dest_repos_path = NULL;
int status;
@@ -639,7 +659,7 @@ auth_checker(request_rec *r)
{
authz_svn_config_rec *conf = ap_get_module_config(r->per_dir_config,
&authz_svn_module);
- const char *repos_path;
+ const char *repos_path = NULL;
const char *dest_repos_path = NULL;
int status;
Re: Expansion of authz policy name leak (was: svn commit: r933194
- /subversion/trunk/subversion/mod_authz_svn/mod_authz_svn.c)
Posted by Peter Samuelson <pe...@p12n.org>.
[C. Michael Pilato]
> IIUC, prior to your change, nobody who had enabled authz at all could
> make use of the SVNListParentPath feature (because the authorization
> for that display would systematically fail). But this also means
> that Subversion never leaked the name of a repository that was
> intended to be private/hidden from particular users. Now, we no
> longer suffer the blanket authz failure, but we are showing the name
> of every repository in the parent directory without regard to any
> authz rules whatsoever.
I have to admit, from my Unix background, this seems perfectly natural.
Unix has an inode-centric view of the filesystem, where a filename
really belongs to the directory it is in, more than to the file itself,
and permission to see the existence of the filename reflects this.
Would this be more surprising to people who don't hve a Unix
background, then? 'Surprising' is the key question - as with any
security choice, we want to do what the administrator will expect and
assume.
Peter
RE: Expansion of authz policy name leak (was: svn commit: r933194 - /subversion/trunk/subversion/mod_authz_svn/mod_authz_svn.c)
Posted by Kamesh Jayachandran <ka...@collab.net>.
>IIUC, prior to your change, nobody who had enabled authz at all could make
>use of the SVNListParentPath feature (because the authorization for that
>display would systematically fail). But this also means that Subversion
>never leaked the name of a repository that was intended to be private/hidden
>from particular users. Now, we no longer suffer the blanket authz failure,
>but we are showing the name of every repository in the parent directory
>without regard to any authz rules whatsoever.
Whoever wanted SVNListParentPath to work with authz prior to this commit was using a workaround of <Location /svn/> which was leaking the same info.
With regards
Kamesh Jayachandran
Expansion of authz policy name leak (was: svn commit: r933194 - /subversion/trunk/subversion/mod_authz_svn/mod_authz_svn.c)
Posted by "C. Michael Pilato" <cm...@collab.net>.
Kamesh Jayachandran wrote:
>> I would expect that a request to the parent directory would yield a
>> listing
>> that included the 'calc' and 'watch' repositories, but not the 'lamp'
>> one.
>>
>> Is that the case?
>>
>>
> No.
>
> These authz rule should *not* list anything inside the repo 'lamp' but
> not lamp itself when requested for the parent path root.
>
> The feature that you ask for is possible only if 'mod_dav_svn'(which
> implements SVNListParentPath) consults mod_authz_svn(or some authorizer)
> for every item listed which is not the case today.
That's right -- we would need to consult the authz file (or files -- there
could be many) in order to make the show/no-show determination for each
repository. But this is authorization/security we're talking about here, so
we need to move forward with caution.
IIUC, prior to your change, nobody who had enabled authz at all could make
use of the SVNListParentPath feature (because the authorization for that
display would systematically fail). But this also means that Subversion
never leaked the name of a repository that was intended to be private/hidden
from particular users. Now, we no longer suffer the blanket authz failure,
but we are showing the name of every repository in the parent directory
without regard to any authz rules whatsoever.
I'll grant that sorta-kinda there is some precedent here in that Subversion
has always compromised (albeit not so explicitly) the names of subtree roots
that are deemed unreadable by authz policy (see "KNOWN LEAKAGE OF UNREADABLE
PATHS" in the authz_policy.txt file). But our currently WebDAV directory
listing (GET handler) doesn't leak names in such a way, and I'm not sure we
should so readily introduce a leak of this sort).
How do other feel about this?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn commit: r933194 - /subversion/trunk/subversion/mod_authz_svn/mod_authz_svn.c
Posted by Kamesh Jayachandran <ka...@collab.net>.
On 04/12/2010 07:49 PM, C. Michael Pilato wrote:
> kameshj@apache.org wrote:
>
>> Author: kameshj
>> Date: Mon Apr 12 11:26:28 2010
>> New Revision: 933194
>>
>> URL: http://svn.apache.org/viewvc?rev=933194&view=rev
>> Log:
>> [issue2753] Fix issue 2753.
>>
>> Relax requests aimed at the repo Parent path from authz control.
>>
>> * subversion/mod_authz_svn/mod_authz_svn.c
>> (create_authz_svn_dir_config): Canonicalize conf->base_path.
>> (req_check_access): When canonicalized 'uri' and 'conf->base_path' are same
>> allow the request.
>> (access_checker, check_user_id, auth_checker):
>> Initialize repos_path to 'NULL' otherwise it can point
>> to stray values when req_check_access relaxes certain requests without
>> initialising the out parameters.
>>
> In a perfect world, I would expect that requests to the parent directory
> would not be authz-denied, but that each repository in the listing of
> repositories would be authz-checked against the authz configuration. In
> other words, say I have a parent-path with three repositories: calc, watch,
> lamp. And say I have an authz file like so:
>
> [lamp:/]
> * =
>
> I would expect that a request to the parent directory would yield a listing
> that included the 'calc' and 'watch' repositories, but not the 'lamp' one.
>
> Is that the case?
>
>
No.
These authz rule should *not* list anything inside the repo 'lamp' but
not lamp itself when requested for the parent path root.
The feature that you ask for is possible only if 'mod_dav_svn'(which
implements SVNListParentPath) consults mod_authz_svn(or some authorizer)
for every item listed which is not the case today.
With regards
Kamesh Jayachandran
Re: svn commit: r933194 - /subversion/trunk/subversion/mod_authz_svn/mod_authz_svn.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
kameshj@apache.org wrote:
> Author: kameshj
> Date: Mon Apr 12 11:26:28 2010
> New Revision: 933194
>
> URL: http://svn.apache.org/viewvc?rev=933194&view=rev
> Log:
> [issue2753] Fix issue 2753.
>
> Relax requests aimed at the repo Parent path from authz control.
>
> * subversion/mod_authz_svn/mod_authz_svn.c
> (create_authz_svn_dir_config): Canonicalize conf->base_path.
> (req_check_access): When canonicalized 'uri' and 'conf->base_path' are same
> allow the request.
> (access_checker, check_user_id, auth_checker):
> Initialize repos_path to 'NULL' otherwise it can point
> to stray values when req_check_access relaxes certain requests without
> initialising the out parameters.
In a perfect world, I would expect that requests to the parent directory
would not be authz-denied, but that each repository in the listing of
repositories would be authz-checked against the authz configuration. In
other words, say I have a parent-path with three repositories: calc, watch,
lamp. And say I have an authz file like so:
[lamp:/]
* =
I would expect that a request to the parent directory would yield a listing
that included the 'calc' and 'watch' repositories, but not the 'lamp' one.
Is that the case?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand