Easy privilege escalation from user preferences

If it is like described (I could not verify it with Mgnl 3.6.1), this issue is a blocker for 3.6.2.
If the fix is a problem and would delay 3.6.2 too much you could also disable this new feature for 3.6.2 and add a correct and secure solution to 3.6.3.
Security definitely is more important than new features.

This is a leftover from MAGNOLIA-574 : since the task was closed ignoring my comments

could you please leave self pity out? No one ignored your comments.

Just create a user with a editor role and readonly access to userroles: he can just type "/superuser" in its preference dialog to gain full access.

But this is exactly what you are telling system to do: if you add user read rights to the userroles, you tell system that such user is allowed to read (and reference) the roles and of course the system allows such user to read the roles and therefore assign them to themselves or to anyone else for that matter.
If user needs to read a role, then such user needs rights to read only role assigned (and should get such rights together with the role assigned), but as I can see it there is no reason why you should grant any user flat unrestricted access to userroles.

Could you describe the scenario in which assigning read access to all roles happens with default installation of Magnolia or in which such setting could not be avoided?

Please note also that in previous versions, this was possible since there was no checking and only prevention measure was hiding the dialog from the users. To assess rights to assign roles based on the access rights to userroles assigned to given user is IMHO the right thing to do instead of hiding the dialog.

> But this is exactly what you are telling system to do: if you add user read rights to the userroles, you tell system that such user is allowed to read (and reference) the roles

if I add rights to read roles it doesn't mean I am adding the permission to modify users, isn't it?
I don't think that granting each user a write permission to its own acls is something desired...

> To assess rights to assign roles based on the access rights to userroles assigned to given user is IMHO the right thing to do instead of hiding the dialog.

sure, I wasn't absolutely suggestion to hide the dialog in order to implement security: security should implemented at a repo level by forbidding any users from modifying its own acls. The group and role form field should be removed because it makes no sense they stay in a user preference dialog which is expected to be used to edit the currently logged in user.

> could you please leave self pity out? No one ignored your comments

well, I am pretty tired of getting harsh responses or seeing that most decisions are taken individually and not open to discussions... anyway: this is not the place to discuss this, sorry for the bad introduction

if I add rights to read roles it doesn't mean I am adding the permission to modify users, isn't it?

True, but each user always had and still has rights to modify their own preferences.

I don't think that granting each user a write permission to its own acls is something desired...

See my comment above, each user always had the permission to modify child nodes of themselves. If you check the core update task, we are adding just read permission to the node itself (so they can see their own user name for example) nothing more then that. The only reason why this worked before is because displaying the dialog was done with system user privileges, and we removed this now.

well, I am pretty tired of getting harsh responses or seeing that most decisions are taken individually and not open to discussions... anyway: this is not the place to discuss this, sorry for the bad introduction

Agreed, I haven't tried to give harsh response here either, so sorry if it came out as one. It might be more useful to get such discussions going in the dev list or as Philipp mentioned while ago, we have scrum meeting every day at 2:30 at irc://irc.freenode.net/magnolia-dev and hang around there for most of the day. It would be even better place to talk to avoid future misunderstanding and to see (and to influence) day to day development.

Now lets try to have a fresh start. I'll try to explain how it works now and why and we can take it from here.
prior 3.6.2:

• the security have been broken since the only protection from user assigning themselves more rights was the (implicitly) hidden dialog.
• to bring up that dialog when you wanted user to be able to change privileges, you had to do through extensive acl configuration to bring up security/admin/username menu for given user.

from 3.6.2 onwards:

