Re: [pve-devel] [PATCH qemu-server 2/2] restore: sanitize config for non-root users

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 08.03.21 13:26, Fabian Ebner wrote:
> by dropping privileged options for unprivileged users. For better backwards
> compatibility for in-place restores, keep the option if the value didn't change.
> 
> Note that this softly "breaks" restoring a backup with such a privileged option
> under a new VM ID in the sense that the options won't be present in the new VM
> configuration. Restoring itself still works. Note that restoring containers
> behaves similarly.
> 
> In a trusted environment, there cannot be any backups that were tampered with,
> but it's still worth adding such checks for resilience and future-proofing.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/QemuServer.pm | 71 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 71 insertions(+)
> 

is this covered by some tests already?

> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 1410ecb..1d74ee2 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -5875,6 +5875,67 @@ my $restore_allocate_devices = sub {
>      return $map;
>  };
>  
> +# Make sure user is allowed to have options in config.
> +my $sanitize_restored_config = sub {
> +    my ($new_config_raw, $oldconf, $authuser) = @_;
> +
> +    return $new_config_raw if $authuser eq 'root@pam';
> +
> +    my $res = '';
> +    $oldconf //= {};
> +
> +    # serialN, usbN, etc. handled below
> +    my $rootonlyoptions = {
> +	args => 1,
> +	lock => 1,
> +	parent => 1,
> +	hookscript => 1,
> +    };
> +
> +    # anything other than 'none' and volume IDs are disallowed here
> +    my $is_bad_drive = sub {
> +	my ($key, $value) = @_;
> +	my $drive = parse_drive($key, $value) // {};
> +	my $volid = $drive->{file} // '';
> +	return 0 if $volid eq 'none';
> +	return 1 if $volid eq 'cdrom'; # disallow physical CD/DVD drive
> +	$volid = PVE::Storage::parse_volume_id($volid, 1);
> +	return 1 if !defined($volid);
> +    };
> +
> +    my @lines = split(/\n/, $new_config_raw);
> +    foreach my $line (@lines) {
> +	if ($line =~ m/^#/) {
> +	    $res .= "$line\n";
> +	} elsif ($line =~ m/^([a-z][a-z_]*\d*):\s*(.+?)\s*$/) {
> +	    my $key = $1;
> +	    my $value = $2;
> +	    my $oldvalue = $oldconf->{$key};
> +
> +	    if (defined($oldvalue) && $oldvalue eq $value) {

To work a bit better this would need to normalize values for comparison, as for
example, one can have a boolean option as '1' or 'on', IIRC, and format strings
may have changed order, so the equal check may fail even if they are
semantically equal.

Not sure how often this can occur in practice as we control the outgoing parser,
especially as this only matters for $rootonlyoptions, but did you thought/checked
this already?

> +		$res .= "$line\n";
> +		next;
> +	    }
> +
> +	    if ($rootonlyoptions->{$key} ||
> +		(is_valid_drivename($key) && $is_bad_drive->($key, $value)) ||
> +		($key =~ m/^serial\d+$/ && $value ne 'socket') ||
> +		($key =~ m/^usb\d+$/ && $value !~ m/spice/) ||
> +		$key =~ m/^parallel\d+$/ ||
> +		$key =~ m/^hostpci\d+$/) {
> +		warn "WARNING: SKIPPING CONFIGURATION LINE '$line'. " .
> +		    "Restore as root to include it.\n";

I'd tone down the capslock a bit, eg.:

warn "WARN: skip restoring line '$line' due to privilege restrictions"
    ." - restore as root to include it\n"

ideally we set the warning count for tasks in the restore worker, like PBS does, so that
this shows up in the gui as "orange" task with some warnings in the task list.

> +	    } else {
> +		$res .= "$line\n";
> +	    }
> +	} else {
> +	    warn "WARNING: INVALID CONFIGURATION LINE '$line'.\n";

I'd tone down capslock here too and rather see how worker task warnings
could be set.

> +	}
> +    }
> +
> +    return $res;
> +};
> +
>  my $restore_update_config_line = sub {
>      my ($cookie, $vmid, $map, $line, $unique) = @_;
>  
> @@ -6281,6 +6342,8 @@ sub restore_proxmox_backup_archive {
>  	die $err;
>      }
>  
> +    $new_conf_raw = $sanitize_restored_config->($new_conf_raw, $oldconf, $user);
> +
>      PVE::Tools::file_set_contents($conffile, $new_conf_raw);
>  
>      PVE::Cluster::cfs_update(); # make sure we read new file
> @@ -6494,6 +6557,8 @@ sub restore_vma_archive {
>  	die $err;
>      }
>  
> +    $new_conf_raw = $sanitize_restored_config->($new_conf_raw, $oldconf, $user);
> +
>      PVE::Tools::file_set_contents($conffile, $new_conf_raw);
>  
>      PVE::Cluster::cfs_update(); # make sure we read new file
> @@ -6513,6 +6578,10 @@ sub restore_tar_archive {
>  
>      my $storecfg = PVE::Storage::config();
>  
> +    # Note: $oldconf is undef if VM does not exists
> +    my $cfs_path = PVE::QemuConfig->cfs_config_path($vmid);
> +    my $oldconf = PVE::Cluster::cfs_read_file($cfs_path);
> +
>      # avoid zombie disks when restoring over an existing VM -> cleanup first
>      # pass keep_empty_config=1 to keep the config (thus VMID) reserved for us
>      # skiplock=1 because qmrestore has set the 'create' lock itself already
> @@ -6603,6 +6672,8 @@ sub restore_tar_archive {
>  
>      rmtree $tmpdir;
>  
> +    $new_conf_raw = $sanitize_restored_config->($new_conf_raw, $oldconf, $user);
> +
>      PVE::Tools::file_set_contents($conffile, $new_conf_raw);
>  
>      PVE::Cluster::cfs_update(); # make sure we read new file
>