[Pkg-owncloud-commits] [owncloud] 31/215: Fix collision on temporary files + adjust permissions
David Prévot
taffit at moszumanska.debian.org
Tue May 5 01:01:17 UTC 2015
This is an automated email from the git hooks/post-receive script.
taffit pushed a commit to branch stable8
in repository owncloud.
commit 155ae44bc678331f4101b034ba4c64b001fde19d
Author: Lukas Reschke <lukas at owncloud.com>
Date: Thu Apr 23 14:29:15 2015 +0200
Fix collision on temporary files + adjust permissions
This changeset hardens the temporary file and directory creation to address multiple problems that may lead to exposure of files to other users, data loss or other unexpected behaviour that is impossible to debug.
**[CWE-668: Exposure of Resource to Wrong Sphere](https://cwe.mitre.org/data/definitions/668.html)**
The temporary file and folder handling as implemented in ownCloud is performed using a MD5 hash over `time()` concatenated with `rand()`. This is insufficiently and leads to the following security problems:
The generated filename could already be used by another user. It is not verified whether the file is already used and thus temporary files might be used for another user as well resulting in all possible stuff such as "user has file of other user".
Effectively this leaves us with:
1. A timestamp based on seconds (no entropy at all)
2. `rand()` which returns usually a number between 0 and 2,147,483,647
Considering the birthday paradox and that we use this method quite often (especially when handling external storage) this is quite error prone and needs to get addressed.
This behaviour has been fixed by using `tempnam` instead for single temporary files. For creating temporary directories an additional postfix will be appended, the solution is for directories still not absolutely bulletproof but the best I can think about at the moment. Improvement suggestions are welcome.
**[CWE-378: Creation of Temporary File With Insecure Permissions](https://cwe.mitre.org/data/definitions/378.html)**
Files were created using `touch()` which defaults to a permission of 0644. Thus other users on the machine may read potentially sensitive information as `/tmp/` is world-readable. However, ownCloud always encourages users to use a dedicated machine to run the ownCloud instance and thus this is no a high severe issue. Permissions have been adjusted to 0600.
**[CWE-379: Creation of Temporary File in Directory with Incorrect Permissions](https://cwe.mitre.org/data/definitions/379.html)**
Files were created using `mkdir()` which defaults to a permission of 0777. Thus other users on the machine may read potentially sensitive information as `/tmp/` is world-readable. However, ownCloud always encourages users to use a dedicated machine to run the ownCloud instance and thus this is no a high severe issue. Permissions have been adjusted to 0700.Please enter the commit message for your changes.
---
lib/private/tempmanager.php | 99 ++++++++++++++++++++++++++++-----------------
tests/lib/tempmanager.php | 29 +++++++++++--
2 files changed, 86 insertions(+), 42 deletions(-)
diff --git a/lib/private/tempmanager.php b/lib/private/tempmanager.php
index 5ab1427..eeffc6b 100644
--- a/lib/private/tempmanager.php
+++ b/lib/private/tempmanager.php
@@ -2,6 +2,7 @@
/**
* @author Morris Jobke <hey at morrisjobke.de>
* @author Robin Appelman <icewind at owncloud.com>
+ * @author Lukas Reschke <lukas at owncloud.com>
*
* @copyright Copyright (c) 2015, ownCloud, Inc.
* @license AGPL-3.0
@@ -26,24 +27,14 @@ use OCP\ILogger;
use OCP\ITempManager;
class TempManager implements ITempManager {
- /**
- * Current temporary files and folders
- *
- * @var string[]
- */
- protected $current = array();
-
- /**
- * i.e. /tmp on linux systems
- *
- * @var string
- */
+ /** @var string[] Current temporary files and folders, used for cleanup */
+ protected $current = [];
+ /** @var string i.e. /tmp on linux systems */
protected $tmpBaseDir;
-
- /**
- * @var \OCP\ILogger
- */
+ /** @var ILogger */
protected $log;
+ /** Prefix */
+ const TMP_PREFIX = 'oc_tmp_';
/**
* @param string $baseDir
@@ -55,35 +46,55 @@ class TempManager implements ITempManager {
}
/**
- * @param string $postFix
+ * 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
*/
- protected function generatePath($postFix) {
- if ($postFix) {
+ private function buildFileNameWithSuffix($absolutePath, $postFix = '') {
+ if($postFix !== '') {
$postFix = '.' . ltrim($postFix, '.');
+ $postFix = str_replace(['\\', '/'], '', $postFix);
+ $absolutePath .= '-';
}
- $postFix = str_replace(['\\', '/'], '', $postFix);
- return $this->tmpBaseDir . '/oc_tmp_' . md5(time() . rand()) . $postFix;
+
+ return $absolutePath . $postFix;
}
/**
* Create a temporary file and return the path
*
- * @param string $postFix
+ * @param string $postFix Postfix appended to the temporary file name
* @return string
*/
public function getTemporaryFile($postFix = '') {
- $file = $this->generatePath($postFix);
if (is_writable($this->tmpBaseDir)) {
- touch($file);
+ // 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;
+
+ // 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 {
$this->log->warning(
'Can not create a temporary file in directory {dir}. Check it exists and has correct permissions',
- array(
- 'dir' => $this->tmpBaseDir
- )
+ [
+ 'dir' => $this->tmpBaseDir,
+ ]
);
return false;
}
@@ -92,21 +103,30 @@ class TempManager implements ITempManager {
/**
* Create a temporary folder and return the path
*
- * @param string $postFix
+ * @param string $postFix Postfix appended to the temporary folder name
* @return string
*/
public function getTemporaryFolder($postFix = '') {
- $path = $this->generatePath($postFix);
if (is_writable($this->tmpBaseDir)) {
- mkdir($path);
+ // 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;
+
+ // Build a name without postfix
+ $path = $this->buildFileNameWithSuffix($uniqueFileName . '-folder', $postFix);
+ mkdir($path, 0700);
$this->current[] = $path;
+
return $path . '/';
} else {
$this->log->warning(
'Can not create a temporary folder in directory {dir}. Check it exists and has correct permissions',
- array(
- 'dir' => $this->tmpBaseDir
- )
+ [
+ 'dir' => $this->tmpBaseDir,
+ ]
);
return false;
}
@@ -119,6 +139,9 @@ class TempManager implements ITempManager {
$this->cleanFiles($this->current);
}
+ /**
+ * @param string[] $files
+ */
protected function cleanFiles($files) {
foreach ($files as $file) {
if (file_exists($file)) {
@@ -127,10 +150,10 @@ class TempManager implements ITempManager {
} catch (\UnexpectedValueException $ex) {
$this->log->warning(
"Error deleting temporary file/folder: {file} - Reason: {error}",
- array(
+ [
'file' => $file,
- 'error' => $ex->getMessage()
- )
+ 'error' => $ex->getMessage(),
+ ]
);
}
}
@@ -151,11 +174,11 @@ class TempManager implements ITempManager {
*/
protected function getOldFiles() {
$cutOfTime = time() - 3600;
- $files = array();
+ $files = [];
$dh = opendir($this->tmpBaseDir);
if ($dh) {
while (($file = readdir($dh)) !== false) {
- if (substr($file, 0, 7) === 'oc_tmp_') {
+ if (substr($file, 0, 7) === self::TMP_PREFIX) {
$path = $this->tmpBaseDir . '/' . $file;
$mtime = filemtime($path);
if ($mtime < $cutOfTime) {
diff --git a/tests/lib/tempmanager.php b/tests/lib/tempmanager.php
index 9bedd7c..72741d0 100644
--- a/tests/lib/tempmanager.php
+++ b/tests/lib/tempmanager.php
@@ -152,16 +152,37 @@ class TempManager extends \Test\TestCase {
$this->assertFalse($manager->getTemporaryFolder());
}
- public function testGeneratePathTraversal() {
+ public function testBuildFileNameWithPostfix() {
$logger = $this->getMock('\Test\NullLogger');
$tmpManager = \Test_Helper::invokePrivate(
$this->getManager($logger),
- 'generatePath',
- ['../Traversal\\../FileName']
+ 'buildFileNameWithSuffix',
+ ['/tmp/myTemporaryFile', 'postfix']
+ );
+
+ $this->assertEquals('/tmp/myTemporaryFile-.postfix', $tmpManager);
+ }
+
+ public function testBuildFileNameWithoutPostfix() {
+ $logger = $this->getMock('\Test\NullLogger');
+ $tmpManager = \Test_Helper::invokePrivate(
+ $this->getManager($logger),
+ 'buildFileNameWithSuffix',
+ ['/tmp/myTemporaryFile', '']
+ );
+
+ $this->assertEquals('/tmp/myTemporaryFile', $tmpManager);
+ }
+
+ public function testBuildFileNameWithSuffixPathTraversal() {
+ $logger = $this->getMock('\Test\NullLogger');
+ $tmpManager = \Test_Helper::invokePrivate(
+ $this->getManager($logger),
+ 'buildFileNameWithSuffix',
+ ['foo', '../Traversal\\../FileName']
);
$this->assertStringEndsNotWith('./Traversal\\../FileName', $tmpManager);
$this->assertStringEndsWith('.Traversal..FileName', $tmpManager);
-
}
}
--
Alioth's /usr/local/bin/git-commit-notice on /srv/git.debian.org/git/pkg-owncloud/owncloud.git
More information about the Pkg-owncloud-commits
mailing list