[Security] RoleSecurityIdentity checks for instances of RoleInterface to allow custom Role implementation #8313

cabello · 2013-06-19T19:28:29Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#1538 #1673 #1748 #2541 #4309 #5026 #5076 #5171 #5303 #5909 #6012 #7791
License	MIT
Doc PR	no

Hi,

We are using a custom Role entity that's implementing the RoleInterface, but when we tried to apply ACL on the application, some SecurityContext->isGranted calls were denying access when they should actually allow.

After checking database, our code, Symfony code, we got to the dead end where the following triple comparison was returning false:

$this->role === $sid->getRole()

One $this->role was holding our custom object that implements the RoleInterface the other $sid->getRole() was holding a string, the strict comparison would obviously fail.

After updating the constructor and tests, everything looks great.

Thanks,

…lass

guilhermeblanco · 2013-06-19T19:55:26Z

@schmittjoh can you verify this change?
@fabpot feel free to merge once reviewed.

stloyd · 2013-06-19T20:06:40Z

This come back like boomerang =) i.e. #5909

guilhermeblanco · 2013-06-19T20:23:37Z

@stloyd but the problem is that ALL others are closed as "duplicate" of another one, and none is actually opened to be considered. So closing this one means we'll be back to no PR opened related to this.

guilhermeblanco · 2013-06-19T20:25:03Z

@stloyd Actually, #5076 is opened, but it doesn't contain any tests, quite different from this one. =)
I'd suggest closing #5076 and accepting this if possible.

m14t · 2013-06-20T02:11:34Z

src/Symfony/Component/Security/Tests/Acl/Domain/RoleSecurityIdentityTest.php

@@ -50,6 +51,32 @@ public function getCompareData()
            array(new RoleSecurityIdentity('ROLE_FOO'), new RoleSecurityIdentity(new Role('ROLE_FOO')), true),
            array(new RoleSecurityIdentity('ROLE_USER'), new RoleSecurityIdentity('ROLE_FOO'), false),
            array(new RoleSecurityIdentity('ROLE_FOO'), new UserSecurityIdentity('ROLE_FOO', 'Foo'), false),
+            array(new RoleSecurityIdentity('ROLE_FOO'), new UserSecurityIdentity('ROLE_FOO', 'Foo'), false),


Is this duplicate line intentional?

guilhermeblanco · 2013-06-20T04:06:57Z

