[pve-devel] [PATCH container] improve mountpoint parsing

[Wolfgang Bumiller]

On Thu, Feb 04, 2016 at 11:36:41AM +0100, Dominik Csapak wrote:
> currently we sanitize mountpoints with sanitize_mountpoint, which
> tries to remove dots, double-dots and multiple slashes, but it does it
> not correctly (e.g. /test/././ gets truncated to /test./ )
> 
> instead of trying to truncate the path, we create a format for mp strings
> which throws an error if /./ or /../ exist (also /. and /.. at the end)
> since there should be no valid use for these in mountpoint paths anyway
> 
> with the new behaviour, we don't need sanitize_mountpoint anymore
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
>  src/PVE/LXC.pm | 33 ++++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index f761f33..2b52a63 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -38,6 +38,7 @@ my $rootfs_desc = {
>      volume => {
>  	type => 'string',
>  	default_key => 1,
> +	format => 'pve-lxc-mp-string',
>  	format_description => 'volume',
>  	description => 'Volume, device or directory to mount into the container.',
>      },
> @@ -367,10 +368,25 @@ for (my $i = 0; $i < $MAX_LXC_NETWORKS; $i++) {
>      };
>  }
>  
> +PVE::JSONSchema::register_format('pve-lxc-mp-string', \&pve_lxc_mp_string);
> +sub pve_lxc_mp_string{

Please prefix the name with 'verify_' (we only left out the prefix when
the format was a json-schema hash, otherwise we use verify_* pretty much
everywhere).

> +    my ($mp, $noerr) = @_;
> +
> +    # do not allow /./ or /../ in path
> +    # also do not allow /. or /.. at the end of the path
> +    if($mp =~ m@/\.(\.)?/@ ||
> +       $mp =~ m@/\.(\.)?$@){

The parenthesis around the second dot aren't needed, the '?' will
attach to only one element right before it.

We should also forbid '../' at the beginning.

> +	return undef if $noerr;
> +	die "$mp contains illegal character sequences\n";
> +    }
> +    return $mp;
> +}
> +
>  my $mp_desc = {
>      %$rootfs_desc,
>      mp => {
>  	type => 'string',
> +	format => 'pve-lxc-mp-string',
>  	format_description => 'Path',
>  	description => 'Path to the mountpoint as seen from inside the container.',
>      },
> @@ -2024,18 +2040,6 @@ sub mountpoint_names {
>      return $reverse ? reverse @names : @names;
>  }
>  
> -# The container might have *different* symlinks than the host. realpath/abs_path
> -# use the actual filesystem to resolve links.
> -sub sanitize_mountpoint {
> -    my ($mp) = @_;
> -    $mp = '/' . $mp; # we always start with a slash
> -    $mp =~ s@/{2,}@/@g; # collapse sequences of slashes
> -    $mp =~ s@/\./@@g; # collapse /./
> -    $mp =~ s@/\.(/)?$@$1@; # collapse a trailing /. or /./
> -    $mp =~ s@(.*)/[^/]+/\.\./@$1/@g; # collapse /../ without regard for symlinks
> -    $mp =~ s@/\.\.(/)?$@$1@; # collapse trailing /.. or /../ disregarding symlinks
> -    return $mp;
> -}
>  
>  sub foreach_mountpoint_full {
>      my ($conf, $reverse, $func) = @_;
> @@ -2046,11 +2050,6 @@ sub foreach_mountpoint_full {
>  	my $mountpoint = $key eq 'rootfs' ? parse_ct_rootfs($value, 1) : parse_ct_mountpoint($value, 1);
>  	next if !defined($mountpoint);
>  
> -	$mountpoint->{mp} = sanitize_mountpoint($mountpoint->{mp});
> -
> -	my $path = $mountpoint->{volume};
> -	$mountpoint->{volume} = sanitize_mountpoint($path) if $path =~ m|^/|;
> -
>  	&$func($key, $mountpoint);
>      }
>  }
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>