From 0b17efedbadb0c47f83e70d5c07bdd92186c7d77 Mon Sep 17 00:00:00 2001 From: Daniel Thee Roperto Date: Mon, 20 Feb 2017 12:01:51 +1100 Subject: [PATCH] Issue #104 - Allowed symlinks in dataroot, added unit tests. --- file.php | 17 ++---- lib.php | 31 ++++++++++ tests/phpunit/lib_test.php | 114 +++++++++++++++++++++++++++++++++++++ 3 files changed, 150 insertions(+), 12 deletions(-) create mode 100644 tests/phpunit/lib_test.php diff --git a/file.php b/file.php index 3bdc90f..d534a2d 100644 --- a/file.php +++ b/file.php @@ -60,23 +60,16 @@ header('Accept-Ranges: none'); * @SupressWarnings(PHPMD) */ function auth_outage_bootstrap_callback() { - global $CFG; - - // We are not using any external libraries or references in this file (cli maintenance is active). - // If you change the path below maybe you need to change maintenance_static_page::get_resources_folder() as well. - $resourcedir = rtrim($CFG->dataroot, '/'); // In case the configuration has a trailing slash. - $resourcedir = $resourcedir.'/auth_outage/climaintenance'; - - // Protect against path traversal attacks. - $file = $resourcedir.'/'.$_GET['file']; - if (realpath($file) !== $file) { + // Not using classes as classloader has not been initialized yet. Keep it minimalist. + require_once(__DIR__.'/lib.php'); + $file = auth_outage_get_climaintenance_resource_file($_GET['file']); + if (is_null($file)) { // @codingStandardsIgnoreStart error_log('Invalid file: '.$_GET['file']); // @codingStandardsIgnoreEnd http_response_code(404); - die('Not found.'); + die('File not found.'); } - readfile($file); exit(0); } diff --git a/lib.php b/lib.php index 142befc..dfda31a 100644 --- a/lib.php +++ b/lib.php @@ -56,3 +56,34 @@ function auth_outage_extend_navigation_user() { function auth_outage_outagelib_prepare_next_outage() { outagelib::prepare_next_outage(); } + +/** + * Used by file.php to fetch a file from sitedata, protecting it from path traversal attacks. + * + * To keep it minimalist it was not added to the outagelib.php class. + * + * @param $file string Filename to fetch from sitedata + * @return string|null Full path to the sitedata file or null if file is not valid. + */ +function auth_outage_get_climaintenance_resource_file($file) { + global $CFG; + + // We are not using any external libraries or references in this file (we have not gully loaded config.php yet). + // If you change the path below maybe you need to change maintenance_static_page::get_resources_folder() as well. + $resourcedir = rtrim($CFG->dataroot, '/'); // In case the configuration has a trailing slash. + $resourcedir = $resourcedir.'/auth_outage/climaintenance'; + + // Protect against path traversal attacks. + $basename = basename($file); + if ($basename !== $file) { + // @codingStandardsIgnoreStart + if (!PHPUNIT_TEST) { + error_log('Possible attempt for Path Traversal Attack (only filename expected): '.$file); + } + // @codingStandardsIgnoreEnd + return null; + } + + $realpath = realpath($resourcedir.'/'.$file); + return ($realpath == false) ? null : $realpath; +} diff --git a/tests/phpunit/lib_test.php b/tests/phpunit/lib_test.php new file mode 100644 index 0000000..f72fb56 --- /dev/null +++ b/tests/phpunit/lib_test.php @@ -0,0 +1,114 @@ +. + +/** + * tests for lib.php + * + * @package auth_outage + * @author Daniel Thee Roperto + * @copyright 2017 Catalyst IT + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); +require_once(__DIR__.'/base_testcase.php'); +require_once(__DIR__.'/../../lib.php'); + +/** + * tests for lib.php + * + * @package auth_outage + * @author Daniel Thee Roperto + * @copyright 2017 Catalyst IT + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @SuppressWarnings(public) Allow as many methods as needed. + */ +class lib_test extends auth_outage_base_testcase { + public function test_auth_outage_get_climaintenance_resource_file_resolves_a_file() { + global $CFG; + $dir = $CFG->dataroot.'/auth_outage/climaintenance'; + mkdir($dir, 0777, true); + + // Create a file. + $expected = $dir.'/example.txt'; + file_put_contents($expected, 'Outage Unit Test Message'); + + // Get that file. + $actual = auth_outage_get_climaintenance_resource_file('example.txt'); + + // Clean up. + unlink($expected); + rmdir($dir); + + self::assertSame($expected, $actual); + } + + /** + * Regression test for issue #104. + */ + public function test_auth_outage_get_climaintenance_resource_file_resolves_a_file_with_symlink() { + global $CFG; + + // Create a file. + $realdir = $CFG->dataroot.'/auth_outage/climaintenance_real'; + mkdir($realdir, 0777, true); + $realfile = $realdir.'/example.txt'; + file_put_contents($realfile, 'Outage Unit Test Message'); + + // Create a symlink + $symdir = $CFG->dataroot.'/auth_outage/climaintenance'; + if (!symlink($realdir, $symdir)) { + unlink($realfile); + rmdir($realdir); + $this->markTestSkipped('Canont create symlinks, maybe OS does not support.'); + return; + } + + // Get that file. + $actual = auth_outage_get_climaintenance_resource_file('example.txt'); + + // Clean up. + unlink($symdir); + unlink($realfile); + rmdir($realdir); + + self::assertSame($realfile, $actual); + } + + public function test_auth_outage_get_climaintenance_resource_file_prevent_path_traversal() { + global $CFG; + + $dir = $CFG->dataroot.'/auth_outage/climaintenance'; + mkdir($dir, 0777, true); + + // Create a file. + $expected = $dir.'/example.txt'; + file_put_contents($expected, 'Outage Unit Test Message'); + + // Create a sensitive file. + $sensitivefile = $CFG->dataroot.'/auth_outage/nuclear_silo_passwords.txt'; + file_put_contents($sensitivefile, 'The password to launch the ICBM: 123456'); + + // Path Traversal Attack. + $actual = auth_outage_get_climaintenance_resource_file('../n\\uclear_silo_passwords.txt'); + + // Clean up. + unlink($expected); + rmdir($dir); + + self::assertNull($actual); + } +}