trac.versioncontrol.admin component should be optional

[Coscop@…]

In 0.12 the RepositoryAdmin? page allows to configure repository access through the web. For systems with multiple user groups who are trusted for their own projects but not for others there's a potential security issue as they could add the foreign repositories to their Trac through the admin page and thus read them.

Disabling trac.versioncontrol.admin.* in trac.ini is not possible because the component is required by e.g. the resync operation. rblank suggested that the component should be moved to a different once making it independent of other functionality.

In general, it would be a great addition if the component could be implemented in a way that makes it usable for a setup as described above, e.g. by adding a configuration option for setting a parent path in the trac.ini which is common to the repositories setup via web. E.g. if the allowed repositories are at /var/svn/myprojects this could be set in trac.ini so that repositories with other parent paths can't be made visible. That way group1 could have their projects at /var/svn/myprojects1 and configure them using RepositoryAdmin? while they can't access /var/svn/myprojects2. Of course, this option may not be editable on the Admin pages.

[rblank]

I wonder if there are other similar holes in Trac... Like the logging setup panel, which could allow overwriting any file writable by the web server by specifying its path as the log file path. But that one can be disabled separately.

[rblank]

The repository admin panel has been moved to a separate component in [9965], and can be disabled by adding the following to the [components] section of your trac.ini:

trac.versioncontrol.admin.RepositoryAdminPanel = disabled

[rblank]

Allow restricting the paths to repositories in the repository admin panel.

[rblank]

9511-repository-dir-prefix-r10018.patch​ is one possible implementation to restrict the allowed repository directories:

• A new option [trac] repository_dir_prefixes containing a list of allowed prefixes.
• New checks when adding and editing a repository, that only allow the operation if the repository is located below one of the given prefixes, and display a warning if not.

Thoughts?

[Ryan J Ollos <ryano@…>]

Replying to rblank:

I wonder if there are other similar holes in Trac... Like the logging setup panel, which could allow overwriting any file writable by the web server by specifying its path as the log file path. But that one can be disabled separately.

As a matter of fact, the hosting provider that hosts my production Trac instance disabled access to the logging panel last year for this very reason, after I unknowingly brought this to their attention. It's a shared hosting environment and they considered this a security hole. Not having access to this panel turned out to be quite inconvenient for me, at least until the ​th:LogViewerPlugin came along.

I had always meant to bring this up and see if a ticket should be filed for this issue.

[Samuel.Degrande@…]

(I'm fluzz on IRC). I just tested your patch, and it works like a charm. Thank you so much !
I had however to apply it by hand (I'm using 0.12 'official' version).

You could perhaps however check realpaths, so that symbolic links can not be used to by-pass the protection. (this is not a problem with my set-up, because users do not have shell access).

[Samuel.Degrande@…]

Replying to Ryan J Ollos <ryano@…>:

As a matter of fact, the hosting provider that hosts my production Trac instance disabled access to the logging panel last year for this very reason, after I unknowingly brought this to their attention.

Just wondering: how did they disabled it ? Through Apache config ?

[rblank]

Thanks for testing!

Replying to Samuel.Degrande@…:

You could perhaps however check realpaths, so that symbolic links can not be used to by-pass the protection. (this is not a problem with my set-up, because users do not have shell access).

I intentionally didn't use realpath() to allow using symlinks as a level of indirection to "reorganize" your repository layout. This is not a safety issue, even if users have shell access, as you can control the permission to create symlinks in a folder with filesystem permissions.

Replying to Samuel.Degrande@…:

Just wondering: how did they disabled it ?

Just disable the LoggingAdminPanel component:

[components]
trac.admin.web_ui.LoggingAdminPanel = disable

[cboos]

Just a small comment regarding the choice of the name for the new setting. I have the feeling that the [trac] section starts to become overcrowded (see TracIni#trac-section). The repository_{dir,type} have always been there, but now that those settings are nearly obsoleted by multirepos support, we shouldn't add more repository related stuff there. Maybe we could add a new [repository] section, and the setting itself could be named restricted_dir_prefixes?

[rblank]

Replying to cboos:

Maybe we could add a new [repository] section, and the setting itself could be named restricted_dir_prefixes?

Fine by me. Of course, the [repositories] section cannot be used, but [repository] is good.

[rblank]

Or [versioncontrol]?

[cboos]

Yes, sounds fine, then repository_dir_prefixes is OK, maybe even allowed_repository_dir_prefixes to be really self-explaining.

[rblank]

Patch applied in [10042], with the changes from comment:11.