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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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