[pve-devel] [PATCH qemu-server 1/2] restore: write new config to variable first

[pve-devel] [PATCH qemu-server 1/2] restore: write new config to variable first

[Fabian Ebner]

and use file_set_contents to really commit it afterwards. Mostly done as a
preparation for the later patch for sanitizing the config on restore, but
shouldn't hurt by itself either.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm | 83 +++++++++++++++++++++++++----------------------
 1 file changed, 45 insertions(+), 38 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 0ac4fcf..1410ecb 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5876,13 +5876,15 @@ my $restore_allocate_devices = sub {
 };
 
 my $restore_update_config_line = sub {
-    my ($outfd, $cookie, $vmid, $map, $line, $unique) = @_;
+    my ($cookie, $vmid, $map, $line, $unique) = @_;
 
-    return if $line =~ m/^\#qmdump\#/;
-    return if $line =~ m/^\#vzdump\#/;
-    return if $line =~ m/^lock:/;
-    return if $line =~ m/^unused\d+:/;
-    return if $line =~ m/^parent:/;
+    return '' if $line =~ m/^\#qmdump\#/;
+    return '' if $line =~ m/^\#vzdump\#/;
+    return '' if $line =~ m/^lock:/;
+    return '' if $line =~ m/^unused\d+:/;
+    return '' if $line =~ m/^parent:/;
+
+    my $res = '';
 
     my $dc = PVE::Cluster::cfs_read_file('datacenter.cfg');
     if (($line =~ m/^(vlan(\d+)):\s*(\S+)\s*$/)) {
@@ -5898,7 +5900,7 @@ my $restore_update_config_line = sub {
 	    };
 	    my $netstr = print_net($net);
 
-	    print $outfd "net$cookie->{netcount}: $netstr\n";
+	    $res .= "net$cookie->{netcount}: $netstr\n";
 	    $cookie->{netcount}++;
 	}
     } elsif (($line =~ m/^(net\d+):\s*(\S+)\s*$/) && $unique) {
@@ -5906,20 +5908,20 @@ my $restore_update_config_line = sub {
 	my $net = parse_net($netstr);
 	$net->{macaddr} = PVE::Tools::random_ether_addr($dc->{mac_prefix}) if $net->{macaddr};
 	$netstr = print_net($net);
-	print $outfd "$id: $netstr\n";
+	$res .= "$id: $netstr\n";
     } elsif ($line =~ m/^((ide|scsi|virtio|sata|efidisk)\d+):\s*(\S+)\s*$/) {
 	my $virtdev = $1;
 	my $value = $3;
 	my $di = parse_drive($virtdev, $value);
 	if (defined($di->{backup}) && !$di->{backup}) {
-	    print $outfd "#$line";
+	    $res .= "#$line";
 	} elsif ($map->{$virtdev}) {
 	    delete $di->{format}; # format can change on restore
 	    $di->{file} = $map->{$virtdev};
 	    $value = print_drive($di);
-	    print $outfd "$virtdev: $value\n";
+	    $res .= "$virtdev: $value\n";
 	} else {
-	    print $outfd $line;
+	    $res .= $line;
 	}
     } elsif (($line =~ m/^vmgenid: (.*)/)) {
 	my $vmgenid = $1;
@@ -5927,17 +5929,19 @@ my $restore_update_config_line = sub {
 	    # always generate a new vmgenid if there was a valid one setup
 	    $vmgenid = generate_uuid();
 	}
-	print $outfd "vmgenid: $vmgenid\n";
+	$res .= "vmgenid: $vmgenid\n";
     } elsif (($line =~ m/^(smbios1: )(.*)/) && $unique) {
 	my ($uuid, $uuid_str);
 	UUID::generate($uuid);
 	UUID::unparse($uuid, $uuid_str);
 	my $smbios1 = parse_smbios1($2);
 	$smbios1->{uuid} = $uuid_str;
-	print $outfd $1.print_smbios1($smbios1)."\n";
+	$res .= $1.print_smbios1($smbios1)."\n";
     } else {
-	print $outfd $line;
+	$res .= $line;
     }
+
+    return $res;
 };
 
 my $restore_deactivate_volumes = sub {
@@ -6141,7 +6145,6 @@ sub restore_proxmox_backup_archive {
     mkpath $tmpdir;
 
     my $conffile = PVE::QemuConfig->config_file($vmid);
-    my $tmpfn = "$conffile.$$.tmp";
      # disable interrupts (always do cleanups)
     local $SIG{INT} =
 	local $SIG{TERM} =
@@ -6151,6 +6154,7 @@ sub restore_proxmox_backup_archive {
     # 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);
+    my $new_conf_raw = '';
 
     my $rpcenv = PVE::RPCEnvironment::get();
     my $devinfo = {};
@@ -6253,15 +6257,18 @@ sub restore_proxmox_backup_archive {
 
 	$fh->seek(0, 0) || die "seek failed - $!\n";
 
-	my $outfd = IO::File->new($tmpfn, "w") || die "unable to write config for VM $vmid\n";
-
 	my $cookie = { netcount => 0 };
 	while (defined(my $line = <$fh>)) {
-	    $restore_update_config_line->($outfd, $cookie, $vmid, $map, $line, $options->{unique});
+	    $new_conf_raw .= $restore_update_config_line->(
+		$cookie,
+		$vmid,
+		$map,
+		$line,
+		$options->{unique},
+	    );
 	}
 
 	$fh->close();
-	$outfd->close();
     };
     my $err = $@;
 
@@ -6270,13 +6277,11 @@ sub restore_proxmox_backup_archive {
     rmtree $tmpdir;
 
     if ($err) {
-	unlink $tmpfn;
 	$restore_destroy_volumes->($storecfg, $devinfo);
 	die $err;
     }
 
-    rename($tmpfn, $conffile) ||
-	die "unable to commit configuration file '$conffile'\n";
+    PVE::Tools::file_set_contents($conffile, $new_conf_raw);
 
     PVE::Cluster::cfs_update(); # make sure we read new file
 
@@ -6351,11 +6356,11 @@ sub restore_vma_archive {
     my $rpcenv = PVE::RPCEnvironment::get();
 
     my $conffile = PVE::QemuConfig->config_file($vmid);
-    my $tmpfn = "$conffile.$$.tmp";
 
     # Note: $oldconf is undef if VM does not exist
     my $cfs_path = PVE::QemuConfig->cfs_config_path($vmid);
     my $oldconf = PVE::Cluster::cfs_read_file($cfs_path);
+    my $new_conf_raw = '';
 
     my %storage_limits;
 
@@ -6423,15 +6428,18 @@ sub restore_vma_archive {
 
 	$fh->seek(0, 0) || die "seek failed - $!\n";
 
-	my $outfd = IO::File->new($tmpfn, "w") || die "unable to write config for VM $vmid\n";
-
 	my $cookie = { netcount => 0 };
 	while (defined(my $line = <$fh>)) {
-	    $restore_update_config_line->($outfd, $cookie, $vmid, $map, $line, $opts->{unique});
+	    $new_conf_raw .= $restore_update_config_line->(
+		$cookie,
+		$vmid,
+		$map,
+		$line,
+		$opts->{unique},
+	    );
 	}
 
 	$fh->close();
-	$outfd->close();
     };
 
     eval {
@@ -6482,13 +6490,11 @@ sub restore_vma_archive {
     rmtree $tmpdir;
 
     if ($err) {
-	unlink $tmpfn;
 	$restore_destroy_volumes->($cfg, $devinfo);
 	die $err;
     }
 
-    rename($tmpfn, $conffile) ||
-	die "unable to commit configuration file '$conffile'\n";
+    PVE::Tools::file_set_contents($conffile, $new_conf_raw);
 
     PVE::Cluster::cfs_update(); # make sure we read new file
 
@@ -6533,7 +6539,7 @@ sub restore_tar_archive {
     local $ENV{VZDUMP_USER} = $user;
 
     my $conffile = PVE::QemuConfig->config_file($vmid);
-    my $tmpfn = "$conffile.$$.tmp";
+    my $new_conf_raw = '';
 
     # disable interrupts (always do cleanups)
     local $SIG{INT} =
@@ -6577,26 +6583,27 @@ sub restore_tar_archive {
 
 	my $srcfd = IO::File->new($confsrc, "r") || die "unable to open file '$confsrc'\n";
 
-	my $outfd = IO::File->new($tmpfn, "w") || die "unable to write config for VM $vmid\n";
-
 	my $cookie = { netcount => 0 };
 	while (defined (my $line = <$srcfd>)) {
-	    $restore_update_config_line->($outfd, $cookie, $vmid, $map, $line, $opts->{unique});
+	    $new_conf_raw .= $restore_update_config_line->(
+		$cookie,
+		$vmid,
+		$map,
+		$line,
+		$opts->{unique},
+	    );
 	}
 
 	$srcfd->close();
-	$outfd->close();
     };
     if (my $err = $@) {
-	unlink $tmpfn;
 	tar_restore_cleanup($storecfg, "$tmpdir/qmrestore.stat") if !$opts->{info};
 	die $err;
     }
 
     rmtree $tmpdir;
 
-    rename $tmpfn, $conffile ||
-	die "unable to commit configuration file '$conffile'\n";
+    PVE::Tools::file_set_contents($conffile, $new_conf_raw);
 
     PVE::Cluster::cfs_update(); # make sure we read new file
 
-- 
2.20.1

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

[Fabian Ebner]

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(+)

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) {
+		$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";
+	    } else {
+		$res .= "$line\n";
+	    }
+	} else {
+	    warn "WARNING: INVALID CONFIGURATION LINE '$line'.\n";
+	}
+    }
+
+    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
-- 
2.20.1

[pve-devel] applied: [PATCH qemu-server 1/2] restore: write new config to variable first

[Thomas Lamprecht]

On 08.03.21 13:26, Fabian Ebner wrote:
> and use file_set_contents to really commit it afterwards. Mostly done as a
> preparation for the later patch for sanitizing the config on restore, but
> shouldn't hurt by itself either.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/QemuServer.pm | 83 +++++++++++++++++++++++++----------------------
>  1 file changed, 45 insertions(+), 38 deletions(-)
> 
>

applied, thanks!

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

[Thomas Lamprecht]

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
> 

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

[Fabian Ebner]

On 09.03.21 08:17, Thomas Lamprecht wrote:
> 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?
> 

Haven't seen any. I can try and add some tests that mirror the 
config-related behavior of the restore_XYZ_archive functions (directly 
testing those doesn't seem feasible to me at the moment, lots of 
PBS/VMA+pipes interaction...)

>> 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?
> 

I did briefly think about it, but decided it wasn't worth the extra 
complexity of parsing and deeply comparing everything. It's not only the 
$rootonlyoptions though, e.g.
     usb0: host=1-1,usb3=1
and
     usb0: usb3=1,host=1-1
would also be affected.

For containers, we don't even look at the current values at all, but 
always drop the 'lxc' options for a non-root user restore. But if you 
think it's worth it, I can try and add that.

>> +		$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.
> 

I wasn't aware of this feature. I suppose we should do that for the 
skipped options for LXC too then.

>> +	    } 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.
> 

Ok.

>> +	}
>> +    }
>> +
>> +    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
>>
> 

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

[Thomas Lamprecht]

On 09.03.21 11:08, Fabian Ebner wrote:
> On 09.03.21 08:17, Thomas Lamprecht wrote:
>> On 08.03.21 13:26, Fabian Ebner wrote:
>>
>> is this covered by some tests already?
>>
> 
> Haven't seen any. I can try and add some tests that mirror the config-related behavior of the restore_XYZ_archive functions (directly testing those doesn't seem feasible to me at the moment, lots of PBS/VMA+pipes interaction...)

Yeah, I'd mostly just do unit testing on sanitize_restored_config directly,
although if more can be covered without a bigger extra cost that'd be naturally
great, but not a must.

>>> +        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?
>>
> 
> I did briefly think about it, but decided it wasn't worth the extra complexity of parsing and deeply comparing everything. It's not only the $rootonlyoptions though, e.g.
>    usb0: host=1-1,usb3=1
> and
>    usb0: usb3=1,host=1-1
> would also be affected.
> 
> For containers, we don't even look at the current values at all, but always drop the 'lxc' options for a non-root user restore. But if you think 
it's worth it, I can try and add that.

hmm, if the output of the config is somewhat stable then I', ok with simple
for now..

>>> +        $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.
>>
> 
> I wasn't aware of this feature. I suppose we should do that for the skipped options for LXC too then.
> 

It's relatively new and introduced in PBS only, so there may be some catch up work to do
in pve-common, but GUI support should be universal.