2f1b9b
From 5c79b9faae0f1dd67cc8288964c72c12e03884f8 Mon Sep 17 00:00:00 2001
2f1b9b
From: =?UTF-8?q?Petr=20P=C3=ADsa=C5=99?= <ppisar@redhat.com>
2f1b9b
Date: Fri, 15 Jun 2018 14:49:47 +0200
2f1b9b
Subject: [PATCH] Prevent from traversing symlinks and parent directories when
2f1b9b
 extracting
2f1b9b
MIME-Version: 1.0
2f1b9b
Content-Type: text/plain; charset=UTF-8
2f1b9b
Content-Transfer-Encoding: 8bit
2f1b9b
2f1b9b
If an attacker-supplied archive contains symbolic links and files that
2f1b9b
referes to the symbolic links in their path components, the user can
2f1b9b
be tricked into overwriting any arbitrary file.
2f1b9b
2f1b9b
The same issue is with archives whose members refer to a parent
2f1b9b
directory (..) in their path components.
2f1b9b
2f1b9b
This patch fixes it by aborting an extraction (extractTree(),
2f1b9b
extractMember(), extractMemberWithoutPaths()) in those cases by not
2f1b9b
traversing the dangerous paths and returning AZ_ERORR instead.
2f1b9b
2f1b9b
However, if a user supplies a local file name, the security checks are
2f1b9b
not performed. This is based on the assumption that a user knows
2f1b9b
what's on his local file system.
2f1b9b
2f1b9b
CVE-2018-10860
2f1b9b
https://bugzilla.redhat.com/show_bug.cgi?id=1591449
2f1b9b
2f1b9b
Signed-off-by: Petr Písař <ppisar@redhat.com>
2f1b9b
---
2f1b9b
 MANIFEST                               |   3 +
2f1b9b
 lib/Archive/Zip.pm                     |   8 ++
2f1b9b
 lib/Archive/Zip/Archive.pm             |  37 +++++++
2f1b9b
 t/25_traversal.t                       | 189 +++++++++++++++++++++++++++++++++
2f1b9b
 t/data/dotdot-from-unexistant-path.zip | Bin 0 -> 245 bytes
2f1b9b
 t/data/link-dir.zip                    | Bin 0 -> 260 bytes
2f1b9b
 t/data/link-samename.zip               | Bin 0 -> 257 bytes
2f1b9b
 7 files changed, 237 insertions(+)
2f1b9b
 create mode 100644 t/25_traversal.t
2f1b9b
 create mode 100644 t/data/dotdot-from-unexistant-path.zip
2f1b9b
 create mode 100644 t/data/link-dir.zip
2f1b9b
 create mode 100644 t/data/link-samename.zip
2f1b9b
2f1b9b
diff --git a/MANIFEST b/MANIFEST
2f1b9b
index 2e9655d..a1bd7d6 100644
2f1b9b
--- a/MANIFEST
2f1b9b
+++ b/MANIFEST
2f1b9b
@@ -59,6 +59,7 @@ t/21_zip64.t
2f1b9b
 t/22_deflated_dir.t
2f1b9b
 t/23_closed_handle.t
2f1b9b
 t/24_unicode_win32.t
2f1b9b
+t/25_traversal.t
2f1b9b
 t/badjpeg/expected.jpg
2f1b9b
 t/badjpeg/source.zip
2f1b9b
 t/common.pm
2f1b9b
@@ -68,6 +69,7 @@ t/data/crypcomp.zip
2f1b9b
 t/data/crypt.zip
2f1b9b
 t/data/def.zip
2f1b9b
 t/data/defstr.zip
2f1b9b
+t/data/dotdot-from-unexistant-path.zip
2f1b9b
 t/data/empty.zip
2f1b9b
 t/data/emptydef.zip
2f1b9b
 t/data/emptydefstr.zip
2f1b9b
@@ -75,6 +77,7 @@ t/data/emptystore.zip
2f1b9b
 t/data/emptystorestr.zip
2f1b9b
 t/data/good_github11.zip
2f1b9b
 t/data/jar.zip
2f1b9b
+t/data/link-dir.zip
2f1b9b
 t/data/linux.zip
2f1b9b
 t/data/mkzip.pl
2f1b9b
 t/data/perl.zip
