Re: [pve-devel] [PATCH v1 qemu-server 5/5] add SuperUser privilege checks for root-only options

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

this needs a rebase anyhow, I reviewed it on HEAD~3 where it still 
applies ;)

there's one check for 'root@pam' that looks okay
same for the one in the move disk API endpoint

it might be worth it to mention that you checked those and not switched 
them to superuser since they are only a shortcut for skipping the 
permission check for root, and not limiting access to some root-only 
action (or maybe even add a comment so that future people looking at it 
don't have to remember that).

On February 8, 2022 2:10 pm, Oguz Bektas wrote:
> analogous to the changes in container.
> 
> we now allow users with SU privilege to edit real device configurations,
> provided that they also have the necessary VM privileges.
> 
> note that root@pam is still able to do everything as usual
> 
> ---
>  PVE/API2/Qemu.pm | 119 +++++++++++++++++++++++++++++------------------
>  1 file changed, 73 insertions(+), 46 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 6992f6f..9d403b4 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -352,7 +352,7 @@ my $cloudinitoptions = {
>  my $check_vm_create_serial_perm = sub {
>      my ($rpcenv, $authuser, $vmid, $pool, $param) = @_;
>  
> -    return 1 if $authuser eq 'root@pam';
> +    return 1 if $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);

nope - this still needs to check VM.Config.HWType in the SuperUser 
case..

e.g., should look like this:

my $check_vm_create_serial_perm = sub {
    my ($rpcenv, $authuser, $vmid, $pool, $param) = @_;

    return 1 if $authuser eq 'root@pam';

    my $is_superuser = $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);

    foreach my $opt (keys %{$param}) {
	next if $opt !~ m/^serial\d+$/;

	$rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.HWType']);
	die "only superusers can set '$opt' config for real devices\n"
	    if !($param->{$opt} eq 'socket' || $is_superuser);
    }

    return 1;
};

>  
>      foreach my $opt (keys %{$param}) {
>  	next if $opt !~ m/^serial\d+$/;
> @@ -370,7 +370,7 @@ my $check_vm_create_serial_perm = sub {
>  my $check_vm_create_usb_perm = sub {
>      my ($rpcenv, $authuser, $vmid, $pool, $param) = @_;
>  
> -    return 1 if $authuser eq 'root@pam';
> +    return 1 if $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);

same here, but with spice instead of socket

>  
>      foreach my $opt (keys %{$param}) {
>  	next if $opt !~ m/^usb\d+$/;
> @@ -388,7 +388,7 @@ my $check_vm_create_usb_perm = sub {
>  my $check_vm_modify_config_perm = sub {
>      my ($rpcenv, $authuser, $vmid, $pool, $key_list) = @_;
>  
> -    return 1 if $authuser eq 'root@pam';
> +    return 1 if $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);

completely wrong, like in pve-container! this effectively skips most 
checks for SuperUser, instead of only skipping the root-only parts..

only the else part of $check_vm_modify_config_perm should check for 
superuser and die otherwise..

>  
>      foreach my $opt (@$key_list) {
>  	# some checks (e.g., disk, serial port, usb) need to be done somewhere
> @@ -1117,9 +1117,10 @@ my $update_vm_api  = sub {
>  	push @paramarr, "-$key", $value;
>      }
>  
> +    my $is_superuser = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
>      my $skiplock = extract_param($param, 'skiplock');
> -    raise_param_exc({ skiplock => "Only root may use this option." })
> -	if $skiplock && $authuser ne 'root@pam';
> +    raise_param_exc({ skiplock => "Only superusers may use this option." })
> +	if $skiplock && !$is_superuser;
>  
>      my $delete_str = extract_param($param, 'delete');
>  
> @@ -1340,16 +1341,18 @@ my $update_vm_api  = sub {
>  		} elsif ($opt =~ m/^serial\d+$/) {
>  		    if ($val eq 'socket') {
>  			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
> -		    } elsif ($authuser ne 'root@pam') {
> -			die "only root can delete '$opt' config for real devices\n";
> +		    } elsif (!$is_superuser) {
> +			die "only superusers can delete '$opt' config for real devices\n"
> +			    if !$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);

same as with check_vm_create_serial_perm above..

>  		    }
>  		    PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
>  		    PVE::QemuConfig->write_config($vmid, $conf);
>  		} elsif ($opt =~ m/^usb\d+$/) {
>  		    if ($val =~ m/spice/) {
>  			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
> -		    } elsif ($authuser ne 'root@pam') {
> -			die "only root can delete '$opt' config for real devices\n";
> +		    } elsif (!$is_superuser) {
> +			die "only superusers can delete '$opt' config for real devices\n"
> +			    if !$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);

also same, but with check_vm_create_usb_perm..

>  		    }
>  		    PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
>  		    PVE::QemuConfig->write_config($vmid, $conf);
> @@ -1392,15 +1395,17 @@ my $update_vm_api  = sub {
>  		} elsif ($opt =~ m/^serial\d+/) {
>  		    if ((!defined($conf->{$opt}) || $conf->{$opt} eq 'socket') && $param->{$opt} eq 'socket') {
>  			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
> -		    } elsif ($authuser ne 'root@pam') {
> -			die "only root can modify '$opt' config for real devices\n";
> +		    } elsif (!$is_superuser) {
> +			die "only superuser can modify '$opt' config for real devices\n"
> +			    if !$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);

again

>  		    }
>  		    $conf->{pending}->{$opt} = $param->{$opt};
>  		} elsif ($opt =~ m/^usb\d+/) {
>  		    if ((!defined($conf->{$opt}) || $conf->{$opt} =~ m/spice/) && $param->{$opt} =~ m/spice/) {
>  			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
> -		    } elsif ($authuser ne 'root@pam') {
> -			die "only root can modify '$opt' config for real devices\n";
> +		    } elsif (!$is_superuser) {
> +			die "only superuser can modify '$opt' config for real devices\n"
> +			    if !$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);

again

>  		    }
>  		    $conf->{pending}->{$opt} = $param->{$opt};
>  		} else {
> @@ -1644,9 +1649,11 @@ __PACKAGE__->register_method({
>  	my $authuser = $rpcenv->get_user();
>  	my $vmid = $param->{vmid};
>  
> +	my $is_superuser = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $skiplock = $param->{skiplock};
> -	raise_param_exc({ skiplock => "Only root may use this option." })
> -	    if $skiplock && $authuser ne 'root@pam';
> +	raise_param_exc({ skiplock => "Only superusers may use this option." })
> +	    if $skiplock && !$is_superuser;
>  
>  	my $early_checks = sub {
>  	    # test if VM exists
> @@ -2291,10 +2298,12 @@ __PACKAGE__->register_method({
>  	my $machine = extract_param($param, 'machine');
>  	my $force_cpu = extract_param($param, 'force-cpu');
>  
> +	my $is_superuser = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $get_root_param = sub {
>  	    my $value = extract_param($param, $_[0]);
> -	    raise_param_exc({ "$_[0]" => "Only root may use this option." })
> -		if $value && $authuser ne 'root@pam';
> +	    raise_param_exc({ "$_[0]" => "Only superusers may use this option." })
> +		if $value && !$is_superuser;
>  	    return $value;

this isn't 100% correct either - most of these are migration-internal 
parameters (which are only ever supposed to be set when called via SSH 
via `qm`, which runs as root@pam anyway)

skiplock can switch to being superuser-limited, the rest should stay 
root@pam..

>  	};
>  
> @@ -2436,17 +2445,19 @@ __PACKAGE__->register_method({
>  	my $node = extract_param($param, 'node');
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_superuser = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $skiplock = extract_param($param, 'skiplock');
> -	raise_param_exc({ skiplock => "Only root may use this option." })
> -	    if $skiplock && $authuser ne 'root@pam';
> +	raise_param_exc({ skiplock => "Only superusers may use this option." })
> +	    if $skiplock && !$is_superuser;
>  
>  	my $keepActive = extract_param($param, 'keepActive');
> -	raise_param_exc({ keepActive => "Only root may use this option." })
> -	    if $keepActive && $authuser ne 'root@pam';
> +	raise_param_exc({ keepActive => "Only superusers may use this option." })
> +	    if $keepActive && !$is_superuser;

same here - internal param only set in vzdump context

>  
>  	my $migratedfrom = extract_param($param, 'migratedfrom');
> -	raise_param_exc({ migratedfrom => "Only root may use this option." })
> -	    if $migratedfrom && $authuser ne 'root@pam';
> +	raise_param_exc({ migratedfrom => "Only superusers may use this option." })
> +	    if $migratedfrom && !$is_superuser;

and this one is again only set when stopping the target VM during 
migration (via ssh, via qm, so always root).

>  
>  
>  	my $storecfg = PVE::Storage::config();
> @@ -2513,9 +2524,11 @@ __PACKAGE__->register_method({
>  
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_superuser = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $skiplock = extract_param($param, 'skiplock');
> -	raise_param_exc({ skiplock => "Only root may use this option." })
> -	    if $skiplock && $authuser ne 'root@pam';
> +	raise_param_exc({ skiplock => "Only superusers may use this option." })
> +	    if $skiplock && !$is_superuser;
>  
>  	die "VM $vmid not running\n" if !PVE::QemuServer::check_running($vmid);
>  
> @@ -2580,13 +2593,15 @@ __PACKAGE__->register_method({
>  	my $node = extract_param($param, 'node');
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_superuser = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $skiplock = extract_param($param, 'skiplock');
> -	raise_param_exc({ skiplock => "Only root may use this option." })
> -	    if $skiplock && $authuser ne 'root@pam';
> +	raise_param_exc({ skiplock => "Only superusers may use this option." })
> +	    if $skiplock && !$is_superuser;
>  
>  	my $keepActive = extract_param($param, 'keepActive');
> -	raise_param_exc({ keepActive => "Only root may use this option." })
> -	    if $keepActive && $authuser ne 'root@pam';
> +	raise_param_exc({ keepActive => "Only superusers may use this option." })
> +	    if $keepActive && !$is_superuser;

same as above - AFAICT this is vzdump only..

>  
>  	my $storecfg = PVE::Storage::config();
>  
> @@ -2739,9 +2754,11 @@ __PACKAGE__->register_method({
>  
>  	my $statestorage = extract_param($param, 'statestorage');
>  
> +	my $is_superuser = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $skiplock = extract_param($param, 'skiplock');
> -	raise_param_exc({ skiplock => "Only root may use this option." })
> -	    if $skiplock && $authuser ne 'root@pam';
> +	raise_param_exc({ skiplock => "Only superusers may use this option." })
> +	    if $skiplock && !$is_superuser;
>  
>  	die "VM $vmid not running\n" if !PVE::QemuServer::check_running($vmid);
>  
> @@ -2811,13 +2828,15 @@ __PACKAGE__->register_method({
>  
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_superuser = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $skiplock = extract_param($param, 'skiplock');
> -	raise_param_exc({ skiplock => "Only root may use this option." })
> -	    if $skiplock && $authuser ne 'root@pam';
> +	raise_param_exc({ skiplock => "Only superusers may use this option." })
> +	    if $skiplock && !$is_superuser;
>  
>  	my $nocheck = extract_param($param, 'nocheck');
> -	raise_param_exc({ nocheck => "Only root may use this option." })
> -	    if $nocheck && $authuser ne 'root@pam';
> +	raise_param_exc({ nocheck => "Only superusers may use this option." })
> +	    if $nocheck && !$is_superuser;

this is also migration only (for resuming while the config is still on 
the source node)

>  
>  	my $to_disk_suspended;
>  	eval {
> @@ -2883,9 +2902,11 @@ __PACKAGE__->register_method({
>  
>  	my $vmid = extract_param($param, 'vmid');
>  
> +	my $is_superuser = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $skiplock = extract_param($param, 'skiplock');
> -	raise_param_exc({ skiplock => "Only root may use this option." })
> -	    if $skiplock && $authuser ne 'root@pam';
> +	raise_param_exc({ skiplock => "Only superusers may use this option." })
> +	    if $skiplock && !$is_superuser;
>  
>  	PVE::QemuServer::vm_sendkey($vmid, $skiplock, $param->{key});
>  
> @@ -3392,6 +3413,8 @@ __PACKAGE__->register_method({
>  
>  	my $storecfg = PVE::Storage::config();
>  
> +	my $is_superuser = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);

not used?

> +
>  	my $move_updatefn =  sub {
>  	    my $conf = PVE::QemuConfig->load_config($vmid);
>  	    PVE::QemuConfig->check_lock($conf);
> @@ -3856,7 +3879,7 @@ __PACKAGE__->register_method({
>  	    },
>  	    force => {
>  		type => 'boolean',
> -		description => "Allow to migrate VMs which use local devices. Only root may use this option.",
> +		description => "Allow to migrate VMs which use local devices. Only superusers may use this option.",
>  		optional => 1,
>  	    },
>  	    migration_type => {
> @@ -3910,15 +3933,17 @@ __PACKAGE__->register_method({
>  
>  	my $vmid = extract_param($param, 'vmid');
>  
> -	raise_param_exc({ force => "Only root may use this option." })
> -	    if $param->{force} && $authuser ne 'root@pam';
> +	my $is_superuser = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
>  
> -	raise_param_exc({ migration_type => "Only root may use this option." })
> -	    if $param->{migration_type} && $authuser ne 'root@pam';
> +	raise_param_exc({ force => "Only superusers may use this option." })
> +	    if $param->{force} && !$is_superuser;
> +
> +	raise_param_exc({ migration_type => "Only superusers may use this option." })
> +	    if $param->{migration_type} && !$is_superuser;
>  
>  	# allow root only until better network permissions are available
> -	raise_param_exc({ migration_network => "Only root may use this option." })
> -	    if $param->{migration_network} && $authuser ne 'root@pam';
> +	raise_param_exc({ migration_network => "Only superusers may use this option." })
> +	    if $param->{migration_network} && !$is_superuser;
>  
>  	# test if VM exists
>  	my $conf = PVE::QemuConfig->load_config($vmid);
> @@ -4098,9 +4123,11 @@ __PACKAGE__->register_method({
>  
>  	my $sizestr = extract_param($param, 'size');
>  
> +	my $is_superuser = $authuser eq 'root@pam' || $rpcenv->check($authuser, "/vms/$vmid", ['SuperUser'], 1);
> +
>  	my $skiplock = extract_param($param, 'skiplock');
> -        raise_param_exc({ skiplock => "Only root may use this option." })
> -            if $skiplock && $authuser ne 'root@pam';
> +        raise_param_exc({ skiplock => "Only superusers may use this option." })
> +            if $skiplock && !$is_superuser;
>  
>          my $storecfg = PVE::Storage::config();
>  
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>