[BUGFIX] Untrusted GP data is unserialized in wizard_colorpicker.php and view_help.php
authorChristian Kuhn <lolli@schwarzbu.ch>
Wed, 27 Jul 2011 10:50:46 +0000 (12:50 +0200)
committerOliver Hader <oliver@typo3.org>
Wed, 27 Jul 2011 10:52:04 +0000 (12:52 +0200)
Change-Id: I173ee6c20b4eca5cbdec37a117e7caf191f2b9af
Resolves: #24577
Reviewed-on: http://review.typo3.org/3778
Reviewed-by: Oliver Hader
Tested-by: Oliver Hader
t3lib/class.t3lib_formmail.php
t3lib/class.t3lib_lock.php
tests/t3lib/t3lib_formmailTest.php [new file with mode: 0644]
tests/t3lib/t3lib_lockTest.php
typo3/sysext/cms/tslib/showpic.php
typo3/wizard_colorpicker.php
typo3/wizard_tsconfig.php

index f9b22f2..4c618e8 100644 (file)
@@ -330,7 +330,9 @@ class t3lib_formmail {
         */
        public function __destruct() {
                foreach ($this->temporaryFiles as $file) {
-                       t3lib_div::unlink_tempfile($file);
+                       if (t3lib_div::isAllowedAbsPath($file) && t3lib_div::isFirstPartOfStr($file, PATH_site . 'typo3temp/upload_temp_')) {
+                               t3lib_div::unlink_tempfile($file);
+                       }
                }
        }
 }
