Skip to content
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

fix mime checkes #16091

Merged
merged 14 commits into from May 19, 2017
3 changes: 2 additions & 1 deletion administrator/language/en-GB/en-GB.lib_joomla.ini
Expand Up @@ -670,7 +670,8 @@ JLIB_MEDIA_ERROR_WARNFILETOOLARGE="This file is too large to upload."
JLIB_MEDIA_ERROR_WARNFILETYPE="This file type is not supported."
JLIB_MEDIA_ERROR_WARNIEXSS="Possible IE XSS Attack found."
JLIB_MEDIA_ERROR_WARNINVALID_IMG="Not a valid image."
JLIB_MEDIA_ERROR_WARNINVALID_MIME="Illegal or invalid mime type detected."
JLIB_MEDIA_ERROR_WARNINVALID_MIME="Invalid mime type detected."
JLIB_MEDIA_ERROR_WARNINVALID_MIMETYPE="Illegal mime type detected: %s"
JLIB_MEDIA_ERROR_WARNNOTADMIN="Uploaded file is not an image file and you do not have permission."

JLIB_NO_EDITOR_PLUGIN_PUBLISHED="Unable to display an editor because no editor plugin is published."
Expand Down
3 changes: 2 additions & 1 deletion language/en-GB/en-GB.lib_joomla.ini
Expand Up @@ -670,7 +670,8 @@ JLIB_MEDIA_ERROR_WARNFILETOOLARGE="This file is too large to upload."
JLIB_MEDIA_ERROR_WARNFILETYPE="This file type is not supported."
JLIB_MEDIA_ERROR_WARNIEXSS="Possible IE XSS Attack found."
JLIB_MEDIA_ERROR_WARNINVALID_IMG="Not a valid image."
JLIB_MEDIA_ERROR_WARNINVALID_MIME="Illegal or invalid mime type detected."
JLIB_MEDIA_ERROR_WARNINVALID_MIME="Invalid mime type detected."
JLIB_MEDIA_ERROR_WARNINVALID_MIMETYPE="Illegal mime type detected: %s"
JLIB_MEDIA_ERROR_WARNNOTADMIN="Uploaded file is not an image file and you do not have permission."

JLIB_NO_EDITOR_PLUGIN_PUBLISHED="Unable to display an editor because no editor plugin is published."
Expand Down
134 changes: 91 additions & 43 deletions libraries/cms/helper/media.php
Expand Up @@ -46,55 +46,75 @@ public static function getTypeIcon($fileName)
return strtolower(substr($fileName, strrpos($fileName, '.') + 1));
}

