[pve-devel] [PATCH access-control 0/3] fix two propagation related bugs

[pve-devel] [PATCH access-control 0/3] fix two propagation related bugs

[Fabian Grünbichler]

these patches fix two related bugs:
- the propagation flag used for priv dumping was set randomly if two
  roles with a common priv exist on a path, one with and one without
  propagation
- user/token priv intersection only took user privs into account that
  had propagation set

the first can affect the second one negatively (if the first bug causes
the propagation flag to be dropped, the second one will drop the priv
from the merged set of privileges for priv-separated tokens).

in both cases there is no possibility to elevate privileges:
- bug #1 sometimes marks privs as non-propagated that are, but only for
  display, not for checking purposes
- bug #2 causes a token to have less privileges than it should, not more

Fabian Grünbichler (3):
  permissions: properly merge propagation flag
  permissions: fix token/user priv intersection
  permissions: add some more comments

 src/PVE/RPCEnvironment.pm | 44 +++++++++++++++++++++++++++++++++++----
 src/test/perm-test8.pl    |  2 +-
 src/test/test8.cfg        |  2 ++
 3 files changed, 43 insertions(+), 5 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH access-control 1/3] permissions: properly merge propagation flag

[Fabian Grünbichler]

when multiple roles are defined on a path that share a privilege, this
randomly took the propagation flag for the priv from the last role
encountered. since perl hashes are iterated randomly, this means the
propagation flag was sometimes set correctly, and sometimes not.

note that this propagation flag is only used for display/dumping
purposes, and for intersection with token privs (see next commit).
actual handling of propagation happens on the role level in
PVE::AccessControl::roles().

modified test case (spuriously) fails without the fix.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/PVE/RPCEnvironment.pm | 2 +-
 src/test/test8.cfg        | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm
