Skip to content

Commit 65c95a0

Browse files
paulbissHhvm Bot
authored and
Hhvm Bot
committed
ZipArchive::extractTo bug 70350
Summary:Don't allow upward directory traversal when extracting zip archive files. Files in zip files with `..` or starting at main root `/` should be normalized to something where the file being extracted winds up within the directory or a subdirectory where the actual extraction is taking place. http://git.php.net/?p=php-src.git;a=commit;h=f9c2bf73adb2ede0a486b0db466c264f2b27e0bb Reviewed By: FBNeal Differential Revision: D2798452 fb-gh-sync-id: 844549c93e011d1e991bb322bf85822246b04e30 shipit-source-id: 844549c93e011d1e991bb322bf85822246b04e30
1 parent 06f3fc8 commit 65c95a0

5 files changed

+259
-8
lines changed

Diff for: hphp/runtime/ext/zip/ext_zip.cpp

+87-8
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include <zip.h>
1818

1919
#include "hphp/runtime/base/array-init.h"
20+
#include "hphp/runtime/base/file-util.h"
2021
#include "hphp/runtime/base/preg.h"
2122
#include "hphp/runtime/base/stream-wrapper-registry.h"
2223
#include "hphp/runtime/ext/extension.h"
@@ -650,25 +651,103 @@ static bool HHVM_METHOD(ZipArchive, deleteName, const String& name) {
650651
return true;
651652
}
652653

