-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unset first node of filename array as we are only checking extension types #2739
Conversation
…types starting from the dot
IIRC, this check is here for when files get uploaded with file names like |
On 30 Dec 2013 08:07, "Michael Babker" notifications@github.com wrote:
Yes that is the reason. Iirc it is on the security tracker.
|
The problem is that the current code checks every part of a filename, including the name and extension, against the
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:
With this change, files named like
|
Seems good. Tested good. |
Shouldn't we just use $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:
In that case, wouldn't |
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 |
There is another mayor flaw in that code. The syntax is wrong! Anyway, I redid the code using an Also removed the useless Having said that: shouldn't the |
Also check out: #2744 |
Apache can interpret any extension within a filename with multiple extensions, so it is a security-relevant decision. ;-) |
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 |
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 ? |
This has been superseded by #3973 |
Addresses issue #2722