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

[Dominik Csapak]

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

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

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel