Edgewall Software
Modify

Opened 8 years ago

Closed 7 years ago

Last modified 5 years ago

#9976 closed defect (fixed)

Permission check of Repository Browser does not work

Reported by: anonymous Owned by: Christian Boos
Priority: normal Milestone: 0.12.3
Component: version control/browser Version: 0.12.1
Severity: normal Keywords: svnauthz, authzsourcepolicy
Cc: dmitry@… Branch:
Release Notes:

AuthzSourcePolicy slightly deviates from mod_authz_svn rules in order to show the parent folders of readable paths, so that one can actually find the latter by browsing.

API Changes:

Description

Permission check of Repository Browser does not work correctly for me, is it a bug(security incident) or according to the specification?

I am using trac0.12.1 and Subversion in my project. I created a tag for my project, and set the access permissions for it. But the folders under the tag can be accessed with repository browser, even if the user doesn't have permission to access it.

And it worked well when I use svn access.


Details:

(1)create a tag named as "REL-1.0"
svn copy http://host/svn/myproject/trunk/ http://host/svn/myproject/tags/REL-1.0


(2)set permissions for the tag
This is part of my svnauth file after I finished setting the permissions:

[myproject:/tags/REL-1.0]
* = r
admin = r

[myproject:/tags/REL-1.0/secret]
* = 
admin = r


(3) login with a user (not admin)
Access the secret folder by repository browser:
http://host/trac/myproject/browser/tags/REL-1.0/secret

The folders and files under "/tags/REL-1.0/secret" are displayed.
I think this is not correct.

(4) Verify by svn access (not admin)
 http://host/svn/myroject/tags/REL-2.0/secret/ 

The access was refused because there is not enough permission.
I think this is correct

P.S. I have debugged the source code of "browser.py" and "perm.py". I found when I accessed "/tags/REL-1.0/secret" folder by repository browser, the permission of "/trunk/secret" was checked by the first line of _render_dir method in "browser.py". (node.resource) I hope the permission of "/tags/REL-1.0/secret" should be checked.

Attachments (1)

svn_authz-really-allow-access-to-all-parent-director.patch (2.9 KB ) - added by Christian Boos 7 years ago.
fix svn authz to allow browsing a folder if there's any permission for a path below that one

Download all attachments as: .zip

Change History (24)

comment:1 Changed 8 years ago by Remy Blank

Keywords: needinfo added
Version: 0.12.1

Did you add AuthzSourcePolicy to [trac] permission_policies, as mentioned on TracUpgrade and TracIni? Is your repository defined in Trac with the name "myproject"? The repository name must correspond to the module name used in the authz file.

comment:2 Changed 8 years ago by anonymous

Yes, I had added AuthzSourcePolicy to [trac] permission_policies. And I think I defined the repository name correctly.

comment:3 Changed 8 years ago by anonymous

Would you please check the following lines in browser.py?

line 540:
        req.perm(node.resource).require('BROWSER_VIEW')

line 616:
        req.perm(node.resource).require('FILE_VIEW')

I found that in the object of "node.resource", there was no any information about the tag ("/tags/REL-1.0/secret") that I had accessed, instead of it, "/trunk/secret" was stored in it and was checked by the Permission check logic.(created_path)

I tried to change this by creating a new resorce object instead of "node.resource" such like:

        resource = Resource('source', node.path,
                                      version=node.created_rev,
                                      parent=node.repos.resource))
        req.perm(resource).require('BROWSER_VIEW')

It seemed work for me, but I don't know if the changes will affect the other programs.

comment:4 Changed 8 years ago by Christian Boos

Milestone: 0.12.3

I'm quite sure we changed to created_path at some point for security concerns as well… ("I have /trunk/secret, now I tagged it and everything below /tags/1.0/secret is visible!"). Maybe we should just check both.

Last edited 8 years ago by Christian Boos (previous) (diff)

comment:5 Changed 8 years ago by Christian Boos

Keywords: needinfo removed

comment:6 Changed 8 years ago by Christian Boos

Component: generalversion control/browser
Priority: highestnormal

comment:7 Changed 8 years ago by anonymous

("I have /trunk/secret, now I tagged it and everything below /tags/1.0/secret is visible!")

I think this is not a problem because it is the same behavior as subversion. Permission under "/tags/1.0" need to be set after it was created.

comment:8 Changed 8 years ago by Christian Boos

See #10110 for a similar use case.

comment:9 Changed 7 years ago by Remy Blank

Owner: set to Christian Boos

Christian, I assume you wanted to grab this?

comment:10 Changed 7 years ago by Dmitry Zamaruev <dmitry@…>

Have the same issue, but in slightly other way. Given the default model of development for Subversion I have /trunk, /branches and /tags in repository. Trunk and Branches are invisible to $anonymous users, so when software released - it is 'svn copy'ed to Tags - so anyone could grab it. Given the following AuthZ configuration:

[test:/]
* = r

[test:/trunk]
$anonymous = 

[test:/branches]
$anonymous = 

It is impossible to access any file/folder copied from /trunk or /branches to /tags - as Node.get_perm() will check against (created_path, created_rev) and those reflects original /trunk or /branches paths - to which $anonymous does not have access. For example we have folder /trunk/src and have 'svn copy /trunk /tags/3.0' Here is some of debug output for accessing /tags/3.0 via "Browse Source":