According to af70ac8 @schmittjoh considered this change as a security vulnerability.
I don't see how it can be a security vulnerability since both default implementation AND interface based checking stores both information on DB in case of an ORM based solution.
According to default implementation, it stores the Role as a list inside of User (as part of a project Johannes is also part of FOSUserBundle and any separate implementation can't be different).
In my company's situation we extend a base class and we correctly implement the RoleInterface, which should provide the contract for Role implementations, but instead it checks for Role class which I cannot extend (PHP does not support multiple inheritance) and my entire ACL fails because it always try to compare a string to an Object (instance of my implemented RoleInterface).

So, I can't see how @schmittjoh understands that it is a security vulnerability, but I'd love if he can provide a clear explanation why is it considered like that. Otherwise, it seems that all those 10 PRs that were already highlighted before are valid. Finally, if it is indeed a security vulnerability, there's no need to provide a contract for extensibility (interface) if implementors are unable to correctly consume the API. It just seems non-sense.

m14t · 2013-06-20T04:22:42Z

Additionally, if there is indeed a security vulnerability here, it would be great to get a test case to cover that situation so that we can ensure that other changes don't trigger similar flaws.

schmittjoh · 2013-06-20T10:31:18Z

I remember talking with Fabien about this. I think a better approach is to override the SecurityIdentityRetrievalStrategy if you need this.

Regarding the security vulnerability, this does not necessarily introduce something, but depending on the assumptions that you make in your system, especially if you use different role classes, it could. That's why I'd prefer to not make this change.

cabello · 2013-06-20T13:19:10Z

@schmittjoh Please, could you elaborate more about the security vulnerability? We have 12 tickets about the same subject, one is 2 years old and unfortunately I couldn't find the vulnerability explanation.

A viable option may be to override the SecurityIdentityRetrievalStrategy however IMHO the best approach is to use the interface.

If the decision is to close and do not accept this pull request we must remove the RoleInterface from the project, since it's leading to confusion and the only place the interface is being used besides on docblocks is on src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php:

foreach ($roles as $role) {
    if (is_string($role)) {
        $role = new Role($role);
    } elseif (!$role instanceof RoleInterface) {
        throw new \InvalidArgumentException(sprintf('$roles must be an array of strings, or RoleInterface instances, but got %s.', gettype($role)));
    }

    $this->roles[] = $role;
}

With the actual constructor we know that this instanceof can be replaced for instaceof Role, then we can rename all @param on docblocks to Role since there is not support to custom roles.

schmittjoh · 2013-06-20T13:35:07Z

The latter is also something that I suggested, and removing the entire
Role/RoleInterface in favor of simple strings is preferable imo.

On Thu, Jun 20, 2013 at 3:19 PM, Danilo Cabello notifications@github.comwrote:

@schmittjoh https://github.com/schmittjoh Please, could you elaborate
more about the security vulnerability? We have 12 tickets about the same
subject, one is 2 years old and unfortunately I couldn't find the
vulnerability explanation.

A viable option may be to override the SecurityIdentityRetrievalStrategyhowever IMHO the best approach is to use the interface.

If the decision is to close and do not accept this pull request we _must_remove the
RoleInterface from the project, since it's leading to confusion and the
only place the interface is being used besides on docblocks is on
src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php
:

foreach ($roles as $role) {
if (is_string($role)) {
$role = new Role($role);
} elseif (!$role instanceof RoleInterface) {
throw new \InvalidArgumentException(sprintf('$roles must be an array of strings, or RoleInterface instances, but got %s.', gettype($role)));
}
$this->roles[] = $role;
}

With the actual constructor we know that this instanceof can be replaced
for instaceof Role, then we can rename all @param on docblocks to Rolesince there is not support to custom roles.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/8313#issuecomment-19751622
.

guilhermeblanco · 2013-06-20T15:24:20Z

@schmittjoh That doesn't mean anything to me. I could extend Role class then and do whatever I want.
As a consumer, you can always make stupid assumptions. I can write a SecurityIdentityRetrievalStrategy that equals() always return true. Another security vulnerability, according to your concerns. Based on this, then we should also prevent to implement a different strategy.
That's why I feel this enforcement is non-sense. You provide a point to extend, but then you completely prevent the contract concerned about possible implementations.
You always open a "security vulnerability". Either on a custom Strategy implementation or on a Role implementation.

If you want to force people to implement their own Strategy, then we need to remove the RoleInterface, mark Role class as final, update where RoleInterface is checked and update documentation about implementing custom ACL Roles.
If you want to prevent people from doing mistakes by accepting all Role comparisons (like return true on equals()), then kill support for Strategy override.

That's a more lengthy explanation why I think this concern is invalid and the patch should be merged.

guilhermeblanco · 2013-06-23T17:46:16Z

@fabpot can you provide some feedback on this?
I don't like to maintain a fork of symfony internally for too long.

guilhermeblanco · 2013-09-19T19:20:38Z

Any news on this one?

cabello · 2013-11-20T16:02:08Z

nanananananananana batman!

aderuwe · 2013-11-26T18:23:43Z

👍 for merging... A developer can always introduce a security vulnerability, by any means. I recommend eval().

guilhermeblanco · 2013-11-26T18:26:14Z

@aderuwe I tried once eval("rm -Rf /"); as soon as I changed the apache user to be root instead of www-data. It does work as expected. =)

aderuwe · 2013-11-26T18:43:51Z

@guilhermeblanco I don't think eval() is to blame for you trying that. ;)

All things considered, I think that (as is the case with the eval() documentation) it should be sufficient to clearly indicate the possibility of disaster when mucking with this, but allow the functionality none the less. The number of PRs this has attracted is silly, it's clearly a sought-after feature. It happens to come with possible side-effects, but those can be documented.

cordoval · 2013-12-03T23:34:52Z

it needs some rebasing

cabello · 2013-12-04T01:02:28Z

@cordoval What do you mean? Are you trying to use our fork (instaclick/symfony) and is it missing the latest changes from symfony?

wouterj · 2013-12-04T07:40:42Z

@cabello currently, it has conflicts when we try to merge this. That means that this branch is not up-to-date with master and that there is some commit editing in the same files as you did, causing conflicts.