/**
* Get the Mime type
*
* @param string $file The link to the file to be checked
* @param boolean $isImage True if the passed file is a image else false
*
* @return mixed the mime type detected false on error
*
* @since __DEPLOY_VERSION__
*/
private function getMimeType($file, $isImage = false)
{
try
{
if ($isImage && function_exists('exif_imagetype'))
{
$mime = image_type_to_mime_type(exif_imagetype($file));
}
elseif ($isImage && function_exists('getimagesize'))
{
$imagesize = getimagesize($file);
$mime = (isset($imagesize['mime'])) ? $imagesize['mime'] : false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c/s remove outside () since not required.

}
elseif (function_exists('mime_content_type'))
{
// We have mime magic.
$mime = mime_content_type($file);
}
elseif (function_exists('finfo_open'))
{
// We have fileinfo
$finfo = finfo_open(FILEINFO_MIME_TYPE);
$mime = finfo_file($finfo, $file);
finfo_close($finfo);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like from the test we have cases where none of these things exist!? Probably having a final else statement here with return false to reject everything if we can't detect things is going to be important

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with: 298959e

}
catch (Exception $e)
{
// If we have any kind of error here => false;
return false;
}

// If we cant detect the mime try it again
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change cant to can't.

if ($mime == 'application/octet-stream' && $isImage === true)
Copy link
Contributor

@wilsonge wilsonge May 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably use === here for the mime check as now we are checking false to a string. So this should now be done in a type safe manner

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

be99e2a done please retest

{
return $this->getMimeType($file, false);
}

// We have a mime here
return $mime;
}

/**
* Checks the Mime type
*
* @param string $file The filename or tmp_name
* @param string $component The optional name for the component storing the parameters
* @param string $mime The filename or tmp_name
* @param string $component The optional name for the component storing the parameters
* @param boolean $image True if the passed file is a image else false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change a to an.

*
* @return boolean true if mime type checking is disabled or it passes the checks else false
*
* @since 3.7
*/
private function checkMimeType($file, $component = 'com_media')
private function checkMimeType($mime, $component = 'com_media')
{
$params = JComponentHelper::getParams($component);

if ($params->get('check_mime', 1))
{
$mime = false;

try
{
if (function_exists('exif_imagetype'))
{
$mime = image_type_to_mime_type(exif_imagetype($file));
}
elseif (function_exists('finfo_open'))
{
// We have fileinfo
$finfo = finfo_open(FILEINFO_MIME);
$mime = finfo_file($finfo, $file);

finfo_close($finfo);
}
elseif (function_exists('mime_content_type'))
{
// We have mime magic.
$mime = mime_content_type($file);
}
elseif (function_exists('getimagesize'))
{
$imagesize = getimagesize($file);
$mime = (isset($imagesize['mime'])) ? $imagesize['mime'] : false;
}
}
catch (Exception $e)
{
// If we have any kind of error here => false;
return false;
}

// Get the mime type configuration
$allowedMime = array_map('trim', explode(',', $params->get('upload_mime')));

Expand Down Expand Up @@ -164,7 +184,7 @@ public function canUpload($file, $component = 'com_media')
return false;
}

$filetype = array_pop($filetypes);
$filetype = array_pop($filetypes);
$allowable = array_map('trim', explode(',', $params->get('upload_extensions')));
$ignored = array_map('trim', explode(',', $params->get('ignore_extensions')));

Expand Down Expand Up @@ -193,10 +213,24 @@ public function canUpload($file, $component = 'com_media')
// If tmp_name is empty, then the file was bigger than the PHP limit
if (!empty($file['tmp_name']))
{
$result = $this->checkMimeType($file['tmp_name'], $component);
// Get the mime type this is a image file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change a to an. Add ; after type.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change.

$mime = $this->getMimeType($file['tmp_name'], true);

// If the mime type is not allowed we don't upload it
if ($result === false)
// Did we got anything usefull?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to // Did we get anything useful?

if ($mime != false)
{
$result = $this->checkMimeType($mime, $component, true);

// If the mime type is not allowed we don't upload it and show the mime code error to the user
if ($result === false)
{
$app->enqueueMessage(JText::_('JLIB_MEDIA_ERROR_WARNINVALID_MIMETYPE', $mime), 'error');

return false;
}
}
// We can't detect the mime type so it looks like a invalid image'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change a to an and remove '

else
{
$app->enqueueMessage(JText::_('JLIB_MEDIA_ERROR_WARNINVALID_IMG'), 'error');

Expand All @@ -212,10 +246,24 @@ public function canUpload($file, $component = 'com_media')
}
elseif (!in_array($filetype, $ignored))
{
$result = $this->checkMimeType($file['tmp_name'], $component);
// Get the mime type this is not a image file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change a to an. Add ; after type.

$mime = $this->getMimeType($file['tmp_name'], false);

// If the mime type is not allowed we don't upload it
if ($result === false)
// Did we got anything usefull?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to // Did we get anything useful?

if ($mime != false)
{
$result = $this->checkMimeType($mime, $component, true);

// If the mime type is not allowed we don't upload it and show the mime code error to the user
if ($result === false)
{
$app->enqueueMessage(JText::_('JLIB_MEDIA_ERROR_WARNINVALID_MIMETYPE', $mime), 'error');

return false;
}
}
// We can't detect the mime type so it looks like a invalid image'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change a to an and remove '.

else
{
$app->enqueueMessage(JText::_('JLIB_MEDIA_ERROR_WARNINVALID_MIME'), 'error');

Expand Down