Trac[main] DEBUG: Dispatching <Request "GET '/browser/test/tags/3.0'">
Trac[perm] DEBUG: AuthzSourcePolicy denies anonymous performing BROWSER_VIEW on <Resource u'repository:test, source:trunk/src@5'>

In sight of ticket:3340:

cboos wrote:

So my thinking was always to refactor the API to have:

  • (rev,path) ⇒ the Node as currently seen and requested
  • (created_rev, created_path) ⇒ the revision and path at which the Node got created (see above for when a Node gets created)

There should be no reason to check (created_rev, created_path) but check (rev,path) to get permissions of "Node as currently seen and requested"

Changing definition of Node's resource from:

resource = property(lambda self: Resource('source', self.created_path,
                                          version=self.created_rev,
                                          parent=self.repos.resource))

to:

resource = property(lambda self: Resource('source', self.path,
                                          version=self.created_rev,
                                          parent=self.repos.resource))

fixes problem, but raise a question about "intended use of 'resource'" and possible security issues.

Looking forward for your comments about intended usage of 'created_path' with permission checking.

PS. trac-0.12.2, subversion-1.6.16, python-2.6.6

comment:11 Changed 7 years ago by Christian Boos

Keywords: svnauthz added

Ok, but I think that if no permissions has been set on (rev,path), then it makes sense to look at those of (created_rev, created_path).

comment:12 Changed 7 years ago by Dmitry Zamaruev <dmitry@…>

Cc: dmitry@… added

cboos wrote:

I think that if no permissions has been set on (rev,path), then it makes sense to look at those of (created_rev, created_path)

Shouldn't behavior mimic the intended one for Subversion AuthZ ? If permissions set and checked on /tags - it will return the only that result and will not dig whether or when I copied path there from "secret" place. Also same behavior is crucial in having one configuration file for "Subversion AuthZ" and for "Browse Source" - you will be always sure that same user will see same resources thru SVN or thru Trac.

PS. as authz file always would have default permissions (in [module:/] or in /) you will always end up with some set of permissions for (rev, path)

comment:13 Changed 7 years ago by Christian Boos

Oh well, I think you're right: if we're trying to be "smarter" than svn authz, then you may indeed wrongly feel safe by seeing that such secret folder is not visible in Trac and forget to adjust your authz file to also get the same protection directly via svn…

comment:14 Changed 7 years ago by Remy Blank

We have tried to mimic Subversion as closely as possible, but there's at least one case where it doesn't make sense. With Subversion, you can forbid access to e.g. /projects, and selectively allow access to /projects/project1 and /projects/project2. Users are then able to checkout these two sub-folders, but neither /projects nor /projects/project3. If you browse /projects on the repository, you get a "403 Forbidden".

If we copied this behavior in Trac, then /projects wouldn't appear at all in the repository browser, and users would have no way to reach /projects/project1. For this reason, we allow access to /projects (the directory but not its content).

Of course, this is unrelated to the case here. It's just meant as an illustration why we don't copy Subversion completely.

comment:15 in reply to:  14 ; Changed 7 years ago by Christian Boos

Replying to rblank:

… If we copied this behavior in Trac, then /projects wouldn't appear at all in the repository browser, and users would have no way to reach /projects/project1. For this reason, we allow access to /projects (the directory but not its content).

That's however not the behavior I observed, or maybe by "accident"…

For example if you use the trac test repository (svnrepos.dump), with the following svn access file content:

# -*- coding: utf-8 -*-

[tractest:/tête/dir1]
* = 

[tractest:/tête/dir1/dir2]
* = r

[tractest:/tags/v1.1/dir1]
* = 

[tractest:/tags/v1.1/dir1/dir2]
* = r

then browsing tractest/tête won't show you dir1, however tractest/tags/v1.1 will. A direct access to either tractest/tête/dir1 or tractest/tags/v1.1/dir1 is rejected, but you can go to tractest/tête/dir1/dir2 and tractest/tags/v1.1/dir1/dir2.

If we fix the node resources to use path instead of created_path as suggested above in comment:10, then the observed behavior for tags/v1.1 becomes the same as the one with tête.

comment:16 in reply to:  15 ; Changed 7 years ago by Christian Boos

Replying to cboos:

That's however not the behavior I observed, or maybe by "accident"…

Looks like it's enough to permute the two blocks "# Walk from resource up parent directories" and "# Allow access to parent directories of allowed resources" (here).

Changed 7 years ago by Christian Boos

fix svn authz to allow browsing a folder if there's any permission for a path below that one

comment:17 in reply to:  16 Changed 7 years ago by Christian Boos

comment:18 Changed 7 years ago by Remy Blank

Looks good.

comment:19 Changed 7 years ago by Christian Boos

That patch committed as r10907.

Now I have to check again if there's anything else in that ticket which is not already covered by #7343 (which that changeset fixed) and by the #10208 topic.

comment:20 Changed 7 years ago by Remy Blank

This should also be fixed with 10208-requested-rev-r10910.patch:ticket:10208, and contains the suggestion from comment:10.

comment:21 Changed 7 years ago by Remy Blank

Patch applied in [10914]. Is there anything else that needs to be done here?

comment:22 Changed 7 years ago by Christian Boos

Release Notes: modified (diff)
Resolution: fixed
Status: newclosed

I think we're done.

As the original report in this ticket was basically a duplicate of #10208, I adapted the notes to reflect the r10907 change.

comment:23 Changed 5 years ago by Ryan J Ollos

Keywords: authzsourcepolicy added

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted.
to The owner will be changed from Christian Boos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.