From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <f.ebner@proxmox.com>
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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; Tue,  9 Mar 2021 11:08:14 +0100 (CET)
To: Thomas Lamprecht <t.lamprecht@proxmox.com>,
 Proxmox VE development discussion <pve-devel@lists.proxmox.com>
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 <f.ebner@proxmox.com>
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 <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=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 <f.ebner@proxmox.com>
>> ---
>>   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
>>
>