[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 at 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 at 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