U4-6929 - User permissions security issue

Created by Richard Terris 06 Aug 2015, 08:46:36 Updated by Shannon Deminick 31 Aug 2017, 05:54:42

Is duplicated by: U4-10267

Relates to: U4-10062

Subtask of: UAASSCRUM-1073

I've discovered something in V7 which didn't exist in previous versions, and is quite a big security issue.

If I create an admin user with access to only (for example) content and media, and then login as that user, I can then create new users and give them access to any section.

This means that the newly created user could then in turn give the original user access to any section.

I don't think you should be able to give people access to something which you yourself don't have access to - the implications of this could be quite serious.

Thanks, Richard

Comments

Shannon Deminick 26 Jun 2017, 07:13:12

Closing issue due to inactivity - see blog post for details https://umbraco.com/blog/issue-tracker-cleanup/


Shannon Deminick 19 Jul 2017, 04:09:47

We need to review what permissions the current user can edit on themselves too


Shannon Deminick 01 Aug 2017, 06:25:04

In 7.7 we have another issue where a user that has access to the User section but doesn't have access to any start node can select the root node of a tree for a user group, thus giving themselves and anyone more access than they have


Shannon Deminick 09 Aug 2017, 06:35:44

Also see notes here http://issues.umbraco.org/issue/U4-10267


Mads Rasmussen 17 Aug 2017, 13:56:42

@Shandem some notes on how we can handle security:

When a user has access to the user section they are allowed to create users, invite users and create user groups. In general, we need to limit the user to not create or update any permissions they don’t have access to themselves. Places where we need to update:

'''Create/invite user:''' It should not be possible to add a user group which has access to sections or start nodes the user hasn’t. We will need a new end point for this to get allowed user groups.

'''Users overview / Bulk actions'''

  • '''Set group:''' same as create/invite

'''Edit user:''' Admin users should be able to update everything. Other users should be able to change language, avatar, and password.

'''Create user group/edit user group:'''

  • '''Sections:''' You should only be able to add sections which you have access to. We need a new end point for getting the allowed sections where admins get the full list and other users only get the sections they have access to.
  • '''Start nodes:''' these should already be working as you can’t pick anything else than what you have access to.
  • '''Users:''' You shouldn’t be allowed to add yourself. We might need a new endpoint which gives us the allowed users. (All for admins and a list without yourself for other users)
  • '''Default/Granular permissions:''' should these be restricted for other than admins also?

We also need to find a solution for users who want to update their own profile: language or avatar but don’t have access to the user section.


Shannon Deminick 22 Aug 2017, 07:56:33

Most of this is fixed without extra endpoints and all of this has server side authorization checks in case people tamper with the data being sent to the server.

  • Create/invite user: It should not be possible to add a user group which has access to sections or start nodes the user hasn’t. --We will need a new end point for this to get allowed user groups.-- ** This is fixed, we don't need another endpoint. If your admin, the full list of groups is returned, if your are not, only the groups you belong to are returned - this goes for all things that list groups
  • Users overview / Bulk actions/ Set group ** Fixed, as above, only the groups that the user belongs to are shown unless you are admin

User group editor

  • A non-admin user that saves a user group can ** Remove allowed sections ** Add sections that they are allowed ** Save a user group even if they already have sections listed that they don't have access to BUT they cannot add a section that they don't have access to ** Save a user group even if they already have start nodes selected that they don't have access to BUT they cannot change the start nodes to something they don't have access to
  • An admin user that saves a user group can ** Add/remove any sections ** Same rules for start nodes apply for admins and non-admins, they cannot pick a start node they don't have access to

... This is working now

Granular permissions

I don't think these should be locked down to admins since that's never been the case currently. Being able to set granular permissions is enabled by either:

  • You have the granular permission to set permissions at the Content level using the context menu
  • You have access to the user section and you can edit a user group's granular permissions that you belong to (or are an admin)

... we don't have to change anything if you agree with this

User Editor

We shouldn't limit non-admins from being able to assign groups to users, since they can assign users to groups in the group editor. We also shouldn't limit non-admins from being able to assign start nodes, we just need to follow the same rules above for users. So:

  • A non-admin user that saves a user can ** Remove assigned groups ** Add groups that they belong to ** Save a user even if they already have groups listed that they don't have access to BUT they cannot add a group that they don't belong to ** Save a user even if they already have start nodes selected that they don't have access to BUT they cannot change the start nodes to something they don't have access to ** not save an admin - admins are not listed in the list for non-admins
  • An admin user that saves a user can ** Add/remove any groups ** Same rules for start nodes apply for admins and non-admins, they cannot pick a start node they don't have access to

