php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Sec Bug #70019 Files extracted from archive may be placed outside of destination directory
Submitted: 2015-07-08 09:33 UTC Modified: 2015-09-09 10:01 UTC
From: stewie at mail dot ru Assigned: stas (profile)
Status: Closed Package: PHAR related
PHP Version: 5.6.10 OS: Windows 7 64bit, OSX 10.10
Private report: No CVE-ID: 2015-6833
 [2015-07-08 09:33 UTC] stewie at mail dot ru
Description:
------------
By modifying filenames in archive to contain paths like "../somefile.ext", after extracting they may be placed in directories outer to destination directory

Test archive is at https://www.dropbox.com/s/yfk10bdmrwlj47l/TestFile.zip?dl=0

Test script:
---------------
<?php
    $phar = new PharData('TestFile.zip');
    $phar->extractTo('c:\php\test\test'); 
?>

Expected result:
----------------
file placed under c:\php\test\test

Actual result:
--------------
file placed under c:\php\test

Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2015-07-08 14:30 UTC] ab@php.net
-Status: Open +Status: Not a bug
 [2015-07-08 14:30 UTC] ab@php.net
Thanks for the report.

The paragraph 4.4.17 of the zip file spec https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT tells that a relative path is a valid item in a zip archive. Thus, it is expected behavior.

Thanks.
 [2015-07-08 21:11 UTC] stewie at mail dot ru
Please take a look at https://github.com/php/php-src/blob/master/ext/zip/php_zip.c#L158

That's about how path sanitizing is done in php_zip, so no files could be written outside the cwd.
 [2015-07-17 15:13 UTC] ab@php.net
-Status: Not a bug +Status: Analyzed
 [2015-07-17 15:13 UTC] ab@php.net
ACK. Sounds plausible to me, we should sync with the ext/zip behavior. Even if we break the spec in this case, it's a more secure way for specific PHP tasks.

Thanks.
 [2015-08-02 12:59 UTC] ab@php.net
I've checked the patch with master. There are a few issues.

- ZMM routines should be used
- for the Windows part, it has to retry (or maybe even try as first) with the backslash
- with the two above solved - the contents of the file is completely broken. While ext/zip extracted file contains "test", the ext/phar extracted file contains just garbage

Cheers.
 [2015-08-02 13:02 UTC] ab@php.net
But the contents of the file part seems to be an unrelated issue, what I had to mention.

Thanks.
 [2015-08-02 17:26 UTC] stas@php.net
Anatol, I'm not sure I fully understand the comments.

> - ZMM routines should be used

Where exactly?

> - for the Windows part, it has to retry (or maybe even try as first) with the backslash

Why? Doesn't Windows accept backslash as path separator? Furthermore, if it started with \, wouldn't path like \Foo be intereted as \\foo, i.e. UNC path? That's not what we want.

> - with the two above solved - the contents of the file is completely broken. While ext/zip extracted file contains "test", the ext/phar extracted file contains just garbage

I don't think this is caused by the patch, unless I miss something. Does it work for other ZIP files without the patch?
 [2015-08-02 20:19 UTC] ab@php.net
Hi Stas,

ZMM routines - yep, looks like all. I see now - it has changed in 5.6 when the virtual cwd was moved into Zend (just forgot it as it was a patch i took over). 5.5 inclusive uses plain CRT routines.

With the backslash - ok, see now it is the difference between ext/phar and ext/zip. The latter accepts both, maybe it lays on libzip, but i havent checked. Just as fact - the exctraction doesn't work when only '/' is checked on Windows. But it is also the part about the actual streams feature in handling paths. Fe ext/zip checks for IS_SLASH https://github.com/php/php-src/blob/master/ext/zip/php_zip.c#L96 . Maybe it's just simpler to check for both back and forward slash to achieve the behavior identical to ext/zip.

With the file contents - it is strange. I've created one archive with file ../hello.txt containing the string "hello". Then i've checked with and without the patch. Linux - both with and without patch file has broken content. Windows - broken file contents with the patch, correct contents without the patch. Seems the patch somehow affects this, but seems the main breakage is not caused by the patch itself, anyway.

