From 68b8eb9ee98b058c5f4c34072ecd4df6419c69e6 Mon Sep 17 00:00:00 2001 From: demeritcowboy Date: Sun, 12 Dec 2021 11:45:13 -0500 Subject: [PATCH] avoid flooding logs when open_basedir is used --- CRM/Admin/Page/APIExplorer.php | 4 +- CRM/Utils/File.php | 45 +++++ tests/phpunit/CRM/Utils/FileTest.php | 244 +++++++++++++++++++++++++++ 3 files changed, 291 insertions(+), 2 deletions(-) diff --git a/CRM/Admin/Page/APIExplorer.php b/CRM/Admin/Page/APIExplorer.php index c769535be3..a7b2553900 100644 --- a/CRM/Admin/Page/APIExplorer.php +++ b/CRM/Admin/Page/APIExplorer.php @@ -63,7 +63,7 @@ class CRM_Admin_Page_APIExplorer extends CRM_Core_Page { $paths = self::uniquePaths(); foreach ($paths as $path) { $dir = \CRM_Utils_File::addTrailingSlash($path) . 'api' . DIRECTORY_SEPARATOR . 'v3' . DIRECTORY_SEPARATOR . 'examples'; - if (is_dir($dir)) { + if (\CRM_Utils_File::isDir($dir)) { foreach (scandir($dir) as $item) { if ($item && strpos($item, '.') === FALSE && array_search($item, $examples) === FALSE) { $examples[] = $item; @@ -96,7 +96,7 @@ class CRM_Admin_Page_APIExplorer extends CRM_Core_Page { $paths = self::uniquePaths(); foreach ($paths as $path) { $dir = \CRM_Utils_File::addTrailingSlash($path) . 'api' . DIRECTORY_SEPARATOR . 'v3' . DIRECTORY_SEPARATOR . 'examples' . DIRECTORY_SEPARATOR . $_GET['entity']; - if (is_dir($dir)) { + if (\CRM_Utils_File::isDir($dir)) { foreach (scandir($dir) as $item) { $item = str_replace('.ex.php', '', $item); if ($item && strpos($item, '.') === FALSE) { diff --git a/CRM/Utils/File.php b/CRM/Utils/File.php index e4df98f11f..493597cc00 100644 --- a/CRM/Utils/File.php +++ b/CRM/Utils/File.php @@ -1103,4 +1103,49 @@ HTACCESS; return pathinfo($path, PATHINFO_EXTENSION); } + /** + * Wrapper for is_dir() to avoid flooding logs when open_basedir is used. + * + * Don't use this function as a swap-in replacement for is_dir() for all + * situations as this might silence errors that you want to know about + * and would help troubleshoot problems. It should only be used when + * doing something like iterating over a set of folders where you know some + * of them might not legitimately exist or might be outside open_basedir + * because you're trying to find the right one. If you expect the path you're + * checking to be inside open_basedir, then you should use the regular + * is_dir(). (e.g. it might not exist but might be something + * like a cache folder in templates_c, which can't be outside open_basedir, + * so there you would use regular is_dir). + * + * **** Security alert **** + * If you change this function so that it would be possible to return + * TRUE without checking the real value of is_dir() then it opens up a + * possible security issue. + * It should either return FALSE, or the value returned from is_dir(). + * + * @param string|null $dir + * @return bool + */ + public static function isDir(?string $dir): bool { + set_error_handler(function($errno, $errstr) { + // If this is open_basedir-related, convert it to an exception so we + // can catch it. + if (strpos($errstr, 'open_basedir restriction in effect') !== FALSE) { + throw new \ErrorException($errstr, $errno); + } + // Continue with normal error handling so other errors still happen. + return FALSE; + }); + try { + $is_dir = is_dir($dir); + } + catch (\ErrorException $e) { + $is_dir = FALSE; + } + finally { + restore_error_handler(); + } + return $is_dir; + } + } diff --git a/tests/phpunit/CRM/Utils/FileTest.php b/tests/phpunit/CRM/Utils/FileTest.php index f475f92746..1f7162876a 100644 --- a/tests/phpunit/CRM/Utils/FileTest.php +++ b/tests/phpunit/CRM/Utils/FileTest.php @@ -176,6 +176,250 @@ class CRM_Utils_FileTest extends CiviUnitTestCase { unlink($file); } + /** + * Test isDir + * + * I stole some of these from php's own unit tests at + * https://github.com/php/php-src/blob/5b01c4863fe9e4bc2702b2bbf66d292d23001a18/ext/standard/tests/file/is_dir_basic.phpt + * and related files. + * + * @dataProvider isDirProvider + * + * @param string|null $input + * @param bool $expected + */ + public function testIsDir(?string $input, bool $expected) { + clearstatcache(); + $this->assertSame($expected, CRM_Utils_File::isDir($input)); + } + + /** + * Test isDir with invalid args. + * + * @dataProvider isDirInvalidArgsProvider + * + * @param mixed $input + * @param bool $expected + */ + public function testIsDirInvalidArgs($input, bool $expected) { + $this->assertSame($expected, CRM_Utils_File::isDir($input)); + } + + /** + * Just trying to include some of the same tests as php itself and + * this doesn't fit in well to a dataprovider so is separate. + */ + public function testIsDirMkdir() { + $a_dir = sys_get_temp_dir() . '/testIsDir'; + mkdir($a_dir); + $this->assertTrue(CRM_Utils_File::isDir($a_dir)); + mkdir($a_dir . '/aSubDir'); + $this->assertTrue(CRM_Utils_File::isDir($a_dir . '/aSubDir')); + clearstatcache(); + $this->assertTrue(CRM_Utils_File::isDir($a_dir)); + rmdir($a_dir . '/aSubDir'); + rmdir($a_dir); + } + + /** + * testIsDirSlashVariations + */ + public function testIsDirSlashVariations() { + $a_dir = sys_get_temp_dir() . '/testIsDir'; + mkdir($a_dir); + + $old_cwd = getcwd(); + $this->assertTrue(chdir(sys_get_temp_dir())); + + $this->assertTrue(CRM_Utils_File::isDir("./testIsDir")); + clearstatcache(); + $this->assertTrue(CRM_Utils_File::isDir("testIsDir/")); + clearstatcache(); + $this->assertTrue(CRM_Utils_File::isDir("./testIsDir/")); + clearstatcache(); + $this->assertTrue(CRM_Utils_File::isDir("testIsDir//")); + clearstatcache(); + $this->assertTrue(CRM_Utils_File::isDir("./testIsDir//")); + clearstatcache(); + $this->assertTrue(CRM_Utils_File::isDir(".//testIsDir//")); + clearstatcache(); + $this->assertFalse(CRM_Utils_File::isDir('testIsDir*')); + + // Note that in php8 is_dir changed in php itself to return false with no warning for these. It used to give `is_dir() expects parameter 1 to be a valid path, string given`. See https://github.com/php/php-src/commit/7bc7a80445f2bb349891d3cccfef2d589c48607e + clearstatcache(); + $this->assertFalse(CRM_Utils_File::isDir('./testIsDir/' . chr(0))); + clearstatcache(); + $this->assertFalse(CRM_Utils_File::isDir("testIsDir\0")); + + $this->assertTrue(chdir($old_cwd)); + rmdir($a_dir); + } + + /** + * Test hard and soft links with isDir + * Note hard links to directories aren't allowed so can only test with file. + */ + public function testIsDirLinks() { + if (strtoupper(substr(PHP_OS, 0, 3)) === 'WIN') { + $this->markTestSkipped('Windows has links but not the same.'); + } + + $a_dir = sys_get_temp_dir() . '/testIsDir'; + mkdir($a_dir); + symlink($a_dir, $a_dir . '_symlink'); + $this->assertTrue(CRM_Utils_File::isDir($a_dir . '_symlink')); + + $a_file = $a_dir . '/testFile'; + touch($a_file); + $this->assertFalse(CRM_Utils_File::isDir($a_file)); + + clearstatcache(); + symlink($a_file, $a_file . '_symlink'); + $this->assertFalse(CRM_Utils_File::isDir($a_file . '_symlink')); + + clearstatcache(); + link($a_file, $a_file . '_hardlink'); + $this->assertFalse(CRM_Utils_File::isDir($a_file . '_hardlink')); + + unlink($a_file . '_symlink'); + unlink($a_file . '_hardlink'); + unlink($a_file); + unlink($a_dir . '_symlink'); + rmdir($a_dir); + } + + /** + * Test isDir with open_basedir + * + * @link https://github.com/php/php-src/blob/5b01c4863fe9e4bc2702b2bbf66d292d23001a18/tests/security/open_basedir_is_dir.phpt + * + * @dataProvider isDirBasedirProvider + * + * @param string|null $input + * @param bool $expected + */ + public function testIsDirWithOpenBasedir(?string $input, bool $expected) { + $originalOpenBasedir = ini_get('open_basedir'); + + // This might not always be under cms root, but let's see how it goes. + $a_dir = \Civi::paths()->getPath('[civicrm.compile]/'); + if (file_exists("{$a_dir}/isDirTest/ok/ok.txt")) { + unlink("{$a_dir}/isDirTest/ok/ok.txt"); + } + if (is_dir("{$a_dir}/isDirTest/ok")) { + rmdir("{$a_dir}/isDirTest/ok"); + } + if (is_dir("{$a_dir}/isDirTest")) { + rmdir("{$a_dir}/isDirTest"); + } + + // We want the cms root path, but in headless tests even though there is + // a real cms strictly speaking the cms is "UNITTESTS", which might return + // something made up (currently NULL). + // \Civi::paths()->getPath('[cms.root]/') + // For now let's try this, assuming a drupal 7 structure where we know + // where this file is: + $cms_root = realpath(__DIR__ . '/../../../../../../../..'); + // We also need temp dir because phpunit creates files in there as it does stuff before we can reset basedir. + ini_set('open_basedir', $cms_root . PATH_SEPARATOR . sys_get_temp_dir()); + + $this->assertTrue(mkdir("{$a_dir}/isDirTest")); + $this->assertTrue(mkdir("{$a_dir}/isDirTest/ok")); + file_put_contents("{$a_dir}/isDirTest/ok/ok.txt", 'Hello World!'); + // hmm the "bad" isn't going to work the same way php's own tests work. We + // need to find a directory outside both cms_root and the sys temp dir. + // Let's just use some known unix files that always exist instead. + // mkdir("{$a_dir}/isDirTest/bad"); + + $old_cwd = getcwd(); + $this->assertTrue(chdir("{$a_dir}/isDirTest/ok")); + + clearstatcache(); + if ($expected) { + $this->assertTrue(CRM_Utils_File::isDir($input)); + } + else { + // Note that except for 'ok.txt', the real is_dir() would give an + // error for these. For 'ok.txt' it would return false, but no error. + // So this is what we are changing about the real function. + $this->assertFalse(CRM_Utils_File::isDir($input)); + } + + ini_set('open_basedir', $originalOpenBasedir); + $this->assertTrue(chdir($old_cwd)); + unlink("{$a_dir}/isDirTest/ok/ok.txt"); + rmdir("{$a_dir}/isDirTest/ok"); + rmdir("{$a_dir}/isDirTest"); + } + + /** + * dataprovider for testIsDir + * + * @return array + */ + public function isDirProvider(): array { + return [ + // explicit indices to make it easier to see which one failed + 0 => [ + // input value + NULL, + // expected value + FALSE, + ], + 1 => ['.', TRUE], + 2 => ['..', TRUE], + 3 => [__FILE__, FALSE], + 4 => [__DIR__, TRUE], + 5 => ['dontexist', FALSE], + 6 => ['/no/such/dir', FALSE], + 7 => [' ', FALSE], + ]; + } + + /** + * dataprovider for testIsDirInvalidArgs + * + * @return array + */ + public function isDirInvalidArgsProvider(): array { + return [ + // explicit indices to make it easier to see which one failed + 0 => [-2.34555, FALSE], + 1 => [TRUE, FALSE], + 2 => [FALSE, FALSE], + 3 => [0, FALSE], + 4 => [1234, FALSE], + ]; + } + + /** + * dataprovider for testIsDirWithOpenBasedir + * + * @return array + */ + public function isDirBasedirProvider(): array { + return [ + // explicit indices to make it easier to see which one failed + 0 => [ + // input value + strtoupper(substr(PHP_OS, 0, 3)) === 'WIN' ? 'C:/windows' : '/etc', + // expected value + FALSE, + ], + 1 => [strtoupper(substr(PHP_OS, 0, 3)) === 'WIN' ? 'C:/windows/win.ini' : '/etc/group', FALSE], + // This assumes a known location for template compile dir relative to + // open_basedir, and that we're 2 dirs below compile dir. + 2 => ['../../../../../../../..', FALSE], + 3 => ['../../../../../../../../', FALSE], + 4 => ['/', FALSE], + 5 => [strtoupper(substr(PHP_OS, 0, 3)) === 'WIN' ? 'C:/windows/../windows/win.ini' : '/etc/../etc/group', FALSE], + 6 => ['./../.', TRUE], + 7 => ['../ok', TRUE], + 8 => ['ok.txt', FALSE], + 9 => ['../ok/ok.txt', FALSE], + ]; + } + /** * dataprovider for testMakeFilenameWithUnicode * @return array -- 2.25.1