Skip to content

Commit

Permalink
Fixed issue #13558: Authenticated remote code execution by unrestrict…
Browse files Browse the repository at this point in the history
…ed file uploads
  • Loading branch information
dominikvitt committed Jun 5, 2018
1 parent 0f710b3 commit 42080b5
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 7 deletions.
9 changes: 4 additions & 5 deletions application/controllers/admin/themes.php
Original file line number Diff line number Diff line change
Expand Up @@ -179,13 +179,12 @@ public function upload()
false
);
}
$checkImage = getimagesize($_FILES["file"]["tmp_name"]);
$debug[] = $checkImage;
if ($checkImage === false || !in_array($checkImage[2], [IMAGETYPE_JPEG, IMAGETYPE_PNG, IMAGETYPE_GIF])) {
$uploadresult = gT("This file is not a supported image - please only upload JPG,PNG or GIF type images.");
Yii::import('application.helpers.common_helper', true);
$checkImage = validateImage($_FILES["file"]["tmp_name"]);
if ($checkImage['check'] === false) {
return Yii::app()->getController()->renderPartial(
'/admin/super/_renderJson',
array('data' => ['success' => $success, 'message' => $uploadresult, 'debug' => $debug]),
array('data' => ['success' => $success, 'message' => $checkImage['uploadresult'], 'debug' => $checkImage['debug']]),
false,
false
);
Expand Down
24 changes: 24 additions & 0 deletions application/helpers/common_helper.php
Original file line number Diff line number Diff line change
Expand Up @@ -4916,3 +4916,27 @@ function get_absolute_path($path)
}
return implode(DIRECTORY_SEPARATOR, $absolutes);
}

/**
* A function to validate images
*
* @param mixed $path
* @return array
*/
function validateImage($path)
{
$result =[];
$checkImage = getimagesize($path);

This comment has been minimized.

Copy link
@Shnoulle

Shnoulle Jun 5, 2018

Collaborator

Like it :) (but why in common_helper) . Don't need @getimagesize($path) if debug is set ?

This comment has been minimized.

Copy link
@dominikvitt

dominikvitt Jun 5, 2018

Author Contributor

This function is used on 2 places already and possible to be used in a future, so it should be in a helper.
Also, function always return some result, so there shouldn't be any errors.

This comment has been minimized.

Copy link
@Shnoulle

Shnoulle Jun 5, 2018

Collaborator

Yes, but it can be in https://github.com/LimeSurvey/LimeSurvey/blob/master/application/core/LSYii_Validators.php or in a new Class with prefix \LimeSurvey\validate for example :)

Then we load and use it only when needed

Or : like https://github.com/LimeSurvey/LimeSurvey/blob/master/application/core/LSYii_EmailIDNAValidator.php . Really more cleaner.

This comment has been minimized.

Copy link
@LouisGac

LouisGac Jun 6, 2018

Contributor

I agree. Dominik, can you move it please? (btw: I'm the one who asked him to put it common_helper)

This comment has been minimized.

Copy link
@Shnoulle

Shnoulle Jun 6, 2018

Collaborator

Personnaly i'm for Replace all common_helper function by fnction inside Class, even if we need one class => one function ;) (kill kill common_helper)

This comment has been minimized.

Copy link
@dominikvitt

dominikvitt Jun 12, 2018

Author Contributor

It's fixed:
e352ce9

This comment has been minimized.

Copy link
@Shnoulle

Shnoulle Jun 12, 2018

Collaborator

Thank you \o/

$result['debug'] = $checkImage;
$result['uploadresult'] = '';
$checkSvg = mime_content_type($path);

if (!($checkImage !== false || $checkSvg === 'image/svg+xml')) {
$result['uploadresult'] = gT("This file is not a supported image - please only upload JPG,PNG,GIF or SVG type images.");
$result['check'] = false;
} else {
$result['check'] = true;
}

return $result;
}
5 changes: 3 additions & 2 deletions application/models/TemplateConfiguration.php
Original file line number Diff line number Diff line change
Expand Up @@ -642,8 +642,9 @@ private function _filterImages($file)

$filepath = App()->getAssetManager()->publish($imagePath);

$checkImage = getimagesize($imagePath);
if (!($checkImage === false || !in_array($checkImage[2], [IMAGETYPE_JPEG, IMAGETYPE_PNG, IMAGETYPE_GIF]))) {
Yii::import('application.helpers.common_helper', true);
$checkImage = validateImage($imagePath);
if (!$checkImage['check'] === false) {
return ['filepath' => $filepath, 'filepathOptions' => $imagePath ,'filename'=>$file['name']];
}
}
Expand Down

0 comments on commit 42080b5

Please sign in to comment.