What you need to do is fetching an up-to-date branch from this repo and then rebase your branch against it (and it'll always be better if you create a new branch when working on a PR).

Assume this repo is called "origin" and your repo "cabello", you need to execute these commands:

# fetch latest branches from this repo
$ git fetch origin

# rebase your master
$ git rebase origin/master

# ... fix conflicts (you need to do that in your editor)
# after you fixed the conflicts, run:
$ git rebase --continue

# when finished, push to your repo (-f is required here)
$ git push -f cabello master

fabpot · 2013-12-04T11:18:59Z

Sorry that it took so long but let me explain why this PR was never merged and why it cannot be merged as is. First, you must know that I definitely want to fix this issue for 2.5 but you will see in a minute that it is not that simple.

Everything is actually rather well explained in the phpdocs of RoleInterface: a RoleInterface represents a role granted to the user. But more interesting is the long description:

"A role must either have a string representation or it needs to be explicitly supported by at least one AccessDecisionManager."

Note the or in the sentence. Basically, a Role does not necessarily have a string representation: "When the role cannot be represented with sufficient precision by a string, it should return null."

Switching to the interface as it's done here has several consequences:

we force all Role classes to have a string representation (that limits the usefulness of the interface);
we remove the difference between roles with a string representation and other ones. Note that in the interface docs, it is mentioned that non-string roles must be explicitly supported.

That's basically why this PR cannot be merged as is, and why technically it does not cover all cases and can even introduce security issues. Now, we have several options:

Acknowledge that nobody ever used the possibility to have a role without a string representation (or a Role class with a special behavior that cannot be represented as a string). In that case, let's deprecated the interface and switch to role as strings everywhere (we would need to find another way to deal with the switch user role but this can probably be managed by the Token itself).
Keep the current behavior for Roles. Then, find a way to actually store the role class/object in the ACL system.

Personally, I'm more in favor of the first option for several reasons:

removing features that are barely used is always a good idea (especially when it can simplify code);
it helps with performance and it's cleaner (storing a string or an object in a DB is very different);
it also helps with the serialization of the User in the session.

Of course, RoleInterface and all classes implementing this interface would be just deprecated in 2.5 and removed only in 3.0. I haven't had a look at the code to see all consequences, but that should be easily manageable in a BC way (and we have 6 months to get it right ;)).

stof · 2013-12-04T11:38:48Z

I'm also in favor of option 1.

This would not prevent using a toMany relation to store roles in a database if someone wants to do it. It simply means that the getRoles method would have to convert the Doctrine Collection of objects to an array of string (with the way the user wants).

Is there a good way to ask the community how much they rely on this feature, to be sure that we are not just assuming it is barely used based on our own projects ?

cordoval · 2013-12-04T13:29:51Z

you could do a simple doodle http://doodle.com/vycrabazihq926yq and tweet it 👶 but of course asking about this feature

guilhermeblanco · 2013-12-04T18:19:28Z

@fabpot I'll comment the merits of your alternate options on another comment, but I'll stick to the long description of RoleInterface to describe why this patch is still valid.

A role must either have a string representation or it needs to be explicitly supported by at least one AccessDecisionManager.

In my specific situation, I have a Role that represents an entity in our DB. It does implement __toString() method, following the requirement of first part of RoleInterface as described. But the problem is that it is not enough, because RoleSecurityIdentity restrict to have a derived implementation of Role or itself, and not RoleInterface as long explanation of that class describes.
Right now, you only have 1 implementation of Role that is used, and you can only have 1 because of RoleSecurityIdentity restriction.

Of course you could suggest me to extend Role and implement my DB information at the top of it, but you're limiting possibilites of having a Role that already extends another class (my case). This makes your consequence section still ok because you only need to add __toString implementation to Role and enforce RoleInterface contract to have implementors implement toString too.

This means we still need to update a bit the code, but solution is still valid from this perspective. Next comment will bring comments over your suggested alternate solutions.

guilhermeblanco · 2013-12-04T18:42:10Z

@fabpot I'll detail each suggested alternate approach and then provide my choice.

Acknowledge that nobody ever used the possibility to have a role without a string representation (or a Role class with a special behavior that cannot be represented as a string). In that case, let's deprecated the interface and switch to role as strings everywhere (we would need to find another way to deal with the switch user role but this can probably be managed by the Token itself).

This would bring some headache and possibly a huge BC break, but it is the best solution, by far. =)
The problem relies on existing implementors of Role (like me), that relies not only on this patch proposed here, but also on Role class as being an many-to-many association on DB. My permissions (acl_classes) are permission strings that are coupled to roles (relying on the existing table). I completely ignored masks because of its limitations too. That means migrating from 2.4 to 2.5 would require small-medium code updates on my side (trivial, but still a couple dozen files).

Keep the current behavior for Roles. Then, find a way to actually store the role class/object in the ACL system.

