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

Fabian Ebner f.ebner at proxmox.com
Thu Mar 18 10:44:51 CET 2021


by dropping privileged options for unprivileged users. For backwards
compatibility for in-place restores, keep the option as long as 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. Restoring containers already
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.

Reported-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
---

Changes from v1:
    * don't capitalize warnings as much
    * add tests
    * add Reported-by tag

 PVE/QemuServer.pm                           | 71 +++++++++++++++++++++
 test/restore-config-expected/1791.conf.root | 17 +++++
 test/restore-config-expected/1791.conf.user | 12 ++++
 test/restore-config-expected/314.conf       | 22 +++++++
 test/restore-config-expected/3141.conf.root | 24 +++++++
 test/restore-config-expected/3141.conf.user | 16 +++++
 test/restore-config-input/1791.conf         | 17 +++++
 test/restore-config-input/314.conf          | 24 +++++++
 test/restore-config-input/3141.conf         | 28 ++++++++
 test/restore-config-old/142.conf            | 14 ++++
 test/restore-config-old/314.conf            | 21 ++++++
 test/run_qemu_restore_config_tests.pl       | 32 +++++++++-
 12 files changed, 295 insertions(+), 3 deletions(-)
 create mode 100644 test/restore-config-expected/1791.conf.root
 create mode 100644 test/restore-config-expected/1791.conf.user
 create mode 100644 test/restore-config-expected/314.conf
 create mode 100644 test/restore-config-expected/3141.conf.root
 create mode 100644 test/restore-config-expected/3141.conf.user
 create mode 100644 test/restore-config-input/1791.conf
 create mode 100644 test/restore-config-input/314.conf
 create mode 100644 test/restore-config-input/3141.conf
 create mode 100644 test/restore-config-old/142.conf
 create mode 100644 test/restore-config-old/314.conf

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 5fb052c..69f7be2 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5899,6 +5899,67 @@ my $restore_allocate_devices = sub {
     return $map;
 };
 
