[pve-devel] [PATCH pve-storage 0/2] fix #623: show isos/tmplt/snippets in subdirs

[pve-devel] [PATCH pve-storage 0/2] fix #623: show isos/tmplt/snippets in subdirs

[Noel Ullreich]

This patch allows for isos, container templates, and snippets in
subdirectories to be displayed by the web interface and the cli tools.
For the isos/templates/snippets to be displayed, they have to be in
their corresponding base directories
(see: https://pve.proxmox.com/pve-docs/pve-admin-guide.html#storage_directory).

I've started working on a new tree-based dropdown menu in the web interface so 
that directories can be collapsed, however I don't know if that is overkill or 
not, I would like some feedback on that.

Also, no depth limit for the subdirs has been set. Should one be set?

Noel Ullreich (2):
  update `list_volumes` to allow subdirs
  change regex to allow subdirs

 PVE/Storage/Plugin.pm | 79 +++++++++++++++++++++++++++----------------
 1 file changed, 50 insertions(+), 29 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH pve-storage 1/2] update `list_volumes` to allow subdirs

[Noel Ullreich]

iterate through subdirs to find all the isos/container
templates/snippets.

Signed-off-by: Noel Ullreich <n.ullreich@proxmox.com>
---
 PVE/Storage/Plugin.pm | 65 ++++++++++++++++++++++++++++---------------
 1 file changed, 43 insertions(+), 22 deletions(-)

diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index ca7a0d4..bf1d564 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -9,6 +9,7 @@ use File::chdir;
 use File::Path;
 use File::Basename;
 use File::stat qw();
+use File::Find;
 
 use PVE::Tools qw(run_command);
 use PVE::JSONSchema qw(get_standard_option register_standard_option);
