From aee071adf322d2df3072579daaf6bbad187faa4a Mon Sep 17 00:00:00 2001 From: =?utf8?q?Fabian=20Gr=C3=BCnbichler?= Date: Thu, 17 Mar 2022 09:57:21 +0100 Subject: [PATCH] userid-group check: distinguish create and update MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Thomas Lamprecht --- src/PVE/API2/User.pm | 4 ++-- src/PVE/RPCEnvironment.pm | 12 ++++++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/PVE/API2/User.pm b/src/PVE/API2/User.pm index 244264e..6495a12 100644 --- 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 b1348fa..f11517f 100644 --- 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; -- 2.39.2