ZIP is still not supported

[senky]

Guys, issue #2 desc says zip is already supported, but try this:

1. zip graphics_standard_testfile.gif from _testfiles
2. ensure you have phpmussel_graphics.db and phpmussel_graphics_regex.db active and up-to-date
3. use Upload Test to upload generated graphics_standard_testfile.gif.zip
4. file passes the test

[DanielRuf]

zip is supported by the phar extension in general.

https://github.com/phpMussel/phpMussel/blob/master/vault/functions.php#L1309-L1334

[Maikuolan]

Quick question; If you try executing this on your server:

<?php var_dump(extension_loaded('phar')); ?>

Do you get bool(true) or bool(false) (or something else)?

If the test file can be scanned correctly normally, but not when zipped, it could be some issue with the PHAR extension maybe.

[senky]

@Maikuolan I get true. gif alone is blocked. When zipped, it's not. Here is zip I use:
graphics_standard_testfile.gif.zip

[neuropass]

same issue here. zip is not scanned it seems and it passes just fine with the graphics_standard_testfile.gif :-/

[DanielRuf]

Which release of phpMussel do you use?

[DanielRuf]

Because #152 was about this issue and it is resolved in master but not published as release.

[senky]

I use latest files directly from github.

[Maikuolan]

Any error_log or error.log data appearing? Also, just in case of some weird caching issue again.. If you try renaming the file, are the results different at all, or exactly the same?

[senky]

Nothing in error logs. Renaming file doesn't change behaviour.

[senky]

@Maikuolan @DanielRuf is my zip file posted above blocked on your end?

[Maikuolan]

On my end, it blocks successfully. Also compared the zipped GIF against my own copy, in case something somehow became corrupted during transfer at some point, but the two copies were exactly the same, so nothing there either.

[senky]

Is there any config value I need to set before zip files are checked?

[Maikuolan]

There is this in the config:

; Attempt to check the contents of archives? False = Don't check; True = Check
; [Default]. Currently, the only archive and compression formats supported are
; BZ/BZIP2, GZ/GZIP, LZF, PHAR, TAR and ZIP (archive and compression formats
; RAR, CAB, 7z and etcetera not currently supported). This is not foolproof!
; While I highly recommend keeping this turned on, I can't guarantee it'll
; always find everything. Also be aware that archive checking currently is not
; recursive for PHARs or ZIPs.
check_archives=true

So, check_archives should be true, in order to be able to scan inside archives. It should normally already be true by default though, unless it gets changed at some point.

I'm actually working on adding some better way of analysing the cache to the front-end at the moment; Hoping we might be able to use it to see exactly what's going on there once and for all. Should have it ready to be committed within a few hours.

[Maikuolan]

Okay; New feature added just now. After updating to the latest commit (1.3.0-DEV+18058112/7375251), a "Cache Data" page should appear in the front-end. After updating, could you try scanning again (both the GIF and the ZIP, separately), then go to the "Cache Data" page, look for "HashCache", click "Show", and let me know what you see appear, and also whether the results are still the same (e.g., blocking the GIF, and not blocking the ZIP, or something else)?

For example, on my end, I see this:

Also, what is the current value you have assigned for max_recursion in your configuration? Double-checking through the code, I've found that that's another possible way that archives could stop being scanned properly.

