Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fixed #3454. #3479

Closed
wants to merge 4 commits into from

3 participants

Jacob Kiers Matthew Weier O'Phinney Maks3w
Jacob Kiers

As stated in the my comment in #3454, access to a resource is denied with a global deny policy and a permission on a parent.

This PR just adds a test to show the behaviour. I might be able to write a fix, but I'm not yet sure of that. The ACL code is quite complex :smile:

jacobkiers added some commits
Jacob Kiers jacobkiers Added test for #3454. 07e4245
Jacob Kiers jacobkiers Fix #3454.
The fix involves forcing a loop through the parent resources when the
current resource is 'TYPE_DENIED'. The loop stops when we are already at
the top-level resource.
3c88e7b
Jacob Kiers

I've just updated this PR with a fix.

Jacob Kiers jacobkiers Ensure privilege escalation is not possible.
With the previous version, a deny() on a lower-lever resource was not
inherited when on a higher-lever resource an allow() was given.

See the updated test for more an example.
58352d1
Jacob Kiers

I found that some privilege escaliation is possible when inheritance does not work corectly. I will check somewhere tonight if this is due to my previous changes, or that it also happens in the master branch.

For now, could someone please review these changes? Thanks!

Maks3w

$return will only have the last $child

Jacob Kiers

@Maks3w I've tested this on the master branch (just the tests), and found that the privilege escalation issue is there too.

What do you propose? Backporting this one or sending in a new PR with just the changes needed to fix the security issue?

Matthew Weier O'Phinney

@jacobkiers I'd suggest backporting the fix for the escalation issue to master via another PR. We can merge both at the same time.

Matthew Weier O'Phinney

@jacobkiers Actually, if you don't mind, I'll simply cherry-pick it into master. This will rewrite history, so you'll need to do a 'reset --hard upstream/master` (as well as upstream/develop) after merged.

Matthew Weier O'Phinney weierophinney closed this pull request from a commit
Matthew Weier O'Phinney weierophinney Merge branch 'hotfix/3479'
Close #3479
Fixes #3454
c0e2d53
Jacob Kiers

@weierophinney I don't mind at all. That means these fixes will make it into 2.0.7, right? That would make me really happy :smile:

Jacob Kiers jacobkiers deleted the branch
Matthew Weier O'Phinney
Deleted user Unknown referenced this pull request from a commit
Matthew Weier O'Phinney weierophinney Merge branch 'hotfix/3479'
Close #3479
Fixes #3454
0217b59
Deleted user Unknown referenced this pull request from a commit
Matthew Weier O'Phinney weierophinney Merge branch 'hotfix/3479' into develop
Forward port #3479
2141c93
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 18, 2013
  1. Jacob Kiers

    Added test for #3454.

    jacobkiers authored
  2. Jacob Kiers

    Fix #3454.

    jacobkiers authored
    The fix involves forcing a loop through the parent resources when the
    current resource is 'TYPE_DENIED'. The loop stops when we are already at
    the top-level resource.
  3. Jacob Kiers

    Ensure privilege escalation is not possible.

    jacobkiers authored
    With the previous version, a deny() on a lower-lever resource was not
    inherited when on a higher-lever resource an allow() was given.
    
    See the updated test for more an example.
  4. Jacob Kiers
This page is out of date. Refresh to see the latest.
31 library/Zend/Permissions/Acl/Acl.php
View
@@ -572,7 +572,9 @@ public function setRule($operation, $type, $roles = null, $resources = null,
$resources = array();
foreach ($resourcesTemp as $resource) {
if (null !== $resource) {
- $resources[] = $this->getResource($resource);
+ $children = $this->getChildResources($this->getResource($resource));
+ $resources = array_merge($resources, $children);
+ $resources[$resource] = $this->getResource($resource);
} else {
$resources[] = null;
}
@@ -660,6 +662,28 @@ public function setRule($operation, $type, $roles = null, $resources = null,
}
/**
+ * Returns all child resources from the given resource.
+ *
+ * @param Resource\ResourceInterface|string $resource
+ * @return Resource\ResourceInterface[]
+ */
+ protected function getChildResources(Resource\ResourceInterface $resource)
+ {
+ $return = array();
+ $id = $resource->getResourceId();
+
+ $children = $this->resources[$id]['children'];
+ foreach($children as $child) {
+ $child_return = $this->getChildResources($child);
+ $child_return[$child->getResourceId()] = $child;
+
+ $return = array_merge($return, $child_return);
+ }
+
+ return $return;
+ }
+
+ /**
* Returns true if and only if the Role has access to the Resource
*
* The $role and $resource parameters may be references to, or the string identifiers for,
@@ -747,7 +771,10 @@ public function isAllowed($role = null, $resource = null, $privilege = null)
if (null !== ($ruleType = $this->getRuleType($resource, null, $privilege))) {
return self::TYPE_ALLOW === $ruleType;
} elseif (null !== ($ruleTypeAllPrivileges = $this->getRuleType($resource, null, null))) {
- return self::TYPE_ALLOW === $ruleTypeAllPrivileges;
+ $result = self::TYPE_ALLOW === $ruleTypeAllPrivileges;
+ if ($result || null === $resource) {
+ return $result;
+ }
}
// try next Resource
25 tests/ZendTest/Permissions/Acl/AclTest.php
View
@@ -1303,4 +1303,29 @@ public function testRemoveDenyWithNullResourceAppliesToAllResources()
$this->assertFalse($this->_acl->isAllowed('guest', 'newsletter', 'read'));
}
+ /**
+ * @group ZF2-3454
+ */
+ public function testAclResourcePermissionsAreInheritedWithMultilevelResourcesAndDenyPolicy()
+ {
+ $this->_acl->addRole('guest');
+ $this->_acl->addResource('blogposts');
+ $this->_acl->addResource('feature', 'blogposts');
+ $this->_acl->addResource('post_1', 'feature');
+ $this->_acl->addResource('post_2', 'feature');
+
+ // Allow a guest to read feature posts and
+ // comment on everything except feature posts.
+ $this->_acl->deny();
+ $this->_acl->allow('guest', 'feature', 'read');
+ $this->_acl->allow('guest', null, 'comment');
+ $this->_acl->deny('guest', 'feature', 'comment');
+
+ $this->assertFalse($this->_acl->isAllowed('guest', 'feature', 'write'));
+ $this->assertTrue($this->_acl->isAllowed('guest', 'post_1', 'read'));
+ $this->assertTrue($this->_acl->isAllowed('guest', 'post_2', 'read'));
+
+ $this->assertFalse($this->_acl->isAllowed('guest', 'post_1', 'comment'));
+ $this->assertFalse($this->_acl->isAllowed('guest', 'post_2', 'comment'));
+ }
}
Something went wrong with that request. Please try again.