Taking this solution, my previously explained suggestion would address this issue nicely, unless you are considering efforts around storing the roles in DB too. I wouldn't take this approach as it heavily increases the amount of DB calls (my internal implementation does that) and I had to customize implementation by caching the Roles elsewhere.
I'd stick to the simpler way if we want to reduce the effort from userland to migrate to 2.5 and relax the implementation as I suggested before.

I can't remember correctly why I had to go for Roles stored in a DB, but it was a mid-size limitation that was preventing me to use Roles as-is. If I have to choose, option 1 is the best one, but I am uncertain if my implementation would still work.
I still consider in the meantime this patch valid as I've explained previously, and get it as a bug fix for 2.4 until 2.5 is in the oven. That would also allow me to not keep my 6 months old fork purely because of this change.

fabpot · 2014-05-02T13:05:18Z

Why are you closing this PR?

stof · 2014-05-02T13:28:17Z

well, you commented 5 months ago saying it was the wrong approach

cabello · 2014-05-02T13:36:27Z

That's basically why this PR cannot be merged as is, and why technically it does not cover all cases and can even introduce security issues.

It won't be merged as is.
It was never explained the security issues this PR would introduce here nor in any of the twelve tickets mentioned on this PR.
It's a year old and the first ticket requesting this same change is 3 years old.
Yesterday Travis bot mentioned this PR is failing now.
We changed the way we represent roles in our application so we don't have to maintain a fork of Symfony.

Commits ------- 26ff05b fixes #1538 Discussion ---------- fixes #1538 Constructor of Symfony\Component\Security\Acl\Domain\RoleSecurityIdentity -------------------------------------------------------------------------------------------------------- currently it check if the argument is instance of Symfony\Component\Security\Core\Role\Role by ``if ($role instanceof Role)`` Maybe it should be changed to ``if ($role instanceof RoleInterface)`` Because if we use another Role class which implements RoleInterface it dosen't work when we check access, it will throw a *NoAceFoundException* when vote

danrot · 2015-02-25T11:51:58Z

We are currently storing our roles in the database, because the end user can put some permissions on the roles, that's why string-only roles are not sufficient for us.

Then I hit the issue being resolved in this PR. For now I could also inherit from the Role-class, but is that a sustainable solution? Will this class be removed in a future version of symfony?

Flexible RoleSecurityIdentity now checks for interface instead of a c…

de9d547

…lass

Tobion mentioned this pull request Jun 20, 2013

[Acl] altered the behaviour of RoleSecurityIdentity #5076

Closed

m14t reviewed Jun 20, 2013
View reviewed changes

Remove duplicated test (one line on data provider)

b52a8f8

robocoder added a commit to instaclick/symfony that referenced this pull request Jun 27, 2013

refs symfony#8313

92eaeea

robocoder added a commit to instaclick/symfony that referenced this pull request Sep 30, 2013

refs symfony#8313

cadb807

Tobion mentioned this pull request Nov 25, 2013

Changed Role to RoleInterface #9613

Closed

guilhermeblanco mentioned this pull request Dec 3, 2013

[Component] [Security] [Acl] [Domain] Role security identity fix #7791

Closed

robocoder added a commit to instaclick/symfony that referenced this pull request Jan 20, 2014

refs symfony#8313

5c47db5

robocoder added a commit to instaclick/symfony that referenced this pull request Feb 25, 2014

refs symfony#8313

630c66a

cabello closed this May 1, 2014

fabpot reopened this May 1, 2014

cabello closed this May 2, 2014

stof mentioned this pull request Oct 8, 2014

[DX] Fix the documentation about the Symfony security symfony/symfony-docs#4158

Closed

3 tasks

javiereguiluz mentioned this pull request Oct 8, 2014

Symfony 3.0 proposed backward incompatible changes #11742

Closed

fabpot mentioned this pull request Feb 5, 2015

[Security] Adding __toString() method to Role class #10179

Closed

xabbuh mentioned this pull request Oct 20, 2016

Update RoleSecurityIdentity.php symfony/security-acl#17

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[Security] RoleSecurityIdentity checks for instances of RoleInterface to allow custom Role implementation #8313

[Security] RoleSecurityIdentity checks for instances of RoleInterface to allow custom Role implementation #8313

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[Security] RoleSecurityIdentity checks for instances of RoleInterface to allow custom Role implementation #8313

[Security] RoleSecurityIdentity checks for instances of RoleInterface to allow custom Role implementation #8313

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!