654+
// Make the path relative to "." by flattening.
655+
// This function is named the same and similar in implementation to that in
656+
// php-src:php_zip.c
657+
// One difference is that we canonicalize here whereas php-src is already
658+
// assumed passed a canonicalized path.
659+
static std::string make_relative_path(const std::string& path) {
660+
if (path.empty()) {
661+
return path;
662+
}
663+
664+
// First get the path to a state where we don't have .. in the middle of it
665+
// etc. canonicalize handles Windows paths too.
666+
std::string canonical(FileUtil::canonicalize(path));
667+
668+
// If we have a slash at the beginning, then just remove it and we are
669+
// relative. This check will hold because we have canonicalized the
670+
// path above to remove .. from the path, so we know we can be sure
671+
// we are at a good place for this check.
672+
if (FileUtil::isDirSeparator(canonical[0])) {
673+
return canonical.substr(1);
674+
}
675+
676+
// If we get here, canonical looks something like:
677+
// a/b/c
678+
679+
// Search through the path and if we find a place where we have a slash
680+
// and a "." just before that slash, then cut the path off right there
681+
// and just take everything after the slash.
682+
std::string relative(canonical);
683+
int idx = canonical.length() - 1;
684+
while (1) {
685+
while (idx > 0 && !(FileUtil::isDirSeparator(canonical[idx]))) {
686+
idx--;
687+
}
688+
// If we ever get to idx == 0, then there were no other slashes to deal with
689+
if (idx == 0) {
690+
return canonical;
691+
}
692+
if (idx >= 1 && (canonical[idx - 1] == '.' || canonical[idx - 1] == ':')) {
693+
relative = canonical.substr(idx + 1);
694+
break;
695+
}
696+
idx--;
697+
}
698+
return relative;
699+
}
700+
653701
static bool extractFileTo(zip* zip, const std::string &file, std::string& to,
654702
char* buf, size_t len) {
655-
auto sep = file.rfind('/');
703+
704+
struct zip_stat zipStat;
705+
// Verify the file to be extracted is actually in the zip file
706+
if (zip_stat(zip, file.c_str(), 0, &zipStat) != 0) {
707+
return false;
708+
}
709+
710+
auto clean_file = file;
711+
auto sep = std::string::npos;
712+
// Normally would just use std::string::rfind here, but if we want to be
713+
// consistent between Windows and Linux, even if techincally Linux won't use
714+
// backslash for a separator, we are checking for both types.
715+
int idx = file.length() - 1;
716+
while (idx >= 0) {
717+
if (FileUtil::isDirSeparator(file[idx])) {
718+
sep = idx;
719+
break;
720+
}
721+
idx--;
722+
}
656723
if (sep != std::string::npos) {
657-
auto path = to + file.substr(0, sep);
724+
// make_relative_path so we do not try to put files or dirs in bad
725+
// places. This securely "cleans" the file.
726+
clean_file = make_relative_path(file);
727+
std::string path = to + clean_file;
728+
bool is_dir_only = true;
729+
if (sep < file.length() - 1) { // not just a directory
730+
auto clean_file_dir = HHVM_FN(dirname)(clean_file);
731+
path = to + clean_file_dir.toCppString();
732+
is_dir_only = false;
733+
}
734+
735+
// Make sure the directory path to extract to exists or can be created
658736
if (!HHVM_FN(is_dir)(path) && !HHVM_FN(mkdir)(path, 0777, true)) {
659737
return false;
660738
}
661739

662-
if (sep == file.length() - 1) {
740+
// If we have a good directory to extract to above, we now check whether
741+
// the "file" parameter passed in is a directory or actually a file.
742+
if (is_dir_only) { // directory, like /usr/bin/
663743
return true;
664744
}
745+
// otherwise file is actually a file, so we actually extract.
665746
}
666747

667-
to.append(file);
668-
struct zip_stat zipStat;
669-
if (zip_stat(zip, file.c_str(), 0, &zipStat) != 0) {
670-
return false;
671-
}
748+
// We have ensured that clean_file will be added to a relative path by the
749+
// time we get here.
750+
to.append(clean_file);
672751

673752
auto zipFile = zip_fopen_index(zip, zipStat.index, 0);
674753
FAIL_IF_INVALID_PTR(zipFile);
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
<?php
2+
$dir = tempnam(sys_get_temp_dir(), __FILE__);
3+
unlink($dir);
4+
mkdir($dir);
5+
$archive = new ZipArchive();
6+
$archive->open("$dir/a.zip",ZipArchive::CREATE);
7+
$archive->addEmptyDir("../dir1/");
8+
$archive->addEmptyDir("/var/www/dir2/");
9+
$archive->addEmptyDir("a/b/../../../dir3");
10+
$archive->addEmptyDir("a/b/../dir4/");
11+
$archive->addEmptyDir("a/b/../c/../../d/dir5/");
12+
$archive->addEmptyDir("./dir6");
13+
$archive->addEmptyDir("x/y/dir7/..");
14+
$archive->addEmptyDir("z/dir8/.");
15+
$archive->addEmptyDir("simple");
16+
$archive->close();
17+
$archive2 = new ZipArchive();
18+
$archive2->open("$dir/a.zip");
19+
$archive2->extractTo($dir);
20+
$archive2->close();
21+
var_dump(file_exists("$dir/dir1/")); // true
22+
var_dump(file_exists("../dir1/")); // false
23+
var_dump(file_exists("$dir/var/www/dir2")); // true
24+
var_dump(file_exists("/var/www/dir2/")); // false
25+
var_dump(file_exists("$dir/dir3/")); // true
26+
var_dump(file_exists("a/b/../../../dir3/")); // false
27+
var_dump(file_exists("$dir/a/dir4/")); // true
28+
var_dump(file_exists("a/b/../dir4/")); // false
29+
var_dump(file_exists("$dir/d/dir5/")); // true
30+
var_dump(file_exists("a/b/../c/../../d/dir5/")); // false
31+
var_dump(file_exists("$dir/dir6")); // true
32+
var_dump(file_exists("./dir6")); // false
33+
var_dump(file_exists("$dir/x/y/")); // true
34+
var_dump(file_exists("x/y/dir7/..")); // false
35+
var_dump(file_exists("$dir/z/dir8")); // true
36+
var_dump(file_exists("z/dir8/.")); // false
37+
var_dump(file_exists("$dir/simple")); // true
38+
var_dump(file_exists("simple")); // false
39+
40+
// Cleanup. Also verifies that everything is where it is supposed to be.
41+
rmdir("$dir/dir1");
42+
rmdir("$dir/var/www/dir2");
43+
rmdir("$dir/var/www");
44+
rmdir("$dir/var");
45+
rmdir("$dir/dir3");
46+
rmdir("$dir/a/dir4");
47+
rmdir("$dir/a");
48+
rmdir("$dir/d/dir5");
49+
rmdir("$dir/d");
50+
rmdir("$dir/dir6");
51+
rmdir("$dir/x/y");
52+
rmdir("$dir/x");
53+
rmdir("$dir/z/dir8");
54+
rmdir("$dir/z");
55+
rmdir("$dir/simple");
56+
unlink("$dir/a.zip");
57+
rmdir($dir);
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
bool(true)
2+
bool(false)
3+
bool(true)
4+
bool(false)
5+
bool(true)
6+
bool(false)
7+
bool(true)
8+
bool(false)
9+
bool(true)
10+
bool(false)
11+
bool(true)
12+
bool(false)
13+
bool(true)
14+
bool(false)
15+
bool(true)
16+
bool(false)
17+
bool(true)
18+
bool(false)
+75
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
<?php
2+
$str = 'temp';
3+
$dir = tempnam(sys_get_temp_dir(), __FILE__);
4+
unlink($dir);
5+
mkdir($dir);
6+
$archive = new ZipArchive();
7+
$archive->open("$dir/a.zip",ZipArchive::CREATE);
8+
$archive->addFromString("../dir1/A.txt", $str);
9+
$archive->addFromString("/var/www/dir2/B", $str);
10+
$archive->addFromString("a/b/../../../dir3/C.txt.other", $str);
11+
$archive->addFromString("a/b/../dir4/D.not.a.file/D.a.file", $str);
12+
$archive->addFromString("a/b/../c/../../d/dir5/E", $str);
13+
$archive->addFromString("./dir6/F.exe", $str);
14+
$archive->addFromString("./G.txt", $str);
15+
$archive->addFromString("H.txt", $str);
16+
$archive->addFromString("x/y/dir7/../I.txt", $str);
17+
$archive->addFromString("z/dir8/./J.txt", $str);
18+
$archive->addFromString("SIMPLE.txt", $str);
19+
$archive->close();
20+
$archive2 = new ZipArchive();
21+
$archive2->open("$dir/a.zip");
22+
$archive2->extractTo($dir);
23+
$archive2->close();
24+
var_dump(file_exists("$dir/dir1/A.txt")); // true
25+
var_dump(file_exists("../dir1/A.txt")); // false
26+
var_dump(file_exists("$dir/var/www/dir2/B")); // true
27+
var_dump(file_exists("/var/www/dir2/B")); // false
28+
var_dump(file_exists("$dir/dir3/C.txt.other")); // true
29+
var_dump(file_exists("a/b/../../../dir3/C.txt.other")); // false
30+
var_dump(file_exists("$dir/a/dir4/D.not.a.file/D.a.file")); // true
31+
var_dump(file_exists("a/b/../dir4/D.not.a.file/D.a.file")); // false
32+
var_dump(file_exists("$dir/d/dir5/E")); // true
33+
var_dump(file_exists("a/b/../c/../../d/dir5/E")); // false
34+
var_dump(file_exists("$dir/dir6/F.exe")); // true
35+
var_dump(file_exists("./dir6/F.exe")); // false
36+
var_dump(file_exists("$dir/G.txt")); // true
37+
var_dump(file_exists("./G.txt")); // false
38+
var_dump(file_exists("$dir/H.txt")); // true
39+
var_dump(file_exists("H.txt")); // false
40+
var_dump(file_exists("$dir/x/y/I.txt")); // true
41+
var_dump(file_exists("x/y/dir7/../I.txt")); // false
42+
var_dump(file_exists("$dir/z/dir8/J.txt")); // true
43+
var_dump(file_exists("z/dir8/./J.txt")); // false
44+
var_dump(file_exists("$dir/SIMPLE.txt")); // true
45+
var_dump(file_exists("SIMPLE.txt")); // false
46+
47+
// Cleanup. Also verifies that everything is where it is supposed to be.
48+
unlink("$dir/dir1/A.txt");
49+
rmdir("$dir/dir1");
50+
unlink("$dir/var/www/dir2/B");
51+
rmdir("$dir/var/www/dir2");
52+
rmdir("$dir/var/www");
53+
rmdir("$dir/var");
54+
unlink("$dir/dir3/C.txt.other");
55+
rmdir("$dir/dir3");
56+
unlink("$dir/a/dir4/D.not.a.file/D.a.file");
57+
rmdir("$dir/a/dir4/D.not.a.file");
58+
rmdir("$dir/a/dir4");
59+
rmdir("$dir/a");
60+
unlink("$dir/d/dir5/E");
61+
rmdir("$dir/d/dir5");
62+
rmdir("$dir/d");
63+
unlink("$dir/dir6/F.exe");
64+
rmdir("$dir/dir6");
65+
unlink("$dir/x/y/I.txt");
66+
rmdir("$dir/x/y");
67+
rmdir("$dir/x");
68+
unlink("$dir/z/dir8/J.txt");
69+
rmdir("$dir/z/dir8");
70+
rmdir("$dir/z");
71+
unlink("$dir/G.txt");
72+
unlink("$dir/H.txt");
73+
unlink("$dir/a.zip");
74+
unlink("$dir/SIMPLE.txt");
75+
rmdir($dir);
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
bool(true)
2+
bool(false)
3+
bool(true)
4+
bool(false)
5+
bool(true)
6+
bool(false)
7+
bool(true)
8+
bool(false)
9+
bool(true)
10+
bool(false)
11+
bool(true)
12+
bool(false)
13+
bool(true)
14+
bool(false)
15+
bool(true)
16+
bool(false)
17+
bool(true)
18+
bool(false)
19+
bool(true)
20+
bool(false)
21+
bool(true)
22+
bool(false)

0 commit comments

Comments
 (0)