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

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
>