index ed5625e..b5da4f2 100644
--- a/src/PVE/RPCEnvironment.pm
+++ b/src/PVE/RPCEnvironment.pm
@@ -74,7 +74,7 @@ my $compile_acl_path = sub {
     foreach my $role (keys %$roles) {
 	if (my $privset = $cfg->{roles}->{$role}) {
 	    foreach my $p (keys %$privset) {
-		$privs->{$p} = $roles->{$role};
+		$privs->{$p} ||= $roles->{$role};
 	    }
 	}
     }
diff --git a/src/test/test8.cfg b/src/test/test8.cfg
index 2f85bfd..d5c7e86 100644
--- a/src/test/test8.cfg
+++ b/src/test/test8.cfg
@@ -16,6 +16,8 @@ role:customer:VM.Audit,VM.PowerMgmt:
 role:vm_admin:VM.Audit,VM.Allocate,Permissions.Modify,VM.Console:
 
 acl:1:/vms:@testgroup1:vm_admin:
+acl:0:/vms/300:max@pve:customer:
+acl:1:/vms/300:max@pve:vm_admin:
 acl:1:/vms/100/:alex@pve,max@pve:customer:
 acl:1:/storage/nfs1:@testgroup2:storage_manager:
 acl:1:/users:max@pve:Administrator:
-- 
2.30.2

[pve-devel] [PATCH access-control 2/3] permissions: fix token/user priv intersection

[Fabian Grünbichler]

the token/user priv intersection could only honored user privs that had
the propagation flag set, reducing the scope of the token more than
intended.

the pre-existing test case actually triggered the broken behaviour, but
the expected value matched it so it was not noticed.

Fixes: e8a0cee47bb477162f291e67144ea0c0df47f2ee "rpcenv: improve user/token intersection"

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/PVE/RPCEnvironment.pm | 2 +-
 src/test/perm-test8.pl    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm
index b5da4f2..a0c7555 100644
--- a/src/PVE/RPCEnvironment.pm
+++ b/src/PVE/RPCEnvironment.pm
@@ -82,7 +82,7 @@ my $compile_acl_path = sub {
     if ($username && $username ne 'root@pam') {
 	# intersect user and token permissions
 	my $user_privs = $cache->{$username}->{privs}->{$path};
-	my $filtered_privs = [ grep { $user_privs->{$_} } keys %$privs ];
+	my $filtered_privs = [ grep { defined($user_privs->{$_}) } keys %$privs ];
 	$privs = { map { $_ => $user_privs->{$_} && $privs->{$_} } @$filtered_privs };
     }
 
diff --git a/src/test/perm-test8.pl b/src/test/perm-test8.pl
index 83ca1f2..5dab6c6 100644
--- a/src/test/perm-test8.pl
+++ b/src/test/perm-test8.pl
@@ -63,7 +63,7 @@ check_roles('max@pve!token', '/vms/200', 'storage_manager');
 check_roles('max@pve!token2', '/vms/200', 'customer');
 
 # check intersection -> token has Administrator, but user only vm_admin
-check_permission('max@pve!token2', '/vms/300', 'Permissions.Modify,VM.Allocate,VM.Audit,VM.Console');
+check_permission('max@pve!token2', '/vms/300', 'Permissions.Modify,VM.Allocate,VM.Audit,VM.Console,VM.PowerMgmt');
 
 print "all tests passed\n";
 
-- 
2.30.2

[pve-devel] [PATCH access-control 3/3] permissions: add some more comments

[Fabian Grünbichler]

for future reference/changes.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/PVE/RPCEnvironment.pm | 40 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm
index a0c7555..08e39ff 100644
--- a/src/PVE/RPCEnvironment.pm
+++ b/src/PVE/RPCEnvironment.pm
@@ -23,15 +23,18 @@ my $compile_acl_path = sub {
 
     return undef if !$cfg->{roles};
 
+    # permissions() has an early return for this case
     die "internal error" if $user eq 'root@pam';
 
     my $cache = $self->{aclcache};
     $cache->{$user} = {} if !$cache->{$user};
     my $data = $cache->{$user};
 
+    # permissions() will always prime the cache for the owning user
     my ($username, undef) = PVE::AccessControl::split_tokenid($user, 1);
     die "internal error" if $username && $username ne 'root@pam' && !defined($cache->{$username});
 
+    # resolve and cache roles of the current user/token for all pool ACL paths
     if (!$data->{poolroles}) {
 	$data->{poolroles} = {};
 
@@ -52,37 +55,54 @@ my $compile_acl_path = sub {
 	}
     }
 
+    # get roles of current user/token on checked path - this already handles
+    # propagation and NoAccess along the path
+    #
+    # hash mapping role name to propagation flag value, a key being defined
+    # means the role is set
     my $roles = PVE::AccessControl::roles($cfg, $user, $path);
 
     # apply roles inherited from pools
-    # Note: assume we do not want to propagate those privs
     if ($data->{poolroles}->{$path}) {
+	# NoAccess must not be trumped by pool ACLs
 	if (!defined($roles->{NoAccess})) {
 	    if ($data->{poolroles}->{$path}->{NoAccess}) {
+		# but pool ACL NoAccess trumps regular ACL
 		$roles = { 'NoAccess' => 0 };
 	    } else {
 		foreach my $role (keys %{$data->{poolroles}->{$path}}) {
+		    # only use role from pool ACL if regular ACL didn't already
+		    # set it, and never set propagation for pool-derived ACLs
 		    $roles->{$role} = 0 if !defined($roles->{$role});
 		}
 	    }
 	}
     }
 
+    # cache roles
     $data->{roles}->{$path} = $roles;
 
+    # derive privs from set roles - hash mapping privilege name to propagation
+    # flag value, a key being defined means the priv is set
     my $privs = {};
     foreach my $role (keys %$roles) {
 	if (my $privset = $cfg->{roles}->{$role}) {
 	    foreach my $p (keys %$privset) {
+		# set priv '$p' to propagated iff any of the set roles
+		# containing it have the propagated flag set
 		$privs->{$p} ||= $roles->{$role};
 	    }
 	}
     }
 
+    # intersect user and token permissions
     if ($username && $username ne 'root@pam') {
-	# intersect user and token permissions
+	# map of set privs to their propagation flag value, for the owning user
 	my $user_privs = $cache->{$username}->{privs}->{$path};
+	# list of privs set both for token and owning user
 	my $filtered_privs = [ grep { defined($user_privs->{$_}) } keys %$privs ];
+	# intersection of privs using filtered list, combining both propagation
+	# flags
 	$privs = { map { $_ => $user_privs->{$_} && $privs->{$_} } @$filtered_privs };
     }
 
@@ -91,11 +111,27 @@ my $compile_acl_path = sub {
 	delete $privs->{$priv} if !defined($privs->{$priv});
     }
 
+    # cache privs
     $data->{privs}->{$path} = $privs;
 
     return $privs;
 };
 
+# this is the method used by permission check helpers below
+#
+# returned value is a hash mapping all set privileges on $path to their
+# respective propagation flag. the propagation flag is informational only -
+# actual propagation is handled in PVE::AccessControl::roles(). to determine
+# whether a privilege is set, check for definedness in the returned hash.
+#
+# compiled ACLs are cached, so repeated checks for the same path and user are
+# almost free.
+#
+# if $user is a tokenid, permissions are calculated depending on the
+# privilege-separation flag value:
+# - non-priv-separated: permissions for owning user are returned
+# - priv-separated: permissions for owning user are calculated and intersected
+#   with those of token
 sub permissions {
     my ($self, $user, $path) = @_;
 
-- 
2.30.2

[pve-devel] applied series: [PATCH access-control 0/3] fix two propagation related bugs

[Thomas Lamprecht]

Am 03/06/2022 um 13:50 schrieb Fabian Grünbichler:
> these patches fix two related bugs:
> - the propagation flag used for priv dumping was set randomly if two
>   roles with a common priv exist on a path, one with and one without
>   propagation
> - user/token priv intersection only took user privs into account that
>   had propagation set
> 
> the first can affect the second one negatively (if the first bug causes
> the propagation flag to be dropped, the second one will drop the priv
> from the merged set of privileges for priv-separated tokens).
> 
> in both cases there is no possibility to elevate privileges:
> - bug #1 sometimes marks privs as non-propagated that are, but only for
>   display, not for checking purposes
> - bug #2 causes a token to have less privileges than it should, not more
> 
> Fabian Grünbichler (3):
>   permissions: properly merge propagation flag
>   permissions: fix token/user priv intersection
>   permissions: add some more comments
> 
>  src/PVE/RPCEnvironment.pm | 44 +++++++++++++++++++++++++++++++++++----
>  src/test/perm-test8.pl    |  2 +-
>  src/test/test8.cfg        |  2 ++
>  3 files changed, 43 insertions(+), 5 deletions(-)
> 


applied series, thanks!