Permission check of Repository Browser does not work

[anonymous]

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.

[Remy Blank]

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.

[anonymous]

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

[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.

[Christian Boos]

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.

[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.

[Christian Boos]

See #10110 for a similar use case.

[Remy Blank]

Christian, I assume you wanted to grab this?

[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

[Christian Boos]

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).

[Dmitry Zamaruev <dmitry@…>]

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)

[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…

[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.

[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.

[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​).

[Christian Boos]

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

[Christian Boos]

The svn_authz-really-allow-access-to-all-parent-director.patch​ fixes this part.

[Remy Blank]

Looks good.

[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.

[Remy Blank]

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

[Remy Blank]

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

[Christian Boos]

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.