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

[pve-devel] [PATCH 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.

pve-guest-common:

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

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

pve-storage:

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

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

qemu-server:

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

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

-- 
2.39.2



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

[pve-devel] [PATCH 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

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

diff --git a/src/PVE/StorageTunnel.pm b/src/PVE/StorageTunnel.pm
index c880889..21780bd 100644
--- a/src/PVE/StorageTunnel.pm
+++ b/src/PVE/StorageTunnel.pm
@@ -280,6 +280,13 @@ 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}) {
+	    PVE::Storage::file_size_info(PVE::Storage::path($cfg, $volid), undef, 1);
+	}
 	return {
 	    status => "complete",
 	    volid => $volid,
-- 
2.39.2



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

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

[Fiona Ebner]

Am 09.08.24 um 13:22 schrieb 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
> 
>  src/PVE/StorageTunnel.pm | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/PVE/StorageTunnel.pm b/src/PVE/StorageTunnel.pm
> index c880889..21780bd 100644
> --- a/src/PVE/StorageTunnel.pm
> +++ b/src/PVE/StorageTunnel.pm
> @@ -280,6 +280,13 @@ 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}) {

Seems a bit like a plugin method would be nicest here. But I guess we
can still add that if/when it becomes necessary for non-path-based
plugins to do such checks.

> +	    PVE::Storage::file_size_info(PVE::Storage::path($cfg, $volid), undef, 1);

Isn't this broken because PVE::Storage::path() uses wantarray?

At least a small test program does "break" in the same scenario:

> febner@enia ~ % cat asdf.pm
> use strict;
> use warnings;
>
> sub a {
> 	return wantarray ? (1, 2) : 1;
> }
>
> sub b {
> 	my ($a, $b, $c) = @_;
>
> 	print "$a $b $c\n";
> }
>
> b(a(), 3, 4);
> febner@enia ~ % perl asdf.pm
> 1 2 3


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

[pve-devel] [PATCH 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>
---
 src/PVE/Storage.pm        |  4 ++--
 src/PVE/Storage/Plugin.pm | 35 ++++++++++++++++++++++++++++++-----
 2 files changed, 32 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 6444390..b03e183 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)) {
@@ -977,12 +987,27 @@ sub file_size_info {
 
     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.2



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

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

[Fiona Ebner]

Am 09.08.24 um 13:22 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>

> @@ -977,12 +987,27 @@ sub file_size_info {
>  

There's a new early return now because of commit 851cc07 ("base plugin:
do not decode the empty string") where we should also die if $untrusted.
Although it is very unlikely that we'll reach that since you do not set
a timeout at the call sites with $untrusted set.

>      my $info = eval { decode_json($json) };
>      if (my $err = $@) {


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

[pve-devel] [PATCH 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

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

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index d25a79fe..30839745 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -412,12 +412,14 @@ my sub create_disks : prototype($$$$$$$$$$) {
 
 		$needs_creation = $live_import;
 
-		if (PVE::Storage::parse_volume_id($source, 1)) { # PVE-managed volume
+		if (my ($source_storage, $source_volid) = PVE::Storage::parse_volume_id($source, 1)) { # 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,
@@ -431,6 +433,12 @@ my sub create_disks : prototype($$$$$$$$$$) {
 			    format => $disk->{format},
 			};
 
+			my $scfg = PVE::Storage::storage_config($storecfg, $source_storage);
+			# check potentially untrusted image file!
+			if ($scfg->{path}) {
+			    PVE::Storage::file_size_info(PVE::Storage::path($storecfg, $source), undef, 1);
+			}
+
 			$dest_info->{efisize} = PVE::QemuServer::get_efivars_size($conf, $disk)
 			    if $ds eq 'efidisk0';
 
@@ -441,7 +449,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.2



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

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

[Fiona Ebner]

Am 09.08.24 um 13:22 schrieb 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
> 
>  PVE/API2/Qemu.pm | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index d25a79fe..30839745 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -412,12 +412,14 @@ my sub create_disks : prototype($$$$$$$$$$) {
>  
>  		$needs_creation = $live_import;
>  
> -		if (PVE::Storage::parse_volume_id($source, 1)) { # PVE-managed volume
> +		if (my ($source_storage, $source_volid) = PVE::Storage::parse_volume_id($source, 1)) { # PVE-managed volume

Nit: line too long and honestly, becomes a bit hard to read.

>  		    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,
> @@ -431,6 +433,12 @@ my sub create_disks : prototype($$$$$$$$$$) {
>  			    format => $disk->{format},
>  			};
>  
> +			my $scfg = PVE::Storage::storage_config($storecfg, $source_storage);
> +			# check potentially untrusted image file!
> +			if ($scfg->{path}) {
> +			    PVE::Storage::file_size_info(PVE::Storage::path($storecfg, $source), undef, 1);

This call is probably broken, see reply to guest-common patch.

Nit: I'd move this to the beginning of the branch to avoid having it
between $dest_info assignments.

> +			}
> +
>  			$dest_info->{efisize} = PVE::QemuServer::get_efivars_size($conf, $disk)
>  			    if $ds eq 'efidisk0';
>  
> @@ -441,7 +449,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') {


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