Re: [pve-devel] [PATCH qemu-server 2/2] restore: sanitize config for non-root users

[Fabian Ebner <f.ebner@proxmox.com>]

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
>>
>