Privilege Separation/Escalation

leonard

I want to make Vanilla available to a user, but I don't want them to be able to change application settings. They should be able to manage accounts though (to approve new members). For this reason I have created a role "User Admin" that has all options until "Notify by email of new applicants" set, plus "IP addresses visible", "Registration configuration" and "Can view debug info", and created a corresponding user "useradmin". (using Vanilla 1.0.3.) However, with "Change user roles" unset "useradmin" is unable to approve accounts. The only way to make this work is to set the "Change user roles" options. This however has the potential for privilege escalation: useradmin applies with a new username and assigns this new user Administrator privileges. Is there intention of making role changing permissions more fine grained?

y2kbg

hmm this subject has come up alt least 3 times now in the last week or so, maybe time for a core change with "more fine grained role changing permissions"

Minisweeper

Hmm. It seems users cant escalate their own role (as i thought) however they can escalate other users beyond themselves. That is something that needs dealing with...

leonard

I suppose the fact that a user can't change his own role is a safety against the last "role changing" user taking that permission away from himself. The problem of role escalation would be a bit less of a practical problem if at least the "approve accounts" would work without having to enable "change roles" (i.e. act as a very limited "change roles", only from "applicant" to "member", but this requires fixing of the test in AssingRole(), using an extra parameter to the function or addition of OldRoleID (0 for applicant) to UserRoleHistory). In that case only top level admins could be assigned the "change roles" privilege. This would still only enable the "user admin" to approve members to the default approval role and not to his own level but could in practice avoid the privilege escalation (for this particular setup). Fixing AssignRole() could be a first step before implementing more fine grained "change roles" permissions.

leonard

Patch below is a temporary workaround to enable applicant approval from role 0 (not DEFAULT_ROLE as that opens up another hole in case DEFAULT_ROLE != 0) to APPROVAL_ROLE. It introduces a new property OldRoleID to the UserRoleHistory class. When the user has "approve applicant" permission, the target role is APPROVAL_ROLE and the OldRoleID is zero the approval is performed.