2f1b9b
diff --git a/lib/Archive/Zip.pm b/lib/Archive/Zip.pm
2f1b9b
index ca82e31..907808b 100644
2f1b9b
--- a/lib/Archive/Zip.pm
2f1b9b
+++ b/lib/Archive/Zip.pm
2f1b9b
@@ -1145,6 +1145,9 @@ member is used as the name of the extracted file or
2f1b9b
 directory.
2f1b9b
 If you pass C<$extractedName>, it should be in the local file
2f1b9b
 system's format.
2f1b9b
+If you do not pass C<$extractedName> and the internal filename traverses
2f1b9b
+a parent directory or a symbolic link, the extraction will be aborted with
2f1b9b
+C<AC_ERROR> for security reason.
2f1b9b
 All necessary directories will be created. Returns C<AZ_OK>
2f1b9b
 on success.
2f1b9b
 
2f1b9b
@@ -1162,6 +1165,9 @@ extracted member (its paths will be deleted too). Otherwise,
2f1b9b
 the internal filename of the member (minus paths) is used as
2f1b9b
 the name of the extracted file or directory. Returns C<AZ_OK>
2f1b9b
 on success.
2f1b9b
+If you do not pass C<$extractedName> and the internal filename is equalled
2f1b9b
+to a local symbolic link, the extraction will be aborted with C<AC_ERROR> for
2f1b9b
+security reason.
2f1b9b
 
2f1b9b
 =item addMember( $member )
2f1b9b
 
2f1b9b
@@ -1609,6 +1615,8 @@ a/x to f:\d\e\x
2f1b9b
 
2f1b9b
 a/b/c to f:\d\e\b\c and ignore ax/d/e and d/e
2f1b9b
 
2f1b9b
+If the path to the extracted file traverses a parent directory or a symbolic
2f1b9b
+link, the extraction will be aborted with C<AC_ERROR> for security reason.
2f1b9b
 Returns an error code or AZ_OK if everything worked OK.
2f1b9b
 
2f1b9b
 =back
