[pve-devel] [PATCH v2 storage/guest-common/qemu-server 0/3] harden import of file-based volumes

[pve-devel] [PATCH v2 storage/guest-common/qemu-server 0/3] harden import of file-based volumes

[Fabian Grünbichler]

this series of patches implements additional hardening when copying
potentially untrusted image files:
- extend file_size_info helper which already does most of the work
- add call to check imported volume in remote migration
- add/adapt calls for `import-from` handling in Qemu

these are not problematic at the moment, and these patches just serve as
additional hardening:
- remote migration requires a special privilege, the source must already
  be trusted
- import-from only allows importing volumes already on the storage,
  which are not untrusted but created by PVE itself, or by a user with
  root privileges

the functionality in PVE::Storage should also be used for future
additions where untrusted image files are processed:
- Dominik's OVA import patch series
- arbitrary disk image upload/download features

where not doing such checks might pose a security risk.

v1->v2:
- incorporate Fiona's feedback

pve-guest-common:

Fabian Grünbichler (1):
  storage tunnel: check just-imported image files

 src/PVE/StorageTunnel.pm | 8 ++++++++
 1 file changed, 8 insertions(+)

pve-storage:

Fabian Grünbichler (1):
  file_size_info: implement untrusted mode

 src/PVE/Storage.pm        |  4 ++--
 src/PVE/Storage/Plugin.pm | 36 +++++++++++++++++++++++++++++++-----
 2 files changed, 33 insertions(+), 7 deletions(-)

qemu-server:

Fabian Grünbichler (1):
  disk import: add additional safeguards for imported image files

 PVE/API2/Qemu.pm | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v2 guest-common 1/1] storage tunnel: check just-imported image files

[Fabian Grünbichler]

remote migration requires elevated privileges already and can thus only be
triggered by trusted sources, but an additional safeguard of checking the image
for external references doesn't hurt.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    requires pve-storage change to actually have an effect
    
    v2: fix issue with array context by storing path in its own variable (thanks Fiona)

 src/PVE/StorageTunnel.pm | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/PVE/StorageTunnel.pm b/src/PVE/StorageTunnel.pm