• the dialog is exposed (this is new) so users can change their own privileges (by default, all except anonymous) (this right was always here).
• the above is configurable and where desirable, users can be taken away such privilege (by making their own node read only) and they will not be able to edit their own preferences any more (this haven't worked before 3.6.2 since the dialog involved used system privileges in case of groups and roles)
• the display and assignment of the groups and roles is controlled by users current rights in respect to roles and groups
• if user has assigned given role/group user gets read right to this role/group via assignment.
• user can (un)assign only roles/groups he/she has rights to read
• assigning user explicitly read to whole userroles workspace is equivalent to assigning that user superuser role in respect to granting roles hence such user can assign any role (even to themselves). If you need user to be able to assign roles, but do not want such user to be able to assign privileges to themselves, you have to explicitly set acl_userroles of that given user to read only for that given user (again this is same as before 3.6.2)
• user can not assign role to themselves without having rights to read that role (this is new in 3.6.2.) and without write permission to own acl_userroles (same as before 3.6.2)

The main focus of the change was to allow editing privileges, close the present security hole and to be backward compatible since this is a minor release.

I hope i managed to cover all unclear points in the above, if not, please shout. If you see any obvious hole in the above, please shout as well.

thanks Jan

> * the dialog is exposed (this is new) so users can change their own privileges (by default, all except anonymous) (this right was always here).

perfect

> * the above is configurable and where desirable, users can be taken away such privilege

great.
My point here is that I can't see user and roles as "user preferences", they are usually a completely different stuff. I would just modify this dialog (not reusing the same one used for editing users) by removing the role/group selector.

> user can (un)assign only roles/groups he/she has rights to read

ok, but just to make it stronger: can we assume the user must have both read privileges on the groups/roles and write permissions on its own acls? And that any user should not have a write permission on its own acl by default?
That's the main part of the whole that looks fragile, actually this all is based to the fact that adding a role the user can't read doesn't work because the save handler isn't able to convert the role name to a uuid.
nb: the problem of a "corrupted" user node if a user manually type a user role he can't access is directly correlated, and pretty important to fix: neither the superuser is able to fix a similar node, he can just delete it and recreate. I assume that if we expose a field called "roles" we will end up with tons of broken users. Who could resist to just try to write "superuser" if you get proposed a similar form? ;D
(ok, it may be a user "mistake", but we should at least don't punish him too much)

> assigning user explicitly read to whole userroles workspace is equivalent to assigning that user superuser role in respect to granting roles hence such user can assign any role (even to themselves). If you need user to be able to assign roles, but do not want such user to be able to assign privileges to themselves, you have to explicitly set acl_userroles of that given user to read only for that given user (again this is same as before 3.6.2)

understood, and my point is: let's remove the write privilege to the acl subnode if not assigned by default. It's true that some of this problems were also in previous versions of magnolia, but adding a edit dialog easily accessible to any user surely could make the problem bigger.

Summing up, this are the improvement I would suggest:

• create a specific dialog for user preferences, without the group/role selector
• bugfix: if a wrong/unreadable role/group is manually inserted in the field, don't break the user node, just don't assign such group/role
• make acls on the own user node not writable by default (so just making it secure by default and allowing to change it if needed).

that's all, and I happily could take care of working at some of these if we agree that it could be an improvement compared to what we have now...
(if we agree on some of these points we could split this discussion in small specific tasks?)

Excellent. I think we have cleaned up all the confusion and are pretty much in sync now.

Summing up, this are the improvement I would suggest:

• create a specific dialog for user preferences, without the group/role selector

yes, but I would prefer not to change this for 3.6.2, but to do it on major version (e.g. 3.7 - current trunk) instead. We also need to take care admin user still has such a dialog available and it is unreachable for ordinary user even if they know the url to access it directly. ... We also need to decide what makes user privileged enough to access such dialog.

• bugfix: if a wrong/unreadable role/group is manually inserted in the field, don't break the user node, just don't assign such group/role

This indeed needs to be fixed before the release. We might also want to add a warning into log files that user this and that tried to assign himself something.

• make acls on the own user node not writable by default (so just making it secure by default and allowing to change it if needed).

again, yes, but preferably for major version rather then for bugfix release. We will also need to decide how to handle update of existing users here - change or keep???

I suggest we keep this issue open for the second point and split two others into separate issues and target them for 3.7. WDYT?

great, I agree on all but I still only have one concern about:

create a specific dialog for user preferences, without the group/role selector
yes, but I would prefer not to change this for 3.6.2, but to do it on major version (e.g. 3.7 - current trunk) instead

Since this is intended for end users (editors, not magnolia administrators) I am pretty scared also by the fact that users can easily lock themselves out using a similar dialog. There is no security involved here, only people that may click on that "remove" button near to the "roles" thing in the dialog (also checking the disable checkbox, but that's to explicit also for a dummy user)... it's just a sort of self-destroy button if consider a kind of user which neither know what roles are in magnolia since they always only took care of editing contents. I think it could be just a little more user friendly to not to give them such power

We also need to take care admin user still has such a dialog available and it is unreachable for ordinary user even if they know the url to access it directly. ... We also need to decide what makes user privileged enough to access such dialog.

what about just creating a simpler dialog for 3.6.2 and leave the other stuff (make the ordinary one unreachable for ordinary users) for 3.7? Should be a simple thing and doesn't changes anything we had in 3.6.1.

(ps: I am still worried about the fact that users have write access to their own acls at repository level but I agree that this could be problematic to do in 3.6.2. We should only carefully check that there is really no way for anybody to inject a role uuid there...)

Since this is intended for end users (editors, not magnolia administrators) I am pretty scared also by the fact that users can easily lock themselves out using a similar dialog. There is no security involved here, only people that may click on that "remove" button near to the "roles" thing in the dialog (also checking the disable checkbox, but that's to explicit also for a dummy user)... it's just a

True on the other hand if you get power user or local admin rights on windows machine you can also inadversely lock yourself out. (not saying that Magnolia should be stupid like that, just that this is a common possibility)

sort of self-destroy button if consider a kind of user which neither know what roles are in magnolia since they always only took care of editing contents. I think it could be just a little more user friendly to not to give them such power

In my experience users fear what they don't know and would not touch it being afraid what can happen.

what about just creating a simpler dialog for 3.6.2 and leave the other stuff (make the ordinary one unreachable for ordinary users) for 3.7? Should be a simple thing and doesn't changes anything we had in 3.6.1.

Right now, the exactly same dialog is used and handling is exactly same for both superuser editing someones preferences and for users themselves. We can change it, but I don't see how this can be done without actually changing the way this works (which is why i don't want to do it for 3.6.x).
But we can always try. If you can solve it with minimal changes, do so on the trunk and we can then backport it on 3.6 branch.
In case we don't solve it before 3.6.2 is going out (most likely next week), we can always just disable the link to user preferences for 3.6.2 so the users won't be tempted.

Perhaps the simplest change in the mean time would be to make this groups/roles control read only, unless user has full rights to userroles/usergroups workspaces (== superuser).

Time is getting short, so here is the plan:

• if we don't have better solution by tomorrow, I'll remove the direct link to user preferences from the admininterface on 3.6 branch
• in that case i will also change fix version of MAGNOLIA-574 to 3.7 only
• I'll create separe issues for having new preferences dialog and for removing write access from users own acl_* and close this issue.

Since the other security fixes, that makes 3.6 more secure then before, but doesn't open way for user to lock themselves out by default final situation should be better and not worse then in previous versions.
Plus we get few months to tackle changes to preferences dialog and to decide what to do with write permission to users own roles/groups.

If you think there is anything wrong with this plan, let me know ... preferably before I proceed.

Integrated in magnolia_main-3.6-branch #15
MAGNOLIA-574, removing link to user preferences due to security concerns

Since there were no objections to what I proposed I went ahead:

• MAGNOLIA-574 have been reverted on 3.6. branch
• making ACL subnodes read only have been split in MAGNOLIA-2399
• spliting Roles/Groups configuration have been split in MAGNOLIA-2400

Since the bug in assignment is fixed, closing this issue for 3.6.2.