... This is working now

This is also fixed as part of this PR: http://issues.umbraco.org/issue/UAASSCRUM-1045


Shannon Deminick 22 Aug 2017, 08:00:06

PR: https://github.com/umbraco/Umbraco-CMS/pull/2143

TODO: I need to fix up unit tests


Mads Rasmussen 22 Aug 2017, 12:29:34

I have done some testing and added some comments for some of the bullets:

'''Create/invite user:''' *[x] If you are an admin, the full list of groups is returned, if you are not, only the groups you belong to are returned - this goes for all things that list groups (This also means that I am only able to see the groups I belong to in the groups overview. Is this the best solution? This is why a suggested another end point for the pickers)

'''Users overview / Bulk actions/ Set group'''

getPagedUsers returns a 500 error for user with id 0 ("There was an error parsing the query. [ Token line number = 2,Token line offset = 3,Token in error = ]”)

*[x] Fixed, as above, only the groups that the user belongs to are shown unless you are admin

'''User group editor'''

'''A non-admin user that saves a user group can''' *[x] Remove allowed sections (Works because I can only see and edit the groups I am part of and therefore have access to all the sections in these groups) *[x] Add sections that they are allowed *[x] Save a user group even if they already have sections listed that they don't have access to BUT they cannot add a section that they don't have access to (Works because I can only see and edit the groups I am part of and therefore have access to all the sections in these groups. Only allow sections I have access to works) *[ ] Save a user group even if they already have start nodes selected that they don't have access to BUT they cannot change the start nodes to something they don't have access to (I am able to change the start node to content I shouldn’t have access to. Case: Assign two groups to a user. Add start nodes to one of the groups and leave the other as root. User has access to everything) '''An admin user that saves a user group can''' *[x] Add/remove any sections *[ ] Same rules for start nodes apply for admins and non-admins, they cannot pick a start node they don't have access to

Granular permissions [x] Let's keep granular permissions the way they work now

'''User Editor'''

Admins can’t save users. They get a 401 unauthorised.

'''A non-admin user that saves a user can''' *[x] Remove assigned groups *[x] Add groups that they belong to *[x] Save a user even if they already have groups listed that they don't have access to BUT they cannot add a group that they don't belong to *[ ] Save a user even if they already have start nodes selected that they don't have access to BUT they cannot change the start nodes to something they don't have access to (Same bug as above) *[x] not save an admin - admins are not listed in the list for non-admins (Is the best way to prevent this to hide the admins totally? It would be nice to see who the admins are if you needed new permissions) '''An admin user that saves a user can''' *[x] Add/remove any groups *[ ] Same rules for start nodes apply for admins and non-admins, they cannot pick a start node they don't have access to (same bug as above)


Shannon Deminick 23 Aug 2017, 05:20:33

Some responses:

If you are an admin, the full list of groups is returned, if you are not, only the groups you belong to are returned - this goes for all things that list groups (This also means that I am only able to see the groups I belong to in the groups overview. Is this the best solution? This is why a suggested another end point for the pickers)

If we allow listing all of the groups to a user that only has access to a couple what will that achieve since they cannot save a group that they do not belong to?

The only thing I can think in this scenario is in the user's list, the groups filter will only show the groups they belong to but maybe when filtering users you'd want to list all groups.

Thoughts?

A non-admin user that saves a user group can - Save a user group even if they already have start nodes selected that they don't have access to BUT they cannot change the start nodes to something they don't have access to (I am able to change the start node to content I shouldn’t have access to. Case: Assign two groups to a user. Add start nodes to one of the groups and leave the other as root. User has access to everything)

This is because of how start nodes work as we've previously discussed. Assigning root to one group that you belong to, or yourself doesn't automatically give you root access. Start nodes are calculated based on the least defined privilege, otherwise it's far too easy to end up with root access that you weren't meant to get. It would be worth adding the calculated start nodes for a user in the extra user (readonly) properties that we haven't added yet. Also note that a group can have zero or 1 start node. A group with a null start node means it has no affect on start nodes, if a group should have root access it needs to be specifically assigned to a root node (but again, this doesn't necessarily mean that a user that is part of this group will get root access, it depends on other groups they belong to and their own start nodes assigned!)

Let me know if that does/doesnt make sense.