+# Make sure user is allowed to have options in config.
+sub sanitize_restored_config {
+    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 "WARN: skipping configuration line '$line' due to " .
+		    "privilege restrictions - restore as root to include it.\n";
+	    } else {
+		$res .= "$line\n";
+	    }
+	} else {
+	    warn "WARN: invalid configuration line '$line'.\n";
+	}
+    }
+
+    return $res;
+}
+
 sub restore_update_config_line {
     my ($cookie, $map, $line, $unique) = @_;
 
@@ -6304,6 +6365,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
@@ -6518,6 +6581,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
@@ -6537,6 +6602,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
@@ -6626,6 +6695,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
diff --git a/test/restore-config-expected/1791.conf.root b/test/restore-config-expected/1791.conf.root
new file mode 100644
index 0000000..2c0e9c4
--- /dev/null
+++ b/test/restore-config-expected/1791.conf.root
@@ -0,0 +1,17 @@
+# absolute disk paths, no map
+boot: order=scsi0;ide2;net0
+cores: 1
+ide2: none,media=cdrom
+memory: 2048
+net0: virtio=26:15:5B:73:3F:7C,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi3: /dev/sda1
+efidisk0: /dev/sda2
+sata2: /dev/sdb3
+ide0: /dev/sdc
+virtio3: /dev/sdc2
+scsihw: virtio-scsi-pci
+smbios1: uuid=1819ead7-a55d-4544-8d38-29ca94869a9c
+sockets: 1
+vmgenid: 0
diff --git a/test/restore-config-expected/1791.conf.user b/test/restore-config-expected/1791.conf.user
new file mode 100644
index 0000000..2797397
--- /dev/null
+++ b/test/restore-config-expected/1791.conf.user
@@ -0,0 +1,12 @@
+# absolute disk paths, no map
+boot: order=scsi0;ide2;net0
+cores: 1
+ide2: none,media=cdrom
+memory: 2048
+net0: virtio=26:15:5B:73:3F:7C,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsihw: virtio-scsi-pci
+smbios1: uuid=1819ead7-a55d-4544-8d38-29ca94869a9c
+sockets: 1
+vmgenid: 0
diff --git a/test/restore-config-expected/314.conf b/test/restore-config-expected/314.conf
new file mode 100644
index 0000000..129fc4d
--- /dev/null
+++ b/test/restore-config-expected/314.conf
@@ -0,0 +1,22 @@
+# many privileged options
+args: -hda /dev/sdk9 -version
+boot: order=scsi0;ide2;net0
+cores: 1
+ide0: none,media=cdrom
+ide2: cdrom,media=cdrom
+memory: 2048
+net0: virtio=AE:E8:DD:4A:1D:7A,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi0: target:314/vm-314-disk-0.qcow2,size=4G
+scsi1: target:314/vm-314-disk-1.raw
+scsihw: virtio-scsi-pci
+serial0: socket
+hostpci2: 01:00.0,pcie=1
+hostpci3: 02:00,pcie=1,x-vga=on
+serial1: /dev/ttyS0
+smbios1: uuid=f2c56d4e-47da-4c23-97f9-6335d5631e90
+sockets: 1
+usb0: spice,usb3=1
+usb1: host=0627:0001,usb3=1
+vmgenid: 0
diff --git a/test/restore-config-expected/3141.conf.root b/test/restore-config-expected/3141.conf.root
new file mode 100644
index 0000000..4445b08
--- /dev/null
+++ b/test/restore-config-expected/3141.conf.root
@@ -0,0 +1,24 @@
+# privileged options no in-place restore
+args: -hda /dev/sdk9 -version
+boot: order=scsi0;ide2;net0
+cores: 1
+ide0: none,media=cdrom
+ide2: cdrom,media=cdrom
+memory: 2048
+net0: virtio=AE:E8:DD:4A:1D:7A,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi0: target:3141/vm-3141-disk-0.qcow2,size=4G
+scsi1: target:3141/vm-3141-disk-1.raw
+scsihw: virtio-scsi-pci
+serial0: socket
+hostpci2: 01:00.0,pcie=1
+hostpci3: 02:00,pcie=1,x-vga=on
+serial1: /dev/ttyS0
+smbios1: uuid=f2c56d4e-47da-4c23-97f9-6335d5631e90
+hookscript: /tmp/script.sh
+sockets: 1
+parallel0: /dev/parport3
+usb0: spice,usb3=1
+usb1: host=0627:0001,usb3=1
+vmgenid: 0
diff --git a/test/restore-config-expected/3141.conf.user b/test/restore-config-expected/3141.conf.user
new file mode 100644
index 0000000..8b5dc97
--- /dev/null
+++ b/test/restore-config-expected/3141.conf.user
@@ -0,0 +1,16 @@
+# privileged options no in-place restore
+boot: order=scsi0;ide2;net0
+cores: 1
+ide0: none,media=cdrom
+memory: 2048
+net0: virtio=AE:E8:DD:4A:1D:7A,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi0: target:3141/vm-3141-disk-0.qcow2,size=4G
+scsi1: target:3141/vm-3141-disk-1.raw
+scsihw: virtio-scsi-pci
+serial0: socket
+smbios1: uuid=f2c56d4e-47da-4c23-97f9-6335d5631e90
+sockets: 1
+usb0: spice,usb3=1
+vmgenid: 0
diff --git a/test/restore-config-input/1791.conf b/test/restore-config-input/1791.conf
new file mode 100644
index 0000000..2c0e9c4
--- /dev/null
+++ b/test/restore-config-input/1791.conf
@@ -0,0 +1,17 @@
+# absolute disk paths, no map
+boot: order=scsi0;ide2;net0
+cores: 1
+ide2: none,media=cdrom
+memory: 2048
+net0: virtio=26:15:5B:73:3F:7C,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi3: /dev/sda1
+efidisk0: /dev/sda2
+sata2: /dev/sdb3
+ide0: /dev/sdc
+virtio3: /dev/sdc2
+scsihw: virtio-scsi-pci
+smbios1: uuid=1819ead7-a55d-4544-8d38-29ca94869a9c
+sockets: 1
+vmgenid: 0
diff --git a/test/restore-config-input/314.conf b/test/restore-config-input/314.conf
new file mode 100644
index 0000000..7307e50
--- /dev/null
+++ b/test/restore-config-input/314.conf
@@ -0,0 +1,24 @@
+# many privileged options
+args: -hda /dev/sdk9 -version
+boot: order=scsi0;ide2;net0
+cores: 1
+ide0: none,media=cdrom
+ide2: cdrom,media=cdrom
+memory: 2048
+net0: virtio=AE:E8:DD:4A:1D:7A,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi0: mydir:314/vm-314-disk-0.qcow2,size=4G
+scsi1: /dev/sdl9
+scsihw: virtio-scsi-pci
+serial0: socket
+hostpci2: 01:00.0,pcie=1
+hostpci3: 02:00,pcie=1,x-vga=on
+serial1: /dev/ttyS0
+smbios1: uuid=f2c56d4e-47da-4c23-97f9-6335d5631e90
+sockets: 1
+usb0: spice,usb3=1
+usb1: host=0627:0001,usb3=1
+vmgenid: 0
+#qmdump#map:scsi0:drive-scsi0:mydir:qcow2:
+#qmdump#map:scsi1:drive-scsi1::raw:
diff --git a/test/restore-config-input/3141.conf b/test/restore-config-input/3141.conf
new file mode 100644
index 0000000..d61a502
--- /dev/null
+++ b/test/restore-config-input/3141.conf
@@ -0,0 +1,28 @@
+# privileged options no in-place restore
+args: -hda /dev/sdk9 -version
+boot: order=scsi0;ide2;net0
+cores: 1
+ide0: none,media=cdrom
+ide2: cdrom,media=cdrom
+memory: 2048
+net0: virtio=AE:E8:DD:4A:1D:7A,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi0: mydir:3141/vm-3141-disk-0.qcow2,size=4G
+scsi1: /dev/sdl9
+scsihw: virtio-scsi-pci
+serial0: socket
+hostpci2: 01:00.0,pcie=1
+hostpci3: 02:00,pcie=1,x-vga=on
+serial1: /dev/ttyS0
+smbios1: uuid=f2c56d4e-47da-4c23-97f9-6335d5631e90
+hookscript: /tmp/script.sh
+sockets: 1
+parallel0: /dev/parport3
+usb0: spice,usb3=1
+usb1: host=0627:0001,usb3=1
+lock: backup
+parent: snap
+vmgenid: 0
+#qmdump#map:scsi0:drive-scsi0:mydir:qcow2:
+#qmdump#map:scsi1:drive-scsi1::raw:
diff --git a/test/restore-config-old/142.conf b/test/restore-config-old/142.conf
new file mode 100644
index 0000000..b39b8a4
--- /dev/null
+++ b/test/restore-config-old/142.conf
@@ -0,0 +1,14 @@
+# plain VM
+bootdisk: scsi0
+cores: 2
+ide2: none,media=cdrom
+memory: 2048
+name: apachenew
+net0: virtio=92:38:11:FD:ED:87,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi0: mydir:142/vm-142-disk-1.qcow2,size=4G
+scsihw: virtio-scsi-pci
+smbios1: uuid=ddf91b3f-a597-42be-9a7e-fb6421dcd5cd
+sockets: 1
+vmgenid: a0779fce-f7bc-4d9a-af06-e2fce88659ba
diff --git a/test/restore-config-old/314.conf b/test/restore-config-old/314.conf
new file mode 100644
index 0000000..42ad8aa
--- /dev/null
+++ b/test/restore-config-old/314.conf
@@ -0,0 +1,21 @@
+args: -hda /dev/sdk9 -version
+boot: order=scsi0;ide2;net0
+cores: 1
+ide0: none,media=cdrom
+ide2: cdrom,media=cdrom
+memory: 2048
+net0: virtio=AE:E8:DD:4A:1D:7A,bridge=vmbr0,firewall=1
+numa: 0
+ostype: l26
+scsi0: target:314/vm-314-disk-0.qcow2,size=4G
+scsi1: /dev/sdl9
+scsihw: virtio-scsi-pci
+serial0: socket
+serial1: /dev/ttyS0
+hostpci2: 01:00.0,pcie=1
+hostpci3: 02:00,pcie=1,x-vga=on
+smbios1: uuid=f2c56d4e-47da-4c23-97f9-6335d5631e90
+sockets: 1
+usb0: spice,usb3=1
+usb1: host=0627:0001,usb3=1
+vmgenid: 3166c634-c2cf-492f-aa55-c8f757adc218
diff --git a/test/run_qemu_restore_config_tests.pl b/test/run_qemu_restore_config_tests.pl
index e5d9f2a..789711a 100755
--- a/test/run_qemu_restore_config_tests.pl
+++ b/test/run_qemu_restore_config_tests.pl
@@ -13,10 +13,13 @@ use PVE::QemuServer;
 use PVE::Tools qw(dir_glob_foreach file_get_contents);
 
 my $INPUT_DIR = './restore-config-input';
+my $OLD_DIR = './restore-config-old';
 my $EXPECTED_DIR = './restore-config-expected';
 
 # NOTE update when you add/remove tests
-plan tests => 4;
+my $NUMBER_OF_FILES = 7;
+
+plan tests => 2 * $NUMBER_OF_FILES;
 
 dir_glob_foreach('./restore-config-input', '[0-9]+.conf', sub {
     my ($file) = @_;
@@ -58,9 +61,32 @@ dir_glob_foreach('./restore-config-input', '[0-9]+.conf', sub {
 	);
     }
 
-    my $expected = file_get_contents("${EXPECTED_DIR}/${file}");
+    my $old_conf;
+    if (-f "${OLD_DIR}/${file}") {
+	my $old_raw = file_get_contents("${OLD_DIR}/${file}");
+	$old_conf = PVE::QemuServer::parse_vm_config("/qemu-server/${vmid}.conf", $old_raw);
+    }
+
+    my $got_root = PVE::QemuServer::sanitize_restored_config($got, $old_conf, 'root at pam');
+    my $got_user = PVE::QemuServer::sanitize_restored_config($got, $old_conf, 'user at pve');
+
+    my $expected_root;
+    my $expected_user;
+
+    my $expected_file = "${EXPECTED_DIR}/${file}";
+
+    if (-f "${expected_file}.root" && -f "${expected_file}.user") {
+	$expected_root = file_get_contents("${expected_file}.root");
+	$expected_user = file_get_contents("${expected_file}.user");
+    } elsif (-f $expected_file) {
+	$expected_root = file_get_contents($expected_file);
+	$expected_user = $expected_root;
+    } else {
+	die "either provide ID.conf or both ID.conf.root and ID.conf.user\n";
+    }
 
-    is_deeply($got, $expected, $file);
+    is_deeply($got_root, $expected_root, $file);
+    is_deeply($got_user, $expected_user, $file);
 });
 
 done_testing();
-- 
2.20.1






More information about the pve-devel mailing list