Edgewall Software
Modify

Opened 3 years ago

Closed 3 years ago

#12597 closed enhancement (fixed)

Prevent recursion in PermissionSystem.check_permission

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.2.1
Component: general Version:
Severity: normal Keywords: permissions
Cc: Branch:
Release Notes:

Fixed recursion in permission policy checks.

API Changes:

Description

Discussed in gdiscussion:trac-users:MNvfBPbiAS0 and other threads, there is a high likelihood of recursion when a permission policy calls Permission.has_permission (usually via ACTION in perm) within its policy.check_permission method. One possible way to avoid this, which I'd like to explore, would be to skip calling policy.check_permission on the policy that invoked PermissionCache.has_permission.

Using CookBook/Configuration/SignedTickets as an example, the policy would be changed:

-                any(a in perm for a in self.admin_actions):
+                any(a in perm(resource, policy=self)
+                    for a in self.admin_actions):

Attachments (0)

Change History (6)

comment:1 by Ryan J Ollos, 3 years ago

The following changes may need additional work, but with a small amount of testing I can say they appear to fix the issue raised on the mailing list: log:rjollos.git:t12597_perm_policy_recursion.

comment:2 by Peter Suter, 3 years ago

I find it quite difficult to think about the implications here. Does this ever unintentionally skip an important permission check?

Might a conservative alternative be to "pre-cache" a negative decision? (untested)

  • trac/perm.py

    diff -r 23adc5e36b28 trac/perm.py
    a b  
    588588            cache_decision, cache_resource = cached
    589589            if resource == cache_resource:
    590590                return cache_decision
     591        # In case of recursive check, disallow permission
     592        self._cache[key] = (False, resource)
    591593        perm = self
    592594        if resource is not self._resource:
    593595            perm = PermissionCache(self.env, self.username, resource,

comment:3 by Ryan J Ollos, 3 years ago

Milestone: 1.0.141.2.1

I'll investigate for 1.2.1. next-stable-1.0.x should be (or soon be) for major defects only.

in reply to:  2 comment:4 by Ryan J Ollos, 3 years ago

Replying to Peter Suter:

Might a conservative alternative be to "pre-cache" a negative decision? (untested)

That appears to be a simple way to solve the issue: rjollos.git:t12597_perm_policy_recursion.1.

With the change, ReadonlySignedTickets policy works with the following change:

  • ReadonlySignedTickets.py

    old new  
    1515        if resource is None or resource.realm != 'ticket' or \
    1616                resource.id is None or \
    1717                action in self.allowed_actions or \
    18                 action in ('TICKET_ADMIN', 'TRAC_ADMIN') or \
    1918                'TICKET_ADMIN' in perm:
    2019            return None
Last edited 3 years ago by Ryan J Ollos (previous) (diff)

comment:5 by Ryan J Ollos, 3 years ago

Additional change in [4831f0f3/rjollos.git].

if resource ... in has_permission didn't seem to be behaving as intended.

  1. if resource would always evaluate to True. I suppose we could implement __nonzero__ / __bool__ on Resource so that Resource(None) would evaluate to False, but unsure whether that's generally useful.
    >>> from trac.resource import Resource
    >>> r = Resource(None)
    >>> bool(r)
    True
    
  2. ACTION in perm should behave the same as ACTION in perm(None), however before the change the cached key would be different:
    >>> from trac.perm import PermissionCache
    >>> from trac.test import EnvironmentStub
    >>> env = EnvironmentStub()
    >>> perm1 = PermissionCache(env)
    >>> perm2 = PermissionCache(env)
    >>> 'PERM' in perm1
    False
    >>> perm1._cache
    {('anonymous', -9223372036584357655, 'PERM'): (False, None)}
    >>> 'PERM' in perm2(None)
    False
    >>> perm2._cache
    {('anonymous', 7976857064515516242, 'PERM'): (False, <Resource ''>)}
    
    After the change:
    >>> from trac.perm import PermissionCache
    >>> from trac.test import EnvironmentStub
    >>> env = EnvironmentStub()
    >>> perm1 = PermissionCache(env)
    >>> perm2 = PermissionCache(env)
    >>> 'PERM' in perm1
    False
    >>> perm1._cache
    {('anonymous', -9223372036583836439, 'PERM'): (False, None)}
    >>> 'PERM' in perm2(None)
    False
    >>> perm2._cache
    {('anonymous', -9223372036583836439, 'PERM'): (False, None)}
    
    Added test case in [2128800/rjollos.git].
  3. Still considering, but implementing __nonzero__ behavior proposed in (1), ACTION in perm(Resource(None)) could be made to behave the same as the two cases discussed in (2): [83ada83e/rjollos.git].

comment:6 by Ryan J Ollos, 3 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Most changes committed in r15340, merged in r15341. I didn't commit [83ada83e/rjollos.git].

Modify Ticket

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