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

[Wolfgang Bumiller]

On Thu, Feb 04, 2016 at 01:40:15PM +0100, Dominik Csapak wrote:
> changes from v1:
> renamed function to verify_*
> added check for ../ at the beginning
> cleaned up regex (\.)? -> \.?
> 
> 
> 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 or
> ../ at the beginning) 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>

Acked-by: Wolfgang Bumiller <w.bumiller at proxmox.com>

Please write the patch changelog after the '---'-line below next time
so they don't get included in the commit message when applying with
git-am.

> ---
>  src/PVE/LXC.pm | 37 ++++++++++++++++++++-----------------
>  1 file changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index f761f33..0ea30f2 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,29 @@ for (my $i = 0; $i < $MAX_LXC_NETWORKS; $i++) {
>      };
>  }
>  
> +PVE::JSONSchema::register_format('pve-lxc-mp-string', \&verify_lxc_mp_string);
> +sub verify_lxc_mp_string{
> +    my ($mp, $noerr) = @_;
> +
> +    # do not allow:
> +    # /./ or /../ 
> +    # /. or /.. at the end
> +    # ../ at the beginning
> +    
> +    if($mp =~ m@/\.\.?/@ ||
> +       $mp =~ m@/\.\.?$@ ||
> +       $mp =~ m@^\.\./@){
> +	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 +2044,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 +2054,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
>