'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.gruenbich...@proxmox.com>
Signed-off-by: Fiona Ebner <f.eb...@proxmox.com>
---
New in v3.
src/PVE/LXC/Create.pm | 67 ++++++++++++++++++++++++++++++++++++++++---
1 file changed, 63 insertions(+), 4 deletions(-)
diff --git a/src/PVE/LXC/Create.pm b/src/PVE/LXC/Create.pm
index d2f675e..bf424f6 100644
--- a/src/PVE/LXC/Create.pm
+++ b/src/PVE/LXC/Create.pm
@@ -99,12 +99,65 @@ 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) = @_;
+
+ # 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+(\d+)\s+(\S+)\s+(\S+)\s+(.*)$!;
+
+ 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;
+ }
+
+ $member_count++;
+ };
+
+ 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 +180,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 +195,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 +228,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 +247,7 @@ sub restore_external_archive {
'.',
];
+ # archive is trusted, we created it
my $extract_cmd = restore_tar_archive_command($conf, undef,
$rootdir, $bwlimit);
eval { PVE::Tools::run_command([$create_cmd, $extract_cmd]); };
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
