From a241926ded0738603791b748565893adaf4e8f64 Mon Sep 17 00:00:00 2001 From: Martin Date: Wed, 5 Oct 2016 13:34:21 +0200 Subject: [PATCH 1/2] Feature PR, references: Exclude Directories (merged, for professional Storage use) owncloud/core/pull/19235 Adds the ability to define directories in config.php which will not be further processed (scanned, shown, created ect). These directories exist but will not handled in nextcloud. Usecase: eg. filesystem which have the ability of snapshots have a defined naming for them. For details pls see also the documentation PR coming asap. --- .../Sabre/CustomPropertiesBackend.php | 3 + apps/dav/lib/Connector/Sabre/Directory.php | 16 +++++ apps/dav/lib/Connector/Sabre/ObjectTree.php | 27 +++++++- config/config.sample.php | 14 +++++ core/Controller/AvatarController.php | 2 +- lib/base.php | 4 +- lib/private/Files/Cache/Scanner.php | 2 +- lib/private/Files/Filesystem.php | 62 ++++++++++++++++--- lib/private/Files/Storage/Common.php | 4 +- lib/private/Files/View.php | 14 +++-- lib/private/Security/CertificateManager.php | 2 +- lib/private/legacy/helper.php | 2 +- tests/lib/Files/FilesystemTest.php | 27 +++++++- 13 files changed, 155 insertions(+), 24 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/CustomPropertiesBackend.php b/apps/dav/lib/Connector/Sabre/CustomPropertiesBackend.php index e9118cfc9e089..aef80f9f5480c 100644 --- a/apps/dav/lib/Connector/Sabre/CustomPropertiesBackend.php +++ b/apps/dav/lib/Connector/Sabre/CustomPropertiesBackend.php @@ -106,6 +106,9 @@ public function propFind($path, PropFind $propFind) { } catch (ServiceUnavailable $e) { // might happen for unavailable mount points, skip return; + } catch (Forbidden $e) { + // might happen for excluded mount points, skip + return; } catch (NotFound $e) { // in some rare (buggy) cases the node might not be found, // we catch the exception to prevent breaking the whole list with a 404 diff --git a/apps/dav/lib/Connector/Sabre/Directory.php b/apps/dav/lib/Connector/Sabre/Directory.php index 25cca40a88980..7fc441a0f10bd 100644 --- a/apps/dav/lib/Connector/Sabre/Directory.php +++ b/apps/dav/lib/Connector/Sabre/Directory.php @@ -107,7 +107,17 @@ public function __construct($view, $info, $tree = null, $shareManager = null) { */ public function createFile($name, $data = null) { + # the check here is necessary, because createFile uses put covered in sabre/file.php + # and not touch covered in files/view.php + if (\OC\Files\Filesystem::isForbiddenFileOrDir($name)) { + throw new \Sabre\DAV\Exception\Forbidden(); + } try { + # the check here is necessary, because createFile uses put covered in sabre/file.php + # and not touch covered in files/view.php + if (\OC\Files\Filesystem::isForbiddenFileOrDir($name)) { + throw new \Sabre\DAV\Exception\Forbidden(); + } // for chunked upload also updating a existing file is a "createFile" // because we create all the chunks before re-assemble them to the existing file. if (isset($_SERVER['HTTP_OC_CHUNKED'])) { @@ -156,7 +166,13 @@ public function createFile($name, $data = null) { * @throws \Sabre\DAV\Exception\ServiceUnavailable */ public function createDirectory($name) { + try { + # the check here is necessary, because createDirectory does not use the methods in files/view.php + if (\OC\Files\Filesystem::isForbiddenFileOrDir($name)) { + throw new \Sabre\DAV\Exception\Forbidden(); + } + if (!$this->info->isCreatable()) { throw new \Sabre\DAV\Exception\Forbidden(); } diff --git a/apps/dav/lib/Connector/Sabre/ObjectTree.php b/apps/dav/lib/Connector/Sabre/ObjectTree.php index af1cf79e1db66..00968a87a7c09 100644 --- a/apps/dav/lib/Connector/Sabre/ObjectTree.php +++ b/apps/dav/lib/Connector/Sabre/ObjectTree.php @@ -109,6 +109,7 @@ public function cacheNode(Node $node) { * @throws InvalidPath * @throws \Sabre\DAV\Exception\Locked * @throws \Sabre\DAV\Exception\NotFound + * @throws \Sabre\DAV\Exception\Forbidden * @throws \Sabre\DAV\Exception\ServiceUnavailable */ public function getNodeForPath($path) { @@ -116,6 +117,11 @@ public function getNodeForPath($path) { throw new \Sabre\DAV\Exception\ServiceUnavailable('filesystem not setup'); } + // check the path, also called when the path has been entered manually eg via a file explorer + if (\OC\Files\Filesystem::isForbiddenFileOrDir($path)) { + throw new \Sabre\DAV\Exception\Forbidden(); + } + $path = trim($path, '/'); if (isset($this->cache[$path])) { @@ -130,6 +136,11 @@ public function getNodeForPath($path) { } } + // check the path, also called when the path has been entered manually eg via a file explorer + if (\OC\Files\Filesystem::isForbiddenFileOrDir($path)) { + throw new \Sabre\DAV\Exception\Forbidden(); + } + // Is it the root node? if (!strlen($path)) { return $this->rootNode; @@ -203,6 +214,11 @@ public function move($sourcePath, $destinationPath) { throw new \Sabre\DAV\Exception\ServiceUnavailable('filesystem not setup'); } + # check the destination path, for source see below + if (\OC\Files\Filesystem::isForbiddenFileOrDir($destinationPath)) { + throw new \Sabre\DAV\Exception\Forbidden(); + } + $infoDestination = $this->fileView->getFileInfo(dirname($destinationPath)); if (dirname($destinationPath) === dirname($sourcePath)) { $sourcePermission = $infoDestination && $infoDestination->isUpdateable(); @@ -222,6 +238,9 @@ public function move($sourcePath, $destinationPath) { } $targetNodeExists = $this->nodeExists($destinationPath); + + // at getNodeForPath we also check the path for isForbiddenFileOrDir + // with that we have covered both source and destination $sourceNode = $this->getNodeForPath($sourcePath); if ($sourceNode instanceof \Sabre\DAV\ICollection && $targetNodeExists) { throw new \Sabre\DAV\Exception\Forbidden('Could not copy directory ' . $sourceNode->getName() . ', target exists'); @@ -316,7 +335,13 @@ public function copy($source, $destination) { throw new Forbidden('No permissions to copy object.'); } - // this will trigger existence check + # check the destination path, for source see below + if (\OC\Files\Filesystem::isForbiddenFileOrDir($destination)) { + throw new \Sabre\DAV\Exception\Forbidden(); + } + + // at getNodeForPath we also check the path for isForbiddenFileOrDir + // with that we have covered both source and destination $this->getNodeForPath($source); list($destinationDir, $destinationName) = \Sabre\HTTP\URLUtil::splitPath($destination); diff --git a/config/config.sample.php b/config/config.sample.php index 3aa0f353c593a..b657d8b9f8496 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -1118,6 +1118,20 @@ */ 'blacklisted_files' => array('.htaccess'), +/** + * Exclude specific directory names and disallow scanning, creating and renaming + * using these names. Case insensitive. + * Excluded directory names are queried at any path part like at the beginning, + * in the middle or at the end and will not be further processed if found. + * Please see the documentation for details and examples. + * Use when the storage backend supports eg snapshot directories to be excluded. + * WARNING: USE THIS ONLY IF YOU KNOW WHAT YOU ARE DOING. + */ +'excluded_directories' => + array ( + '.snapshot', + '~snapshot', + ), /** * Define a default folder for shared files and folders other than root. */ diff --git a/core/Controller/AvatarController.php b/core/Controller/AvatarController.php index b27d11822253c..191773d36e434 100644 --- a/core/Controller/AvatarController.php +++ b/core/Controller/AvatarController.php @@ -194,7 +194,7 @@ public function postAvatar($path) { if ( $files['error'][0] === 0 && is_uploaded_file($files['tmp_name'][0]) && - !\OC\Files\Filesystem::isFileBlacklisted($files['tmp_name'][0]) + !\OC\Files\Filesystem::isForbiddenFileOrDir($files['tmp_name'][0]) ) { if ($files['size'][0] > 20*1024*1024) { return new JSONResponse( diff --git a/lib/base.php b/lib/base.php index 7d86245818df0..04c6926c0e9e7 100644 --- a/lib/base.php +++ b/lib/base.php @@ -874,8 +874,8 @@ public static function registerLogRotate() { */ public static function registerFilesystemHooks() { // Check for blacklisted files - OC_Hook::connect('OC_Filesystem', 'write', 'OC\Files\Filesystem', 'isBlacklisted'); - OC_Hook::connect('OC_Filesystem', 'rename', 'OC\Files\Filesystem', 'isBlacklisted'); + OC_Hook::connect('OC_Filesystem', 'write', 'OC\Files\Filesystem', 'isForbiddenFileOrDir_Hook'); + OC_Hook::connect('OC_Filesystem', 'rename', 'OC\Files\Filesystem', 'isForbiddenFileOrDir_Hook'); } /** diff --git a/lib/private/Files/Cache/Scanner.php b/lib/private/Files/Cache/Scanner.php index 28f7be0b65ace..219f7c54eee93 100644 --- a/lib/private/Files/Cache/Scanner.php +++ b/lib/private/Files/Cache/Scanner.php @@ -347,7 +347,7 @@ protected function getNewChildren($folder) { if ($dh = $this->storage->opendir($folder)) { if (is_resource($dh)) { while (($file = readdir($dh)) !== false) { - if (!Filesystem::isIgnoredDir($file)) { + if (!Filesystem::isIgnoredDir($file) && !Filesystem::isForbiddenFileOrDir($file)) { $children[] = trim(\OC\Files\Filesystem::normalizePath($file), '/'); } } diff --git a/lib/private/Files/Filesystem.php b/lib/private/Files/Filesystem.php index d835fea7f4c1a..2ca862654ff41 100644 --- a/lib/private/Files/Filesystem.php +++ b/lib/private/Files/Filesystem.php @@ -565,29 +565,26 @@ static public function isValidPath($path) { * * @param array $data from hook */ - static public function isBlacklisted($data) { + static public function isForbiddenFileOrDir_Hook($data) { if (isset($data['path'])) { $path = $data['path']; } else if (isset($data['newpath'])) { $path = $data['newpath']; } if (isset($path)) { - if (self::isFileBlacklisted($path)) { + if (self::isForbiddenFileOrDir($path)) { $data['run'] = false; } } } /** + * depriciated, replaced by isForbiddenFileOrDir * @param string $filename - * @return bool + * @return boolean */ static public function isFileBlacklisted($filename) { - $filename = self::normalizePath($filename); - - $blacklist = \OC::$server->getConfig()->getSystemValue('blacklisted_files', array('.htaccess')); - $filename = strtolower(basename($filename)); - return in_array($filename, $blacklist); + return self::isForbiddenFileOrDir($filename); } /** @@ -604,6 +601,55 @@ static public function isIgnoredDir($dir) { return false; } +/** + * Check if the directory path / file name contains a Blacklisted or Excluded name + * config.php parameter arrays can contain file names to be blacklisted or directory names to be excluded + * Blacklist ... files that may harm the owncloud environment like a foreign .htaccess file + * Excluded ... directories that are excluded from beeing further processed, like snapshot directories + * The parameter $ed is only used in conjunction with unit tests as we handover here the excluded + * directory name to be tested against. $ed and the query with can be redesigned if filesystem.php will get + * a constructor where it is then possible to define the excluded directory names for unit tests. + * @param string $FileOrDir + * @param array $ed + * @return boolean + */ + static public function isForbiddenFileOrDir($FileOrDir, $ed = array()) { + $excluded = array(); + $blacklist = array(); + $path_parts = array(); + $ppx = array(); + $blacklist = \OC::$server->getSystemConfig()->getValue('blacklisted_files', array('.htaccess')); + if ($ed) { + $excluded = $ed; + } else { + $excluded = \OC::$server->getSystemConfig()->getValue('excluded_directories', $ed); + } + // explode '/' + $ppx = array_filter(explode('/', $FileOrDir), 'strlen'); + $ppx = array_map('strtolower', $ppx); + // further explode each array element with '\' and add to result array if found + foreach($ppx as $pp) { + // only add an array element if strlen != 0 + $path_parts = array_merge($path_parts, array_filter(explode('\\', $pp), 'strlen')); + } + if ($excluded) { + $excluded = array_map('trim', $excluded); + $excluded = array_map('strtolower', $excluded); + $match = array_intersect($path_parts, $excluded); + if ($match) { + return true; + } + } + $blacklist = array_map('trim', $blacklist); + $blacklist = array_map('strtolower', $blacklist); + $match = array_intersect($path_parts, $blacklist); + if ($match) { + return true; + } + return false; + } + + /** /** * following functions are equivalent to their php builtin equivalents for arguments/return values. */ diff --git a/lib/private/Files/Storage/Common.php b/lib/private/Files/Storage/Common.php index 63d3c004fd2ea..e4b6b42f9f8f6 100644 --- a/lib/private/Files/Storage/Common.php +++ b/lib/private/Files/Storage/Common.php @@ -209,7 +209,7 @@ public function copy($path1, $path2) { $dir = $this->opendir($path1); $this->mkdir($path2); while ($file = readdir($dir)) { - if (!Filesystem::isIgnoredDir($file)) { + if (!Filesystem::isIgnoredDir($file) && !Filesystem::isForbiddenFileOrDir($file)) { if (!$this->copy($path1 . '/' . $file, $path2 . '/' . $file)) { return false; } @@ -557,7 +557,7 @@ public function copyFromStorage(\OCP\Files\Storage $sourceStorage, $sourceIntern $result = $this->mkdir($targetInternalPath); if (is_resource($dh)) { while ($result and ($file = readdir($dh)) !== false) { - if (!Filesystem::isIgnoredDir($file)) { + if (!Filesystem::isIgnoredDir($file) && !Filesystem::isForbiddenFileOrDir($file)) { $result &= $this->copyFromStorage($sourceStorage, $sourceInternalPath . '/' . $file, $targetInternalPath . '/' . $file); } } diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index fa6ba20c342f3..34a8c8457024b 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -625,7 +625,7 @@ public function file_put_contents($path, $data) { if (is_resource($data)) { //not having to deal with streams in file_put_contents makes life easier $absolutePath = Filesystem::normalizePath($this->getAbsolutePath($path)); if (Filesystem::isValidPath($path) - and !Filesystem::isFileBlacklisted($path) + and !Filesystem::isForbiddenFileOrDir($path) ) { $path = $this->getRelativePath($absolutePath); @@ -722,7 +722,7 @@ public function rename($path1, $path2) { if ( Filesystem::isValidPath($path2) and Filesystem::isValidPath($path1) - and !Filesystem::isFileBlacklisted($path2) + and !Filesystem::isForbiddenFileOrDir($path2) ) { $path1 = $this->getRelativePath($absolutePath1); $path2 = $this->getRelativePath($absolutePath2); @@ -843,7 +843,7 @@ public function copy($path1, $path2, $preserveMtime = false) { if ( Filesystem::isValidPath($path2) and Filesystem::isValidPath($path1) - and !Filesystem::isFileBlacklisted($path2) + and !Filesystem::isForbiddenFileOrDir($path2) ) { $path1 = $this->getRelativePath($absolutePath1); $path2 = $this->getRelativePath($absolutePath2); @@ -1094,7 +1094,7 @@ private function basicOperation($operation, $path, $hooks = [], $extraParam = nu $postFix = (substr($path, -1, 1) === '/') ? '/' : ''; $absolutePath = Filesystem::normalizePath($this->getAbsolutePath($path)); if (Filesystem::isValidPath($path) - and !Filesystem::isFileBlacklisted($path) + and !Filesystem::isForbiddenFileOrDir($path) ) { $path = $this->getRelativePath($absolutePath); if ($path == null) { @@ -1408,13 +1408,17 @@ public function getDirectoryContent($directory, $mimetype_filter = '') { /** * @var \OC\Files\FileInfo[] $files */ + $files = array_filter($contents, function(ICacheEntry $content) { + return (!\OC\Files\Filesystem::isForbiddenFileOrDir($content['path'])); + }); + $files = array_map(function (ICacheEntry $content) use ($path, $storage, $mount, $sharingDisabled) { if ($sharingDisabled) { $content['permissions'] = $content['permissions'] & ~\OCP\Constants::PERMISSION_SHARE; } $owner = $this->getUserObjectForOwner($storage->getOwner($content['path'])); return new FileInfo($path . '/' . $content['name'], $storage, $content['path'], $content, $mount, $owner); - }, $contents); + }, $files); //add a folder for any mountpoint in this directory and add the sizes of other mountpoints to the folders $mounts = Filesystem::getMountManager()->findIn($path); diff --git a/lib/private/Security/CertificateManager.php b/lib/private/Security/CertificateManager.php index f7bf0df58c5ff..a572e5974ac29 100644 --- a/lib/private/Security/CertificateManager.php +++ b/lib/private/Security/CertificateManager.php @@ -139,7 +139,7 @@ public function createCertificateBundle() { * @throws \Exception If the certificate could not get added */ public function addCertificate($certificate, $name) { - if (!Filesystem::isValidPath($name) or Filesystem::isFileBlacklisted($name)) { + if (!Filesystem::isValidPath($name) or Filesystem::isForbiddenFileOrDir($name)) { throw new \Exception('Filename is not valid'); } diff --git a/lib/private/legacy/helper.php b/lib/private/legacy/helper.php index b19e58a9e6cde..543ca0f8b3f79 100644 --- a/lib/private/legacy/helper.php +++ b/lib/private/legacy/helper.php @@ -187,7 +187,7 @@ static function copyr($src, $dest) { self::copyr("$src/$file", "$dest/$file"); } } - } elseif (file_exists($src) && !\OC\Files\Filesystem::isFileBlacklisted($src)) { + } elseif (file_exists($src) && !\OC\Files\Filesystem::isForbiddenFileOrDir($src)) { copy($src, $dest); } } diff --git a/tests/lib/Files/FilesystemTest.php b/tests/lib/Files/FilesystemTest.php index 210ce4edc69ca..71ac8ad980945 100644 --- a/tests/lib/Files/FilesystemTest.php +++ b/tests/lib/Files/FilesystemTest.php @@ -259,7 +259,7 @@ public function isFileBlacklistedData() { array('.htaccess\\', true), array('/etc/foo\bar/.htaccess\\', true), array('/etc/foo\bar/.htaccess/', true), - array('/etc/foo\bar/.htaccess/foo', false), + array('/etc/foo\bar/.htaccess/foo', true), array('//foo//bar/\.htaccess/', true), array('\foo\bar\.HTAccess', true), ); @@ -269,7 +269,30 @@ public function isFileBlacklistedData() { * @dataProvider isFileBlacklistedData */ public function testIsFileBlacklisted($path, $expected) { - $this->assertSame($expected, \OC\Files\Filesystem::isFileBlacklisted($path)); + $this->assertSame($expected, \OC\Files\Filesystem::isForbiddenFileOrDir($path)); + } + + public function isExcludedData() { + return array( + array('.snapshot', true), + array('.snapshot/', true), + array('.snapshot\\', true), + array('/etc/foo/bar/foo.txt', false), + array('/.snapshot/etc/foo/bar/foo.txt', true), + array('/.snapShot/etc/foo/bar/foo.txt', true), + array('\.snapshot\etc\foo/bar\foo.txt', true), + array('/etc/foo/.snapshot', true), + array('/etc/foo/.snapshot/bar', true), + ); + } + + /** + * The parameter array can be redesigned if filesystem.php will get a constructor where it is possible to + * define the excluded directories for unit tests + * @dataProvider isExcludedData + */ + public function testIsExcluded($path, $expected) { + $this->assertSame($expected, \OC\Files\Filesystem::isForbiddenFileOrDir($path, array('.snapshot'))); } public function testNormalizePathUTF8() { From 76ad84944d100e312a432e948dc9063361ef43a7 Mon Sep 17 00:00:00 2001 From: Martin Date: Wed, 5 Oct 2016 14:39:43 +0200 Subject: [PATCH 2/2] Corrections according nickvergessen suggestions --- apps/dav/lib/Connector/Sabre/Directory.php | 9 ++------- apps/dav/lib/Connector/Sabre/ObjectTree.php | 14 +++++--------- lib/private/Files/Filesystem.php | 1 - 3 files changed, 7 insertions(+), 17 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/Directory.php b/apps/dav/lib/Connector/Sabre/Directory.php index 7fc441a0f10bd..6910a4a40ee52 100644 --- a/apps/dav/lib/Connector/Sabre/Directory.php +++ b/apps/dav/lib/Connector/Sabre/Directory.php @@ -107,17 +107,12 @@ public function __construct($view, $info, $tree = null, $shareManager = null) { */ public function createFile($name, $data = null) { - # the check here is necessary, because createFile uses put covered in sabre/file.php - # and not touch covered in files/view.php + // the check here is necessary, because createFile uses put covered in sabre/file.php + // and not touch covered in files/view.php if (\OC\Files\Filesystem::isForbiddenFileOrDir($name)) { throw new \Sabre\DAV\Exception\Forbidden(); } try { - # the check here is necessary, because createFile uses put covered in sabre/file.php - # and not touch covered in files/view.php - if (\OC\Files\Filesystem::isForbiddenFileOrDir($name)) { - throw new \Sabre\DAV\Exception\Forbidden(); - } // for chunked upload also updating a existing file is a "createFile" // because we create all the chunks before re-assemble them to the existing file. if (isset($_SERVER['HTTP_OC_CHUNKED'])) { diff --git a/apps/dav/lib/Connector/Sabre/ObjectTree.php b/apps/dav/lib/Connector/Sabre/ObjectTree.php index 00968a87a7c09..d2071091f977a 100644 --- a/apps/dav/lib/Connector/Sabre/ObjectTree.php +++ b/apps/dav/lib/Connector/Sabre/ObjectTree.php @@ -113,15 +113,16 @@ public function cacheNode(Node $node) { * @throws \Sabre\DAV\Exception\ServiceUnavailable */ public function getNodeForPath($path) { - if (!$this->fileView) { - throw new \Sabre\DAV\Exception\ServiceUnavailable('filesystem not setup'); - } - // check the path, also called when the path has been entered manually eg via a file explorer if (\OC\Files\Filesystem::isForbiddenFileOrDir($path)) { throw new \Sabre\DAV\Exception\Forbidden(); } + + if (!$this->fileView) { + throw new \Sabre\DAV\Exception\ServiceUnavailable('filesystem not setup'); + } + $path = trim($path, '/'); if (isset($this->cache[$path])) { @@ -136,11 +137,6 @@ public function getNodeForPath($path) { } } - // check the path, also called when the path has been entered manually eg via a file explorer - if (\OC\Files\Filesystem::isForbiddenFileOrDir($path)) { - throw new \Sabre\DAV\Exception\Forbidden(); - } - // Is it the root node? if (!strlen($path)) { return $this->rootNode; diff --git a/lib/private/Files/Filesystem.php b/lib/private/Files/Filesystem.php index 2ca862654ff41..086ee71bce117 100644 --- a/lib/private/Files/Filesystem.php +++ b/lib/private/Files/Filesystem.php @@ -649,7 +649,6 @@ static public function isForbiddenFileOrDir($FileOrDir, $ed = array()) { return false; } - /** /** * following functions are equivalent to their php builtin equivalents for arguments/return values. */