From d43542a434c6a214c7533f76f3b1dc7afe84e5cf Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Tue, 12 Nov 2013 00:46:43 +0100 Subject: [PATCH] [ticket/11997] Use $phpbb_filesystem->clean_path() for proper redirect paths PHPBB3-11997 --- phpBB/includes/functions.php | 15 +++------ tests/security/redirect_test.php | 58 +++++++++++++++++++------------- 2 files changed, 39 insertions(+), 34 deletions(-) diff --git a/phpBB/includes/functions.php b/phpBB/includes/functions.php index da90dfea10..0a10a9604c 100644 --- a/phpBB/includes/functions.php +++ b/phpBB/includes/functions.php @@ -2645,7 +2645,7 @@ function generate_board_url($without_script_path = false) */ function redirect($url, $return = false, $disable_cd_check = false) { - global $db, $cache, $config, $user, $phpbb_root_path; + global $db, $cache, $config, $user, $phpbb_root_path, $phpbb_filesystem; $failover_flag = false; @@ -2713,16 +2713,7 @@ function redirect($url, $return = false, $disable_cd_check = false) $root_dirs = array_diff_assoc($root_dirs, $intersection); $page_dirs = array_diff_assoc($page_dirs, $intersection); - // Due to the dirname returned by pathinfo, the - // realpath for the $page_dirs array will be 2 - // superordinate folders up from the phpBB root - // path even if the supplied path is in the - // phpBB root path. We need to subtract these 2 - // folders in order to be able to correctly - // prepend '../' to the supplied path. - $superordinate_dirs_count = sizeof($root_dirs) - 2; - - $dir = (($superordinate_dirs_count > 0) ? str_repeat('../', $superordinate_dirs_count) : '') . implode('/', $page_dirs); + $dir = str_repeat('../', sizeof($root_dirs)) . implode('/', $page_dirs); // Strip / from the end if ($dir && substr($dir, -1, 1) == '/') @@ -2765,6 +2756,8 @@ function redirect($url, $return = false, $disable_cd_check = false) trigger_error('INSECURE_REDIRECT', E_USER_ERROR); } + $url = $phpbb_filesystem->clean_path($url); + if ($return) { return $url; diff --git a/tests/security/redirect_test.php b/tests/security/redirect_test.php index e934a4ab1b..6ea94d33be 100644 --- a/tests/security/redirect_test.php +++ b/tests/security/redirect_test.php @@ -17,26 +17,32 @@ class phpbb_security_redirect_test extends phpbb_security_test_base { // array(Input -> redirect(), expected triggered error (else false), expected returned result url (else false)) return array( - array('data://x', false, 'http://localhost/phpBB'), - array('bad://localhost/phpBB/index.php', 'INSECURE_REDIRECT', false), - array('http://www.otherdomain.com/somescript.php', false, 'http://localhost/phpBB'), - array("http://localhost/phpBB/memberlist.php\n\rConnection: close", 'INSECURE_REDIRECT', false), - array('javascript:test', false, 'http://localhost/phpBB/javascript:test'), - array('http://localhost/phpBB/index.php;url=', 'INSECURE_REDIRECT', false), - array('http://localhost/phpBB/app.php/foobar', false, 'http://localhost/phpBB/app.php/foobar'), - array('./app.php/foobar', false, 'http://localhost/phpBB/app.php/foobar'), - array('app.php/foobar', false, 'http://localhost/phpBB/app.php/foobar'), - array('./../app.php/foobar', false, 'http://localhost/phpBB/app.php/foobar'), - array('./../app.php/foo/bar', false, 'http://localhost/phpBB/app.php/foo/bar'), - array('./../foo/bar', false, 'http://localhost/phpBB/foo/bar'), - array('app.php/', false, 'http://localhost/phpBB/app.php/'), - array('./app.php/', false, 'http://localhost/phpBB/app.php/'), - array('foobar', false, 'http://localhost/phpBB/foobar'), - array('./foobar', false, 'http://localhost/phpBB/foobar'), - array('foo/bar', false, 'http://localhost/phpBB/foo/bar'), - array('./foo/bar', false, 'http://localhost/phpBB/foo/bar'), - array('./../index.php', false, 'http://localhost/phpBB/index.php'), - array('../index.php', false, 'http://localhost/phpBB/index.php'), + array('data://x', false, false, 'http://localhost/phpBB'), + array('bad://localhost/phpBB/index.php', false, 'INSECURE_REDIRECT', false), + array('http://www.otherdomain.com/somescript.php', false, false, 'http://localhost/phpBB'), + array("http://localhost/phpBB/memberlist.php\n\rConnection: close", false, 'INSECURE_REDIRECT', false), + array('javascript:test', false, false, 'http://localhost/phpBB/javascript:test'), + array('http://localhost/phpBB/index.php;url=', false, 'INSECURE_REDIRECT', false), + array('http://localhost/phpBB/app.php/foobar', false, false, 'http://localhost/phpBB/app.php/foobar'), + array('./app.php/foobar', false, false, 'http://localhost/phpBB/app.php/foobar'), + array('app.php/foobar', false, false, 'http://localhost/phpBB/app.php/foobar'), + array('./../app.php/foobar', false, false, 'http://localhost/app.php/foobar'), + array('./../app.php/foobar', true, false, 'http://localhost/app.php/foobar'), + array('./../app.php/foo/bar', false, false, 'http://localhost/app.php/foo/bar'), + array('./../app.php/foo/bar', true, false, 'http://localhost/app.php/foo/bar'), + array('./../foo/bar', false, false, 'http://localhost/foo/bar'), + array('./../foo/bar', true, false, 'http://localhost/foo/bar'), + array('app.php/', false, false, 'http://localhost/phpBB/app.php/'), + array('./app.php/', false, false, 'http://localhost/phpBB/app.php/'), + array('foobar', false, false, 'http://localhost/phpBB/foobar'), + array('./foobar', false, false, 'http://localhost/phpBB/foobar'), + array('foo/bar', false, false, 'http://localhost/phpBB/foo/bar'), + array('./foo/bar', false, false, 'http://localhost/phpBB/foo/bar'), + array('./../index.php', false, false, 'http://localhost/index.php'), + array('./../index.php', true, false, 'http://localhost/index.php'), + array('../index.php', false, false, 'http://localhost/index.php'), + array('../index.php', true, false, 'http://localhost/index.php'), + array('./index.php', false, false, 'http://localhost/phpBB/index.php'), ); } @@ -52,21 +58,27 @@ class phpbb_security_redirect_test extends phpbb_security_test_base /** * @dataProvider provider */ - public function test_redirect($test, $expected_error, $expected_result) + public function test_redirect($test, $disable_cd_check, $expected_error, $expected_result) { - global $user; + global $user, $phpbb_root_path; + + $temp_phpbb_root_path = $phpbb_root_path; + // We need to hack phpbb_root_path here, so it matches the actual fileinfo of the testing script. + // Otherwise the paths are returned incorrectly. + $phpbb_root_path = ''; if ($expected_error !== false) { $this->setExpectedTriggerError(E_USER_ERROR, $expected_error); } - $result = redirect($test, true); + $result = redirect($test, true, $disable_cd_check); // only verify result if we did not expect an error if ($expected_error === false) { $this->assertEquals($expected_result, $result); } + $phpbb_root_path = $temp_phpbb_root_path; } }