[MAGNOLIA-2388] Easy privilege escalation from user preferences Created: 22/Sep/08 Updated: 23/Jan/13 Resolved: 29/Sep/08 |
|
| Status: | Closed |
| Project: | Magnolia |
| Component/s: | security |
| Affects Version/s: | 3.6.2 |
| Fix Version/s: | 3.6.2, 3.6.3 |
| Type: | Bug | Priority: | Blocker |
| Reporter: | Fabrizio Giustina | Assignee: | Fabrizio Giustina |
| Resolution: | Fixed | Votes: | 1 |
| Labels: | None | ||
| Σ Remaining Estimate: | Not Specified | Remaining Estimate: | Not Specified |
| Σ Time Spent: | Not Specified | Time Spent: | Not Specified |
| Σ Original Estimate: | Not Specified | Original Estimate: | Not Specified |
| Issue Links: |
|
||||||||||||||||||||
| Sub-Tasks: |
|
||||||||||||||||||||
| Template: |
|
||||||||||||||||||||
| Acceptance criteria: |
Empty
|
||||||||||||||||||||
| Task DoD: |
[ ]*
Doc/release notes changes? Comment present?
[ ]*
Downstream builds green?
[ ]*
Solution information and context easily available?
[ ]*
Tests
[ ]*
FixVersion filled and not yet released
[ ] 
Architecture Decision Record (ADR)
|
||||||||||||||||||||
| Bug DoR: |
[ ]*
Steps to reproduce, expected, and actual results filled
[ ]*
Affected version filled
|
||||||||||||||||||||
| Date of First Response: | |||||||||||||||||||||
| Description |
|
This is a leftover from After the change in The are multiple issues/tasks associated to this:
as previously discussed, IMHO a better solution would be allowing only readonly access to own user node by default and using a custom save handler for the preference page which allow editing of checked properties using a system operation. User preferences should use obviously a different dialog from the standard user edit dialog. Nobody else cares about this? |
| Comments |
| Comment by Wolfgang Habicht [ 23/Sep/08 ] |
|
If it is like described (I could not verify it with Mgnl 3.6.1), this issue is a blocker for 3.6.2. |
| Comment by Jan Haderka [ 23/Sep/08 ] |
could you please leave self pity out? No one ignored your comments.
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. 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. |
| Comment by Fabrizio Giustina [ 23/Sep/08 ] |
|
> 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? > 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 |
| Comment by Jan Haderka [ 23/Sep/08 ] |
True, but each user always had and still has rights to modify their own preferences.
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.
Agreed, I haven't tried to give harsh response here either, so sorry if it came out as one. 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.
from 3.6.2 onwards:
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. |
| Comment by Fabrizio Giustina [ 23/Sep/08 ] |
|
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. > 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? > 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:
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... |
| Comment by Jan Haderka [ 23/Sep/08 ] |
|
Excellent. I think we have cleaned up all the confusion and are pretty much in sync now.
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.
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.
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? |
| Comment by Fabrizio Giustina [ 23/Sep/08 ] |
|
great, I agree on all but I still only have one concern about:
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
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...) |
| Comment by Jan Haderka [ 24/Sep/08 ] |
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)
In my experience users fear what they don't know and would not touch it being afraid what can happen.
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). 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). |
| Comment by Jan Haderka [ 25/Sep/08 ] |
|
Time is getting short, so here is the plan:
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. If you think there is anything wrong with this plan, let me know ... preferably before I proceed. |
| Comment by Jan Haderka [ 29/Sep/08 ] |
|
Since there were no objections to what I proposed I went ahead:
Since the bug in assignment is fixed, closing this issue for 3.6.2. |