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>
check => [
'and',
[ 'userid-param', 'Realm.AllocateUser'],
check => [
'and',
[ 'userid-param', 'Realm.AllocateUser'],
- [ 'userid-group', ['User.Modify'], groups_param => 1],
+ [ 'userid-group', ['User.Modify'], groups_param => 'create'],
],
},
description => "Create new user.",
],
},
description => "Create new user.",
path => '{userid}',
method => 'PUT',
permissions => {
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 => {
},
description => "Update user configuration.",
parameters => {
} elsif ($test eq 'userid-group') {
my $userid = $param->{userid};
my ($t, $privs, %options) = @$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)) {
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}) {
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};
}
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};
}
+ }
+ 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;
my $allowed_users = $self->group_member_join([keys %$groups]);
if (!$allowed_users->{$userid}) {
return 0 if $noerr;