2f1b9b
diff --git a/lib/Archive/Zip/Archive.pm b/lib/Archive/Zip/Archive.pm
2f1b9b
index 48f0d1a..b0d3e46 100644
2f1b9b
--- a/lib/Archive/Zip/Archive.pm
2f1b9b
+++ b/lib/Archive/Zip/Archive.pm
2f1b9b
@@ -185,6 +185,8 @@ sub extractMember {
2f1b9b
         $dirName = File::Spec->catpath($volumeName, $dirName, '');
2f1b9b
     } else {
2f1b9b
         $name = $member->fileName();
2f1b9b
+        if ((my $ret = _extractionNameIsSafe($name))
2f1b9b
+            != AZ_OK) { return $ret; }
2f1b9b
         ($dirName = $name) =~ s{[^/]*$}{};
2f1b9b
         $dirName = Archive::Zip::_asLocalName($dirName);
2f1b9b
         $name    = Archive::Zip::_asLocalName($name);
2f1b9b
@@ -218,6 +220,8 @@ sub extractMemberWithoutPaths {
2f1b9b
     unless ($name) {
2f1b9b
         $name = $member->fileName();
2f1b9b
         $name =~ s{.*/}{};    # strip off directories, if any
2f1b9b
+        if ((my $ret = _extractionNameIsSafe($name))
2f1b9b
+            != AZ_OK) { return $ret; }
2f1b9b
         $name = Archive::Zip::_asLocalName($name);
2f1b9b
     }
2f1b9b
     my $rc = $member->extractToFileNamed($name, @_);
2f1b9b
@@ -827,6 +831,37 @@ sub addTreeMatching {
2f1b9b
     return $self->addTree($root, $dest, $matcher, $compressionLevel);
2f1b9b
 }
2f1b9b
 
2f1b9b
+# Check if one of the components of a path to the file or the file name
2f1b9b
+# itself is an already existing symbolic link. If yes then return an
2f1b9b
+# error. Continuing and writing to a file traversing a link posseses
2f1b9b
+# a security threat, especially if the link was extracted from an
2f1b9b
+# attacker-supplied archive. This would allow writing to an arbitrary
2f1b9b
+# file. The same applies when using ".." to escape from a working
2f1b9b
+# directory. <https://bugzilla.redhat.com/show_bug.cgi?id=1591449>
2f1b9b
+sub _extractionNameIsSafe {
2f1b9b
+    my $name = shift;
2f1b9b
+    my ($volume, $directories) = File::Spec->splitpath($name, 1);
2f1b9b
+    my @directories = File::Spec->splitdir($directories);
2f1b9b
+    if (grep '..' eq $_, @directories) {
2f1b9b
+        return _error(
2f1b9b
+            "Could not extract $name safely: a parent directory is used");
2f1b9b
+    }
2f1b9b
+    my @path;
2f1b9b
+    my $path;
2f1b9b
+    for my $directory (@directories) {
2f1b9b
+        push @path, $directory;
2f1b9b
+        $path = File::Spec->catpath($volume, File::Spec->catdir(@path), '');
2f1b9b
+        if (-l $path) {
2f1b9b
+            return _error(
2f1b9b
+                "Could not extract $name safely: $path is an existing symbolic link");
2f1b9b
+        }
2f1b9b
+        if (!-e $path) {
2f1b9b
+            last;
2f1b9b
+        }
2f1b9b
+    }
2f1b9b
+    return AZ_OK;
2f1b9b
+}
2f1b9b
+
2f1b9b
 # $zip->extractTree( $root, $dest [, $volume] );
2f1b9b
 #
2f1b9b
 # $root and $dest are Unix-style.
2f1b9b
@@ -861,6 +896,8 @@ sub extractTree {
2f1b9b
         $fileName =~ s{$pattern}{$dest};       # in Unix format
2f1b9b
                                                # convert to platform format:
2f1b9b
         $fileName = Archive::Zip::_asLocalName($fileName, $volume);
2f1b9b
+        if ((my $ret = _extractionNameIsSafe($fileName))
2f1b9b
+            != AZ_OK) { return $ret; }
2f1b9b
         my $status = $member->extractToFileNamed($fileName);
2f1b9b
         return $status if $status != AZ_OK;
2f1b9b
     }
2f1b9b
diff --git a/t/25_traversal.t b/t/25_traversal.t
2f1b9b
new file mode 100644
2f1b9b
index 0000000..d03dede
2f1b9b
--- /dev/null
2f1b9b
+++ b/t/25_traversal.t
2f1b9b
@@ -0,0 +1,189 @@
2f1b9b
+use strict;
2f1b9b
+use warnings;
2f1b9b
+
2f1b9b
+use Archive::Zip qw( :ERROR_CODES );
2f1b9b
+use File::Spec;
2f1b9b
+use File::Path;
2f1b9b
+use lib 't';
2f1b9b
+use common;
2f1b9b
+
2f1b9b
+use Test::More tests => 41;
2f1b9b
+
2f1b9b
+# These tests check for CVE-2018-10860 vulnerabilities.
2f1b9b
+# If an archive contains a symlink and then a file that traverses that symlink,
2f1b9b
+# extracting the archive tree could write into an abitrary file selected by
2f1b9b
+# the symlink value.
2f1b9b
+# Another issue is if an archive contains a file whose path component refers
2f1b9b
+# to a parent direcotory. Then extracting that file could write into a file
2f1b9b
+# out of current working directory subtree.
2f1b9b
+# These tests check extracting of these files is refuses and that they are
2f1b9b
+# indeed not created.
2f1b9b
+
2f1b9b
+# Suppress croaking errors, the tests produce some.
2f1b9b
+Archive::Zip::setErrorHandler(sub {});
2f1b9b
+my ($existed, $ret, $zip, $allowed_file, $forbidden_file);
2f1b9b
+
2f1b9b
+# Change working directory to a temporary directory because some tested
2f1b9b
+# functions operarates there and we need prepared symlinks there.
2f1b9b
+my @data_path = (File::Spec->splitdir(File::Spec->rel2abs('.')), 't', 'data');
2f1b9b
+ok(chdir TESTDIR, "Working directory changed");
2f1b9b
+
2f1b9b
+# Case 1:
2f1b9b
+#   link-dir -> /tmp
2f1b9b
+#   link-dir/gotcha-linkdir
2f1b9b
+# writes into /tmp/gotcha-linkdir file.
2f1b9b
+SKIP: {
2f1b9b
+    # Symlink tests make sense only if a file system supports them.
2f1b9b
+    my $link = 'trylink';
2f1b9b
+    $ret = eval { symlink('.', $link)};
2f1b9b
+    skip 'Symbolic links are not supported', 12 if $@;
2f1b9b
+    unlink $link;
2f1b9b
+
2f1b9b
+    # Extracting an archive tree must fail
2f1b9b
+    $zip = Archive::Zip->new();
2f1b9b
+    isa_ok($zip, 'Archive::Zip');
2f1b9b
+    is($zip->read(File::Spec->catfile(@data_path, 'link-dir.zip')), AZ_OK,
2f1b9b
+        'Archive read');
2f1b9b
+    $existed = -e File::Spec->catfile('', 'tmp', 'gotcha-linkdir');
2f1b9b
+    $ret = eval { $zip->extractTree() };
2f1b9b
+    is($ret, AZ_ERROR, 'Tree extraction aborted');
2f1b9b
+    SKIP: {
2f1b9b
+        skip 'A canary file existed before the test', 1 if $existed;
2f1b9b
+        ok(! -e File::Spec->catfile('link-dir', 'gotcha-linkdir'),
2f1b9b
+            'A file was not created in a symlinked directory');
2f1b9b
+    }
2f1b9b
+    ok(unlink(File::Spec->catfile('link-dir')), 'link-dir removed');
2f1b9b
+
2f1b9b
+    # The same applies to extracting an archive member without an explicit
2f1b9b
+    # local file name. It must abort.
2f1b9b
+    $link = 'link-dir';
2f1b9b
+    ok(symlink('.', $link), 'A symlink to a directory created');
2f1b9b
+    $forbidden_file = File::Spec->catfile($link, 'gotcha-linkdir');
2f1b9b
+    $existed = -e $forbidden_file;
2f1b9b
+    $ret = eval { $zip->extractMember('link-dir/gotcha-linkdir') };
2f1b9b
+    is($ret, AZ_ERROR, 'Member extraction without a local name aborted');
2f1b9b
+    SKIP: {
2f1b9b
+        skip 'A canary file existed before the test', 1 if $existed;
2f1b9b
+        ok(! -e $forbidden_file,
2f1b9b
+            'A file was not created in a symlinked directory');
2f1b9b
+    }
2f1b9b
+
2f1b9b
+    # But allow extracting an archive member into a supplied file name
2f1b9b
+    $allowed_file = File::Spec->catfile($link, 'file');
2f1b9b
+    $ret = eval { $zip->extractMember('link-dir/gotcha-linkdir', $allowed_file) };
2f1b9b
+    is($ret, AZ_OK, 'Member extraction passed');
2f1b9b
+    ok(-e $allowed_file, 'File created');
2f1b9b
+    ok(unlink($allowed_file), 'File removed');
2f1b9b
+    ok(unlink($link), 'A symlink to a directory removed');
2f1b9b
+}
2f1b9b
+
2f1b9b
+# Case 2:
2f1b9b
+#   unexisting/../../../../../tmp/gotcha-dotdot-unexistingpath
2f1b9b
+# writes into ../../../../tmp/gotcha-dotdot-unexistingpath, that is
2f1b9b
+# /tmp/gotcha-dotdot-unexistingpath file if CWD is not deeper than
2f1b9b
+# 4 directories.
2f1b9b
+$zip = Archive::Zip->new();
2f1b9b
+isa_ok($zip, 'Archive::Zip');
2f1b9b
+is($zip->read(File::Spec->catfile(@data_path,
2f1b9b
+            'dotdot-from-unexistant-path.zip')), AZ_OK, 'Archive read');
2f1b9b
+$forbidden_file = File::Spec->catfile('..', '..', '..', '..', 'tmp',
2f1b9b
+    'gotcha-dotdot-unexistingpath');
2f1b9b
+$existed = -e $forbidden_file;
2f1b9b
+$ret = eval { $zip->extractTree() };
2f1b9b
+is($ret, AZ_ERROR, 'Tree extraction aborted');
2f1b9b
+SKIP: {
2f1b9b
+    skip 'A canary file existed before the test', 1 if $existed;
2f1b9b
+    ok(! -e $forbidden_file, 'A file was not created in a parent directory');
2f1b9b
+}
2f1b9b
+
2f1b9b
+# The same applies to extracting an archive member without an explicit local
2f1b9b
+# file name. It must abort.
2f1b9b
+$existed = -e $forbidden_file;
2f1b9b
+$ret = eval { $zip->extractMember(
2f1b9b
+        'unexisting/../../../../../tmp/gotcha-dotdot-unexistingpath',
2f1b9b
+    ) };
2f1b9b
+is($ret, AZ_ERROR, 'Member extraction without a local name aborted');
2f1b9b
+SKIP: {
2f1b9b
+    skip 'A canary file existed before the test', 1 if $existed;
2f1b9b
+    ok(! -e $forbidden_file, 'A file was not created in a parent directory');
2f1b9b
+}
2f1b9b
+
2f1b9b
+# But allow extracting an archive member into a supplied file name
2f1b9b
+ok(mkdir('directory'), 'Directory created');
2f1b9b
+$allowed_file = File::Spec->catfile('directory', '..', 'file');
2f1b9b
+$ret = eval { $zip->extractMember(
2f1b9b
+        'unexisting/../../../../../tmp/gotcha-dotdot-unexistingpath',
2f1b9b
+        $allowed_file
2f1b9b
+    ) };
2f1b9b
+is($ret, AZ_OK, 'Member extraction passed');
2f1b9b
+ok(-e $allowed_file, 'File created');
2f1b9b
+ok(unlink($allowed_file), 'File removed');
2f1b9b
+
2f1b9b
+# Case 3:
2f1b9b
+#   link-file -> /tmp/gotcha-samename
2f1b9b
+#   link-file
2f1b9b
+# writes into /tmp/gotcha-samename. It must abort. (Or replace the symlink in
2f1b9b
+# more relaxed mode in the future.)
2f1b9b
+$zip = Archive::Zip->new();
2f1b9b
+isa_ok($zip, 'Archive::Zip');
2f1b9b
+is($zip->read(File::Spec->catfile(@data_path, 'link-samename.zip')), AZ_OK,
2f1b9b
+    'Archive read');
2f1b9b
+$existed = -e File::Spec->catfile('', 'tmp', 'gotcha-samename');
2f1b9b
+$ret = eval { $zip->extractTree() };
2f1b9b
+is($ret, AZ_ERROR, 'Tree extraction aborted');
2f1b9b
+SKIP: {
2f1b9b
+    skip 'A canary file existed before the test', 1 if $existed;
2f1b9b
+    ok(! -e File::Spec->catfile('', 'tmp', 'gotcha-samename'),
2f1b9b
+        'A file was not created through a symlinked file');
2f1b9b
+}
2f1b9b
+ok(unlink(File::Spec->catfile('link-file')), 'link-file removed');
2f1b9b
+
2f1b9b
+# The same applies to extracting an archive member using extractMember()
2f1b9b
+# without an explicit local file name. It must abort.
2f1b9b
+my $link = 'link-file';
2f1b9b
+my $target = 'target';
2f1b9b
+ok(symlink($target, $link), 'A symlink to a file created');
2f1b9b
+$forbidden_file = File::Spec->catfile($target);
2f1b9b
+$existed = -e $forbidden_file;
2f1b9b
+# Select a member by order due to same file names.
2f1b9b
+my $member = ${[$zip->members]}[1];
2f1b9b
+ok($member, 'A member to extract selected');
2f1b9b
+$ret = eval { $zip->extractMember($member) };
2f1b9b
+is($ret, AZ_ERROR,
2f1b9b
+    'Member extraction using extractMember() without a local name aborted');
2f1b9b
+SKIP: {
2f1b9b
+    skip 'A canary file existed before the test', 1 if $existed;
2f1b9b
+    ok(! -e $forbidden_file,
2f1b9b
+        'A symlinked target file was not created');
2f1b9b
+}
2f1b9b
+
2f1b9b
+# But allow extracting an archive member using extractMember() into a supplied
2f1b9b
+# file name.
2f1b9b
+$allowed_file = $target;
2f1b9b
+$ret = eval { $zip->extractMember($member, $allowed_file) };
2f1b9b
+is($ret, AZ_OK, 'Member extraction using extractMember() passed');
2f1b9b
+ok(-e $allowed_file, 'File created');
2f1b9b
+ok(unlink($allowed_file), 'File removed');
2f1b9b
+
2f1b9b
+# The same applies to extracting an archive member using
2f1b9b
+# extractMemberWithoutPaths() without an explicit local file name.
2f1b9b
+# It must abort.
2f1b9b
+$existed = -e $forbidden_file;
2f1b9b
+# Select a member by order due to same file names.
2f1b9b
+$ret = eval { $zip->extractMemberWithoutPaths($member) };
2f1b9b
+is($ret, AZ_ERROR,
2f1b9b
+    'Member extraction using extractMemberWithoutPaths() without a local name aborted');
2f1b9b
+SKIP: {
2f1b9b
+    skip 'A canary file existed before the test', 1 if $existed;
2f1b9b
+    ok(! -e $forbidden_file,
2f1b9b
+        'A symlinked target file was not created');
2f1b9b
+}
2f1b9b
+
2f1b9b
+# But allow extracting an archive member using extractMemberWithoutPaths()
2f1b9b
+# into a supplied file name.
2f1b9b
+$allowed_file = $target;
2f1b9b
+$ret = eval { $zip->extractMemberWithoutPaths($member, $allowed_file) };
2f1b9b
+is($ret, AZ_OK, 'Member extraction using extractMemberWithoutPaths() passed');
2f1b9b
+ok(-e $allowed_file, 'File created');
2f1b9b
+ok(unlink($allowed_file), 'File removed');
2f1b9b
+ok(unlink($link), 'A symlink to a file removed');
2f1b9b
diff --git a/t/data/dotdot-from-unexistant-path.zip b/t/data/dotdot-from-unexistant-path.zip
2f1b9b
new file mode 100644
2f1b9b
index 0000000000000000000000000000000000000000..faaa5bb95c4310ad3dfa8ea7bbad6850da3f2095
2f1b9b
GIT binary patch
2f1b9b
literal 245
2f1b9b
zcmWIWW@Zs#0D%jBS9~Vyb&-_^vO(Aih)eTQD>92qGV{{)_4H6sNp69DdVWcAMxt&?
2f1b9b
zehCoiBGeWnmSjNWtQ7S06v{J8G87Q93LxnKZ$>5&X51D7?FNHwjUWo48O04iClPW+
2f1b9b
TfHx}}$OJ|p%mC8mAPxfn{*XXp
2f1b9b
2f1b9b
literal 0
2f1b9b
HcmV?d00001
2f1b9b
2f1b9b
diff --git a/t/data/link-dir.zip b/t/data/link-dir.zip
2f1b9b
new file mode 100644
2f1b9b
index 0000000000000000000000000000000000000000..99fbb437ec0bd694b8122cdb1ce8221a3da2e453
2f1b9b
GIT binary patch
2f1b9b
literal 260
2f1b9b
zcmWIWW@Zs#0D
2f1b9b
z`6bC2iMk-2K#dTdLRn^_0+6Qw66Ff;W@Hj!#%(FkG%)zT5JbV8fUXPO0T4Y54BHyD
2f1b9b
ZkaX#zIw!!Jl?|kj2?(o!bTNp-004oiIM)CG
2f1b9b
2f1b9b
literal 0
2f1b9b
HcmV?d00001
2f1b9b
2f1b9b
diff --git a/t/data/link-samename.zip b/t/data/link-samename.zip
2f1b9b
new file mode 100644
2f1b9b
index 0000000000000000000000000000000000000000..e9036c0348f5fb9536cb7ee0f2c09b9ef595a12c
2f1b9b
GIT binary patch
2f1b9b
literal 257
2f1b9b
zcmWIWW@Zs#00HsHOFm_vR
2f1b9b
zfTHz8cF##^pcW8D(F)O}P?njf0Me-o(wd?GGMOvDn~_O`8MpO7qrl+*LJ$Ra47xUS
2f1b9b
bt09^g7`8Q9qiSPi14%IfVIGjK1#uVvx^_1y
2f1b9b
2f1b9b
literal 0
2f1b9b
HcmV?d00001
2f1b9b
2f1b9b
-- 
2f1b9b
2.14.4
2f1b9b