Edgewall Software
Modify

Opened 10 years ago

Closed 9 years ago

#9511 closed defect (fixed)

trac.versioncontrol.admin component should be optional

Reported by: Coscop@… Owned by: Remy Blank
Priority: high Milestone: 0.12.1
Component: version control Version: 0.12
Severity: critical Keywords:
Cc: Branch:
Release Notes:
API Changes:

Description

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.

Attachments (1)

9511-repository-dir-prefix-r10018.patch (7.1 KB ) - added by Remy Blank 9 years ago.
Allow restricting the paths to repositories in the repository admin panel.

Download all attachments as: .zip

Change History (13)

comment:1 by Remy Blank, 10 years ago

Component: generalversion control
Priority: normalhigh

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.

comment:2 by Remy Blank, 10 years ago

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

by Remy Blank, 9 years ago

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

comment:3 by Remy Blank, 9 years ago

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?

in reply to:  1 ; comment:4 by Ryan J Ollos <ryano@…>, 9 years ago

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.

comment:5 by Samuel.Degrande@…, 9 years ago

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

in reply to:  4 comment:6 by Samuel.Degrande@…, 9 years ago

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 ?

in reply to:  5 comment:7 by Remy Blank, 9 years ago

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

comment:8 by Christian Boos, 9 years ago

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?

in reply to:  8 comment:9 by Remy Blank, 9 years ago

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.

comment:10 by Remy Blank, 9 years ago

Or [versioncontrol]?

comment:11 by Christian Boos, 9 years ago

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

comment:12 by Remy Blank, 9 years ago

Resolution: fixed
Status: newclosed

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Remy Blank.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Remy Blank 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.