@@ -1275,46 +1276,66 @@ sub list_volumes {
     my $vmlist = PVE::Cluster::get_vmlist();
     foreach my $type (@$content_types) {
 	my $data;
+	my @list_of_dirs;
 
 	if ($type eq 'images' || $type eq 'rootdir') {
 	    $data = $class->list_images($storeid, $scfg, $vmid);
+	    push (@list_of_dirs, $data) if $data; #fix this
 	} elsif ($scfg->{path}) {
 	    my $path = $class->get_subdir($scfg, $type);
+	    my @subdirs;
+	    my $wanted = sub {
+		my ($dev,$ino,$mode,$nlink,$uid,$gid);
+
+		(($dev,$ino,$mode,$nlink,$uid,$gid) = lstat($_)) &&
+		-d _
+		&& push(@subdirs, $File::Find::name);
+	    };
+	    File::Find::find({wanted => $wanted, untaint => 1}, $path);
 
 	    if ($type eq 'iso' && !defined($vmid)) {
-		$data = $get_subdir_files->($storeid, $path, 'iso');
+		for (@subdirs) {
+		    $data = $get_subdir_files->($storeid, $_, 'iso',);
+		    push @list_of_dirs, $data;
+		}
 	    } elsif ($type eq 'vztmpl'&& !defined($vmid)) {
-		$data = $get_subdir_files->($storeid, $path, 'vztmpl');
+		for (@subdirs) {
+		    $data = $get_subdir_files->($storeid, $_, 'vztmpl');
+		    push @list_of_dirs, $data;
+		}
 	    } elsif ($type eq 'backup') {
 		$data = $get_subdir_files->($storeid, $path, 'backup', $vmid);
+		push(@list_of_dirs, $data) if $data;
 	    } elsif ($type eq 'snippets') {
-		$data = $get_subdir_files->($storeid, $path, 'snippets');
+		for (@subdirs){
+		    $data = $get_subdir_files->($storeid, $_, 'snippets');
+		    push(@list_of_dirs, $data) if $data;
+		}
 	    }
 	}
-
-	next if !$data;
-
-	foreach my $item (@$data) {
-	    if ($type eq 'images' || $type eq 'rootdir') {
-		my $vminfo = $vmlist->{ids}->{$item->{vmid}};
-		my $vmtype;
-		if (defined($vminfo)) {
-		    $vmtype = $vminfo->{type};
-		}
-		if (defined($vmtype) && $vmtype eq 'lxc') {
-		    $item->{content} = 'rootdir';
+	next if !@list_of_dirs; #if list of dirs is empty, there is no data either
+
+	for (@list_of_dirs) {
+	    foreach my $item (@$_) {
+		if ($type eq 'images' || $type eq 'rootdir') {
+		    my $vminfo = $vmlist->{ids}->{$item->{vmid}};
+		    my $vmtype;
+		    if (defined($vminfo)) {
+			$vmtype = $vminfo->{type};
+		    }
+		    if (defined($vmtype) && $vmtype eq 'lxc') {
+			$item->{content} = 'rootdir';
+		    } else {
+			$item->{content} = 'images';
+		    }
+		    next if $type ne $item->{content};
 		} else {
-		    $item->{content} = 'images';
+		    $item->{content} = $type;
 		}
-		next if $type ne $item->{content};
-	    } else {
-		$item->{content} = $type;
+		push @$res, $item;
 	    }
-
-	    push @$res, $item;
 	}
     }
-
     return $res;
 }
 
-- 
2.30.2

Re: [pve-devel] [PATCH pve-storage 1/2] update `list_volumes` to allow subdirs

[Fabian Grünbichler]

On March 3, 2023 3:50 pm, Noel Ullreich wrote:
> iterate through subdirs to find all the isos/container
> templates/snippets.

might be worth it to call out that this patch is broken without the second one,
unless you have appropriate "middle dirs" to make the unmodified REs in
get_subdir_files match.

e.g.,

$storage_path/template/iso/foobar/something.iso

doesn't work (it will return $storage:iso/something.iso as volume ID)

$storage_path/template/iso/foobar/template/iso/nested.iso

is even worse, since it's also broken *with* the second patch and returns
$storage:iso/nested.iso as volume ID.. but more about this in comments for the
second patch..

> 
> Signed-off-by: Noel Ullreich <n.ullreich@proxmox.com>
> ---
>  PVE/Storage/Plugin.pm | 65 ++++++++++++++++++++++++++++---------------
>  1 file changed, 43 insertions(+), 22 deletions(-)
> 
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index ca7a0d4..bf1d564 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -9,6 +9,7 @@ use File::chdir;
>  use File::Path;
>  use File::Basename;
>  use File::stat qw();
> +use File::Find;

File::Find docs contain the following warning about warnings ;)

WARNINGS
    If you run your program with the "-w" switch, or if you use the
    "warnings" pragma, File::Find will report warnings for several weird
    situations. You can disable these warnings by putting the statement

        no warnings 'File::Find';

    in the appropriate scope. See warnings for more info about lexical
    warnings.

this is also visible (I think?) in the list_volumes test case output when
building the package.

>  
>  use PVE::Tools qw(run_command);
>  use PVE::JSONSchema qw(get_standard_option register_standard_option);
> @@ -1275,46 +1276,66 @@ sub list_volumes {
>      my $vmlist = PVE::Cluster::get_vmlist();
>      foreach my $type (@$content_types) {
>  	my $data;
> +	my @list_of_dirs;
>  
>  	if ($type eq 'images' || $type eq 'rootdir') {
>  	    $data = $class->list_images($storeid, $scfg, $vmid);
> +	    push (@list_of_dirs, $data) if $data; #fix this

fix what? or leftover?

>  	} elsif ($scfg->{path}) {
>  	    my $path = $class->get_subdir($scfg, $type);
> +	    my @subdirs;
> +	    my $wanted = sub {
> +		my ($dev,$ino,$mode,$nlink,$uid,$gid);
> +
> +		(($dev,$ino,$mode,$nlink,$uid,$gid) = lstat($_)) &&
> +		-d _
> +		&& push(@subdirs, $File::Find::name);

nit: style (&& placement!)

do we want/need to forbid symlinks here? we don't for regular files or content
dirs IIRC.. if we want to forbid them, we'd need to also add appropriate checks
in other places whenever translation from volid to path happens..

one thing I wondered here is whether it wouldn't be easier to extend
get_subdir_files with a "$recursive" boolean, and then in the beginning of that
helper change the "next" to recurse? did you try that and it was worse (it would
save us from iterating over each dir twice and having to use File::Find..)

> +	    };
> +	    File::Find::find({wanted => $wanted, untaint => 1}, $path);
>  
>  	    if ($type eq 'iso' && !defined($vmid)) {
> -		$data = $get_subdir_files->($storeid, $path, 'iso');
> +		for (@subdirs) {

nit: I prefer

for my $dir (@subdirs) {

}

although our style guide is not really clear (just says to prefer `for` over
`foreach`), so not sure whether that is consensus ;)

> +		    $data = $get_subdir_files->($storeid, $_, 'iso',);
> +		    push @list_of_dirs, $data;
> +		}
>  	    } elsif ($type eq 'vztmpl'&& !defined($vmid)) {
> -		$data = $get_subdir_files->($storeid, $path, 'vztmpl');
> +		for (@subdirs) {
> +		    $data = $get_subdir_files->($storeid, $_, 'vztmpl');
> +		    push @list_of_dirs, $data;

so list of dirs is not actually a list of dirs? :-P

> +		}
>  	    } elsif ($type eq 'backup') {
>  		$data = $get_subdir_files->($storeid, $path, 'backup', $vmid);
> +		push(@list_of_dirs, $data) if $data;
>  	    } elsif ($type eq 'snippets') {
> -		$data = $get_subdir_files->($storeid, $path, 'snippets');
> +		for (@subdirs){
> +		    $data = $get_subdir_files->($storeid, $_, 'snippets');
> +		    push(@list_of_dirs, $data) if $data;
> +		}
>  	    }
>  	}
> -
> -	next if !$data;
> -
> -	foreach my $item (@$data) {
> -	    if ($type eq 'images' || $type eq 'rootdir') {
> -		my $vminfo = $vmlist->{ids}->{$item->{vmid}};
> -		my $vmtype;
> -		if (defined($vminfo)) {
> -		    $vmtype = $vminfo->{type};
> -		}
> -		if (defined($vmtype) && $vmtype eq 'lxc') {
> -		    $item->{content} = 'rootdir';
> +	next if !@list_of_dirs; #if list of dirs is empty, there is no data either
> +
> +	for (@list_of_dirs) {

same nit as above..

> +	    foreach my $item (@$_) {
> +		if ($type eq 'images' || $type eq 'rootdir') {
> +		    my $vminfo = $vmlist->{ids}->{$item->{vmid}};
> +		    my $vmtype;
> +		    if (defined($vminfo)) {
> +			$vmtype = $vminfo->{type};
> +		    }
> +		    if (defined($vmtype) && $vmtype eq 'lxc') {
> +			$item->{content} = 'rootdir';
> +		    } else {
> +			$item->{content} = 'images';
> +		    }
> +		    next if $type ne $item->{content};
>  		} else {
> -		    $item->{content} = 'images';
> +		    $item->{content} = $type;
>  		}
> -		next if $type ne $item->{content};
> -	    } else {
> -		$item->{content} = $type;
> +		push @$res, $item;
>  	    }
> -
> -	    push @$res, $item;
>  	}
>      }
> -
>      return $res;
>  }
>  
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

[pve-devel] [PATCH pve-storage 2/2] change regex to allow subdirs

[Noel Ullreich]

change the regex in `parse_volname` and `get_subdir_files` to allow
subdirectories.

Signed-off-by: Noel Ullreich <n.ullreich@proxmox.com>
---
 PVE/Storage/Plugin.pm | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index bf1d564..e8b2b95 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -620,9 +620,9 @@ sub parse_volname {
 	my ($vmid, $name) = ($1, $2);
 	my (undef, $format, $isBase) = parse_name_dir($name);
 	return ('images', $name, $vmid, undef, undef, $isBase, $format);
-    } elsif ($volname =~ m!^iso/([^/]+$PVE::Storage::ISO_EXT_RE_0)$!) {
+    } elsif ($volname =~ m!^iso/(.+$PVE::Storage::ISO_EXT_RE_0)$!) {
 	return ('iso', $1);
-    } elsif ($volname =~ m!^vztmpl/([^/]+$PVE::Storage::VZTMPL_EXT_RE_1)$!) {
+    } elsif ($volname =~ m!^vztmpl/(.+$PVE::Storage::VZTMPL_EXT_RE_1)$!) {
 	return ('vztmpl', $1);
     } elsif ($volname =~ m!^rootdir/(\d+)$!) {
 	return ('rootdir', $1, $1);
@@ -632,7 +632,7 @@ sub parse_volname {
 	    return ('backup', $fn, $2);
 	}
 	return ('backup', $fn);
-    } elsif ($volname =~ m!^snippets/([^/]+)$!) {
+    } elsif ($volname =~ m!^snippets/(.+)$!) {
 	return ('snippets', $1);
     }
 
@@ -1214,12 +1214,12 @@ my $get_subdir_files = sub {
 	my $info;
 
 	if ($tt eq 'iso') {
-	    next if $fn !~ m!/([^/]+$PVE::Storage::ISO_EXT_RE_0)$!i;
+	    next if $fn !~ m!/.+\/template\/iso\/(.+$PVE::Storage::ISO_EXT_RE_0)$!i;
 
 	    $info = { volid => "$sid:iso/$1", format => 'iso' };
 
 	} elsif ($tt eq 'vztmpl') {
-	    next if $fn !~ m!/([^/]+$PVE::Storage::VZTMPL_EXT_RE_1)$!;
+	    next if $fn !~ m!/.+\/template\/cache\/(.+$PVE::Storage::VZTMPL_EXT_RE_1)$!;
 
 	    $info = { volid => "$sid:vztmpl/$1", format => "t$2" };
 
@@ -1251,9 +1251,9 @@ my $get_subdir_files = sub {
 
 	    $info->{protected} = 1 if -e PVE::Storage::protection_file_path($original);
 	} elsif ($tt eq 'snippets') {
-
+	    next if $fn !~ m!/.+\/snippets/(.+)!;
 	    $info = {
-		volid => "$sid:snippets/". basename($fn),
+		volid => "$sid:snippets/$1",
 		format => 'snippet',
 	    };
 	}
-- 
2.30.2

Re: [pve-devel] [PATCH pve-storage 2/2] change regex to allow subdirs

[Fabian Grünbichler]

On March 3, 2023 3:50 pm, Noel Ullreich wrote:
> change the regex in `parse_volname` and `get_subdir_files` to allow
> subdirectories.
> 
> Signed-off-by: Noel Ullreich <n.ullreich@proxmox.com>
> ---
>  PVE/Storage/Plugin.pm | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index bf1d564..e8b2b95 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -620,9 +620,9 @@ sub parse_volname {
>  	my ($vmid, $name) = ($1, $2);
>  	my (undef, $format, $isBase) = parse_name_dir($name);
>  	return ('images', $name, $vmid, undef, undef, $isBase, $format);
> -    } elsif ($volname =~ m!^iso/([^/]+$PVE::Storage::ISO_EXT_RE_0)$!) {
> +    } elsif ($volname =~ m!^iso/(.+$PVE::Storage::ISO_EXT_RE_0)$!) {

so this needs to forbid at least '..', else the following is possible

1. place symlink to sensitive, root-only host file somewhere writable by a user
(e.g., container volume on ZFS which has a host-visible mount point by default,
but also something like an arbitrary container volume and tricking an admin into
running `pct mount` would work, or possible other similar scenarios)
2. qm set XXX -ide2 $storage:iso/../../../path/to/symlink,medium=cdrom
3. (in VM) cat /dev/sr0

I am not sure whether that's the only thing that's bad, it might make sense to
start with a restricted set of allowed characters for the dirs in-between and
extend that as needed after evaluating.. if that route is taken, it obviously
also should be done for patch #1 to be consistent ;)

it might also make sense to limit the nesting here and in patch #1, and/or limit
the length of the resulting volume ID?

one problem we have here is that we only operate on a volume ID (or in sub path,
on a "hypothetical" path) so it's a bit hard to guard against certain types of
attacks that would require actually evaluating and analyzing the resulting path,
since we might be called in a context where the volume not yet existing is
totally fine :-/

all in all, this series is the kind of change that warrants close attention, and
IMHO, when in doubt, might benefit from a few restrictions that don't really
harm usability but make locking it down more easy.

>  	return ('iso', $1);
> -    } elsif ($volname =~ m!^vztmpl/([^/]+$PVE::Storage::VZTMPL_EXT_RE_1)$!) {
> +    } elsif ($volname =~ m!^vztmpl/(.+$PVE::Storage::VZTMPL_EXT_RE_1)$!) {

same here

>  	return ('vztmpl', $1);
>      } elsif ($volname =~ m!^rootdir/(\d+)$!) {
>  	return ('rootdir', $1, $1);
> @@ -632,7 +632,7 @@ sub parse_volname {
>  	    return ('backup', $fn, $2);
>  	}
>  	return ('backup', $fn);
> -    } elsif ($volname =~ m!^snippets/([^/]+)$!) {
> +    } elsif ($volname =~ m!^snippets/(.+)$!) {

and here! this would even allow directly including host files via cloud init I
think?

>  	return ('snippets', $1);
>      }
>  
> @@ -1214,12 +1214,12 @@ my $get_subdir_files = sub {
>  	my $info;
>  
>  	if ($tt eq 'iso') {
> -	    next if $fn !~ m!/([^/]+$PVE::Storage::ISO_EXT_RE_0)$!i;
> +	    next if $fn !~ m!/.+\/template\/iso\/(.+$PVE::Storage::ISO_EXT_RE_0)$!i;

same applies here

>  
>  	    $info = { volid => "$sid:iso/$1", format => 'iso' };
>  
>  	} elsif ($tt eq 'vztmpl') {
> -	    next if $fn !~ m!/([^/]+$PVE::Storage::VZTMPL_EXT_RE_1)$!;
> +	    next if $fn !~ m!/.+\/template\/cache\/(.+$PVE::Storage::VZTMPL_EXT_RE_1)$!;

and here

>  
>  	    $info = { volid => "$sid:vztmpl/$1", format => "t$2" };
>  
> @@ -1251,9 +1251,9 @@ my $get_subdir_files = sub {
>  
>  	    $info->{protected} = 1 if -e PVE::Storage::protection_file_path($original);
>  	} elsif ($tt eq 'snippets') {
> -
> +	    next if $fn !~ m!/.+\/snippets/(.+)!;

and also here

>  	    $info = {
> -		volid => "$sid:snippets/". basename($fn),
> +		volid => "$sid:snippets/$1",
>  		format => 'snippet',
>  	    };
>  	}
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH pve-storage 0/2] fix #623: show isos/tmplt/snippets in subdirs

[Fiona Ebner]

Am 03.03.23 um 15:50 schrieb Noel Ullreich:
> This patch allows for isos, container templates, and snippets in
> subdirectories to be displayed by the web interface and the cli tools.
> For the isos/templates/snippets to be displayed, they have to be in
> their corresponding base directories
> (see: https://pve.proxmox.com/pve-docs/pve-admin-guide.html#storage_directory).
> 

AFAIU, the series not only allows scanning subdirectories, but makes it
the default and only behavior. Should this rather be opt-in via a
storage config option? Or do we want to change the default behavior for
PVE 8.0?