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.eb...@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
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
