You can not select more than 25 topics Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.

TempManagerTest.php 5.9KB

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.
9 years ago
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.
9 years ago
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.
9 years ago
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.
9 years ago
123456789101112131415161718192021222324252627282930313233343536373839404142434445464748495051525354555657585960616263646566676869707172737475767778798081828384858687888990919293949596979899100101102103104105106107108109110111112113114115116117118119120121122123124125126127128129130131132133134135136137138139140141142143144145146147148149150151152153154155156157158159160161162163164165166167168169170171172173174175176177178179180181182183184185186187188189190191192193194195196197198199200201202203204
  1. <?php
  2. /**
  3. * Copyright (c) 2014 Robin Appelman <icewind@owncloud.com>
  4. * This file is licensed under the Affero General Public License version 3 or
  5. * later.
  6. * See the COPYING-README file.
  7. */
  8. namespace Test;
  9. use bantu\IniGetWrapper\IniGetWrapper;
  10. use OCP\IConfig;
  11. use Psr\Log\LoggerInterface;
  12. class TempManagerTest extends \Test\TestCase {
  13. protected $baseDir = null;
  14. protected function setUp(): void {
  15. parent::setUp();
  16. $this->baseDir = $this->getManager()->getTempBaseDir() . $this->getUniqueID('/oc_tmp_test');
  17. if (!is_dir($this->baseDir)) {
  18. mkdir($this->baseDir);
  19. }
  20. }
  21. protected function tearDown(): void {
  22. \OC_Helper::rmdirr($this->baseDir);
  23. $this->baseDir = null;
  24. parent::tearDown();
  25. }
  26. /**
  27. * @param \OCP\ILogger $logger
  28. * @param \OCP\IConfig $config
  29. * @return \OC\TempManager
  30. */
  31. protected function getManager($logger = null, $config = null) {
  32. if (!$logger) {
  33. $logger = $this->createMock(LoggerInterface::class);
  34. }
  35. if (!$config) {
  36. $config = $this->createMock(IConfig::class);
  37. $config->method('getSystemValue')
  38. ->with('tempdirectory', null)
  39. ->willReturn('/tmp');
  40. }
  41. $iniGetWrapper = $this->createMock(IniGetWrapper::class);
  42. $manager = new \OC\TempManager($logger, $config, $iniGetWrapper);
  43. if ($this->baseDir) {
  44. $manager->overrideTempBaseDir($this->baseDir);
  45. }
  46. return $manager;
  47. }
  48. public function testGetFile() {
  49. $manager = $this->getManager();
  50. $file = $manager->getTemporaryFile('txt');
  51. $this->assertStringEndsWith('.txt', $file);
  52. $this->assertTrue(is_file($file));
  53. $this->assertTrue(is_writable($file));
  54. file_put_contents($file, 'bar');
  55. $this->assertEquals('bar', file_get_contents($file));
  56. }
  57. public function testGetFolder() {
  58. $manager = $this->getManager();
  59. $folder = $manager->getTemporaryFolder();
  60. $this->assertStringEndsWith('/', $folder);
  61. $this->assertTrue(is_dir($folder));
  62. $this->assertTrue(is_writable($folder));
  63. file_put_contents($folder . 'foo.txt', 'bar');
  64. $this->assertEquals('bar', file_get_contents($folder . 'foo.txt'));
  65. }
  66. public function testCleanFiles() {
  67. $manager = $this->getManager();
  68. $file1 = $manager->getTemporaryFile('txt');
  69. $file2 = $manager->getTemporaryFile('txt');
  70. $this->assertTrue(file_exists($file1));
  71. $this->assertTrue(file_exists($file2));
  72. $manager->clean();
  73. $this->assertFalse(file_exists($file1));
  74. $this->assertFalse(file_exists($file2));
  75. }
  76. public function testCleanFolder() {
  77. $manager = $this->getManager();
  78. $folder1 = $manager->getTemporaryFolder();
  79. $folder2 = $manager->getTemporaryFolder();
  80. touch($folder1 . 'foo.txt');
  81. touch($folder1 . 'bar.txt');
  82. $this->assertTrue(file_exists($folder1));
  83. $this->assertTrue(file_exists($folder2));
  84. $this->assertTrue(file_exists($folder1 . 'foo.txt'));
  85. $this->assertTrue(file_exists($folder1 . 'bar.txt'));
  86. $manager->clean();
  87. $this->assertFalse(file_exists($folder1));
  88. $this->assertFalse(file_exists($folder2));
  89. $this->assertFalse(file_exists($folder1 . 'foo.txt'));
  90. $this->assertFalse(file_exists($folder1 . 'bar.txt'));
  91. }
  92. public function testCleanOld() {
  93. $manager = $this->getManager();
  94. $oldFile = $manager->getTemporaryFile('txt');
  95. $newFile = $manager->getTemporaryFile('txt');
  96. $folder = $manager->getTemporaryFolder();
  97. $nonOcFile = $this->baseDir . '/foo.txt';
  98. file_put_contents($nonOcFile, 'bar');
  99. $past = time() - 2 * 3600;
  100. touch($oldFile, $past);
  101. touch($folder, $past);
  102. touch($nonOcFile, $past);
  103. $manager2 = $this->getManager();
  104. $manager2->cleanOld();
  105. $this->assertFalse(file_exists($oldFile));
  106. $this->assertFalse(file_exists($folder));
  107. $this->assertTrue(file_exists($nonOcFile));
  108. $this->assertTrue(file_exists($newFile));
  109. }
  110. public function testLogCantCreateFile() {
  111. $this->markTestSkipped('TODO: Disable because fails on drone');
  112. $logger = $this->createMock(LoggerInterface::class);
  113. $manager = $this->getManager($logger);
  114. chmod($this->baseDir, 0500);
  115. $logger->expects($this->once())
  116. ->method('warning')
  117. ->with($this->stringContains('Can not create a temporary file in directory'));
  118. $this->assertFalse($manager->getTemporaryFile('txt'));
  119. }
  120. public function testLogCantCreateFolder() {
  121. $this->markTestSkipped('TODO: Disable because fails on drone');
  122. $logger = $this->createMock(LoggerInterface::class);
  123. $manager = $this->getManager($logger);
  124. chmod($this->baseDir, 0500);
  125. $logger->expects($this->once())
  126. ->method('warning')
  127. ->with($this->stringContains('Can not create a temporary folder in directory'));
  128. $this->assertFalse($manager->getTemporaryFolder());
  129. }
  130. public function testBuildFileNameWithPostfix() {
  131. $logger = $this->createMock(LoggerInterface::class);
  132. $tmpManager = self::invokePrivate(
  133. $this->getManager($logger),
  134. 'buildFileNameWithSuffix',
  135. ['/tmp/myTemporaryFile', 'postfix']
  136. );
  137. $this->assertEquals('/tmp/myTemporaryFile-.postfix', $tmpManager);
  138. }
  139. public function testBuildFileNameWithoutPostfix() {
  140. $logger = $this->createMock(LoggerInterface::class);
  141. $tmpManager = self::invokePrivate(
  142. $this->getManager($logger),
  143. 'buildFileNameWithSuffix',
  144. ['/tmp/myTemporaryFile', '']
  145. );
  146. $this->assertEquals('/tmp/myTemporaryFile', $tmpManager);
  147. }
  148. public function testBuildFileNameWithSuffixPathTraversal() {
  149. $logger = $this->createMock(LoggerInterface::class);
  150. $tmpManager = self::invokePrivate(
  151. $this->getManager($logger),
  152. 'buildFileNameWithSuffix',
  153. ['foo', '../Traversal\\../FileName']
  154. );
  155. $this->assertStringEndsNotWith('./Traversal\\../FileName', $tmpManager);
  156. $this->assertStringEndsWith('.Traversal..FileName', $tmpManager);
  157. }
  158. public function testGetTempBaseDirFromConfig() {
  159. $dir = $this->getManager()->getTemporaryFolder();
  160. $config = $this->createMock(IConfig::class);
  161. $config->expects($this->once())
  162. ->method('getSystemValue')
  163. ->with('tempdirectory', null)
  164. ->willReturn($dir);
  165. $this->baseDir = null; // prevent override
  166. $tmpManager = $this->getManager(null, $config);
  167. $this->assertEquals($dir, $tmpManager->getTempBaseDir());
  168. }
  169. }