Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions ext/phar/phar_object.c
Original file line number Diff line number Diff line change
Expand Up @@ -4282,6 +4282,40 @@ static zend_result phar_extract_file(bool overwrite, phar_entry_info *entry, cha
return SUCCESS;
}

{
const char *last_sep = zend_memrchr(fullpath + dest_len + 1, '/', strlen(fullpath) - dest_len - 1);
if (last_sep) {
char parent_path[MAXPATHLEN];
char resolved[MAXPATHLEN];
char resolved_dest[MAXPATHLEN];
size_t parent_len = last_sep - fullpath;
if (parent_len >= MAXPATHLEN) {
spprintf(error, 4096, "Cannot extract \"%s\", path too long", entry->filename);
efree(fullpath);
return FAILURE;
}
if (!VCWD_REALPATH(dest, resolved_dest)) {
spprintf(error, 4096, "Cannot extract \"%s\", could not resolve destination directory", entry->filename);
efree(fullpath);
return FAILURE;
}
memcpy(parent_path, fullpath, parent_len);
parent_path[parent_len] = '\0';
if (!VCWD_REALPATH(parent_path, resolved)) {
spprintf(error, 4096, "Cannot extract \"%s\", could not resolve parent directory", entry->filename);
efree(fullpath);
return FAILURE;
}
size_t resolved_dest_len = strlen(resolved_dest);
if (strncmp(resolved, resolved_dest, resolved_dest_len) != 0 ||
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could use zend_binary_strncmp

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping strncmp. phar uses it throughout for path prefix checks (util.c:1296, stream.c:615, phar.c:509). zend_binary_strncmp carries three-way-compare length semantics for userland zend_string comparison, not a prefix check on NUL-terminated realpath output.

(!IS_SLASH(resolved[resolved_dest_len]) && resolved[resolved_dest_len] != '\0')) {
spprintf(error, 4096, "Cannot extract \"%s\", symlink traversal detected", entry->filename);
efree(fullpath);
return FAILURE;
}
}
}

fp = php_stream_open_wrapper(fullpath, "w+b", REPORT_ERRORS, NULL);

if (!fp) {
Expand Down
59 changes: 59 additions & 0 deletions ext/phar/tests/gh-ss008-extractto-symlink.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
--TEST--
extractTo: symlink traversal via pre-existing symlink in destination directory
--EXTENSIONS--
phar
--SKIPIF--
<?php
if (PHP_OS_FAMILY === 'Windows') {
if (false === include __DIR__ . '/../../standard/tests/file/windows_links/common.inc') {
die('skip windows_links/common.inc is not available');
}
skipIfSeCreateSymbolicLinkPrivilegeIsDisabled(__FILE__);
}
?>
--INI--
phar.readonly=0
--FILE--
<?php
$pharFile = __DIR__ . '/gh_ss008.phar';
$dest = __DIR__ . '/gh_ss008_dest';
$target = __DIR__ . '/gh_ss008_target';

@mkdir($dest, 0777, true);
@mkdir($target, 0777, true);

$p = new Phar($pharFile);
$p->addFromString('subdir/evil.txt', 'should not arrive');
unset($p);

symlink($target, $dest . '/subdir');

try {
$phar = new Phar($pharFile);
$phar->extractTo($dest, null, true);
echo "EXTRACTED (unexpected)\n";
} catch (PharException $e) {
echo "Caught PharException: " . $e->getMessage() . "\n";
}

if (file_exists($target . '/evil.txt')) {
echo "FAIL: target was written\n";
} else {
echo "OK: target not written\n";
}
?>
--CLEAN--
<?php
$pharFile = __DIR__ . '/gh_ss008.phar';
$dest = __DIR__ . '/gh_ss008_dest';
$target = __DIR__ . '/gh_ss008_target';

@unlink($dest . '/subdir');
@unlink($target . '/evil.txt');
@rmdir($dest);
@rmdir($target);
@unlink($pharFile);
?>
--EXPECTF--
Caught PharException: %s
OK: target not written
Loading