Regards

Anatol
 [2015-08-02 20:52 UTC] stas@php.net
I'm not sure what you're proposing with regard to slashes on Windows. Current patch does not "check" anything, it just expands the path, eliminating the .. and . elements. Doesn't it work on Windows? What is the result? Sorry, I don't have windows setup handy, so I can't check. Also can't check the broken extraction - on my machine, it's broken before and after patch.
 [2015-08-02 21:51 UTC] stas@php.net
The garbage seems to be the result of bug #69279.
 [2015-08-02 22:26 UTC] ab@php.net
Yep, in the current state, per test, it'll fail to unzip on winodws. What I refer to is that new_state.cwd[0] = '\\'; should be checked as well - either as a retry or prior to '/'. It only regards to the relative path evaluation and cleanup. Merely what is being evaluated is the pure path before it even gets extracted, so on windows both '\\' and '/' are valid.

The file contents is a different issue, as you point out, too. Good that there's one more confirmation for that.

Thanks.
 [2015-08-02 23:32 UTC] stas@php.net
I'm not sure I understand - if both / amd \ are valid, why code with / failing right now on Windows? What about UNC paths - if we just try it with \ on Windows, it would interpret a path beginning with \\ as UNC path, not?
 [2015-08-03 00:30 UTC] ab@php.net
Now that i step through virtual_file_ex, the exact place causing it is http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_virtual_cwd.c#1238 because it checks for DEFAULT_SLASH. If it's '/', it'll run into the first if branch and implicitly an UNC path. That will be misinterpreted later at http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_virtual_cwd.c#1268. This happens due to how http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_virtual_cwd.h#70 IS_UNC_PATH is declared, say "\\/" would be interpreted same as "\\\\" ... This part is same in all the versions of virtual_file_ex. Seems to me on Windows it has to be only '\\', maybe just new_state.cwd[0] = DEFAULT_SLASH.

Not sure whether it is possible to do a PR to the gist on github, but i've seen it is possible to fork. Maybe it were easier me to supply a forked version of your patches, so we can check and update them mutually?

Thanks.
 [2015-08-03 07:08 UTC] stas@php.net
I'm fine with that.
 [2015-08-03 10:36 UTC] ab@php.net
Hi Stas,

here's a modified patch https://gist.github.com/weltling/718ef2c5a1d9062e4e3a , using the DEFAULT_SLASH approach and one more improvement for long file names tested in ext\phar\tests\*extract*.phpt. Applies to 5.4 and 5.5. For 5.6 and 7.0 it seems to apply and work as well, just need to replace malloc/free with emalloc/efree before.

Thanks.
 [2015-08-04 22:22 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=dda81f0505217a95db065e6bf9cc2d81eb902417
Log: Fix bug #70019 - limit extracted files to given directory
 [2015-08-04 22:22 UTC] stas@php.net
-Status: Analyzed +Status: Closed
 [2015-08-04 22:23 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=dda81f0505217a95db065e6bf9cc2d81eb902417
Log: Fix bug #70019 - limit extracted files to given directory
 [2015-08-04 22:30 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=dda81f0505217a95db065e6bf9cc2d81eb902417
Log: Fix bug #70019 - limit extracted files to given directory
 [2015-08-05 07:29 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=dda81f0505217a95db065e6bf9cc2d81eb902417
Log: Fix bug #70019 - limit extracted files to given directory
 [2015-08-05 10:12 UTC] ab@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=dda81f0505217a95db065e6bf9cc2d81eb902417
Log: Fix bug #70019 - limit extracted files to given directory
 [2015-09-09 10:01 UTC] kaplan@php.net
-Assigned To: +Assigned To: stas -CVE-ID: +CVE-ID: 2015-6833
 [2016-03-23 13:08 UTC] mustafa at yavuzm dot com
I had same problem. When I extraxt on desktop files are working normally but when I extract zip file with phar command (extractTo) although all files are extracted they are broken o not working.
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Tue Mar 19 03:01:29 2024 UTC