index f734e9e..aebcfb9 100644 (file)
@@ -206,8 +206,10 @@ class t3lib_lock {
                $success = TRUE;
                switch ($this->method) {
                        case 'simple':
-                               if (unlink($this->resource) == FALSE) {
-                                       $success = FALSE;
+                               if (t3lib_div::isAllowedAbsPath($this->resource) && t3lib_div::isFirstPartOfStr($this->resource, PATH_site . 'typo3temp/locks/')) {
+                                       if (unlink($this->resource) == FALSE) {
+                                               $success = FALSE;
+                                       }
                                }
                        break;
                        case 'flock':
@@ -215,7 +217,9 @@ class t3lib_lock {
                                        $success = FALSE;
                                }
                                fclose($this->filepointer);
-                               unlink($this->resource);
+                               if (t3lib_div::isAllowedAbsPath($this->resource) && t3lib_div::isFirstPartOfStr($this->resource, PATH_site . 'typo3temp/locks/')) {
+                                       unlink($this->resource);
+                               }
                        break;
                        case 'semaphore':
                                if (@sem_release($this->resource)) {
diff --git a/tests/t3lib/t3lib_formmailTest.php b/tests/t3lib/t3lib_formmailTest.php
new file mode 100644 (file)
index 0000000..d8d2476
--- /dev/null
@@ -0,0 +1,88 @@
+<?php
+/***************************************************************
+ *  Copyright notice
+ *
+ *  (c) 2011 Christian Kuhn <lolli@schwarzbu.ch>
+ *  All rights reserved
+ *
+ *  This script is part of the TYPO3 project. The TYPO3 project is
+ *  free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  The GNU General Public License can be found at
+ *  http://www.gnu.org/copyleft/gpl.html.
+ *
+ *  This script is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  This copyright notice MUST APPEAR in all copies of the script!
+ ***************************************************************/
+
+/**
+ * Testcase for t3lib_formmail
+ *
+ * @author Christian Kuhn <lolli@schwarzbu.ch>
+ *
+ * @package TYPO3
+ * @subpackage t3lib
+ */
+class t3lib_formmailTest extends tx_phpunit_testcase {
+
+       ///////////////////////////////
+       // tests concerning __destruct
+       ///////////////////////////////
+
+       /**
+        * Dataprovider for destructorDoesNotRemoveFilesNotWithinTypo3TempDirectory
+        */
+       public function invalidFileReferences() {
+               return array(
+                       'not within PATH_site' => array('/tmp/TYPO3-Formmail-Test'),
+                       'does not start with upload_temp_' => array(PATH_site . 'typo3temp/foo'),
+                       'directory traversal' => array(PATH_site . 'typo3temp/../typo3temp/upload_temp_foo'),
+               );
+       }
+
+       /**
+        * @test
+        * @dataProvider invalidFileReferences
+        */
+       public function destructorDoesNotRemoveFilesNotWithinTypo3TempDirectory($file) {
+               if (TYPO3_OS === 'WIN') {
+                       $this->markTestSkipped('destructorDoesNotRemoveFilesNotWithinTypo3TempDirectory() test not available on Windows.');
+               }
+                       // Reflection needs php 5.3.2 or above
+               if (version_compare(phpversion(), '5.3.2', '<')) {
+                       $this->markTestSkipped('destructorDoesNotRemoveFilesNotWithinTypo3TempDirectory() test not available with php version smaller than 5.3.2');
+               }
+
+                       // Create test file
+               touch($file);
+               if (!is_file($file)) {
+                       $this->markTestSkipped('destructorDoesNotRemoveFilesNotWithinTypo3TempDirectory() skipped: Test file could not be created');
+               }
+
+                       // Create t3lib_formmail instance, inject invalid file
+               $instance = new t3lib_formmail(999999999, $lockMethod);
+               $t3libLockReflection = new ReflectionClass('t3lib_formmail');
+               $t3libLockReflectionResourceProperty = $t3libLockReflection->getProperty('temporaryFiles');
+               $t3libLockReflectionResourceProperty->setAccessible(TRUE);
+               $t3libLockReflectionResourceProperty->setValue($instance, array($file));
+
+                       // Call release method
+               $instance->__destruct();
+
+                       // Check if file is still there and clean up
+               $fileExists = is_file($file);
+               if (is_file($file)) {
+                       unlink($file);
+               }
+
+               $this->assertTrue($fileExists);
+       }
+}
+?>
index 65305ca..e726434 100644 (file)
@@ -73,5 +73,95 @@ class t3lib_lockTest extends tx_phpunit_testcase {
 
                $this->assertEquals($resultFilePermissions, '0777');
        }
+
+       ///////////////////////////////
+       // tests concerning release
+       ///////////////////////////////
+
+       /**
+        * Dataprovider for releaseRemovesLockfileInTypo3TempLocks
+        */
+       public function fileBasedLockMethods() {
+               return array(
+                       'simple' => array('simple'),
+                       'flock' => array('flock'),
+               );
+       }
+
+       /**
+        * @test
+        * @dataProvider fileBasedLockMethods
+        */
+       public function releaseRemovesLockfileInTypo3TempLocks($lockMethod) {
+                       // Use a very high id to be unique
+               $instance = new t3lib_lock(999999999, 'simple');
+                       // Disable logging
+               $instance->setEnableLogging(FALSE);
+                       // File pointer to current lock file
+               $lockFile = $instance->getResource();
+               $instance->acquire();
+
+               $instance->release();
+
+               $this->assertFalse(is_file($lockFile));
+       }
+
+       /**
+        * Dataprovider for releaseDoesNotRemoveFilesNotWithinTypo3TempLocksDirectory
+        */
+       public function invalidFileReferences() {
+               return array(
+                       'simple not within PATH_site' => array('simple', '/tmp/TYPO3-Lock-Test'),
+                       'flock not withing PATH_site' => array('flock', '/tmp/TYPO3-Lock-Test'),
+                       'simple directory traversal' => array('simple', PATH_site . 'typo3temp/../typo3temp/locks/foo'),
+                       'flock directory traversal' => array('flock', PATH_site . 'typo3temp/../typo3temp/locks/foo'),
+                       'simple directory traversal 2' => array('simple', PATH_site . 'typo3temp/locks/../locks/foo'),
+                       'flock directory traversal 2' => array('flock', PATH_site . 'typo3temp/locks/../locks/foo'),
+                       'simple within uploads' => array('simple', PATH_site . 'uploads/TYPO3-Lock-Test'),
+                       'flock within uploads' => array('flock', PATH_site . 'uploads/TYPO3-Lock-Test'),
+               );
+       }
+
+       /**
+        * @test
+        * @dataProvider invalidFileReferences
+        */
+       public function releaseDoesNotRemoveFilesNotWithinTypo3TempLocksDirectory($lockMethod, $file) {
+               if (TYPO3_OS === 'WIN') {
+                       $this->markTestSkipped('releaseDoesNotRemoveFilesNotWithinTypo3TempLocksDirectory() test not available on Windows.');
+               }
+                       // Reflection needs php 5.3.2 or above
+               if (version_compare(phpversion(), '5.3.2', '<')) {
+                       $this->markTestSkipped('releaseDoesNotRemoveFilesNotWithinTypo3TempLocksDirectory() test not available with php version smaller than 5.3.2');
+               }
+
+                       // Create test file
+               touch($file);
+               if (!is_file($file)) {
+                       $this->markTestSkipped('releaseDoesNotRemoveFilesNotWithinTypo3TempLocksDirectory() skipped: Test file could not be created');
+               }
+
+                       // Create t3lib_lock instance, set lockfile to invalid path
+               $instance = new t3lib_lock(999999999, $lockMethod);
+               $instance->setEnableLogging(FALSE);
+               $t3libLockReflection = new ReflectionClass('t3lib_lock');
+               $t3libLockReflectionResourceProperty = $t3libLockReflection->getProperty('resource');
+               $t3libLockReflectionResourceProperty->setAccessible(TRUE);
+               $t3libLockReflectionResourceProperty->setValue($instance, $file);
+               $t3libLockReflectionAcquiredProperty = $t3libLockReflection->getProperty('isAcquired');
+               $t3libLockReflectionAcquiredProperty->setAccessible(TRUE);
+               $t3libLockReflectionAcquiredProperty->setValue($instance, TRUE);
+
+                       // Call release method
+               $instance->release();
+
+                       // Check if file is still there and clean up
+               $fileExists = is_file($file);
+               if (is_file($file)) {
+                       unlink($file);
+               }
+
+               $this->assertTrue($fileExists);
+       }
 }
 ?>
index 516a5c6..1eb14cf 100644 (file)
@@ -183,7 +183,7 @@ class SC_tslib_showpic {
                        )
                );
 
-               if ($md5_value!=$this->md5) {
+               if ($md5_value !== $this->md5) {
                        throw new UnexpectedValueException('Parameter Error: Wrong parameters sent.');
                }
 
index 41fea31..304f023 100644 (file)
@@ -448,7 +448,7 @@ class SC_wizard_colorpicker {
        protected function areFieldChangeFunctionsValid() {
                return (
                        $this->fieldChangeFunc && $this->fieldChangeFuncHash
-                       && $this->fieldChangeFuncHash == t3lib_div::hmac($this->fieldChangeFunc)
+                       && $this->fieldChangeFuncHash === t3lib_div::hmac($this->fieldChangeFunc)
                );
        }
 }
index 714cbee..0e753b2 100644 (file)
@@ -633,7 +633,7 @@ class SC_wizard_tsconfig {
        protected function areFieldChangeFunctionsValid() {
                return (
                        isset($this->P['fieldChangeFunc']) && is_array($this->P['fieldChangeFunc']) && isset($this->P['fieldChangeFuncHash'])
-                       && $this->P['fieldChangeFuncHash'] == t3lib_div::hmac(serialize($this->P['fieldChangeFunc']))
+                       && $this->P['fieldChangeFuncHash'] === t3lib_div::hmac(serialize($this->P['fieldChangeFunc']))
                );
        }
 }