From ff9aeadb41e09a68b89f57fd2ec8433381afe878 Mon Sep 17 00:00:00 2001 From: Seamus Lee Date: Sat, 19 Jan 2019 09:01:17 +1100 Subject: [PATCH] security/core#26 Add in a generated Hash to download files so that URLs can't just be tested by annon users --- CRM/Batch/BAO/Batch.php | 5 +++-- CRM/Core/BAO/CustomField.php | 6 ++++-- CRM/Core/BAO/CustomGroup.php | 6 ++++-- CRM/Core/BAO/File.php | 17 ++++++++++++++++- CRM/Core/Page/File.php | 22 ++++++++++++++++++++++ CRM/PCP/Page/PCPInfo.php | 3 ++- CRM/Profile/Form.php | 3 ++- api/v3/Attachment.php | 3 ++- tests/phpunit/api/v3/AttachmentTest.php | 8 ++++++++ 9 files changed, 63 insertions(+), 10 deletions(-) diff --git a/CRM/Batch/BAO/Batch.php b/CRM/Batch/BAO/Batch.php index 2e4775cb42..21d61188a1 100644 --- a/CRM/Batch/BAO/Batch.php +++ b/CRM/Batch/BAO/Batch.php @@ -333,7 +333,8 @@ class CRM_Batch_BAO_Batch extends CRM_Batch_DAO_Batch { $activityParams = array('source_record_id' => $values['id'], 'activity_type_id' => $aid); $exportActivity = CRM_Activity_BAO_Activity::retrieve($activityParams, $val); $fid = CRM_Core_DAO::getFieldValue('CRM_Core_DAO_EntityFile', $exportActivity->id, 'file_id', 'entity_id'); - $tokens = array_merge(array('eid' => $exportActivity->id, 'fid' => $fid), $tokens); + $fileHash = CRM_Core_BAO_File::generateFileHash($exportActivity->id, $fid); + $tokens = array_merge(array('eid' => $exportActivity->id, 'fid' => $fid, 'fcs' => $fileHash), $tokens); } $values['action'] = CRM_Core_Action::formLink( $newLinks, @@ -486,7 +487,7 @@ class CRM_Batch_BAO_Batch extends CRM_Batch_DAO_Batch { 'download' => array( 'name' => ts('Download'), 'url' => 'civicrm/file', - 'qs' => 'reset=1&id=%%fid%%&eid=%%eid%%', + 'qs' => 'reset=1&id=%%fid%%&eid=%%eid%%&fcs=%%fcs%%', 'title' => ts('Download Batch'), ), ); diff --git a/CRM/Core/BAO/CustomField.php b/CRM/Core/BAO/CustomField.php index c6933779a3..fefffa4870 100644 --- a/CRM/Core/BAO/CustomField.php +++ b/CRM/Core/BAO/CustomField.php @@ -1495,8 +1495,9 @@ class CRM_Core_BAO_CustomField extends CRM_Core_DAO_CustomField { 'file_id' ); list($path) = CRM_Core_BAO_File::path($fileID, $entityId); + $fileHash = CRM_Core_BAO_File::generateFileHash($eid, $fid); $url = CRM_Utils_System::url('civicrm/file', - "reset=1&id=$fileID&eid=$contactID", + "reset=1&id=$fileID&eid=$contactID&fcs=$fileHash", $absolute, NULL, TRUE, TRUE ); $result['file_url'] = CRM_Utils_File::getFileURL($path, $fileType, $url); @@ -1507,8 +1508,9 @@ class CRM_Core_BAO_CustomField extends CRM_Core_DAO_CustomField { $fileID, 'uri' ); + $fileHash = CRM_Core_BAO_File::generateFileHash($eid, $fid); $url = CRM_Utils_System::url('civicrm/file', - "reset=1&id=$fileID&eid=$contactID", + "reset=1&id=$fileID&eid=$contactID&fcs=$fileHash", $absolute, NULL, TRUE, TRUE ); $result['file_url'] = CRM_Utils_File::getFileURL($uri, $fileType, $url); diff --git a/CRM/Core/BAO/CustomGroup.php b/CRM/Core/BAO/CustomGroup.php index b2a5cccf7b..c10e09218c 100644 --- a/CRM/Core/BAO/CustomGroup.php +++ b/CRM/Core/BAO/CustomGroup.php @@ -874,18 +874,19 @@ ORDER BY civicrm_custom_group.weight, $fileDAO->id = $dao->$fieldName; if ($fileDAO->find(TRUE)) { + $fileHash = CRM_Core_BAO_File::generateFileHash($dao->$entityIDName, $fileDAO->id); $entityIDName = "{$table}_entity_id"; $customValue['id'] = $dao->$idName; $customValue['data'] = $fileDAO->uri; $customValue['fid'] = $fileDAO->id; - $customValue['fileURL'] = CRM_Utils_System::url('civicrm/file', "reset=1&id={$fileDAO->id}&eid={$dao->$entityIDName}"); + $customValue['fileURL'] = CRM_Utils_System::url('civicrm/file', "reset=1&id={$fileDAO->id}&eid={$dao->$entityIDName}&fcs=$fileHash"); $customValue['displayURL'] = NULL; $deleteExtra = ts('Are you sure you want to delete attached file.'); $deleteURL = array( CRM_Core_Action::DELETE => array( 'name' => ts('Delete Attached File'), 'url' => 'civicrm/file', - 'qs' => 'reset=1&id=%%id%%&eid=%%eid%%&fid=%%fid%%&action=delete', + 'qs' => 'reset=1&id=%%id%%&eid=%%eid%%&fid=%%fid%%&action=delete&fcs=%%fcs%%', 'extra' => 'onclick = "if (confirm( \'' . $deleteExtra . '\' ) ) this.href+=\'&confirmed=1\'; else return false;"', ), @@ -896,6 +897,7 @@ ORDER BY civicrm_custom_group.weight, 'id' => $fileDAO->id, 'eid' => $dao->$entityIDName, 'fid' => $fieldID, + 'fcs' => $fileHash, ), ts('more'), FALSE, diff --git a/CRM/Core/BAO/File.php b/CRM/Core/BAO/File.php index 6b09fff8b2..d02ec078ce 100644 --- a/CRM/Core/BAO/File.php +++ b/CRM/Core/BAO/File.php @@ -333,6 +333,7 @@ class CRM_Core_BAO_File extends CRM_Core_DAO_File { $dao = CRM_Core_DAO::executeQuery($sql, $params); $results = array(); while ($dao->fetch()) { + $fileHash = self::generateFileHash($dao->entity_id, $dao->cfID); $result['fileID'] = $dao->cfID; $result['entityID'] = $dao->cefID; $result['mime_type'] = $dao->mime_type; @@ -340,7 +341,7 @@ class CRM_Core_BAO_File extends CRM_Core_DAO_File { $result['description'] = $dao->description; $result['cleanName'] = CRM_Utils_File::cleanFileName($dao->uri); $result['fullPath'] = $config->customFileUploadDir . DIRECTORY_SEPARATOR . $dao->uri; - $result['url'] = CRM_Utils_System::url('civicrm/file', "reset=1&id={$dao->cfID}&eid={$dao->entity_id}"); + $result['url'] = CRM_Utils_System::url('civicrm/file', "reset=1&id={$dao->cfID}&eid={$dao->entity_id}&fcs={$fileHash}"); $result['href'] = "{$result['cleanName']}"; $result['tag'] = CRM_Core_BAO_EntityTag::getTag($dao->cfID, 'civicrm_file'); $result['icon'] = CRM_Utils_File::getIconFromMimeType($dao->mime_type); @@ -766,4 +767,18 @@ AND CEF.entity_id = %2"; return NULL; } + /** + * Generates a MD5 Hash to be appended to file URLS to be checked when trying to download the file. + * @param int $eid entity id the file is attached to + * @param int $fid file ID + * @return string + */ + public static function generateFileHash($eid = NULL, $fid = NULL) { + // Use multiple (but stable) inputs for hash information. + $siteKey = defined('CIVICRM_SITE_KEY') ? CIVICRM_SITE_KEY : 'NO_SITE_KEY'; + // Trim 8 chars off the string, make it slightly easier to find + // but reveals less information from the hash. + return substr(md5("{$siteKey}_{$eid}_{$fid}"), 8); + } + } diff --git a/CRM/Core/Page/File.php b/CRM/Core/Page/File.php index d737223427..9b11dde3bc 100644 --- a/CRM/Core/Page/File.php +++ b/CRM/Core/Page/File.php @@ -45,6 +45,10 @@ class CRM_Core_Page_File extends CRM_Core_Page { $eid = CRM_Utils_Request::retrieve('eid', 'Positive', $this, TRUE); $fid = CRM_Utils_Request::retrieve('fid', 'Positive', $this, FALSE); $id = CRM_Utils_Request::retrieve('id', 'Positive', $this, TRUE); + $hash = CRM_Utils_Request::retrieve('fcs', 'Alphanumeric', $this); + if (!self::validateFileHash($hash, $eid, $fid)) { + CRM_Core_Error::statusBounce('URL for file is not valid'); + } list($path, $mimeType) = CRM_Core_BAO_File::path($id, $eid); $mimeType = CRM_Utils_Request::retrieveValue('mime-type', 'String', $mimeType, FALSE); @@ -80,4 +84,22 @@ class CRM_Core_Page_File extends CRM_Core_Page { } } + /** + * Validate a file Hash + * @param string $hash + * @param int $eid Entity Id the file is attached to + * @param int $fid File Id + * @return bool + */ + public static function validateFileHash($hash, $eid, $fid) { + if (empty($hash)) { + return TRUE; + } + $testHash = CRM_Core_BAO_File::generateFileHash($eid, $fid); + if ($testHash == $hash) { + return TRUE; + } + return FALSE; + } + } diff --git a/CRM/PCP/Page/PCPInfo.php b/CRM/PCP/Page/PCPInfo.php index bd51d17df2..3a3c2b34bf 100644 --- a/CRM/PCP/Page/PCPInfo.php +++ b/CRM/PCP/Page/PCPInfo.php @@ -202,8 +202,9 @@ class CRM_PCP_Page_PCPInfo extends CRM_Core_Page { if (!empty($entityFile)) { $fileInfo = reset($entityFile); $fileId = $fileInfo['fileID']; + $fileHash = CRM_Core_BAO_File::generateFileHash($this->_id, $fileId); $image = '_id}" + "reset=1&id=$fileId&eid={$this->_id}&fcs={$fileHash}" ) . '" />'; $this->assign('image', $image); } diff --git a/CRM/Profile/Form.php b/CRM/Profile/Form.php index 7dfdbdd10f..399f189c22 100644 --- a/CRM/Profile/Form.php +++ b/CRM/Profile/Form.php @@ -511,8 +511,9 @@ class CRM_Profile_Form extends CRM_Core_Form { $deleteExtra = ts("Are you sure you want to delete attached file?"); $fileId = $url['file_id']; + $fileHash = CRM_Core_BAO_File::generateFileHash($entityId, $fileId); $deleteURL = CRM_Utils_System::url('civicrm/file', - "reset=1&id={$fileId}&eid=$entityId&fid={$key}&action=delete" + "reset=1&id={$fileId}&eid=$entityId&fid={$key}&action=delete&fcs={$fileHash}" ); $text = ts("Delete Attached File"); $customFiles[$field['name']]['deleteURL'] = "$text"; diff --git a/api/v3/Attachment.php b/api/v3/Attachment.php index e9bc6cad95..a96a35886d 100644 --- a/api/v3/Attachment.php +++ b/api/v3/Attachment.php @@ -435,8 +435,9 @@ function _civicrm_api3_attachment_format_result($fileDao, $entityFileDao, $retur 'icon' => CRM_Utils_File::getIconFromMimeType($fileDao->mime_type), 'created_id' => $fileDao->created_id, ); + $fileHash = CRM_Core_BAO_File::generateFileHash($result['entity_id'], $result['id']); $result['url'] = CRM_Utils_System::url( - 'civicrm/file', 'reset=1&id=' . $result['id'] . '&eid=' . $result['entity_id'], + 'civicrm/file', 'reset=1&id=' . $result['id'] . '&eid=' . $result['entity_id'] . '&fcs=' . $fileHash, TRUE, NULL, FALSE, diff --git a/tests/phpunit/api/v3/AttachmentTest.php b/tests/phpunit/api/v3/AttachmentTest.php index d98b4ca585..b02abf868e 100644 --- a/tests/phpunit/api/v3/AttachmentTest.php +++ b/tests/phpunit/api/v3/AttachmentTest.php @@ -505,6 +505,14 @@ class api_v3_AttachmentTest extends CiviUnitTestCase { $getResult = $this->callAPISuccess('Attachment', 'get', $getParams); $actualNames = array_values(CRM_Utils_Array::collect('name', $getResult['values'])); + // Verify the hash generated by the API is valid if we were to try and load the file. + foreach ($getResult['values'] as $result) { + $queryResult = []; + $parsedURl = parse_url($result['url']); + $parsedQuery = parse_str($parsedURl['query'], $queryResult); + $this->assertTrue(CRM_Core_Page_File::validateFileHash($queryResult['fcs'], $queryResult['eid'], $queryResult['id'])); + } + sort($actualNames); sort($expectedNames); $this->assertEquals($expectedNames, $actualNames); -- 2.25.1