IIRC, this check is here for when files get uploaded with file names like malicious.php.txt or something like that. I'd have to dig through the archives known as the tracker and Skype to remember for sure.

On 30 Dec 2013 08:07, "Michael Babker" notifications@github.com wrote:

IIRC, this check is here for when files get uploaded with file names like
malicious.php.txt or something like that.

Yes that is the reason. Iirc it is on the security tracker.

I'd have to dig through the archives known as the tracker and Skype to
remember for sure.

—
Reply to this email directly or view it on GitHub.

The problem is that the current code checks every part of a filename, including the name and extension, against the $executable array that contains executable file type extensions. This results in files named something like php.jpg being falsely detected as executable as the file extension array created for the comparison match is currently like:

    Array
    (
        [0] => php <= filename, not extension
        [1] => jpg
    )

My fix is to shift off the first node of the array, which should not be checked, so that the file extension array that is created more accurately looks like:

Array
(
    [1] => jpg
)

With this change, files named like malicious.php.txt are still being correctly checked as the array created to check against is more accurate, like:

Array
(
    [1] => php
    [2] => txt
)

Tracker item at http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=33063

Seems good. Tested good.

Shouldn't we just use pathinfo to get the file extension and then check if it's in the forbidden array?

$extension = pathinfo($file['name'], PATHINFO_EXTENSION);

if (in_array($extension, $executable))
{
        $app->enqueueMessage(JText::_('JLIB_MEDIA_ERROR_WARNFILETYPE'), 'notice');

        return false;
}

Good question @phproberto

Per http://www.php.net/manual/en/function.pathinfo.php:

Note:
If the path has more than one extension, PATHINFO_EXTENSION returns only the last one and PATHINFO_FILENAME only strips the last one. (see first example below).

In that case, wouldn't malicious.php.txt not be detected as potentially malicious?

IIRC PATHINFO_EXTENSION is not enough as it only returns the final extension. The vulnerability that has to be solved is to prevent name.ext.ext being uploaded. This was discussed for a long time on the JSST and there were several options discussed. An alternative to the current method was proposed by @dongilbert at the time

Ah I see. Then maybe array_diff of file extensions vs executable extensions will save us that foreach.

There is another mayor flaw in that code. The syntax is wrong!
The > 2 part is inside the count function (both in original code as in this fix):
if (count($explodedFileName > 2))
should be:
if (count($explodedFileName) > 2)

Anyway, I redid the code using an array_intersect instead of a foreach loop:
#2743

Also removed the useless
$format = strtolower(JFile::getExt($file['name']));
as we already have the file name split into parts. So a simple array_pop is sufficient to get the actual extension.

Having said that: shouldn't the upload_extensions and ignore_extensions also be done on all file 'extensions' instead of only on the actual ending extension?
So if you want to ignore jpg, should it allow filenames like image.jpg.txt?

Also check out: #2744
Fixes the weird html tag check further on in that function.

So if you want to ignore jpg, should it allow filenames like image.jpg.txt?

Apache can interpret any extension within a filename with multiple extensions, so it is a security-relevant decision. ;-)

should it allow filenames like image.jpg.txt?

In the scope of this PR, the goal is to check for potentially executable files. Given that potentially executable files might be named something like image.php.txt, then yes, I'd block it.

Well, those are already blocked in the hardcoded checks.

@betweenbrain So did you take a look at https://github.com/joomla/joomla-cms/pull/2743/files ?
Should be a lot more stable and faster code.

From a quick review, yes #2743 does seem like a good solution. My goal here was to just fix the small issue at #2722.

This has been superseded by #3973