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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id D45B4687E6 for ; Tue, 9 Mar 2021 11:08:45 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id B694C82CE for ; Tue, 9 Mar 2021 11:08:15 +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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id E134582BB for ; Tue, 9 Mar 2021 11:08:14 +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 AA6A041B63 for ; Tue, 9 Mar 2021 11:08:14 +0100 (CET) To: Thomas Lamprecht , Proxmox VE development discussion References: <20210308122658.31781-1-f.ebner@proxmox.com> <20210308122658.31781-2-f.ebner@proxmox.com> <04a999a8-ce86-ac53-e3d5-00f17c11ee81@proxmox.com> From: Fabian Ebner Message-ID: <358dabb5-1b45-a1e4-1795-6fb310b78df3@proxmox.com> Date: Tue, 9 Mar 2021 11:08:13 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 MIME-Version: 1.0 In-Reply-To: <04a999a8-ce86-ac53-e3d5-00f17c11ee81@proxmox.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.001 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 10:08:45 -0000 On 09.03.21 08:17, Thomas Lamprecht wrote: > 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? > Haven't seen any. I can try and add some tests that mirror the config-related behavior of the restore_XYZ_archive functions (directly testing those doesn't seem feasible to me at the moment, lots of PBS/VMA+pipes interaction...) >> 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? > I did briefly think about it, but decided it wasn't worth the extra complexity of parsing and deeply comparing everything. It's not only the $rootonlyoptions though, e.g. usb0: host=1-1,usb3=1 and usb0: usb3=1,host=1-1 would also be affected. For containers, we don't even look at the current values at all, but always drop the 'lxc' options for a non-root user restore. But if you think it's worth it, I can try and add that. >> + $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. > I wasn't aware of this feature. I suppose we should do that for the skipped options for LXC too then. >> + } 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. > Ok. >> + } >> + } >> + >> + 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 >> >