diff --git a/MANIFEST b/MANIFEST index dd9675a..37d8b8d 100644 --- a/MANIFEST +++ b/MANIFEST @@ -59,7 +59,6 @@ t/21_zip64.t t/22_deflated_dir.t t/23_closed_handle.t t/24_unicode_win32.t -t/25_traversal.t t/badjpeg/expected.jpg t/badjpeg/source.zip t/common.pm @@ -69,7 +68,6 @@ t/data/crypcomp.zip t/data/crypt.zip t/data/def.zip t/data/defstr.zip -t/data/dotdot-from-unexistant-path.zip t/data/empty.zip t/data/emptydef.zip t/data/emptydefstr.zip @@ -77,7 +75,6 @@ t/data/emptystore.zip t/data/emptystorestr.zip t/data/good_github11.zip t/data/jar.zip -t/data/link-dir.zip t/data/linux.zip t/data/mkzip.pl t/data/perl.zip diff --git a/lib/Archive/Zip.pm b/lib/Archive/Zip.pm index 907808b..ca82e31 100644 --- a/lib/Archive/Zip.pm +++ b/lib/Archive/Zip.pm @@ -1145,9 +1145,6 @@ member is used as the name of the extracted file or directory. If you pass C<$extractedName>, it should be in the local file system's format. -If you do not pass C<$extractedName> and the internal filename traverses -a parent directory or a symbolic link, the extraction will be aborted with -C for security reason. All necessary directories will be created. Returns C on success. @@ -1165,9 +1162,6 @@ extracted member (its paths will be deleted too). Otherwise, the internal filename of the member (minus paths) is used as the name of the extracted file or directory. Returns C on success. -If you do not pass C<$extractedName> and the internal filename is equalled -to a local symbolic link, the extraction will be aborted with C for -security reason. =item addMember( $member ) @@ -1615,8 +1609,6 @@ a/x to f:\d\e\x a/b/c to f:\d\e\b\c and ignore ax/d/e and d/e -If the path to the extracted file traverses a parent directory or a symbolic -link, the extraction will be aborted with C for security reason. Returns an error code or AZ_OK if everything worked OK. =back diff --git a/lib/Archive/Zip/Archive.pm b/lib/Archive/Zip/Archive.pm index b0d3e46..48f0d1a 100644 --- a/lib/Archive/Zip/Archive.pm +++ b/lib/Archive/Zip/Archive.pm @@ -185,8 +185,6 @@ sub extractMember { $dirName = File::Spec->catpath($volumeName, $dirName, ''); } else { $name = $member->fileName(); - if ((my $ret = _extractionNameIsSafe($name)) - != AZ_OK) { return $ret; } ($dirName = $name) =~ s{[^/]*$}{}; $dirName = Archive::Zip::_asLocalName($dirName); $name = Archive::Zip::_asLocalName($name); @@ -220,8 +218,6 @@ sub extractMemberWithoutPaths { unless ($name) { $name = $member->fileName(); $name =~ s{.*/}{}; # strip off directories, if any - if ((my $ret = _extractionNameIsSafe($name)) - != AZ_OK) { return $ret; } $name = Archive::Zip::_asLocalName($name); } my $rc = $member->extractToFileNamed($name, @_); @@ -831,37 +827,6 @@ sub addTreeMatching { return $self->addTree($root, $dest, $matcher, $compressionLevel); } -# Check if one of the components of a path to the file or the file name -# itself is an already existing symbolic link. If yes then return an -# error. Continuing and writing to a file traversing a link posseses -# a security threat, especially if the link was extracted from an -# attacker-supplied archive. This would allow writing to an arbitrary -# file. The same applies when using ".." to escape from a working -# directory. -sub _extractionNameIsSafe { - my $name = shift; - my ($volume, $directories) = File::Spec->splitpath($name, 1); - my @directories = File::Spec->splitdir($directories); - if (grep '..' eq $_, @directories) { - return _error( - "Could not extract $name safely: a parent directory is used"); - } - my @path; - my $path; - for my $directory (@directories) { - push @path, $directory; - $path = File::Spec->catpath($volume, File::Spec->catdir(@path), ''); - if (-l $path) { - return _error( - "Could not extract $name safely: $path is an existing symbolic link"); - } - if (!-e $path) { - last; - } - } - return AZ_OK; -} - # $zip->extractTree( $root, $dest [, $volume] ); # # $root and $dest are Unix-style. @@ -896,8 +861,6 @@ sub extractTree { $fileName =~ s{$pattern}{$dest}; # in Unix format # convert to platform format: $fileName = Archive::Zip::_asLocalName($fileName, $volume); - if ((my $ret = _extractionNameIsSafe($fileName)) - != AZ_OK) { return $ret; } my $status = $member->extractToFileNamed($fileName); return $status if $status != AZ_OK; } diff --git a/t/25_traversal.t b/t/25_traversal.t deleted file mode 100644 index d03dede..0000000 --- a/t/25_traversal.t +++ /dev/null @@ -1,189 +0,0 @@ -use strict; -use warnings; - -use Archive::Zip qw( :ERROR_CODES ); -use File::Spec; -use File::Path; -use lib 't'; -use common; - -use Test::More tests => 41; - -# These tests check for CVE-2018-10860 vulnerabilities. -# If an archive contains a symlink and then a file that traverses that symlink, -# extracting the archive tree could write into an abitrary file selected by -# the symlink value. -# Another issue is if an archive contains a file whose path component refers -# to a parent direcotory. Then extracting that file could write into a file -# out of current working directory subtree. -# These tests check extracting of these files is refuses and that they are -# indeed not created. - -# Suppress croaking errors, the tests produce some. -Archive::Zip::setErrorHandler(sub {}); -my ($existed, $ret, $zip, $allowed_file, $forbidden_file); - -# Change working directory to a temporary directory because some tested -# functions operarates there and we need prepared symlinks there. -my @data_path = (File::Spec->splitdir(File::Spec->rel2abs('.')), 't', 'data'); -ok(chdir TESTDIR, "Working directory changed"); - -# Case 1: -# link-dir -> /tmp -# link-dir/gotcha-linkdir -# writes into /tmp/gotcha-linkdir file. -SKIP: { - # Symlink tests make sense only if a file system supports them. - my $link = 'trylink'; - $ret = eval { symlink('.', $link)}; - skip 'Symbolic links are not supported', 12 if $@; - unlink $link; - - # Extracting an archive tree must fail - $zip = Archive::Zip->new(); - isa_ok($zip, 'Archive::Zip'); - is($zip->read(File::Spec->catfile(@data_path, 'link-dir.zip')), AZ_OK, - 'Archive read'); - $existed = -e File::Spec->catfile('', 'tmp', 'gotcha-linkdir'); - $ret = eval { $zip->extractTree() }; - is($ret, AZ_ERROR, 'Tree extraction aborted'); - SKIP: { - skip 'A canary file existed before the test', 1 if $existed; - ok(! -e File::Spec->catfile('link-dir', 'gotcha-linkdir'), - 'A file was not created in a symlinked directory'); - } - ok(unlink(File::Spec->catfile('link-dir')), 'link-dir removed'); - - # The same applies to extracting an archive member without an explicit - # local file name. It must abort. - $link = 'link-dir'; - ok(symlink('.', $link), 'A symlink to a directory created'); - $forbidden_file = File::Spec->catfile($link, 'gotcha-linkdir'); - $existed = -e $forbidden_file; - $ret = eval { $zip->extractMember('link-dir/gotcha-linkdir') }; - is($ret, AZ_ERROR, 'Member extraction without a local name aborted'); - SKIP: { - skip 'A canary file existed before the test', 1 if $existed; - ok(! -e $forbidden_file, - 'A file was not created in a symlinked directory'); - } - - # But allow extracting an archive member into a supplied file name - $allowed_file = File::Spec->catfile($link, 'file'); - $ret = eval { $zip->extractMember('link-dir/gotcha-linkdir', $allowed_file) }; - is($ret, AZ_OK, 'Member extraction passed'); - ok(-e $allowed_file, 'File created'); - ok(unlink($allowed_file), 'File removed'); - ok(unlink($link), 'A symlink to a directory removed'); -} - -# Case 2: -# unexisting/../../../../../tmp/gotcha-dotdot-unexistingpath -# writes into ../../../../tmp/gotcha-dotdot-unexistingpath, that is -# /tmp/gotcha-dotdot-unexistingpath file if CWD is not deeper than -# 4 directories. -$zip = Archive::Zip->new(); -isa_ok($zip, 'Archive::Zip'); -is($zip->read(File::Spec->catfile(@data_path, - 'dotdot-from-unexistant-path.zip')), AZ_OK, 'Archive read'); -$forbidden_file = File::Spec->catfile('..', '..', '..', '..', 'tmp', - 'gotcha-dotdot-unexistingpath'); -$existed = -e $forbidden_file; -$ret = eval { $zip->extractTree() }; -is($ret, AZ_ERROR, 'Tree extraction aborted'); -SKIP: { - skip 'A canary file existed before the test', 1 if $existed; - ok(! -e $forbidden_file, 'A file was not created in a parent directory'); -} - -# The same applies to extracting an archive member without an explicit local -# file name. It must abort. -$existed = -e $forbidden_file; -$ret = eval { $zip->extractMember( - 'unexisting/../../../../../tmp/gotcha-dotdot-unexistingpath', - ) }; -is($ret, AZ_ERROR, 'Member extraction without a local name aborted'); -SKIP: { - skip 'A canary file existed before the test', 1 if $existed; - ok(! -e $forbidden_file, 'A file was not created in a parent directory'); -} - -# But allow extracting an archive member into a supplied file name -ok(mkdir('directory'), 'Directory created'); -$allowed_file = File::Spec->catfile('directory', '..', 'file'); -$ret = eval { $zip->extractMember( - 'unexisting/../../../../../tmp/gotcha-dotdot-unexistingpath', - $allowed_file - ) }; -is($ret, AZ_OK, 'Member extraction passed'); -ok(-e $allowed_file, 'File created'); -ok(unlink($allowed_file), 'File removed'); - -# Case 3: -# link-file -> /tmp/gotcha-samename -# link-file -# writes into /tmp/gotcha-samename. It must abort. (Or replace the symlink in -# more relaxed mode in the future.) -$zip = Archive::Zip->new(); -isa_ok($zip, 'Archive::Zip'); -is($zip->read(File::Spec->catfile(@data_path, 'link-samename.zip')), AZ_OK, - 'Archive read'); -$existed = -e File::Spec->catfile('', 'tmp', 'gotcha-samename'); -$ret = eval { $zip->extractTree() }; -is($ret, AZ_ERROR, 'Tree extraction aborted'); -SKIP: { - skip 'A canary file existed before the test', 1 if $existed; - ok(! -e File::Spec->catfile('', 'tmp', 'gotcha-samename'), - 'A file was not created through a symlinked file'); -} -ok(unlink(File::Spec->catfile('link-file')), 'link-file removed'); - -# The same applies to extracting an archive member using extractMember() -# without an explicit local file name. It must abort. -my $link = 'link-file'; -my $target = 'target'; -ok(symlink($target, $link), 'A symlink to a file created'); -$forbidden_file = File::Spec->catfile($target); -$existed = -e $forbidden_file; -# Select a member by order due to same file names. -my $member = ${[$zip->members]}[1]; -ok($member, 'A member to extract selected'); -$ret = eval { $zip->extractMember($member) }; -is($ret, AZ_ERROR, - 'Member extraction using extractMember() without a local name aborted'); -SKIP: { - skip 'A canary file existed before the test', 1 if $existed; - ok(! -e $forbidden_file, - 'A symlinked target file was not created'); -} - -# But allow extracting an archive member using extractMember() into a supplied -# file name. -$allowed_file = $target; -$ret = eval { $zip->extractMember($member, $allowed_file) }; -is($ret, AZ_OK, 'Member extraction using extractMember() passed'); -ok(-e $allowed_file, 'File created'); -ok(unlink($allowed_file), 'File removed'); - -# The same applies to extracting an archive member using -# extractMemberWithoutPaths() without an explicit local file name. -# It must abort. -$existed = -e $forbidden_file; -# Select a member by order due to same file names. -$ret = eval { $zip->extractMemberWithoutPaths($member) }; -is($ret, AZ_ERROR, - 'Member extraction using extractMemberWithoutPaths() without a local name aborted'); -SKIP: { - skip 'A canary file existed before the test', 1 if $existed; - ok(! -e $forbidden_file, - 'A symlinked target file was not created'); -} - -# But allow extracting an archive member using extractMemberWithoutPaths() -# into a supplied file name. -$allowed_file = $target; -$ret = eval { $zip->extractMemberWithoutPaths($member, $allowed_file) }; -is($ret, AZ_OK, 'Member extraction using extractMemberWithoutPaths() passed'); -ok(-e $allowed_file, 'File created'); -ok(unlink($allowed_file), 'File removed'); -ok(unlink($link), 'A symlink to a file removed'); diff --git a/t/data/dotdot-from-unexistant-path.zip b/t/data/dotdot-from-unexistant-path.zip deleted file mode 100644 index faaa5bb..0000000 Binary files a/t/data/dotdot-from-unexistant-path.zip and /dev/null differ diff --git a/t/data/link-dir.zip b/t/data/link-dir.zip deleted file mode 100644 index 99fbb43..0000000 Binary files a/t/data/link-dir.zip and /dev/null differ diff --git a/t/data/link-samename.zip b/t/data/link-samename.zip deleted file mode 100644 index e9036c0..0000000 Binary files a/t/data/link-samename.zip and /dev/null differ