Skip to content

Commit

Permalink
Add whitelist back in and validate extension of file is permtted for …
Browse files Browse the repository at this point in the history
…the mime-type supplied and use mime-type from db if supplied with an fid and eid

Switch to different libary that is php5.6 compatable
  • Loading branch information
seamuslee001 committed May 15, 2019
1 parent 430858d commit 6cb3fe2
Show file tree
Hide file tree
Showing 6 changed files with 127 additions and 17 deletions.
20 changes: 10 additions & 10 deletions CRM/Core/Page/File.php
Original file line number Diff line number Diff line change
Expand Up @@ -68,22 +68,22 @@ public function run() {
$mimeType = '';
$path = CRM_Core_Config::singleton()->customFileUploadDir . $fileName;
}
$passedInMimeType = CRM_Utils_Request::retrieveValue('mime-type', 'String', $mimeType, FALSE);

if (!$path) {
CRM_Core_Error::statusBounce('Could not retrieve the file');
}
if (!empty($mimeType) && !empty($passedInMimeType)) {
if ($passedInMimeType != $mimeType) {
throw new CRM_Core_Exception("Supplied Mime Type does not match file Mime Type");

if (empty($mimeType)) {

This comment has been minimized.

Copy link
@MegaphoneJon

MegaphoneJon Aug 6, 2020

Contributor

@seamuslee001 I've been getting Civi errors that I think are related to this line. Shouldn't the logic on this if statement be reversed?

This comment has been minimized.

Copy link
@seamuslee001

seamuslee001 Aug 6, 2020

Author Contributor

In what way and what errors? @MegaphoneJon are you able to open a lab ticket?

This comment has been minimized.

Copy link
@MegaphoneJon

MegaphoneJon Aug 6, 2020

Contributor

@seamuslee001 I can, but I wanted to check in with you first. The error is:

php TypeError: Argument 1 passed to MimeTyper\Repository\AbstractRepository::setFromMap() must be of the type array, null given, called in /var/www/www.communitydesign.org/htdocs/modules/civicrm/vendor/adrienrn/php-mimetyper/src/Repository/MimeDbRepository.php on line 52 in MimeTyper\Repository\AbstractRepository->setFromMap() (line 18 of /var/www/www.example.org/htdocs/modules/civicrm/vendor/adrienrn/php-mimetyper/src/Repository/AbstractRepository.php

It seems closely related to #17265, where we established that the new "MimeTyper" library won't accept NULL as a MIME type.

The reason I think the logic is reversed is a) based on the line it's replacing (which used !empty rather than empty) and b) now that I look at this again - how can $mimeType be anything but ''? So now I think the issue is that this conditional shouldn't be there, and my real problem is in another castle.

OK, now that this is non-trivial, I'll probably just take it back myself. I asked because I thought it would be something you could eyeball and say, "oh yeah, that's clearly not right."

This comment has been minimized.

Copy link
@seamuslee001

seamuslee001 Aug 6, 2020

Author Contributor

@MegaphoneJon yeh so just thinking through

you have either passed in a file name or a hash and no mime type so we get the mimeType out of the civicrm_file table and as such $mimeType will be set. It if is not set then we try and get it from the url with the param mime-type.

In the former case we set mimeType to be an empty string. and we check the passed in Mime Type is acceptable.

We then say ok lets look at the file extension from the file path and see if it matches a known ok extension for the passedin MimeType if yes then we allow it to continue. So my guessing mime-type might be missing from your URL perhaps

$passedInMimeType = CRM_Utils_Request::retrieveValue('mime-type', 'String', $mimeType, FALSE);
if (!in_array($passedInMimeType, explode(',', Civi::settings()->get('requestableMimeTypes')))) {
throw new CRM_Core_Exception("Supplied mime-type is not accepted");
}
}
elseif (!empty($passedInMimeType)) {
$testMimeType = CRM_Utils_File::getMimeType($path);
if ($testMimeType != $passedInMimeType) {
throw new CRM_Core_Exception("Supplied Mime Type does not match file Mime Type");
$extension = CRM_Utils_File::getExtensionFromPath($path);
$candidateExtensions = CRM_Utils_File::getAcceptableExtensionsForMimeType($passedInMimeType);
if (!in_array($extension, $candidateExtensions)) {
throw new CRM_Core_Exception("Supplied mime-type does not match file extension");
}
// Now that we have ensured that the mime-type matches to what we believe is the mime-type of the file
// Now that we have validated mime-type supplied as much as possible lets now set the MimeType variable/
$mimeType = $passedInMimeType;
}

Expand Down
26 changes: 21 additions & 5 deletions CRM/Utils/File.php
Original file line number Diff line number Diff line change
Expand Up @@ -1067,12 +1067,28 @@ public static function isValidFileName($fileName = NULL) {
}

/**
* Get the Mime-Type of a file based on the url path
* @param string $path full filename path
* @return string|bool
* Get the extensions that this MimeTpe is for
* @param string $mimeType the mime-type we want extensions for
* @return array
*/
public static function getAcceptableExtensionsForMimeType($mimeType = NULL) {
$mapping = \MimeType\Mapping::$types;
$extensions = [];
foreach ($mapping as $extension => $type) {
if ($mimeType == $type) {
$extensions[] = $extension;
}
}
return $extensions;
}

/**
* Get the extension of a file based on its path
* @param string $path path of the file to query
* @return string
*/
public function getMimeType($path = NULL) {
return mime_content_type($path);
public static function getExtensionFromPath($path) {
return pathinfo($path, PATHINFO_EXTENSION);
}

}
3 changes: 2 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@
"psr/simple-cache": "~1.0.1",
"cweagans/composer-patches": "~1.0",
"pear/log": "1.13.1",
"ezyang/htmlpurifier": "4.10"
"ezyang/htmlpurifier": "4.10",
"katzien/php-mime-type": "2.1.0"
},
"scripts": {
"post-install-cmd": [
Expand Down
46 changes: 45 additions & 1 deletion composer.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 14 additions & 0 deletions settings/Core.setting.php
Original file line number Diff line number Diff line change
Expand Up @@ -1052,4 +1052,18 @@
'help_text' => NULL,
'validate_callback' => 'CRM_Utils_Rule::color',
],
'requestableMimeTypes' => [
'group_name' => 'CiviCRM Preferences',
'group' => 'core',
'name' => 'requestableMimeTypes',
'type' => 'String',
'html_type' => 'Text',
'default' => 'image/jpeg,image/pjpeg,image/gif,image/x-png,image/png,image/jpg,text/html,application/pdf',
'add' => '5.13',
'title' => ts('Mime Types that can be passed as URL params'),
'is_domain' => 1,
'is_contact' => 0,
'description' => ts('Acceptable Mime Types that can be used as part of file urls'),
'help_text' => NULL,
],
];
35 changes: 35 additions & 0 deletions tests/phpunit/CRM/Utils/FileTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -95,4 +95,39 @@ public function testFileNameValid($fileName, $expectedResult) {
$this->assertEquals($expectedResult, CRM_Utils_File::isValidFileName($fileName));
}

public function pathToFileExtension() {
$cases = [];
$cases[] = ['/evil.pdf', 'pdf'];
$cases[] = ['/helloworld.jpg', 'jpg'];
$cases[] = ['/smartwatch_1736683_1280_9af3657015e8660cc234eb1601da871.jpg', 'jpg'];
return $cases;
}

/**
* Test returning appropriate file extension
* @dataProvider pathToFileExtension
* @param string $path
* @param string $expectedExtension
*/
public function testPathToExtension($path, $expectedExtension) {
$this->assertEquals($expectedExtension, CRM_Utils_File::getExtensionFromPath($path));
}

public function mimeTypeToExtension() {
$cases = [];
$cases[] = ['text/plain', ['txt', 'text', 'conf', 'def', 'list', 'log', 'in']];
$cases[] = ['image/jpeg', ['jpeg','jpg', 'jpe']];
$cases[] = ['image/png', ['png']];
return $cases;
}

/**
* @dataProvider mimeTypeToExtension
* @param stirng $mimeType
* @param array $expectedExtensions
*/
public function testMimeTypeToExtension($mimeType, $expectedExtensions) {
$this->assertEquals($expectedExtensions, CRM_Utils_File::getAcceptableExtensionsForMimeType($mimeType));
}

}

0 comments on commit 6cb3fe2

Please sign in to comment.