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.

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.

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.

proposed fix: https://gist.github.com/smalyshev/2d63ef849583b3097560

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.

But the contents of the file part seems to be an unrelated issue, what I had to mention.

Thanks.

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?

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

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.

The garbage seems to be the result of bug #69279.

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.

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?

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.

I'm fine with that.

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.

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.