[SECURITY] Prevent edit of file metadata of files with no access 04/40804/2 04/40804/3 04/40804/4
authorMarc Bastian Heinrichs <typo3@mbh-software.de>
Wed, 23 Apr 2014 15:28:46 +0000 (17:28 +0200)
committerBenjamin Mack <benni@typo3.org>
Wed, 1 Jul 2015 14:09:27 +0000 (16:09 +0200)
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: I0f0704af2e7f01d16b9420f9ba4ac1a7846b5270
Reviewed-on: http://review.typo3.org/40804
Reviewed-by: Benjamin Mack <benni@typo3.org>
Tested-by: Benjamin Mack <benni@typo3.org>
Reviewed-by: Helmut Hummel <helmut.hummel@typo3.org>
Tested-by: Helmut Hummel <helmut.hummel@typo3.org>
typo3/sysext/backend/Classes/Form/Container/InlineRecordContainer.php
typo3/sysext/core/Classes/Resource/Security/FileMetadataPermissionsAspect.php [new file with mode: 0644]
typo3/sysext/core/Documentation/Changelog/master/Feature-56644-AddHookToInlineRecordContainerCheckAccess.rst [new file with mode: 0644]
typo3/sysext/core/ext_localconf.php

index ddd3d4a..d74fe09 100644 (file)
@@ -654,6 +654,20 @@ class InlineRecordContainer extends AbstractContainer {
                if (!$backendUser->check('tables_modify', $table)) {
                        $hasAccess = FALSE;
                }
+               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 = $backendUser->errorMsg;
                        if ($deniedAccessReason) {
diff --git a/typo3/sysext/core/Classes/Resource/Security/FileMetadataPermissionsAspect.php b/typo3/sysext/core/Classes/Resource/Security/FileMetadataPermissionsAspect.php
new file mode 100644 (file)
index 0000000..b9ae4a9
--- /dev/null
@@ -0,0 +1,169 @@
+<?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_'));
+                       }
+                       $fileObject = ResourceFactory::getInstance()->getFileObject((int)$file);
+                       $accessAllowed = $fileObject->checkActionPermission('write');
+               }
+               return $accessAllowed;
+       }
+
+}
diff --git a/typo3/sysext/core/Documentation/Changelog/master/Feature-56644-AddHookToInlineRecordContainerCheckAccess.rst b/typo3/sysext/core/Documentation/Changelog/master/Feature-56644-AddHookToInlineRecordContainerCheckAccess.rst
new file mode 100644 (file)
index 0000000..3efdfce
--- /dev/null
@@ -0,0 +1,18 @@
+=========================================================================
+Feature: #56644 - Hook for InlineRecordContainer::checkAccess()
+=========================================================================
+
+Description
+===========
+
+Hook to post-process ``InlineRecordContainer::checkAccess``
+result. ``InlineRecordContainer::checkAccess`` is used to check
+the access to related inline records. It's implemented in the same way to
+the hook $GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS']['typo3/alt_doc.php']['makeEditForm_accessCheck']
+in the EditDocumentController.
+
+Register like this:
+
+.. code-block:: php
+
+       $GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS']['t3lib/class.t3lib_tceforms_inline.php']['checkAccess'][] = 'My\\Package\\HookClass->hookMethod';
\ No newline at end of file
index 46641ed..69eb610 100644 (file)
@@ -5,12 +5,19 @@ defined('TYPO3_MODE') or die();
 $signalSlotDispatcher = \TYPO3\CMS\Core\Utility\GeneralUtility::makeInstance(\TYPO3\CMS\Extbase\SignalSlot\Dispatcher::class);
 
 if (TYPO3_MODE === 'BE' && !(TYPO3_REQUESTTYPE & TYPO3_REQUESTTYPE_INSTALL)) {
+       // FAL SECURITY CHECKS
        $signalSlotDispatcher->connect(
                \TYPO3\CMS\Core\Resource\ResourceFactory::class,
                \TYPO3\CMS\Core\Resource\ResourceFactoryInterface::SIGNAL_PostProcessStorage,
                \TYPO3\CMS\Core\Resource\Security\StoragePermissionsAspect::class,
                'addUserPermissionsToStorage'
        );
+       $GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS']['t3lib/class.t3lib_tcemain.php']['processDatamapClass'][] = 'TYPO3\\CMS\\Core\\Resource\\Security\\FileMetadataPermissionsAspect';
+       $GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS']['typo3/alt_doc.php']['makeEditForm_accessCheck'][] = 'TYPO3\\CMS\\Core\\Resource\\Security\\FileMetadataPermissionsAspect->isAllowedToShowEditForm';
+       $GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS']['t3lib/class.t3lib_tceforms_inline.php']['checkAccess'][] = 'TYPO3\\CMS\\Core\\Resource\\Security\\FileMetadataPermissionsAspect->isAllowedToShowEditForm';
+       $GLOBALS['TYPO3_CONF_VARS']['SC_OPTIONS']['t3lib/class.t3lib_tcemain.php']['checkModifyAccessList'][] = 'TYPO3\\CMS\\Core\\Resource\\Security\\FileMetadataPermissionsAspect';
+
+       // PACKAGE MANAGEMENT
        $signalSlotDispatcher->connect(
                'PackageManagement',
                'packagesMayHaveChanged',