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

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