AuthzSourcePolicy does not conform to mod_authz_svn rules

[dmcr@…]

Hi,

I noticed a discrepancy between AuthzSourcePolicy and mod_authz_svn that I think qualifies as a bug in the AuthzSourcePolicy implementation.

We have a user that wants to give universal read-only access to his repo's 'tags' project, but only give specific user access to the 'trunk' and 'branches' projects. In the simplest case, if I give someone r or rw access to tags, but not to trunk and branches, nor to the parent level, they can see all the files in the tags project (and none of the others) when accessing the repo using mod_authz_svn, by using a browser for example.

Example:

[myrepo:/] user1 = r

[myrepo:/trunk] user2 = r

[myrepo:/tags] user3 = r

In this example, user1 and user3 can both see the contents of the tags project when using a browser to directly view the svn 'myrepo' repo. But user3 can't see the trunk project while user1 can. This is what you would expect.

But when they try to browse from inside the associated Trac project, user3 can't see any files or directories in the svn 'tags' project, whereas user1 can. This is because the files in the tags project are the result of 'svn copy' operations from the trunk project to the tags project. Presumably, this does not copy the actual data, but instead provides a pointer in the tags project to the data residing in the trunk project, since the log file shows permission failures for trunk-resident files when browsing the tags project. So it would appear that AuthzSourcePolicy is coded to check permissions on the pointers, which it should *not* do, since the internal arrangement of data should be of no concern to the user.

But as a result, my Trac administrator has to choose between not giving the world read-only access to his tags project, or also giving the world read-only access to his trunk project, which he does not want to do since some of the code there is confidential and also not ready to run.

So it would be very helpful if AuthzSourcePolicy were to conform to the behavior of mod_authz_svn in this regard.

Thanks,

Dennis

[dmcr@…]

Hi again,

I know you guys are very busy, but I was wondering if someone was likely to be working on this ticket. I myself am not familiar with the internals of the svn native filesystem, and was hoping that someone at your end was.

Thanks,

Dennis

[Remy Blank]

Nobody is currently working on it, but we should definitely take a look.

[Itamar Oren]

Just ran into the same issue today (also with trac-0.12.stable).

Did not have any luck figuring out why it happens. Hope I'll find some time taking a deeper look…

[Itamar Oren]

OK, here's what I came up with:

The AuthzSourcePolicy::check_permission()​ method uses resource.id as the node path in checking against the authz file, but the Node​ class sets the resource.id​ to be the created_path property of the Node and not the path property, which makes the AuthzSourcePolicy to return the permission according to the created_path instead of the current actual path.

The fix is obvious (let resource.id be path instead of created_path), but there must be some other reason the resource.id was set this way… What is that reason? How can this be fixed?

[Itamar Oren]

suggested patch to resolve the issue

[Itamar Oren]

I have cofirmed my concern that this issue also manifests as a security vulnrability. To illustrate, assume I have this repository:

- Private
- Public
  - TestDir
    - test_file

And my SVN authz file gives global read for /Public, and no access to everything else.

If I svn-move TestDir from Public to Private, I can still direct my browser to /browser/Private/TestDir/test_file, and get the file!

I have attached a patch​ to this ticket that resolves the issue.

[Remy Blank]

I have also always wondered why we use created_*. Can you please confirm that svn_authz_mod does the right thing with the test repository you created?

[Christian Boos]

Btw, I did the same change when working on #9976, so I'm also OK for the change.

[Itamar Oren]

Replying to rblank:

I have also always wondered why we use created_*. Can you please confirm that svn_authz_mod does the right thing with the test repository you created?

Confirmed

[Christian Boos]

Replying to cboos:

Btw, I did the same change when working on #9976, so I'm also OK for the change.

In order to answer Remy's question about why we use created_, I re-read #3340, #7744 and r8797 and now I have a doubt: we still have self.rev = self.created_rev in SubversionNode.__init__, so the change I talked about in #3340 never happened. As a consequence, (self.path, self.rev) can be inconsistent and we should use (self.path, self._requested_rev). And in trunk we can probably make the switch to self.rev = self._requested_rev.

[Dennis McRitchie <dmcr@…>]

So Christian, are you saying that 0.12.1 can be patched with the fix for #9976, plus the patch for this ticket as changed below?

resource = property(lambda self: Resource('source', self.path,

version=self.requested_rev, parent=self.repos.resource))

Also, I'm not sure what your last sentence means: does it refer to an additional change that's needed for the fix? If so, where does it go?

Thanks, Dennis

[Christian Boos]

version=self._requested_rev, yes. But I can't test at the moment, so maybe Itamar could confirm…

The last two sentences just mean that the above looks a bit ugly, and while it's OK for 0.12-stable we can probably do better on trunk and do the "planned" change to fix the Node.rev at the version which was requested.

