[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] applied: [PATCH v2 guest-common 1/1] storage tunnel: check just-imported image files

[Thomas Lamprecht]

Am 04.11.24 um 11:42 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
>     
>     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(+)
> 
>

applied, thanks!


_______________________________________________
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

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

[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

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

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

[Fiona Ebner]

On 04.11.24 11:42 AM, Fabian Grünbichler wrote:
> 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}) {

Is there a special reason this is made conditional on the presence of
$scfg->{path}? Note that the above check for live import isn't.
Shouldn't matter much right now (except if there is a weird third-party
plugin that has qcow2 and doesn't use $scfg->{path}), but as long as we
get a result for PVE::Storage::path(), we should be able to call
file_size_info() too, right? Would potentially help detecting other
issues with a source volume early.

> +			    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') {



_______________________________________________
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

[Fiona Ebner]

On 15.11.24 10:42 AM, Fiona Ebner wrote:
> On 04.11.24 11:42 AM, Fabian Grünbichler wrote:
>> 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}) {
> 
> Is there a special reason this is made conditional on the presence of
> $scfg->{path}? Note that the above check for live import isn't.
> Shouldn't matter much right now (except if there is a weird third-party
> plugin that has qcow2 and doesn't use $scfg->{path}), but as long as we
> get a result for PVE::Storage::path(), we should be able to call
> file_size_info() too, right?
> 

Or maybe not, but then live-import should've used volume_size_info() too
I guess? I think it would be good to add an 'untrusted' flag to
volume_size_info() too and have the storage layer do the right thing,
rather than manually checking $scfg->{path} 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 qemu-server 1/1] disk import: add additional safeguards for imported image files

[Dominik Csapak]

On 11/15/24 10:49, Fiona Ebner wrote:
> On 15.11.24 10:42 AM, Fiona Ebner wrote:
>> On 04.11.24 11:42 AM, Fabian Grünbichler wrote:
>>> 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}) {
>>
>> Is there a special reason this is made conditional on the presence of
>> $scfg->{path}? Note that the above check for live import isn't.
>> Shouldn't matter much right now (except if there is a weird third-party
>> plugin that has qcow2 and doesn't use $scfg->{path}), but as long as we
>> get a result for PVE::Storage::path(), we should be able to call
>> file_size_info() too, right?
>>
> 
> Or maybe not, but then live-import should've used volume_size_info() too
> I guess? I think it would be good to add an 'untrusted' flag to
> volume_size_info() too and have the storage layer do the right thing,
> rather than manually checking $scfg->{path} here.
> 
> 
my 2 cents:

the checking of untrusted images is mainly helpful for file based volumes
(such as qcow2/vmdk/etc.) where we can detect backing images etc.

the check for 'path' is how we ususually determine if a storage is
file-based or not (would probably better to have that check
in storage and it should probably check for qcow2/vmdk/etc. support)

but we already use that check in some places


_______________________________________________
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

[Fiona Ebner]

On 15.11.24 10:55 AM, Dominik Csapak wrote:
> On 11/15/24 10:49, Fiona Ebner wrote:
>> On 15.11.24 10:42 AM, Fiona Ebner wrote:
>>> On 04.11.24 11:42 AM, Fabian Grünbichler wrote:
>>>> 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}) {
>>>
>>> Is there a special reason this is made conditional on the presence of
>>> $scfg->{path}? Note that the above check for live import isn't.
>>> Shouldn't matter much right now (except if there is a weird third-party
>>> plugin that has qcow2 and doesn't use $scfg->{path}), but as long as we
>>> get a result for PVE::Storage::path(), we should be able to call
>>> file_size_info() too, right?
>>>
>>
>> Or maybe not, but then live-import should've used volume_size_info() too
>> I guess? I think it would be good to add an 'untrusted' flag to
>> volume_size_info() too and have the storage layer do the right thing,
>> rather than manually checking $scfg->{path} here.
>>
>>
> my 2 cents:
> 
> the checking of untrusted images is mainly helpful for file based volumes
> (such as qcow2/vmdk/etc.) where we can detect backing images etc.
> 

Yes, currently. But like this, third-party plugins without a path won't
be able to have checks and maybe we'd like to add different checks in
the future that won't require file-based volumes.

My point is about the mismatch between the check here and the
live-import one. Either we can call file_size_info() for any result of
path() (not sure, because e.g. ISCSIDirectPlugin's path() will return an
'iscsi://' protocol path for exmplae) or we should use
volume_size_info() instead.

But I guess that's orthogonal and can still be improved later.

> the check for 'path' is how we ususually determine if a storage is
> file-based or not (would probably better to have that check
> in storage and it should probably check for qcow2/vmdk/etc. support)
> 
> but we already use that check in some places
> 


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

On 11/15/24 11:05, Fiona Ebner wrote:
> On 15.11.24 10:55 AM, Dominik Csapak wrote:
>> On 11/15/24 10:49, Fiona Ebner wrote:
>>> On 15.11.24 10:42 AM, Fiona Ebner wrote:
>>>> On 04.11.24 11:42 AM, Fabian Grünbichler wrote:
>>>>> 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}) {
>>>>
>>>> Is there a special reason this is made conditional on the presence of
>>>> $scfg->{path}? Note that the above check for live import isn't.
>>>> Shouldn't matter much right now (except if there is a weird third-party
>>>> plugin that has qcow2 and doesn't use $scfg->{path}), but as long as we
>>>> get a result for PVE::Storage::path(), we should be able to call
>>>> file_size_info() too, right?
>>>>
>>>
>>> Or maybe not, but then live-import should've used volume_size_info() too
>>> I guess? I think it would be good to add an 'untrusted' flag to
>>> volume_size_info() too and have the storage layer do the right thing,
>>> rather than manually checking $scfg->{path} here.
>>>
>>>
>> my 2 cents:
>>
>> the checking of untrusted images is mainly helpful for file based volumes
>> (such as qcow2/vmdk/etc.) where we can detect backing images etc.
>>
> 
> Yes, currently. But like this, third-party plugins without a path won't
> be able to have checks and maybe we'd like to add different checks in
> the future that won't require file-based volumes.
> 
> My point is about the mismatch between the check here and the
> live-import one. Either we can call file_size_info() for any result of
> path() (not sure, because e.g. ISCSIDirectPlugin's path() will return an
> 'iscsi://' protocol path for exmplae) or we should use
> volume_size_info() instead.

true, this should be consistent

just want to note that if path() does not return an actual
path for the filesystem, we'll fail to 'stat' it and die here

while previously we returned undef in that case und died later down
because size wasn't checked

so AFAIU it importing non file based volumes was broken anyway?

> But I guess that's orthogonal and can still be improved later.
> 
>> the check for 'path' is how we ususually determine if a storage is
>> file-based or not (would probably better to have that check
>> in storage and it should probably check for qcow2/vmdk/etc. support)
>>
>> but we already use that check in some places
>>
> 
> 



_______________________________________________
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

[Fiona Ebner]

On 04.11.24 11:42 AM, Fabian Grünbichler wrote:
> 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,

Actually, this breaks disk import from the image of an existing linked
clone (which is trusted):

> root@pve8a1 /mnt/pve/sparschwein/125 # qm set 125 --scsi7 local:0,import-from=dir:101/vm-101-disk-0.qcow2
> update VM 125: -scsi7 local:0,import-from=dir:101/vm-101-disk-0.qcow2
> backing file not allowed for untrusted image '/mnt/pve/dir/images/101/vm-101-disk-0.qcow2'!




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