Edgewall Software
Modify

Opened 3 years ago

Closed 7 months ago

Last modified 7 months ago

#10285 closed defect (fixed)

Components in permission_policies should be always loaded

Reported by: Mitar Owned by: rjollos
Priority: normal Milestone: 1.0.2
Component: general Version: 0.12.1
Severity: normal Keywords: permission, exception
Cc: mmitar@…, leho@…, dkg@…, ethan.jucovy@…, shoffmann, jomae
Release Notes:

Loading of all permission_policies is enforced by raising an exception if any of the named Components are not found. request_filters, action_controllers and any plugin configuration options utilizing OrderedExtensionOption will raise an exception if one of the named Components is not found.

API Changes:

OrderedExtensionsOption raises a ConfigurationError if one of the named implementations of the interface is not found.

Description

Trac should enforce loading of all components in permission_policies and restrain from starting if this is not possible. Otherwise it is possible that some component does not load (because of some system upgrade or any other Python error) and some security enforcing is turned off. This could have grave security implications.

More about this topic in this ticket.

Attachments (2)

patch10285_trac0.12.3.patch (3.5 KB) - added by Steven R. Loomis <srl@…> 17 months ago.
patch against milestone:0.12.3 implementing MandatoryOrderedExtensionsOption?
ConfigurationError.png (22.2 KB) - added by rjollos 8 months ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 3 years ago by lkraav <leho@…>

  • Cc leho@… added

comment:2 follow-up: Changed 3 years ago by rblank

  • Keywords permission added
  • Milestone set to next-minor-0.12.x

We could extend OrderedExtensionOption to do that (or create a MandatoryOrderedExtensionOption subclass). Sounds like a good idea.

OTOH, a whitelist is inherently safer than the blacklist currently implemented in SensitiveTicketsPlugin. Trac supports both policies, and actually, if a permission isn't granted explicitly by an IPermissionPolicy, it's denied. Maybe the plugin should also be modified to use a blacklist?

comment:3 Changed 3 years ago by Ryan J Ollos <ryano@…>

  • Cc ryano@… added

comment:4 follow-up: Changed 3 years ago by cboos

Also have a look at the discussion in #5535, where similar concerns about security when permission policies couldn't be loaded were raised.

