Skip to content

Commit

Permalink
[SECURITY] Prevent edit of file metadata of files with no access
Browse files Browse the repository at this point in the history 
By forging edit URLs it was possible to edit
meta data records of files which were not
within a user mount.

Implement several hooks to check access to the file
and only grant access to a meta data record if the
user has access to the file.

Resolves: #56644
Releases: master, 6.2
Security-Bulletin: TYPO3-CORE-SA-2015-002
Change-Id: I5e2de49e4af8cc68ecae604a9ef6b7e5917de769
Reviewed-on: http://review.typo3.org/40818
Reviewed-by: Helmut Hummel <helmut.hummel@typo3.org>
Tested-by: Helmut Hummel <helmut.hummel@typo3.org>
Reviewed-by: Benjamin Mack <benni@typo3.org>
Tested-by: Benjamin Mack <benni@typo3.org>
  • Loading branch information
@Mabahe @bmack
Mabahe authored and bmack committed Jul 1, 2015
1 parent fac6e13 commit bff9fa5
Show file tree
Hide file tree
Showing 3 changed files with 189 additions and 0 deletions.
14 changes: 14 additions & 0 deletions typo3/sysext/backend/Classes/Form/Element/InlineElement.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2146,6 +2146,20 @@ public function checkAccess($cmd, $table, $theUid) {
if (!$GLOBALS['BE_USER']->check('tables_modify', $table)) {
$hasAccess = 0;
}
if (
!empty($GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS']['t3lib/class.t3lib_tceforms_inline.php']['checkAccess'])
&& is_array($GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS']['t3lib/class.t3lib_tceforms_inline.php']['checkAccess'])
) {
foreach ($GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS']['t3lib/class.t3lib_tceforms_inline.php']['checkAccess'] as $_funcRef) {
$_params = array(
'table' => $table,
'uid' => $theUid,
'cmd' => $cmd,
'hasAccess' => $hasAccess
);
$hasAccess = GeneralUtility::callUserFunction($_funcRef, $_params, $this);
}
}
if (!$hasAccess) {
$deniedAccessReason = $GLOBALS['BE_USER']->errorMsg;
if ($deniedAccessReason) {
Expand Down
168 changes: 168 additions & 0 deletions typo3/sysext/core/Classes/Resource/Security/FileMetadataPermissionsAspect.php
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
<?php
namespace TYPO3\CMS\Core\Resource\Security;

/*
* This file is part of the TYPO3 CMS project.
*
* It is free software; you can redistribute it and/or modify it under
* the terms of the GNU General Public License, either version 2
* of the License, or any later version.
*
* For the full copyright and license information, please read the
* LICENSE.txt file that was distributed with this source code.
*
* The TYPO3 project - inspiring people to share!
*/

use TYPO3\CMS\Backend\Utility\BackendUtility;
use TYPO3\CMS\Core\DataHandling\DataHandler;
use TYPO3\CMS\Core\DataHandling\DataHandlerCheckModifyAccessListHookInterface;
use TYPO3\CMS\Core\Resource\ResourceFactory;
use TYPO3\CMS\Core\SingletonInterface;
use TYPO3\CMS\Core\Utility\MathUtility;

/**
* We do not have AOP in TYPO3 for now, thus the aspect which
* deals with file metadata data security is a assembly of hooks to
* check permissions on files belonging to file meta data records
*/
class FileMetadataPermissionsAspect implements DataHandlerCheckModifyAccessListHookInterface, SingletonInterface {

/**
* This hook is called before any write operation by DataHandler
*
* @param string $table
* @param int $id
* @param array $fileMetadataRecord
* @param int|NULL $otherHookGrantedAccess
* @param \TYPO3\CMS\Core\DataHandling\DataHandler $dataHandler
* @return int|null
*/
public function checkRecordUpdateAccess($table, $id, $fileMetadataRecord, $otherHookGrantedAccess, DataHandler $dataHandler) {
$accessAllowed = $otherHookGrantedAccess;
if ($table === 'sys_file_metadata' && $accessAllowed !== 0) {
$existingFileMetadataRecord = BackendUtility::getRecord('sys_file_metadata', $id);
if ($existingFileMetadataRecord === NULL || (empty($existingFileMetadataRecord['file']) && !empty($fileMetadataRecord['file']))) {
$existingFileMetadataRecord = $fileMetadataRecord;
}
$accessAllowed = $this->checkFileWriteAccessForFileMetaData($existingFileMetadataRecord) ? 1 : 0;
}

return $accessAllowed;
}

/**
* Hook that determines whether a user has access to modify a table.
* We "abuse" it here to actually check if access is allowed to sys_file_metadata.
*
*
* @param int &$accessAllowed Whether the user has access to modify a table
* @param string $table The name of the table to be modified
* @param DataHandler $parent The calling parent object
* @throws \UnexpectedValueException
* @return void
*/
public function checkModifyAccessList(&$accessAllowed, $table, DataHandler $parent) {
if ($table === 'sys_file_metadata') {
if (isset($parent->cmdmap[$table]) && is_array($parent->cmdmap[$table])) {
foreach ($parent->cmdmap[$table] as $id => $command) {
if (empty($id) || !MathUtility::canBeInterpretedAsInteger($id)) {
throw new \UnexpectedValueException(
'Integer expected for data manipulation command.
This can only happen in the case of an attack attempt or when something went horribly wrong.
To not compromise security, we exit here.',
1399982816
);
}

$fileMetadataRecord = BackendUtility::getRecord('sys_file_metadata', $id);
$accessAllowed = $this->checkFileWriteAccessForFileMetaData($fileMetadataRecord);
if (!$accessAllowed) {
// If for any item in the array, access is not allowed, we deny the whole operation
break;
}
}
}

if (isset($parent->datamap[$table]) && is_array($parent->datamap[$table])) {
foreach ($parent->datamap[$table] as $id => $data) {

$recordAccessAllowed = FALSE;

if (strpos($id, 'NEW') === FALSE) {
$fileMetadataRecord = BackendUtility::getRecord('sys_file_metadata', $id);
if ($fileMetadataRecord !== NULL) {
if ($parent->isImporting && empty($fileMetadataRecord['file'])) {
// when importing the record was added with an empty file relation as first step
$recordAccessAllowed = TRUE;
} else {
$recordAccessAllowed = $this->checkFileWriteAccessForFileMetaData($fileMetadataRecord);
}
}
} else {
// for new records record access is allowed
$recordAccessAllowed = TRUE;
}

if (isset($data['file'])) {
if ($parent->isImporting && empty($data['file'])) {
// when importing the record will be created with an empty file relation as first step
$dataAccessAllowed = TRUE;
} elseif (empty($data['file'])) {
$dataAccessAllowed = FALSE;
} else {
$dataAccessAllowed = $this->checkFileWriteAccessForFileMetaData($data);
}
} else {
$dataAccessAllowed = TRUE;
}

if (!$recordAccessAllowed || !$dataAccessAllowed) {
// If for any item in the array, access is not allowed, we deny the whole operation
$accessAllowed = FALSE;
break;
}
}
}
}
}

/**
* Deny access to the edit form. This is not mandatory, but better to show this right away that access is denied.
*
* @param array $parameters
* @return bool
*/
public function isAllowedToShowEditForm(array $parameters) {
$table = $parameters['table'];
$uid = $parameters['uid'];
$cmd = $parameters['cmd'];
$accessAllowed = $parameters['hasAccess'];

if ($accessAllowed && $table === 'sys_file_metadata' && $cmd === 'edit') {
$fileMetadataRecord = BackendUtility::getRecord('sys_file_metadata', $uid);
$accessAllowed = $this->checkFileWriteAccessForFileMetaData($fileMetadataRecord);
}
return $accessAllowed;
}

/**
* Checks write access to the file belonging to a metadata entry
*
* @param array $fileMetadataRecord
* @return bool
*/
protected function checkFileWriteAccessForFileMetaData($fileMetadataRecord) {
$accessAllowed = FALSE;
if (is_array($fileMetadataRecord) && !empty($fileMetadataRecord['file'])) {
$file = $fileMetadataRecord['file'];
// the file relation could be written as sys_file_[uid], strip this off before checking the rights
if (strpos($file, 'sys_file_') !== FALSE) {
$file = substr($file, strlen('sys_file_'));
}