diff --git a/application/utils.inc.php b/application/utils.inc.php index 291a180f2..a0973133a 100644 --- a/application/utils.inc.php +++ b/application/utils.inc.php @@ -2099,18 +2099,29 @@ class utils } /** - * Checks that path does not contains illegal characters, like '../' + * @param string $sPath for example '/var/www/html/itop/data/backups/manual/itop_27-2019-10-03_15_35.tar.gz' + * @param string $sBasePath for example '/var/www/html/itop/data/' * - * @param string $sPath + * @return bool false if path : + * * invalid + * * not allowed + * * not contained in base path + * Otherwise return the real path (see realpath()) * - * @return bool true if path is allowed, false otherwise - * - * @since 2.7.0 + * @since 2.7.0 N°2538 */ - final public static function IsAllowedPath($sPath) + final public static function RealPath($sPath, $sBasePath) { - $sPathNoDotDotPattern = "/^((?![\/\\\\]\.\.[\/\\\\]).)*$/"; + $sFileRealPath = realpath($sPath); + if ($sFileRealPath === false) + { + return false; + } + if (!self::StartsWith($sFileRealPath, $sBasePath)) + { + return false; + } - return preg_match($sPathNoDotDotPattern, $sPath) == 1; + return $sFileRealPath; } } diff --git a/approot.inc.php b/approot.inc.php index d07b3e6cd..76685ab32 100644 --- a/approot.inc.php +++ b/approot.inc.php @@ -1,6 +1,6 @@ DownloadBackup($sBackupDir.$sFile); + $oBackup->DownloadBackup($sBackupFilePath); } else { - throw new InvalidParameterException('Invalid file path'); + throw new CoreUnexpectedValue('Invalid file path'); } break; } diff --git a/test/application/UtilsTest.php b/test/application/UtilsTest.php index d67d4b423..f0fc39866 100644 --- a/test/application/UtilsTest.php +++ b/test/application/UtilsTest.php @@ -50,4 +50,39 @@ class UtilsTest extends \Combodo\iTop\Test\UnitTest\ItopTestCase [false, 1024, 2048], ]; } + + /** + * @dataProvider realPathDataProvider + */ + public function testRealPath($sPath, $sBasePath, $expected) + { + $this->assertSame($expected, utils::RealPath($sPath, $sBasePath)); + } + + public function realPathDataProvider() + { + parent::setUp(); // if not called, APPROOT won't be defined :( + + $sItopRootPath = APPROOT; + $sSep = DIRECTORY_SEPARATOR; + + return [ + 'licence.txt' => [$sItopRootPath.'license.txt', $sItopRootPath, $sItopRootPath.'license.txt'], + 'unexisting file' => [$sItopRootPath.'license_DOES_NOT_EXIST.txt', $sItopRootPath, false], + '/license.txt' => [$sItopRootPath.$sSep.'license.txt', $sItopRootPath, $sItopRootPath.'license.txt'], + '%2flicense.txt' => [$sItopRootPath.'%2flicense.txt', $sItopRootPath, false], + '../license.txt' => [$sItopRootPath.'..'.$sSep.'license.txt', $sItopRootPath, false], + '%2e%2e%2flicense.txt' => [$sItopRootPath.'%2e%2e%2flicense.txt', $sItopRootPath, false], + 'application/utils.inc.php with basepath=APPROOT' => [ + $sItopRootPath.'application/utils.inc.php', + $sItopRootPath, + $sItopRootPath.'application'.$sSep.'utils.inc.php', + ], + 'application/utils.inc.php with basepath=APPROOT/application' => [ + $sItopRootPath.'application/utils.inc.php', + $sItopRootPath.'application', + $sItopRootPath.'application'.$sSep.'utils.inc.php', + ], + ]; + } } \ No newline at end of file