index c880889..fa7889c 100644
--- a/src/PVE/StorageTunnel.pm
+++ b/src/PVE/StorageTunnel.pm
@@ -280,6 +280,14 @@ sub handle_query_disk_import {
 	delete $state->{sockets}->{$unix};
 	delete $state->{disk_import};
 	$state->{cleanup}->{volumes}->{$volid} = 1;
+	my $cfg = PVE::Storage::config();
+	my ($storage, $volume) = PVE::Storage::parse_volume_id($volid);
+	my $scfg = PVE::Storage::storage_config($cfg, $storage);
+	# check imported image for bad references
+	if ($scfg->{path}) {
+	    my $path = PVE::Storage::path($cfg, $volid);
+	    PVE::Storage::file_size_info($path, undef, 1);
+	}
 	return {
 	    status => "complete",
 	    volid => $volid,
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v2 storage 1/1] file_size_info: implement untrusted mode

[Fabian Grünbichler]

this allows checking some extra attributes for images which come from a
potentially malicious source.

since file_size_info is not part of the plugin API, no API bump is needed. if
desired, a similar check could also be implemented in volume_size_info, which
would entail bumping both APIVER and APIAGE (since the additional parameter
would make checking untrusted volumes opt-in for external plugins).

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
---

Notes:
    v2: adapt to new early return, add Fiona's R-b

 src/PVE/Storage.pm        |  4 ++--
 src/PVE/Storage/Plugin.pm | 36 +++++++++++++++++++++++++++++++-----
 2 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 57b2038..3f0b9ae 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -233,9 +233,9 @@ sub storage_ids {
 }
 
 sub file_size_info {
-    my ($filename, $timeout) = @_;
+    my ($filename, $timeout, $untrusted) = @_;
 
-    return PVE::Storage::Plugin::file_size_info($filename, $timeout);
+    return PVE::Storage::Plugin::file_size_info($filename, $timeout, $untrusted);
 }
 
 sub get_volume_attribute {
diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 8cc693c..215214f 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -943,15 +943,25 @@ sub free_image {
     return undef;
 }
 
+# set $untrusted if the file in question might be malicious since it isn't
+# created by our stack
+# this makes certain checks fatal, and adds extra checks for known problems like
+# - backing files (qcow2/vmdk)
+# - external data files (qcow2)
 sub file_size_info {
-    my ($filename, $timeout) = @_;
+    my ($filename, $timeout, $untrusted) = @_;
 
     my $st = File::stat::stat($filename);
 
     if (!defined($st)) {
 	my $extramsg = -l $filename ? ' - dangling symlink?' : '';
-	warn "failed to stat '$filename'$extramsg\n";
-	return undef;
+	my $msg = "failed to stat '$filename'$extramsg\n";
+	if ($untrusted) {
+	    die $msg;
+	} else {
+	    warn $msg;
+	    return undef;
+	}
     }
 
     if (S_ISDIR($st->mode)) {
@@ -975,18 +985,34 @@ sub file_size_info {
 	warn $err_output;
     }
     if (!$json) {
+    	die "failed to query file information with qemu-img\n" if $untrusted;
 	# skip decoding if there was no output, e.g. if there was a timeout.
 	return wantarray ? (undef, undef, undef, undef, $st->ctime) : undef;
     }
 
     my $info = eval { decode_json($json) };
     if (my $err = $@) {
-	warn "could not parse qemu-img info command output for '$filename' - $err\n";
-	return wantarray ? (undef, undef, undef, undef, $st->ctime) : undef;
+	my $msg = "could not parse qemu-img info command output for '$filename' - $err\n";
+	if ($untrusted) {
+	    die $msg;
+	} else {
+	    warn $msg;
+	    return wantarray ? (undef, undef, undef, undef, $st->ctime) : undef;
+	}
+    }
+
+    if ($untrusted) {
+	if (my $format_specific = $info->{'format-specific'}) {
+	    if ($format_specific->{type} eq 'qcow2' && $format_specific->{data}->{"data-file"}) {
+		die "$filename: 'data-file' references are not allowed!\n";
+	    }
+	}
     }
 
     my ($size, $format, $used, $parent) = $info->@{qw(virtual-size format actual-size backing-filename)};
 
+    die "backing file not allowed for untrusted image '$filename'!\n" if $untrusted && $parent;
+
     ($size) = ($size =~ /^(\d+)$/); # untaint
     die "size '$size' not an integer\n" if !defined($size);
     # coerce back from string
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v2 qemu-server 1/1] disk import: add additional safeguards for imported image files

[Fabian Grünbichler]

creating non-raw disk images with arbitrary content is only possible with raw
access to the storage, but checking for references to external files doesn't
hurt.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    requires pve-storage change to actually have an effect
    
    v2:
    - re-order code to improve readability
    - extract $path into variable to avoid scalar vs array context issue

 PVE/API2/Qemu.pm | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 848001b6..ae0c39bf 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -412,18 +412,29 @@ my sub create_disks : prototype($$$$$$$$$$) {
 
 		$needs_creation = $live_import;
 
-		if (PVE::Storage::parse_volume_id($source, 1)) { # PVE-managed volume
+		my ($source_storage, $source_volid) = PVE::Storage::parse_volume_id($source, 1);
+
+		if ($source_storage) { # PVE-managed volume
 		    if ($live_import && $ds ne 'efidisk0') {
 			my $path = PVE::Storage::path($storecfg, $source)
 			    or die "failed to get a path for '$source'\n";
 			$source = $path;
-			($size, my $source_format) = PVE::Storage::file_size_info($source);
+			# check potentially untrusted image file!
+			($size, my $source_format) = PVE::Storage::file_size_info($source, undef, 1);
+
 			die "could not get file size of $source\n" if !$size;
 			$live_import_mapping->{$ds} = {
 			    path => $source,
 			    format => $source_format,
 			};
 		    } else {
+			# check potentially untrusted image file!
+			my $scfg = PVE::Storage::storage_config($storecfg, $source_storage);
+			if ($scfg->{path}) {
+			    my $path = PVE::Storage::path($storecfg, $source);
+			    PVE::Storage::file_size_info($path, undef, 1);
+			}
+
 			my $dest_info = {
 			    vmid => $vmid,
 			    drivename => $ds,
@@ -441,7 +452,8 @@ my sub create_disks : prototype($$$$$$$$$$) {
 		    }
 		} else {
 		    $source = PVE::Storage::abs_filesystem_path($storecfg, $source, 1);
-		    ($size, my $source_format) = PVE::Storage::file_size_info($source);
+		    # check potentially untrusted image file!
+		    ($size, my $source_format) = PVE::Storage::file_size_info($source, undef, 1);
 		    die "could not get file size of $source\n" if !$size;
 
 		    if ($live_import && $ds ne 'efidisk0') {
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v2 storage 1/1] file_size_info: implement untrusted mode

[Fiona Ebner]

Am 04.11.24 um 11:42 schrieb Fabian Grünbichler:
> this allows checking some extra attributes for images which come from a
> potentially malicious source.
> 
> since file_size_info is not part of the plugin API, no API bump is needed. if
> desired, a similar check could also be implemented in volume_size_info, which
> would entail bumping both APIVER and APIAGE (since the additional parameter
> would make checking untrusted volumes opt-in for external plugins).
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

Tested-by: Fiona Ebner <f.ebner@proxmox.com>

(FWIW it breaks my directory-based backup provider example in case of
incremental backups, because that relied on qcow2 backing files O:P)

> @@ -975,18 +985,34 @@ sub file_size_info {
>  	warn $err_output;
>      }
>      if (!$json) {
> +    	die "failed to query file information with qemu-img\n" if $untrusted;

git complains about "space before tab" here


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v2 storage 1/1] file_size_info: implement untrusted mode

[Dominik Csapak]

rebased and tested my ovf import series with this
so consider it:

Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
Tested-by: Dominik Csapak <d.csapak@proxmox.com>


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v2 qemu-server 1/1] disk import: add additional safeguards for imported image files

[Dominik Csapak]

rebased and tested my ovf import series with this
so consider it:

Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
Tested-by: Dominik Csapak <d.csapak@proxmox.com>


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] applied: [PATCH v2 storage 1/1] file_size_info: implement untrusted mode

[Thomas Lamprecht]

Am 04.11.24 um 11:42 schrieb Fabian Grünbichler:
> this allows checking some extra attributes for images which come from a
> potentially malicious source.
> 
> since file_size_info is not part of the plugin API, no API bump is needed. if
> desired, a similar check could also be implemented in volume_size_info, which
> would entail bumping both APIVER and APIAGE (since the additional parameter
> would make checking untrusted volumes opt-in for external plugins).
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> Notes:
>     v2: adapt to new early return, add Fiona's R-b
> 
>  src/PVE/Storage.pm        |  4 ++--
>  src/PVE/Storage/Plugin.pm | 36 +++++++++++++++++++++++++++++++-----
>  2 files changed, 33 insertions(+), 7 deletions(-)
> 
>

applied, with Fiona's and Dominik's review trailers, thanks!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel