diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index be0729a3929d0..caac4904706df 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -3194,12 +3194,6 @@ - - - - - - diff --git a/lib/private/TempManager.php b/lib/private/TempManager.php index 0df31dce3fff1..31ae8181df94a 100644 --- a/lib/private/TempManager.php +++ b/lib/private/TempManager.php @@ -34,6 +34,7 @@ use bantu\IniGetWrapper\IniGetWrapper; use OCP\IConfig; use OCP\ITempManager; +use OCP\Security\ISecureRandom; use Psr\Log\LoggerInterface; class TempManager implements ITempManager { @@ -58,51 +59,25 @@ public function __construct(LoggerInterface $logger, IConfig $config, IniGetWrap $this->tmpBaseDir = $this->getTempBaseDir(); } - /** - * Builds the filename with suffix and removes potential dangerous characters - * such as directory separators. - * - * @param string $absolutePath Absolute path to the file / folder - * @param string $postFix Postfix appended to the temporary file name, may be user controlled - * @return string - */ - private function buildFileNameWithSuffix($absolutePath, $postFix = '') { + private function generateTemporaryPath(string $postFix): string { + $secureRandom = \OCP\Server::get(ISecureRandom::class); + $absolutePath = $this->tmpBaseDir . '/' . self::TMP_PREFIX . $secureRandom->generate(32, ISecureRandom::CHAR_ALPHANUMERIC); + if ($postFix !== '') { $postFix = '.' . ltrim($postFix, '.'); $postFix = str_replace(['\\', '/'], '', $postFix); - $absolutePath .= '-'; } return $absolutePath . $postFix; } - /** - * Create a temporary file and return the path - * - * @param string $postFix Postfix appended to the temporary file name - * @return string - */ - public function getTemporaryFile($postFix = '') { - if (is_writable($this->tmpBaseDir)) { - // To create an unique file and prevent the risk of race conditions - // or duplicated temporary files by other means such as collisions - // we need to create the file using `tempnam` and append a possible - // postfix to it later - $file = tempnam($this->tmpBaseDir, self::TMP_PREFIX); - $this->current[] = $file; + public function getTemporaryFile($postFix = ''): string|false { + $path = $this->generateTemporaryPath($postFix); - // If a postfix got specified sanitize it and create a postfixed - // temporary file - if ($postFix !== '') { - $fileNameWithPostfix = $this->buildFileNameWithSuffix($file, $postFix); - touch($fileNameWithPostfix); - chmod($fileNameWithPostfix, 0600); - $this->current[] = $fileNameWithPostfix; - return $fileNameWithPostfix; - } - - return $file; - } else { + $old_umask = umask(0077); + $fp = fopen($path, 'x'); + umask($old_umask); + if ($fp === false) { $this->log->warning( 'Can not create a temporary file in directory {dir}. Check it exists and has correct permissions', [ @@ -111,30 +86,16 @@ public function getTemporaryFile($postFix = '') { ); return false; } - } - /** - * Create a temporary folder and return the path - * - * @param string $postFix Postfix appended to the temporary folder name - * @return string - */ - public function getTemporaryFolder($postFix = '') { - if (is_writable($this->tmpBaseDir)) { - // To create an unique directory and prevent the risk of race conditions - // or duplicated temporary files by other means such as collisions - // we need to create the file using `tempnam` and append a possible - // postfix to it later - $uniqueFileName = tempnam($this->tmpBaseDir, self::TMP_PREFIX); - $this->current[] = $uniqueFileName; + fclose($fp); + $this->current[] = $path; + return $path; + } - // Build a name without postfix - $path = $this->buildFileNameWithSuffix($uniqueFileName . '-folder', $postFix); - mkdir($path, 0700); - $this->current[] = $path; + public function getTemporaryFolder($postFix = ''): string|false { + $path = $this->generateTemporaryPath($postFix) . '/'; - return $path . '/'; - } else { + if (mkdir($path, 0700) === false) { $this->log->warning( 'Can not create a temporary folder in directory {dir}. Check it exists and has correct permissions', [ @@ -143,6 +104,9 @@ public function getTemporaryFolder($postFix = '') { ); return false; } + + $this->current[] = $path; + return $path; } /** diff --git a/lib/public/ITempManager.php b/lib/public/ITempManager.php index f594bad3992e5..bca8eb63b5855 100644 --- a/lib/public/ITempManager.php +++ b/lib/public/ITempManager.php @@ -32,20 +32,20 @@ interface ITempManager { /** * Create a temporary file and return the path * - * @param string $postFix - * @return string + * @param string $postFix Postfix appended to the temporary file name + * * @since 8.0.0 */ - public function getTemporaryFile($postFix = ''); + public function getTemporaryFile(string $postFix = ''): string|false; /** * Create a temporary folder and return the path * - * @param string $postFix - * @return string + * @param string $postFix Postfix appended to the temporary folder name + * * @since 8.0.0 */ - public function getTemporaryFolder($postFix = ''); + public function getTemporaryFolder(string $postFix = ''): string|false; /** * Remove the temporary files and folders generated during this request diff --git a/tests/lib/TempManagerTest.php b/tests/lib/TempManagerTest.php index 4b342c365b595..e6a862921a78a 100644 --- a/tests/lib/TempManagerTest.php +++ b/tests/lib/TempManagerTest.php @@ -155,34 +155,23 @@ public function testLogCantCreateFolder() { $this->assertFalse($manager->getTemporaryFolder()); } - public function testBuildFileNameWithPostfix() { + public function testGenerateTemporaryPathWithPostfix(): void { $logger = $this->createMock(LoggerInterface::class); $tmpManager = self::invokePrivate( $this->getManager($logger), - 'buildFileNameWithSuffix', - ['/tmp/myTemporaryFile', 'postfix'] + 'generateTemporaryPath', + ['postfix'] ); - $this->assertEquals('/tmp/myTemporaryFile-.postfix', $tmpManager); + $this->assertStringEndsWith('.postfix', $tmpManager); } - public function testBuildFileNameWithoutPostfix() { + public function testGenerateTemporaryPathTraversal(): void { $logger = $this->createMock(LoggerInterface::class); $tmpManager = self::invokePrivate( $this->getManager($logger), - 'buildFileNameWithSuffix', - ['/tmp/myTemporaryFile', ''] - ); - - $this->assertEquals('/tmp/myTemporaryFile', $tmpManager); - } - - public function testBuildFileNameWithSuffixPathTraversal() { - $logger = $this->createMock(LoggerInterface::class); - $tmpManager = self::invokePrivate( - $this->getManager($logger), - 'buildFileNameWithSuffix', - ['foo', '../Traversal\\../FileName'] + 'generateTemporaryPath', + ['../Traversal\\../FileName'] ); $this->assertStringEndsNotWith('./Traversal\\../FileName', $tmpManager);