getPagedUsers returns a 500 error for user with id 0 ("There was an error parsing the query. [ Token line number = 2,Token line offset = 3,Token in error = ]”)

Fixed

Admins can’t save users. They get a 401 unauthorised.

I've found another issue in review that is fixed as part of this PR: http://issues.umbraco.org/issue/U4-10346 and I've fixed up the unit tests.


Mads Rasmussen 23 Aug 2017, 18:58:05

[x] Admins can save user again [x] Admins user overview is working again

'''User group overview. ''' If I have access to the user section and are able to create user groups, I think it would be a bit weird if I wasn’t able to see all the user groups which already exists. Then I could end up in a trial and error situation if I wanted to add new groups.

In my opinion, you should be able to see all available user groups in the user group overview but you should not be able to add access to any nodes or sections you don’t have access to yourself. Are there any security risks in being able to save user groups you are not part of?

Side note: I just found a bug. If I try to add a user group with a system alias “administrator”, “editors” etc. I get a YSOD. I get a nice error if I just try to add a “normal” group which already exists.

'''Start nodes''' I think we say the same thing about start nodes. I am just not able to get it to work.

My test case: If I make a user and assign two groups: one with root access and one with a start node. I have access to everything but I should only have access to the start node assigned to the one group. Or am I totally crazy here? 😃


Shannon Deminick 24 Aug 2017, 09:52:12

User group overview

Ok cool makes sense, i'll update this endpoint and get this working

Side note: I just found a bug. If I try to add a user group with a system alias “administrator”, “editors” etc. I get a YSOD. I get a nice error if I just try to add a “normal” group which already exists.

Will fix tomorrow

Start nodes

haha, ok i'll test :)


Shannon Deminick 28 Aug 2017, 15:20:06

OK, think this is all working now:

  • Starts nodes - user group start nodes are additive so a user belonging to Group A (root) and Group B (1234) will have root node access. BUT user start nodes are restrictive, so if this user belongs to Group A and B and also has their own start node of 1234, then they're start node will be 1234 and not root
  • User editor fixed, before you couldn't save a user that had groups that you didn't belong to but like listed above, you can save a user that has groups that you don't belong to if they are already assigned and you can remove groups and you can add them to a group that you have access
  • Users list now shows all groups so you can filter - however for non-admins, the Administrator group is not listed because non-admins cannot edit admins
  • User group list now shows all groups but you cannot select or editor a group you do not belong to

TODO:

  • The design of this should be updated a little so that the non-editable groups are greyed out (or similar) // madsrasmussen

Last note for me, these security policies are getting quite tricky with the UI and trying to prohibit the UI from allowing you to do things you are not allowed. Here's the last issue I have:

  • User A is in Group X (root start node) and Group Y (1234 start node)
  • User A has access to the User section but is not an admin
  • User A has a custom start node assigned to them of 1234 (which means they have 1234 as their start node)
  • User A edits User B and adds Group X ... kabum! Unauthorized because they are trying to add a group to User B that will give user B start node access that User A doesn't have even though User A belongs to Group X! hrm....

So I'm going to change thsi logic - a user can assign a group to another user if they also belong to that group regardless of start nodes. This simplifies things quite a bit


Mads Rasmussen 30 Aug 2017, 11:31:45

[x] start nodes works [ ] user editor fix (saving) not working [x] user list works as described (I still think it would be better to show all users and then make a read/write state of the user editor profile, but let’s keep it like this for now) [x] user group list works as described (same as users)

I still get kicked out when I try to save a user with a different start node than mine: http://recordit.co/R88kk5xudc

I have adding styling for the user group list so user groups you don’t have access to is grayed out and the cursor is changed to “not-allowed. I did a small fix for admins because they were also restricted to only being able to edit the groups they were members of.

PR: https://github.com/umbraco/Umbraco-CMS/pull/2161


Shannon Deminick 31 Aug 2017, 05:54:31

@madsrasmussen all looks good!

Admins not seeing other admins is just how it was before, we can update in the future if requested.

I've verified the issue you were seeing, i've updated the logic and added 4 more unit tests to confirm all is working the correct way and tested. Thanks for all your testing, it's been super great!

I'm going to close this one down now :)


Priority: Normal

Type: Bug

State: Fixed

Assignee:

Difficulty: Normal

Category: Security

Backwards Compatible: True

Fix Submitted:

Affected versions: 7.2.6

Due in version: 7.7.0

Sprint: Sprint 66

Story Points: 3

Cycle: