[pve-devel] [PATCH container v6 34/37] restore tar archive: check potentially untrusted archive

[Fiona Ebner <f.ebner@proxmox.com>]

'tar' itself already protects against '..' in component names and
strips absolute member names when extracting (if not used with the
--absolute-names option) and in general seems sane for extracting.
Additionally, the extraction already happens in the user namespace
associated to the container. So for now, start out with some basic
sanity checks. The helper can still be extended with more checks.

Checks:

* list files in archive - will already catch many corrupted/bogus
  archives.

* check that there are at least 10 members - should also catch
  archives not actually containing a container root filesystem or
  structural issues early.

* check that /sbin directory or link exists in archive - ideally the
  check would be for /sbin/init, but this cannot be done efficiently
  before extraction, because it would require to keep track of the
  whole archive to be able to follow symlinks.

* abort if there is a multi-volume member in the archive - cheap and
  is never expected.

Checks that were considered, but not (yet) added:

* abort when a file has unrealistically large size - while this could
  help to detect certain kinds of bogus archives, there can be valid.
  use cases for extremely large sparse files, so it's not clear what
  a good limit would be (1 EiB maybe?). Also, an attacker could just
  adapt to such a limit creating multiple files and the actual
  extraction is already limited by the size of the allocated container
  volume.

* check that /sbin/init exists after extracting - cannot be done
  efficiently before extraction, because it would require to keep
  track of the whole archive to be able to follow symlinks. During
  setup there already is detection of /etc/os-release, so issues with
  the structure will already be noticed. Adding a hard fail for
  untrusted archives would require either passing that information to
  the setup phase or extracting the protected_call method from there
  into a helper.

* adding 'restrict' to the (common) tar flags - the tar manual (not
  the man page) documents: "Disable use of some potentially harmful
  'tar' options.  Currently this option disables shell invocation from
  multi-volume menu.". The flag was introduced in 2005 and this is
  still the only thing it is used for. Trying to restore a
  multi-volume archive already fails without giving multiple '--file'
  arguments and '--multi-volume', so don't bother adding the flag.

* check format of tar file - would require yet another invocation of
  the decompressor and there seems to be no built-in way to just
  display the format with 'tar'. The 'file' program could be used, but
  it seems to not make a distinction between old GNU and GNU and old
  POSIX and POSIX formats, with the old ones being candidates to
  prohibit. So that would leave just detecting the old 'v7' format.

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/LXC/Create.pm | 75 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 71 insertions(+), 4 deletions(-)

diff --git a/src/PVE/LXC/Create.pm b/src/PVE/LXC/Create.pm
index 43fc5fe..53c584b 100644
--- a/src/PVE/LXC/Create.pm
+++ b/src/PVE/LXC/Create.pm
@@ -99,12 +99,73 @@ my sub tar_compression_option {
     }
 }
 
+# Basic checks trying to detect issues with a potentially untrusted or bogus tar archive.
+# Just listing the files is already a good check against corruption.
+# 'tar' itself already protects against '..' in component names and strips absolute member names
+# when extracting, so no need to check for those here.
+my sub check_tar_archive {
+    my ($archive) = @_;
+
+    print "checking archive..\n";
+
+    # To resolve links to get to 'sbin/init' would mean keeping track of everything in the archive,
+    # because the target might be ordered first. Check only that 'sbin' exists here.
+    my $found_sbin;
+
+    # Just to detect bogus archives, any valid container filesystem should have more than this.
+    my $required_members = 10;
+    my $member_count = 0;
+
+    my $check_file_list = sub {
+	my ($line) = @_;
+
+	$member_count++;
+
+	# Not always just a single number, e.g. for character devices.
+	my $size_re = qr/\d+(?:,\d+)?/;
+
+	# The date is in ISO 8601 format. The last part contains the potentially quoted file name,
+	# potentially followed by some additional info (e.g. where a link points to).
+	my ($type, $perms, $uid, $gid, $size, $date, $time, $file_info) =
+	    $line =~ m!^([a-zA-Z\-])(\S+)\s+(\d+)/(\d+)\s+($size_re)\s+(\S+)\s+(\S+)\s+(.*)$!;
+	if (!defined($type)) {
+	    print "check tar: unable to parse line: $line\n";
+	    return;
+	}
+
+	die "found multi-volume member in archive\n" if $type eq 'M';
+
+	if (!$found_sbin && (
+	    ($file_info =~ m!^(?:\./)?sbin/$! && $type eq 'd')
+	    || ($file_info =~ m!^(?:\./)?sbin ->! && $type eq 'l')
+	    || ($file_info =~ m!^(?:\./)?sbin link to! && $type eq 'h')
+	)) {
+	    $found_sbin = 1;
+	}
+
+    };
+
+    my $compression_opt = tar_compression_option($archive);
+
+    my $cmd = ['tar', '-tvf', $archive];
+    push $cmd->@*, $compression_opt if $compression_opt;
+    push $cmd->@*, '--numeric-owner';
+
+    PVE::Tools::run_command($cmd, outfunc => $check_file_list);
+
+    die "no 'sbin' directory (or link) found in archive '$archive'\n" if !$found_sbin;
+    die "less than 10 members in archive '$archive'\n" if $member_count < $required_members;
+}
+
 my sub restore_tar_archive_command {
-    my ($conf, $compression_opt, $rootdir, $bwlimit) = @_;
+    my ($conf, $compression_opt, $rootdir, $bwlimit, $untrusted) = @_;
 
     my ($id_map, $root_uid, $root_gid) = PVE::LXC::parse_id_maps($conf);
     my $userns_cmd = PVE::LXC::userns_command($id_map);
 
+    die "refusing to restore privileged container backup from external source\n"
+	if $untrusted && ($root_uid == 0 || $root_gid == 0);
+
     my $cmd = [@$userns_cmd, 'tar', 'xpf', '-'];
     push $cmd->@*, $compression_opt if $compression_opt;
     push $cmd->@*, '--totals';
@@ -127,7 +188,7 @@ my sub restore_tar_archive_command {
 }
 
 sub restore_tar_archive {
-    my ($archive, $rootdir, $conf, $no_unpack_error, $bwlimit) = @_;
+    my ($archive, $rootdir, $conf, $no_unpack_error, $bwlimit, $untrusted) = @_;
 
     my $archive_fh;
     my $tar_input = '<&STDIN';
@@ -142,7 +203,12 @@ sub restore_tar_archive {
 	$tar_input = '<&'.fileno($archive_fh);
     }
 
-    my $cmd = restore_tar_archive_command($conf, $compression_opt, $rootdir, $bwlimit);
+    if ($untrusted) {
+	die "cannot verify untrusted archive on STDIN\n" if $archive eq '-';
+	check_tar_archive($archive);
+    }
+
+    my $cmd = restore_tar_archive_command($conf, $compression_opt, $rootdir, $bwlimit, $untrusted);
 
     if ($archive eq '-') {
 	print "extracting archive from STDIN\n";
@@ -170,7 +236,7 @@ sub restore_external_archive {
 	    my $tar_path = $info->{'tar-path'}
 		or die "did not get path to tar file from backup provider\n";
 	    die "not a regular file '$tar_path'" if !-f $tar_path;
-	    restore_tar_archive($tar_path, $rootdir, $conf, $no_unpack_error, $bwlimit);
+	    restore_tar_archive($tar_path, $rootdir, $conf, $no_unpack_error, $bwlimit, 1);
 	} elsif ($mechanism eq 'directory') {
 	    my $directory = $info->{'archive-directory'}
 		or die "did not get path to archive directory from backup provider\n";
@@ -189,6 +255,7 @@ sub restore_external_archive {
 		'.',
 	    ];
 
+	    # archive is trusted, we created it
 	    my $extract_cmd = restore_tar_archive_command($conf, undef, $rootdir, $bwlimit);
 
 	    my $cmd;
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel