[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