--- People.000/People.Class.UserRoleHistory.php 2006-08-21 16:10:40.000000000 +0200
+++ People/People.Class.UserRoleHistory.php     2007-02-22 14:54:00.000000000 +0100
@ class UserRoleHistory {
        var $UserID;
        var $Username;
        var $FullName;
+       var $OldRoleID;
        var $RoleID;
        var $Role;
        var $RoleDescription;
@ class UserRoleHistory {
                $this->UserID = 0;
                $this->Username = '';
                $this->FullName = '';
+               $this->OldRoleID = 0;
                $this->RoleID = 0;
                $this->Role = '';
                $this->RoleDescription = '';
@ class UserRoleHistory {
                $this->Username = ForceString(@$DataSet['Username'],'');
                $this->FullName = ForceString(@$DataSet['FullName'],'');
                $this->RoleID = ForceInt(@$DataSet['RoleID'],0);
+               $this->OldRoleID = $this->RoleID;
                $this->Role = ForceString(@$DataSet['Role'],'');
                $this->RoleDescription = ForceString(@$DataSet['RoleDescription'],'');
                $this->RoleIcon = ForceString(@$DataSet['RoleIcon'],'');
--- People.000/People.Class.UserManager.php     2006-08-21 16:10:34.000000000 +0200
+++ People/People.Class.UserManager.php 2007-02-22 14:56:14.000000000 +0100
@ class UserManager extends Delegation {

        function AssignRole($UserRoleHistory, $NewUser = '0') {
                $NewUser = ForceBool($NewUser, 0);
-               if (!$this->Context->Session->User->Permission('PERMISSION_CHANGE_USER_ROLE') && !$NewUser) {
+               if (!$this->Context->Session->User->Permission('PERMISSION_CHANGE_USER_ROLE') && !$NewUser &&
+                       !($this->Context->Session->User->Permission('PERMISSION_APPROVE_APPLICANTS') && $UserRoleHistory->RoleID == $this->Context->Configuration['APPROVAL_ROLE']) && $UserRoleHistory->OldRoleID == 0) {
                        $this->Context->WarningCollector->Add($this->Context->GetDefinition('ErrPermissionInsufficient'));
                } elseif ($UserRoleHistory->Notes == '') {
                        $this->Context->WarningCollector->Add($this->Context->GetDefinition('ErrRoleNotes'));

leonard

Related issue: People with PERMISSION_APPROVE_APPLICANTS can remove *any* user with a direct URL. This is because the test for permission is only performed in account.php when the control panel is build, but not in RemoveApplicant() in People.Class.UserManager.php. As RemoveApplicant does not even test for the user being removed being an applicant *any* user can be removed this way. This is easily fixable by adding appropriate tests in RemoveApplicant().

leonard

Something like this:

--- People.Class.UserManager.php        2007-02-22 15:41:04.000000000 +0100
+++ People.Class.UserManager.php        2007-02-22 17:13:08.000000000 +0100
@ class UserManager extends Delegation {
        }

        function RemoveApplicant($UserID) {
+               // Make sure caller has permission to perform this action and this is indeed an applicant being removed
+               // First get the RoleID of the user being removed
+               $s = $this->Context->ObjectFactory->NewContextObject($this->Context, 'SqlBuilder');
+               $s->SetMainTable('User', 'u');
+               $s->AddSelect('RoleID', 'u');
+               $s->AddWhere('u', 'UserID', '', $UserID, '=');
+
+               $Result = $this->Context->Database->Select($s, $this->Name, 'RemoveApplicant', 'An error occurred while removing the user.');
+// ADD: Need a proper error definition here, then uncomment these two lines
+//             if ($this->Context->Database->RowCount($Result) != 1) $this->Context->WarningCollector->Add($this->Context->GetDefinition('ErrRemoveUser'));
+//             if ($this->Context->WarningCollector->Count() > 0) return false;
+               $row = $this->Context->Database->GetRow($Result);
+
+               // And actually check role and permissions are appropriate
+               if ($row['RoleID'] != 0 || !($this->Context->Session->User->Permission('PERMISSION_CHANGE_USER_ROLE') ||
+                       $this->Context->Session->User->Permission('PERMISSION_APPROVE_APPLICANTS'))) {
+                       $this->Context->WarningCollector->Add($this->Context->GetDefinition('ErrPermissionInsufficient'));
+                       return false;
+               };
+
                // Ensure that the user has not made any contributions to the application in any way

                // Styles
-      $s = $this->Context->ObjectFactory->NewContextObject($this->Context, 'SqlBuilder');
+               $s->Clear();
                $s->SetMainTable('Style', 's');
                $s->AddSelect('StyleID', 's');
                $s->AddWhere('s', 'AuthUserID', '', $UserID, '=');
+
                $Result = $this->Context->Database->Select($s, $this->Name, 'RemoveApplicant', 'An error occurred while removing the user.');
                if ($this->Context->Database->RowCount($Result) > 0) $this->Context->WarningCollector->Add($this->Context->GetDefinition('ErrRemoveUserStyle'));
                if ($this->Context->WarningCollector->Count() > 0) return false;
-
+
                // Comments
                $s->Clear();
                $s->SetMainTable('Comment', 'm');

Mark

Leonard: thanks for your email.

From your email:

1) Users that have permission to change roles can escalate their own
role via a new user that they can approve and then assign a higher
level role than their own.

I've discussed this on here before. I've placed weights on the roles in Vanilla - but I haven't placed any meaning on those weights yet. It may come in the next revision, but it is unlikely as I've got bigger fish to fry. In the meantime you will have to either (a) trust the people who you give role management permissions implicitly or (b) handle roles yourself.


2) Users with PERMISSION_APPROVE_APPLICANTS can remove *any* user from
the system via a direct URL because of lacking permission checking in
RemoveApplicant(). Permission checking is only performed when the
control panel is being build, but this is insufficient as the URL to
remove a user is easy guessable, f.e.
http://lussumo.com/community/account.php?u=1&PostBackAction=DeclineUser


There is actually a much bigger exploit that is quite similar to this problem, and I have resolved that exploit in subversion. Coincidentally, it solves this problem as well. That url will not work for any user - even if they have the permissions. When the next rev comes out (very soon), a user will need to use Vanilla directly in order for any forms to work - posting or linking externally will not fly.

birdmanx35

All this talk of next revisions get some of us very anxious ;) Nice job, Mark... Also, have you ever considered taking on another core developer? It might take less pressure off you and improve Vanilla as a whole for users...

Minisweeper

The problem would be finding another super human with Marks skills, vision, dedication, and mindset.

birdmanx35

Very true, but it seems to me there are several other heavy contributers here...

leonard

Mark, thanks for your reply. I didn't have time to check this discussion earlier than today. I've been looking at the site and coulnt' find a reference, but is it possible to browse or download the current subversion somewhere?

leonard

O, and what about the fact that one needs to have "change role" permission to approve applicants? I suppose that is a bug in the straight forward sense, as the naming of the "approve applicants" permission suggests one can approve applicants ;-) . An idea which is supported by the fact that a user with "approve applicants" permission can already deny applicants without needing "change role" permission. As you yourself say the weighing of the roles is not implemented yet, so it would be good if one could at least let a "user admin" approve applicants without enabling them to change roles. For that to be possible a patch similar to the one in comment #5 is a necessity. Any chance of such a fix being integrated? (FYI, this latter issue I mentioned in my first two mails on February 20th.)

leonard

Testing vanilla-1.1.1: Logging in as "useradmin" with permissions as described in the initial comment in this thread I do not see an option to approve applicants on login, nor do applicants show up in a user search. A direct URL doesn't work either, which is at least consistent (but too restrictive). Am I supposed to set some other options that I didn't need before? For AssignRole() you've changed the test to also accept a change when user has PERMISSION_APPROVE_APPLICANTS, but in this case you should also test if the destination role is the default APPROVAL_ROLE and the role from which you change to APPROVAL_ROLE should be applicant. You should not allow a user with PERMISSION_APPROVE_APPLICANTS to change from any role to any role (they could manipulate the approve applicant form (assuming it is made available again)). Please have a look at comment #5 and see what I try to achieve there.

Mark

Hi Leonard,

1. I changed the way approval works in Vanilla as of 1.1. You no longer go to the account page to approve people, so that would explain why you don't see the option there anymore. Instead, you should have access to the settings.php page and from there you can click the "membership applicants" option in the menu to see and approve applicants. If you want more notifications available, you should download the "New Applicants" extension which places a notification above the discussion list when there are new applicants (and you have approve applicants permission).

2. Because the only form a user with "approve applicants" permission can use to approve applicants is on that "membership applicants" page in the settings tab, there is no way for that user to "hack" the AssignRole() method. They will never be able to tell it to use some other roleid, so there is no need to enforce any extra constrictions. If you can prove me wrong, I'd be willing to change it. But otherwise, I don't see the need.

leonard

Regarding 1), when logging in as "full" admin I immediately see a option to review applications in people.php, and I have a "settings" tab. This is *not* the case when I log in as "useradmin" with only "approve applicants" permission. Searching for users also does *not* show applicants. Ability to approve applicants seemed to have disappeared altogether for this setup (1.1.1). Regarding 2), I will study the code a bit more later on. Notwithstanding the fact that you probably sufficiently check the rights of the user before jumping to AssingRole() (well, too strict atm) I would say it's a good strategy to check permissions at the lowest level, as that will prevent holes occuring because of mistakes in higher levels that could be introduced at a later point. But this is your call of course.

leonard

Oops, I screwed up. Didn't change "useradmin" to the appropriate role on my new installation (or changed it back to "member"). After fixing this everything works as expected, user with just "approve applicants" permission can approve applicants. Thank you very much for fixing this.

Mark

No problem!