[Itamar Oren]

Replying to cboos:

version=self._requested_rev, yes. But I can't test at the moment, so maybe Itamar could confirm…

I think this change might be problematic. The _requested_rev property is specific to the SubversionNode implementation, while the resource property is defined in the api, which means this change should work fine with SVN repositories, but will fail with other implementations that do not define _requested_rev…

I don't known why the self.rev = self.created_rev line exists, but I would like to go with the switch to self.rev = self._requested_rev in the stable branch, unless I'm missing deeper repercussions…

[Christian Boos]

Replying to itamaro:

Replying to cboos:

version=self._requested_rev, yes. But I can't test at the moment, so maybe Itamar could confirm…

I think this change might be problematic. The _requested_rev property is specific to the SubversionNode implementation, while the resource property is defined in the api, which means this change should work fine with SVN repositories, but will fail with other implementations that do not define _requested_rev…

Sure, sorry for the approximations, I didn't test.

I don't known why the self.rev = self.created_rev line exists, but I would like to go with the switch to self.rev = self._requested_rev in the stable branch, unless I'm missing deeper repercussions…

I'm not sure about possible other repercussions either, but just seeing that I wanted to do that for quite some time and that it nevertheless "resisted" a few major releases, I don't feel inclined to do it now on stable, especially on a .3 that will get released "soon" ;-)

[Remy Blank]

I would like to finish this, but I have a hard time understanding what the solution should be. Actually, I don't understand how .created_path can be different from .path: for a normal commit, they must be identical, and for a copy, it should be the path of the destination, hence also identical to the path passed to the constructor. Or would this be the case only if the copy also had a change to the file's content? So they would differ only if the operation was a pure copy? Even in that case, I don't see the use of .created_path.

I do see the use of .created_rev, so that one should certainly stay. But maybe we should simply remove .created_path (or set it equal to .path for backward compatibility). For the .resource property, I would keep .created_rev (it won't make any difference for authz, as the revision isn't used, but it feels more correct).

[Remy Blank]

Use .path and .rev for permission checks.

[Remy Blank]

10208-requested-rev-r10910.patch​ includes Itamar's patch changing .resource to use .path and .rev instead of .created_path and .created_rev, and also fixes SubversionNode to set .rev to the requested revision instead of .created_rev (and incidentally removes ._requested_rev.

Of course, this breaks some tests that check the value of .rev, these are fixed in the patch as well. All tests pass now. On to some more testing, possibly with more unit tests. A second pair of eyes appreciated, too.

[Itamar Oren]

Tested Remy's updated patch. Works for me, with my test cases.

[Christian Boos]

There are a few issues with the patch, as I feared in the TracBrowser:

• when visiting a "deleted" file, starting from the changeset which deleted it, one can see its last revision but from there we can't navigate to previous revisions … forget it, also happens without the patch ;-)
• same thing for moves actually, when viewing a file or a folder below a directory which has been copied, Last Change goes to a non-existing path (this problem only with the patch!)

Bottom line: the change is very positive (e.g. #10358 verified to be fixed), but there are some more adaptations needed in the browser to cope with it, at least for the Last Change link.

[Remy Blank]

Fix a few issues in the source browser.

[Remy Blank]

10208-requested-rev-2-r10910.patch​ fixes a few issues I found in the browser, for example all files listed in a directory were showing the same revision. I wasn't able to reproduce the issue described in comment:18, though. Can you please give some details about the repository "shape" and the actions you execute?

It looks like there are quite a few places in the browser where .rev and .created_rev were used interchangeably, and I'm not all too familiar with the code, so I wouldn't mind a little help with polishing this patch (also, I'm very tired ATM, which doesn't help, so I'll stop for now).

To find the remaining issues, perhaps it would make sense to write a small script that scrapes a set of pages, once without and once with the patch, and shows the diffs, possibly hiding some known differences. This could even be generally useful, e.g. for refactoring.

[Christian Boos]

My issue with Last Change is gone with the new patch. The problem was actually happening for a move operation (e.g. svn mv trunk/dirA trunk/dirB, commit 10) then the Last Change for a dirB/sub path was pointing to /changeset/<requested-rev>/<created-path> (i.e. /changeset/10/trunk/dirA/sub) which didn't exist (anymore).

Looks like you nailed all the cases, as I couldn't trigger another issue. Only further testing will tell (we have given ourselves a week for this, so I think it's quite safe to commit 10208-requested-rev-2-r10910.patch now).

[Remy Blank]

Patch applied in [10914]. It may be a good idea to add a specific note about this fix in the release notes for 0.13.2rc1, and ask people to specifically test for regressions in the source browser (and possibly the log and changeset views, too).