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 2D0356B396 for ; Tue, 9 Mar 2021 08:18:03 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 198BD25833 for ; Tue, 9 Mar 2021 08:17:33 +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 BC7922581C for ; Tue, 9 Mar 2021 08:17:31 +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 8260B4581E for ; Tue, 9 Mar 2021 08:17:31 +0100 (CET) Message-ID: <04a999a8-ce86-ac53-e3d5-00f17c11ee81@proxmox.com> Date: Tue, 9 Mar 2021 08:17:30 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:87.0) Gecko/20100101 Thunderbird/87.0 Content-Language: en-US To: Proxmox VE development discussion , Fabian Ebner References: <20210308122658.31781-1-f.ebner@proxmox.com> <20210308122658.31781-2-f.ebner@proxmox.com> From: Thomas Lamprecht In-Reply-To: <20210308122658.31781-2-f.ebner@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.050 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) 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] Subject: Re: [pve-devel] [PATCH qemu-server 2/2] 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: Tue, 09 Mar 2021 07:18:03 -0000 On 08.03.21 13:26, Fabian Ebner wrote: > 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 > --- > PVE/QemuServer.pm | 71 +++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 71 insertions(+) > is this covered by some tests already? > 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@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) { To work a bit better this would need to normalize values for comparison, as for example, one can have a boolean option as '1' or 'on', IIRC, and format strings may have changed order, so the equal check may fail even if they are semantically equal. Not sure how often this can occur in practice as we control the outgoing parser, especially as this only matters for $rootonlyoptions, but did you thought/checked this already? > + $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"; I'd tone down the capslock a bit, eg.: warn "WARN: skip restoring line '$line' due to privilege restrictions" ." - restore as root to include it\n" ideally we set the warning count for tasks in the restore worker, like PBS does, so that this shows up in the gui as "orange" task with some warnings in the task list. > + } else { > + $res .= "$line\n"; > + } > + } else { > + warn "WARNING: INVALID CONFIGURATION LINE '$line'.\n"; I'd tone down capslock here too and rather see how worker task warnings could be set. > + } > + } > + > + 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 >