diff --git a/phpBB/includes/functions.php b/phpBB/includes/functions.php index aea13f7679..33d0e36a28 100644 --- a/phpBB/includes/functions.php +++ b/phpBB/includes/functions.php @@ -2696,89 +2696,27 @@ function redirect($url, $return = false, $disable_cd_check = false) // Relative uri $pathinfo = pathinfo($url); - // Also treat URLs that have a non-existing basename and fit - // controller style URLs - if (!$disable_cd_check && (!file_exists($pathinfo['dirname'] . '/') || (!file_exists($url) && preg_match('/^[\.]?+[\/]?+(?:app\.php)?+[a-zA-Z0-9\/]/', $url)))) + // Is the uri pointing to the current directory? + if ($pathinfo['dirname'] == '.') { - $url = str_replace('../', '', $url); - $pathinfo = pathinfo($url); + $url = str_replace('./', '', $url); - // Also treat URLs that have a non-existing basename - if (!file_exists($pathinfo['dirname'] . '/') || (!file_exists($url) && preg_match('/^[\.]?+[\/]?+(?:app\.php)?+[a-zA-Z0-9\/]/', $url))) + // Strip / from the beginning + if ($url && substr($url, 0, 1) == '/') { - // fallback to "last known user page" - // at least this way we know the user does not leave the phpBB root - if ($phpbb_path_helper instanceof \phpbb\path_helper) - { - $url = $phpbb_path_helper->get_controller_redirect_url($url); - } - else - { - $url = generate_board_url() . '/' . $user->page['page']; - } - $failover_flag = true; + $url = substr($url, 1); } } - if (!$failover_flag) - { - // Is the uri pointing to the current directory? - if ($pathinfo['dirname'] == '.') - { - $url = str_replace('./', '', $url); + $url = generate_board_url() . '/' . $url; + } - // Strip / from the beginning - if ($url && substr($url, 0, 1) == '/') - { - $url = substr($url, 1); - } + // Clean URL and check if we go outside the forum directory + $url = $phpbb_path_helper->clean_url($url); - if ($user->page['page_dir']) - { - $url = generate_board_url() . '/' . $user->page['page_dir'] . '/' . $url; - } - else - { - $url = generate_board_url() . '/' . $url; - } - } - else - { - // Used ./ before, but $phpbb_root_path is working better with urls within another root path - $root_dirs = explode('/', str_replace('\\', '/', phpbb_realpath($phpbb_root_path))); - $page_dirs = explode('/', str_replace('\\', '/', phpbb_realpath($pathinfo['dirname']))); - $intersection = array_intersect_assoc($root_dirs, $page_dirs); - - $root_dirs = array_diff_assoc($root_dirs, $intersection); - $page_dirs = array_diff_assoc($page_dirs, $intersection); - - $dir = str_repeat('../', sizeof($root_dirs)) . implode('/', $page_dirs); - - // Strip / from the end - if ($dir && substr($dir, -1, 1) == '/') - { - $dir = substr($dir, 0, -1); - } - - // Strip / from the beginning - if ($dir && substr($dir, 0, 1) == '/') - { - $dir = substr($dir, 1); - } - - $url = str_replace($pathinfo['dirname'] . '/', '', $url); - - // Strip / from the beginning - if (substr($url, 0, 1) == '/') - { - $url = substr($url, 1); - } - - $url = (!empty($dir) ? $dir . '/' : '') . $url; - $url = generate_board_url() . '/' . $url; - } - $url = $phpbb_path_helper->clean_url($url);; - } + if (!$disable_cd_check && strpos($url, generate_board_url(true)) === false) + { + trigger_error('INSECURE_REDIRECT', E_USER_ERROR); } // Make sure no linebreaks are there... to prevent http response splitting for PHP < 4.4.2 diff --git a/tests/security/redirect_test.php b/tests/security/redirect_test.php index 13db4c299b..ce2d865c44 100644 --- a/tests/security/redirect_test.php +++ b/tests/security/redirect_test.php @@ -13,8 +13,13 @@ require_once dirname(__FILE__) . '/../../phpBB/includes/functions.php'; class phpbb_security_redirect_test extends phpbb_security_test_base { + protected $path_helper; + + protected $controller_helper; + public function provider() { + $this->controller_helper = $this->get_controller_helper(); // array(Input -> redirect(), expected triggered error (else false), expected returned result url (else false)) return array( array('data://x', false, false, 'http://localhost/phpBB'), @@ -26,13 +31,15 @@ class phpbb_security_redirect_test extends phpbb_security_test_base 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/phpBB/app.php/foobar'), - array('./../app.php/foobar', true, false, 'http://../../../foobar'), - array('./../app.php/foo/bar', false, false, 'http://localhost/phpBB/app.php/foo/bar'), - array('./../app.php/foo/bar', true, false, 'http://../../../bar'), - array('./../foo/bar', false, false, 'http://localhost/phpBB/foo/bar'), - array('./../foo/bar', true, false, 'http://../../../bar'), + 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($this->controller_helper->url('a'), false, false, 'http://localhost/phpBB/app.php/a'), + array($this->controller_helper->url(''), 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'), @@ -46,6 +53,47 @@ class phpbb_security_redirect_test extends phpbb_security_test_base ); } + protected function get_path_helper() + { + if (!($this->path_helper instanceof \phpbb\path_helper)) + { + $this->path_helper = new \phpbb\path_helper( + new \phpbb\symfony_request( + new phpbb_mock_request() + ), + new \phpbb\filesystem(), + $this->phpbb_root_path, + 'php' + ); + } + return $this->path_helper; + } + + protected function get_controller_helper() + { + if (!($this->controller_helper instanceof \phpbb\controller\helper)) + { + global $phpbb_dispatcher; + + $phpbb_dispatcher = new phpbb_mock_event_dispatcher; + $this->user = $this->getMock('\phpbb\user'); + $phpbb_path_helper = new \phpbb\path_helper( + new \phpbb\symfony_request( + new phpbb_mock_request() + ), + new \phpbb\filesystem(), + $phpbb_root_path, + $phpEx + ); + $this->template = new phpbb\template\twig\twig($phpbb_path_helper, $config, $this->user, new \phpbb\template\context()); + + // We don't use mod_rewrite in these tests + $config = new \phpbb\config\config(array('enable_mod_rewrite' => '0')); + $this->controller_helper = new \phpbb\controller\helper($this->template, $this->user, $config, '', 'php'); + } + return $this->controller_helper; + } + protected function setUp() { parent::setUp(); @@ -54,14 +102,8 @@ class phpbb_security_redirect_test extends phpbb_security_test_base 'force_server_vars' => '0', ); - $this->path_helper = new \phpbb\path_helper( - new \phpbb\symfony_request( - new phpbb_mock_request() - ), - new \phpbb\filesystem(), - $this->phpbb_root_path, - 'php' - ); + $this->path_helper = $this->get_path_helper(); + $this->controller_helper = $this->get_controller_helper(); } /**