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

[Dominik Csapak]

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