Due to the way the code is written currently:

    /**
     * Begin archive phase.
     * Note: Archive phase will only occur when "check_archives" is enabled and
     * when no problems were detected with the file/object being scanned by
     * this stage of the scan.
     */
    if (
        $phpMussel['Config']['files']['check_archives'] &&
        !empty($in) &&
        $phpMussel['Config']['files']['max_recursion'] > 1
    ) {

..the archive phase of the scan process mightn't execute properly if max_recursion is <= 1.

(As a side-note.. Something that I might need to either modify to document better at some point, I think; Not sure whether it'll make sense to everyone in its current form).

[senky]

check_archives=true
max_recursion=10

Still the same result - gif is blocked, zip is not.

[Maikuolan]

Hm, interesting. Values look good; Cache data there looks good too.

Still looking into it but haven't thought of any other possible leads yet. Will reply again when I think of something.

[senky]

@Maikuolan I investigated a bit and this is what I found:
Condition if (is_dir('phar://' . $f) && is_readable('phar://' . $f)) is equal to false, so $PharData['DoScan'] = false and thus archive is not scanned. That's very strange, because php -m returns Phar line. Any idea?

[Maikuolan]

Hm, I think you're onto something there.

When I attempt to scan the ZIP locally, for me, the condition is true, and like you mentioned, the contents of the ZIP won't be scanned if the condition is false, due to that phpMussel will think that the file isn't an archive.

As for why the condition evaluates differently between your installation and mine, offhandedly, I'm not entirely sure, but maybe there's a readability issue with the file (i.e., the is_readable part evaluating as false because of the file being locked for some reason or for being inaccessible to PHP)? Of course, if PHP couldn't read the file, I'd normally expect errors to pop up elsewhere, plus the theory doesn't hold up so well in consideration that the GIF file by itself seems to be able to be scanned correctly (and I assume that the server stores all temporary uploads in the same locations, regardless of the type of file in question, so accessibility / read permissions should all be the same), but it's one possible theory anyway.

Not using an outdated version of the Phar extension, per chance..? If you are, I could have a dig through the Phar extension changelogs, to see whether there's been some bug fixed in the past which could explain the problem we're experiencing here. Otherwise though, if your Phar extension is up-to-date, I doubt that'd be the problem here, seeing as I'm not seeing the problem replicated on my end at the moment.

[senky]

is_dir('phar://' . $f) returns false. So the script doesn't even check for readability. As I think of it now, I have never seen using phar://. Is it ok when my $f is /private/var/tmp/phpkp8Bnl? That way is_dir will check for phar:///private/var/tmp/phpkp8Bnl. Maybe there is some flag in php.ini to allow phar:// paths?

[Maikuolan]

In regards to phar://, according to the PHP docs, it should be fine to use from PHP 5.3.0 onward:

http://php.net/manual/en/wrappers.phar.php

Only information relating to Phar that I can immediately see in the latest php.ini file is this:

[Phar]
; http://php.net/phar.readonly
phar.readonly = Off

; http://php.net/phar.require-hash
;phar.require_hash = On

;phar.cache_list =

Still.. It's a possible lead. Will do some more digging.

[senky]

@Maikuolan got it!!!

Since uploaded file didn't have .zip extension, for some reason, my phar extension refused to scan the file (Cannot create phar '/private/var/tmp/phpESdUyi', file extension (or combination) not recognised or the directory does not exist). As soon as I renamed the file:

rename($f, $f . '.zip');
$f = $f . '.zip';

scan successfully blocked the file: Image chameleon attack detected (._graphics_standard_testfile.gif)!

Though I had to comment out file manipulation check...

[Maikuolan]

Oh wow. o.0

Glad to hear you've figured out what's causing the problem! Still.. Bizarre.

Renaming the ZIP file locally, I've found that, when I give it a non-ZIP extension (but still having some extension), it seems to still scan correctly, but when I remove the extension completely, I'm able to replicate the problem (scan reports "No problems found.").

[senky]

Good to hear you can replicate the problem finally! Still, the only solution as I think of it now seems to be give each archive it's extension back before scanning...

[DanielRuf]

Still, the only solution as I think of it now seems to be give each archive it's extension back before scanning

This is not possible in most cases as you would have to detect the archive format based on the magic bytes of the file header.

Doesn't the phar extension support reading files even if they have no extension?
There should be an easy way without modifying the file.

[Maikuolan]

Contemplating whether I should report this as a bug to the PHP bug tracker. Kind of feels like something that someone should've already reported by now at this point in time, as I'm sure there's no way we could've possibly been the first people to ever try to use the Phar extension with extensionless archives, but I've been digging through the bug tracker and changelogs a bit more carefully and haven't found anything yet that mentions issues with file extensions and Phar (though I could just be missing it; lot of stuff in the changelogs and bug tracker).

[Maikuolan]

PS, if anyone else wanted to give it a whirl:

• PHP bug tracker: https://bugs.php.net/
• PHAR docs: http://php.net/manual/en/book.phar.php
• PHP5 Changelog: http://php.net/ChangeLog-5.php
• PHP7 Changelog: http://php.net/ChangeLog-7.php

I haven't yet found anything useful there for this issue though.

[Maikuolan]

Wanting to check whether it's an issue specifically with is_dir or an issue with Phar generally, I tried doing a var_dump of a simple scandir of the file, firstly with an extension, then secondly without any extension.

With the extension:

%PATH%>php.exe -r "$foo=scandir('phar://x:/xxx.xxx');var_dump($foo);

Produces this (i.e., seems okay):

array(1) {
  [0]=>
  string(30) "graphics_standard_testfile.gif"
}

Without the extension:

%PATH%>php.exe -r "$foo=scandir('phar://x:/xxx');var_dump($foo);

Produces this:

PHP Warning:  scandir(phar://x:/xxx): failed to open dir: phar error: no directory in "phar://x:/xxx", must have at least phar://x:/xxx/ for root directory (always use full path to a new phar)
phar url "phar://x:/xxx" is unknown in Command line code on line 1
PHP Warning:  scandir(): (errno 9): Bad file descriptor in Command line code on line 1
bool(false)

[Maikuolan]

Problem seems specific to Phar. Using PHP's own zip_open function seems to work okay:

%PATH%>php.exe -r "$foo=zip_open('X:/xxx');var_dump($foo);

Produces this (expected):

resource(4) of type (Zip Directory)

Per the docs:

Returns a resource handle for later use with zip_read() and zip_close() or returns the number of error if filename does not exist or in case of other error.

So, worst case scenario, I could always revert back to using zip_open in the codebase. However, zip_open will only work for ZIP files (i.e., it won't be able to open TAR or PHAR files correctly), meaning this problem with opening extensionless archives would persist for non-ZIP archives using the Phar extension.

Definitely seems like a bug in Phar to me at this point. :-/

[senky]

Found something that at least deals with the same error message:
https://stackoverflow.com/questions/14102585/using-phar-stream-to-write-phardata-files

This is not possible in most cases as you would have to detect the archive format based on the magic bytes of the file header.

@DanielRuf as @Maikuolan states every other archive except for zip works fine. So why don't we attach zip extension only when we detect $PharType == 'ZIP'?

@Maikuolan can you open a new issue in PHP bug tracker? I hope there is someone who can quickly reproduce the problem and be skilled enough to fix it as well...

[senky]

So, worst case scenario, I could always revert back to using zip_open in the codebase.

Or maybe only try to use zip_open when phar fails.

[Maikuolan]

Or maybe only try to use zip_open when phar fails.

True, and good point.

@Maikuolan can you open a new issue in PHP bug tracker? I hope there is someone who can quickly reproduce the problem and be skilled enough to fix it as well...

Sounds like the best next course of action, I think. Will do so and include a link to the relevant bug tracker page here for referencing.

[senky]

Also I think a unit test that checks for this anomaly should be created. I assume you use Windows for development, but many servers are using linux/unix OS that probably generates the same temp filenames as my PC, so it's highly probable that archives are ignored for most users and that's why no one complains about it. Travis uses linux, so unit test would fail.

[Maikuolan]

Reported: https://bugs.php.net/bug.php?id=76061

[Maikuolan]

No responses on their end yet. Hopefully they'll respond soon, but we'll see. There seems to be 37 open issues (i.e., not closed, verified, or resolved) in their bug tracker at the moment that specifically relate to the Phar extension. Some of those are identified as being related to EoL PHP branches (not sure why they've kept those issues open, seeing as EoL would imply that those related bugs will never be fixed), but the majority (20 of them) are either identified as being related to non-EoL branches ( >= 5.6 ), or as not relating to specific versions. If I exclude anything older than 3 years, that's still 16 open issues. Fingers crossed though.

[neuropass]

Hi @Maikuolan any chance you would be so kind to add the fix suggested by @senky while we wait for the PHP fix? (we could be potentially waiting forever for their FIX :-/ )

Or maybe only try to use zip_open when phar fails.

[Maikuolan]

I'll take a look at this again in the morning and see what I can do. :-)

[neuropass]

Fantastic! you are very kind. Thanks so much in advance!

[Maikuolan]

I've coded and committed a quick workaround just now that should hopefully address this problem (for ZIP files at least), until something more permanent can be done about the problem.

I won't close this issue yet regardless, because I think the root of the problem (i.e., the bug as per reported to the PHP bug tracker) really needs to be fixed before I call this issue "closed", but that aside.. When you get a chance to update and test out the workaround, let me know how it goes, whether there are any problems and so on. :-)

[Maikuolan]

kelunik posted a not identical, but very similar issue towards the end of March (#76128), just a few weeks after I posted our issue on the bug tracker. Fast-forward to today.. it's mid-August, and still nobody has replied to the original issue which I'd posted earlier, but cmb has replied to the issue posted by kelunik, and in the absence of any reply to the issue which I posted, the reply which cmb gave to kelunik would probably suffice, given the similarity of the issues.

The reply:

The documentation states[1]:

The file name's extension must contain .phar.

So this is not a bug, but rather a feature request, in my opinion.

..which sounds awfully close to that old "it's not a bug, it's a feature" response, IMO (which I find a little disappointing personally, but considering I don't really have enough time to actively contribute any pull requests or code changes to the phar extension myself after setting aside time for the projects I'm already involved with, I'm not in much of a position to argue with them over that).

So.. I'll probably mark this as a "Won't Fix", I guess. (Even though we actually do now already have a viable workaround for ourselves, to solve the problem insofar as phpMussel is concerned.. But seeing as the source of the bug lies with the phar extension, and the current response, plus lack thereof, implies that they don't have any immediate plans to fix it in the near future, I think, not much point keeping this issue here open anymore).

The silver lining: One less issue to track at this repository here.

[neuropass]

I have to say this is disappointing... Especially for people who wait for fixes/updates and have no knowledge on how to do it themselves... Also considering the fact this is supposed to work just fine on shared hosting for example. In fact it does not as you cannot control your server and install what you need or mess around with libraries.

[Maikuolan]

(On a side-note.. I've been reading about some other, different but similarly problematic concerns that others have encountered with the phar wrapper elsewhere recently, and I've been considering that I might ditch using phar entirely in favour of something else at some point in the future, once I can figure out the best way to approach this.. though it'll likely require a complete rewrite and overhaul of how phpMussel approaches archives, and may involve possibly bumping phpMussel's major version, i.e., from v1.x.x to v2.x.x. Will post more details about this later though, in a separate issue).