[pve-devel] [PATCH storage v5 04/12] ovf: improve and simplify path checking code

[Dominik Csapak]

On 11/15/24 14:44, Fiona Ebner wrote:
> On 15.11.24 2:39 PM, Dominik Csapak wrote:
>> On 11/15/24 14:35, Fiona Ebner wrote:
>>> On 14.11.24 10:32 AM, Dominik Csapak wrote:
>>>> @@ -244,22 +235,31 @@ ovf:Item[rasd:InstanceID='%s']/
>>>> rasd:ResourceType", $controller_id);
>>>>        my $adress_on_controller = $xpc-
>>>>> findvalue('rasd:AddressOnParent', $item_node);
>>>>        my $pve_disk_address = id_to_pve($controller_type) .
>>>> $adress_on_controller;
>>>>    +    # from Disk Node, find corresponding filepath
>>>> +    my $xpath_find_filepath = sprintf("/ovf:Envelope/ovf:References/
>>>> ovf:File[\@ovf:id='%s']/\@ovf:href", $fileref);
>>>> +    my $filepath = $xpc->findvalue($xpath_find_filepath);
>>>> +    if (!$filepath) {
>>>> +        warn "invalid file reference $fileref, skipping\n";
>>>> +        next;
>>>> +    }
>>>> +    print "file path: $filepath\n" if $debug;
>>>> +    my $original_filepath = $filepath;
>>>> +    ($filepath) = $filepath =~ m|
>>>> ^(${PVE::Storage::SAFE_CHAR_CLASS_RE}+)$|; # untaint & check no sub/
>>>> parent dirs
>>>> +    die "referenced path '$original_filepath' is invalid\n" if !
>>>> $filepath || $filepath eq "." || $filepath eq "..";
>>>> +
>>>>        # resolve symlinks and relative path components
>>>>        # and die if the diskimage is not somewhere under the $ovf path
>>>> -    my $ovf_dir = realpath(dirname(File::Spec->rel2abs($ovf)));
>>>> -    my $backing_file_path = realpath(join ('/', $ovf_dir, $filepath));
>>>> -    if ($backing_file_path !~ /^\Q${ovf_dir}\E/) {
>>>> -        die "error parsing $filepath, are you using a symlink ?\n";
>>>> -    }
>>>
>>> Don't we still need this check against symlinks?
>>
>> yeah i think you're right, but only in the ovf case so I'd add it...
>>
> 
> Why only then? We still return it as part of the disk info, so I'd
> rather just always do it. Or does it break something?
> 

true, i was caught up in the 'ovas don't have the files extracted yet'
and thought that does not work, but actually realpath does only check
the last component if it's a symlink,
so

realpath("/foo/bar")

works as long as foo exists, regardless if bar is a symlink or does not exist