From: Fabian Ebner <f.ebner@proxmox.com>
To: pve-devel@lists.proxmox.com
Subject: [pve-devel] [PATCH v2 qemu-server 3/4] restore: sanitize config for non-root users
Date: Thu, 18 Mar 2021 10:44:51 +0100 [thread overview]
Message-ID: <20210318094452.738-3-f.ebner@proxmox.com> (raw)
In-Reply-To: <20210318094452.738-1-f.ebner@proxmox.com>
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@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@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@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@pam');
+ my $got_user = PVE::QemuServer::sanitize_restored_config($got, $old_conf, 'user@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
next prev parent reply other threads:[~2021-03-18 9:45 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-18 9:44 [pve-devel] [PATCH v2 qemu-server 1/4] test: add tests for restoring config Fabian Ebner
2021-03-18 9:44 ` [pve-devel] [PATCH v2 qemu-server 2/4] restore: update config: remove unused parameter Fabian Ebner
2021-04-18 16:15 ` [pve-devel] applied: " Thomas Lamprecht
2021-03-18 9:44 ` Fabian Ebner [this message]
2021-04-18 16:17 ` [pve-devel] [PATCH v2 qemu-server 3/4] restore: sanitize config for non-root users Thomas Lamprecht
2021-03-18 9:44 ` [pve-devel] [PATCH v2 qemu-server 4/4] restore: sanitize config: use new warning system Fabian Ebner
2021-04-18 16:15 ` [pve-devel] applied: [PATCH v2 qemu-server 1/4] test: add tests for restoring config Thomas Lamprecht
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210318094452.738-3-f.ebner@proxmox.com \
--to=f.ebner@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal