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