userid-group check: distinguish create and update

and check both existing groups and the groups parameter in the update
case. the following user.cfg settings can be used for testing:

user:test@pve:1:0:t::::x:
user:other@pve:1:0:t::::x:
group:test:test@pve::
group:test3:::
role:RealmUserAllocator:Realm.AllocateUser:
role:UserModifier:User.Modify:
acl:0:/access/groups/test:test@pve:UserModifier:
acl:0:/access/groups/test3:@test:UserModifier:
acl:0:/access/realm/pve:test@pve:RealmUserAllocator:

unchanged: the user 'test@pve' can allocate new '@pve' users, but
only if the created user will belong to at least one of 'test'
(direct ACL for that user) or 'test3' (indirect ACL via 'test' group)
groups.

changed: if the user 'test@pve' updates an existing user, they need
to (A) have 'User.Modify' on at least one existing group of that
user, and (B) 'User.Modify' on all of the groups passed in via the
'groups' parameter. A is the general rule for 'allowed to modify
user' across the board, but was missing for this specific variant of
the check. B was the case before, but just checking this without also
checking A allows a user to pull off-limits users into groups that
they can modify, which then in turn allows them to modify those users
via A which is now passing.

for example, without this patch 'test@pve' would be able to add
'other@pve' to either 'test' or 'test3', and then in turn call any of
the API endpoints that require 'User.Modify' on a user's group
(change TFA, change password or delete user if realm is pve, ..).

all the other userid-group checks without group_param set remain
unchanged as well, since $check_existing_user is true in that case.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>

diff --git a/src/PVE/API2/User.pm b/src/PVE/API2/User.pm
index 244264e88e4136d52bbc27eb36e47b411eae6624..6495a12f225d28881468b1d6a791e131c7034645 100644 (file)
--- a/src/PVE/API2/User.pm
+++ b/src/PVE/API2/User.pm
@@ -221,7 +221,7 @@ __PACKAGE__->register_method ({
        check => [
            'and',
            [ 'userid-param', 'Realm.AllocateUser'],
-           [ 'userid-group', ['User.Modify'], groups_param => 1],
+           [ 'userid-group', ['User.Modify'], groups_param => 'create'],
        ],
     },
     description => "Create new user.",
@@ -345,7 +345,7 @@ __PACKAGE__->register_method ({
     path => '{userid}',
     method => 'PUT',
     permissions => {
-       check => ['userid-group', ['User.Modify'], groups_param => 1 ],
+       check => ['userid-group', ['User.Modify'], groups_param => 'update' ],
     },
     description => "Update user configuration.",
     parameters => {

diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm
index b1348fab8f38141606cbce5a61d2d6a4ded3253d..f11517fc138325ec8cec013666d609ec9f244317 100644 (file)
--- a/src/PVE/RPCEnvironment.pm
+++ b/src/PVE/RPCEnvironment.pm
@@ -408,17 +408,25 @@ sub exec_api2_perm_check {
     } elsif ($test eq 'userid-group') {
        my $userid = $param->{userid};
        my ($t, $privs, %options) = @$check;
-       return 0 if !$options{groups_param} && !$self->check_user_exist($userid, $noerr);
+
+       my $check_existing_user = !$options{groups_param} || $options{groups_param} ne 'create';
+       return 0 if $check_existing_user && !$self->check_user_exist($userid, $noerr);
+
+       # check permission for ALL groups (and thus ALL users)
        if (!$self->check_any($username, "/access/groups", $privs, 1)) {
+           # list of groups $username has any of $privs on
            my $groups = $self->filter_groups($username, $privs, 1);
            if ($options{groups_param}) {
+               # does $username have any of $privs on all new/updated/.. groups?
                my @group_param = PVE::Tools::split_list($param->{groups});
                raise_perm_exc("/access/groups, " . join("|", @$privs)) if !scalar(@group_param);
                foreach my $pg (@group_param) {
                    raise_perm_exc("/access/groups/$pg, " . join("|", @$privs))
                        if !$groups->{$pg};
                }
-           } else {
+           }
+           if ($check_existing_user) {
+               # does $username have any of $privs on any existing group of $userid
                my $allowed_users = $self->group_member_join([keys %$groups]);
                if (!$allowed_users->{$userid}) {
                    return 0 if $noerr;