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

[Fiona Ebner]

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?

>>
>>> +    my $ovf_dir = realpath(dirname(File::Spec->rel2abs($ovf)))
>>> +        or die "could not get absolute path of $ovf: $!\n";
>>> +    my $backing_file_path = realpath(join ('/', $ovf_dir, $filepath))
>>> +        or die "could not get absolute path of $filepath: $!\n";
>>> +
>>> +    ($backing_file_path) = $backing_file_path =~ m|^(/.*)|; # untaint
>>>         if (!-e $backing_file_path && !$isOva) {
>>>           die "error parsing $filepath, file seems not to exist at
>>> $backing_file_path\n";
>>>       }
>>>   -    ($backing_file_path) = $backing_file_path =~ m|^(/.*)|; # untaint
>>> -    ($filepath) = $filepath =~ m|
>>> ^(${PVE::Storage::SAFE_CHAR_CLASS_RE}+)$|; # untaint & check no sub/
>>> parent dirs
>>> -    die "invalid path\n" if !$filepath;
>>> -
>>>       if (!$isOva) {
> 
> here ?
> 
>>>           my $size = PVE::Storage::file_size_info($backing_file_path);
>>>           die "error parsing $backing_file_path, cannot determine
>>> file size\n"
>>
> 
>