From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 18F186BBC6 for ; Thu, 18 Mar 2021 10:45:32 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 05825EF0D for ; Thu, 18 Mar 2021 10:45:02 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id DB0C9EF00 for ; Thu, 18 Mar 2021 10:44:59 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id A6EA3458DE for ; Thu, 18 Mar 2021 10:44:59 +0100 (CET) From: Fabian Ebner To: pve-devel@lists.proxmox.com Date: Thu, 18 Mar 2021 10:44:51 +0100 Message-Id: <20210318094452.738-3-f.ebner@proxmox.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20210318094452.738-1-f.ebner@proxmox.com> References: <20210318094452.738-1-f.ebner@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.047 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment PROLO_LEO1 0.1 Meta Catches all Leo drug variations so far RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [qemuserver.pm, script.sh] Subject: [pve-devel] [PATCH v2 qemu-server 3/4] restore: sanitize config for non-root users X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 18 Mar 2021 09:45:32 -0000 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 Signed-off-by: Fabian Ebner --- 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