[pve-devel] [PATCH qemu-server 2/2] restore: sanitize config for non-root users
Fabian Ebner
f.ebner at proxmox.com
Tue Mar 9 11:08:13 CET 2021
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 at 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 at 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
>>
>
More information about the pve-devel
mailing list