([OT] where's the (-) Remove this change button? I was about to remove ryano@ from the CC: ;-) )

comment:5 in reply to: ↑ 4 Changed 3 years ago by rblank

Replying to cboos:

([OT] where's the (-) Remove this change button? I was about to remove ryano@ from the CC: ;-) )

Yes, I had the same experience with comment:1. The thing is, it's more difficult to implement than I initially thought, and I haven't had time to dive in lately. Still on my list, though.

comment:6 in reply to: ↑ 2 Changed 3 years ago by Mitar

Replying to rblank:

We could extend OrderedExtensionOption to do that (or create a MandatoryOrderedExtensionOption subclass). Sounds like a good idea.

This really sounds as a good idea.

BTW, why is required Component flag/field not checked anywhere, it seems? I saw it in some official Trac components being set to True.

Trac supports both policies, and actually, if a permission isn't granted explicitly by an IPermissionPolicy, it's denied. Maybe the plugin should also be modified to use a blacklist?

Yes, but the idea is that you augment the existing permission policy components (like official one for tickets). Otherwise you have to replace whole chain with your own permission policy. I believe MandatoryOrderedExtensionOption is a very simple solution which would make things easy to develop with predicting results. So good work/effect ratio in my opinion.

comment:7 follow-up: Changed 2 years ago by dkg@…

  • Cc dkg@… added

I'm the new maintainer for the SensitiveTicketsPlugin. For this plugin to be able to deny access to certain tickets, I would very much like to see a way to force trac to fail closed if the plugin isn't properly loaded.

I don't know enough about trac internals to advocate for any of the proposed mechanisms above, but i agree that one of them needs to be available for plugins like SensitiveTickets.

If such a mechanism already exists, i'd be happy to learn about it and would use it.

If it doesn't exist, what can i do to help it along?

comment:8 in reply to: ↑ 7 Changed 2 years ago by rblank

Replying to dkg@…:

If it doesn't exist, what can i do to help it along?

It currently doesn't. The best idea so far is still comment:2: enhance OrderedExtensionOption with an optional boolean argument require_all, defaulting to False, and when True, have the option check that all specified components actually exist. This is probably going to require a few changes to ExtensionPoint as well.

comment:9 Changed 20 months ago by Ethan Jucovy <ethan.jucovy@…>

  • Cc ethan.jucovy@… added

comment:10 follow-up: Changed 18 months ago by fd766@…

Is there a work around for this problem? I disabled then re-enabled the plugin, now it doesn't work and client is not happy to say the least.

comment:11 in reply to: ↑ 10 Changed 18 months ago by Ryan J Ollos <ryan.j.ollos@…>

Replying to fd766@…:

Is there a work around for this problem? I disabled then re-enabled the plugin, now it doesn't work and client is not happy to say the least.

I can't make sense of your question in the context of this thread. Please post your question to the MailingList.

comment:12 Changed 17 months ago by shoffmann

  • Cc shoffmann added

Changed 17 months ago by Steven R. Loomis <srl@…>

Changed 8 months ago by rjollos

comment:13 Changed 8 months ago by rjollos

  • Cc ryano@… removed

In #11272, I've stumbled into an implementation of what Rémy suggested in comment:2, modifying OrderedExtensionsOption to raise a ConfigurationError if it can't find the named implementation of the interface. Since ExtensionOption raises an AttributeError when the specified implementation of the interface can't be found, I couldn't see a reason that OrderedExtensionOption should behave differently. However, if we want to preserve the existing behavior for the sake of maintaining consistent behavior of the API, then adding a require parameter seems like a good solution.

In Trac, OrderedExtensionOption is used for: [ticket-workflow] action_controllers, [trac] permission_policies and [trac] request_filters. My starting point for #11272 was trying to catch errors in the configuration in order to make it easier for users to work with AuthzPermissionPolicy, and for the workflow and request_filters options as well I can't see a reason why Trac shouldn't raise an error if one of the extensions isn't found. Silently failing to apply the extension just seems likely to make typos in the option value more difficult to debug, and lead to situations like the one discussed in this ticket where Trac may not be applying the permission policy, request filter or workflow controller that the user is expecting.

The result per all of the testing I've done done is that if the component providing named implementation of the interface is not found, everyone is effectively locked out of Trac through the Web interface:

I'm not entirely confident of the changes, so I appreciate any review and testing that others can provide, as well as challenges to the assumptions I've made about how Trac should behave. The changes can be found in rjollos.git:t11272.

Last edited 7 months ago by rjollos (previous) (diff)

comment:14 Changed 8 months ago by Mitar

Thanks. I think that there is no real existing behavior which we want to maintain. If some Trac installation has an invalid entry somewhere in OrderedExtensionOption, they will find very fast and if they didn't notice until now, they can then simply remove the option and Trac will work as it was working before the update.

comment:15 Changed 8 months ago by rjollos

While working on #11244, I had the thought that we might want to list the implementations of the interface in the error dialog to help the user in resolving the error. For example,

Cannot find implementations(s) … The enabled implementations of "IPermissionPolicy" are "ReadOnlyWikiPolicy", "LegacyAttachmentPolicy", "PrivateWikiPolicy", "DefaultPermissionPolicy".

comment:16 Changed 8 months ago by Mitar

Please don't leak internal configuration to a end-user. This can go to the error log.

comment:17 Changed 8 months ago by rjollos

I'll be committing the other changes for #11272 with the exception of [db6a244b/rjollos.git], and will return to continue working on the ticket eventually.

comment:18 Changed 7 months ago by rjollos

  • Milestone changed from next-minor-0.12.x to 1.0.2
  • Owner set to rjollos
  • Status changed from new to assigned

The OrderedExtensionsOption is not a Component, so it doesn't have an instance of the logger or the Environment object. Most likely, this is why the ExtensionOption class took the approach of raising an AttributeError that has internal configuration data in the message: tags/trac-1.0.1/trac/config.py@:693-697#L679.

I'm considering some ways to work around this. If we added a log_messsage attribute to the TracError class, the log_message could be added where the exception is raised, and the message could be logged at the places where the exception is trapped in RequestDispatcher.dispatch, such as tags/trac-1.0.1/trac/web/main.py@:256#L251.

comment:19 Changed 7 months ago by jomae

  • Cc jomae added

The changes seem good to me except the following.

  • I think ExtensionOption.__get__ should consistently raise ConfigurationError instead of AttributeError in the same manner with ChoiceOption and OrderedExtensionsOption if settings are wrong.
  • The message of a exception from ExtensionOption and OrderedExtensionsOption should be translated.

comment:20 follow-up: Changed 7 months ago by rjollos

Changes have been prepared in rjollos.git:t10285, incorporating suggestions from comment:19. If [f8a7f571/jomae.git] from #11284 is committed before this change, then the implementation of assertIsInstance can be removed.

I'm more convinced now that leaking of internal information is a general problem with the config module, so unless there are ideas on how to fix that now, I'll raise a separate ticket for dealing with that issue latter on, possibly pursuing the idea from comment:18. While it is true that OrderedExtensionsOption now shows configuration data to the end users when an exception is raised, this is something that ExtensionOption and other classes are already doing.

comment:21 in reply to: ↑ 20 Changed 7 months ago by rjollos

Replying to rjollos:

If [f8a7f571/jomae.git] from #11284 is committed before this change, then the implementation of assertIsInstance can be removed.

Or, I suppose I can just use self.assertTrue(isinstance(...) like in [12054].

comment:22 Changed 7 months ago by rjollos

I must not have run all of the tests previously, because I found just now that several tests were failing with errors such as:

======================================================================
ERROR: test_require (trac.tests.perm.PermissionCacheTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/user/Workspace/t10285/teo-rjollos.git/trac/tests/perm.py", line 183, in test_require
    self.perm.require('TEST_MODIFY')
  File "/home/user/Workspace/t10285/teo-rjollos.git/trac/perm.py", line 578, in require
    if not self._has_permission(action, resource):
  File "/home/user/Workspace/t10285/teo-rjollos.git/trac/perm.py", line 570, in _has_permission
    check_permission(action, perm.username, resource, perm)
  File "/home/user/Workspace/t10285/teo-rjollos.git/trac/perm.py", line 460, in check_permission
    for policy in self.policies:
  File "/home/user/Workspace/t10285/teo-rjollos.git/trac/config.py", line 771, in __get__
    section=self.section, name=self.name))
ConfigurationError: Cannot find implementation(s) of the "IPermissionPolicy" interface named "LegacyAttachmentPolicy".  Please update the option trac.permission_policies in trac.ini.

I've fixed the issues by enabling the components and setting trac.permissions_policies where necessary.

The latest changes can be found in rjollos.git:t10285.2, including the change described in comment:21.

comment:23 Changed 7 months ago by psuter

  • Keywords exception added

comment:24 Changed 7 months ago by rjollos

  • API Changes modified (diff)
  • Release Notes modified (diff)
  • Resolution set to fixed
  • Status changed from assigned to closed

Committed to 1.0-stable in [12066:12068] and merged to trunk in [12069].

comment:25 Changed 7 months ago by Mitar

Great! Thanks!

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed The owner will remain rjollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from rjollos to the specified user.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.