diff options
author | Tristan Darricau <github@nicofuma.fr> | 2014-11-12 11:44:56 +0100 |
---|---|---|
committer | Tristan Darricau <github@nicofuma.fr> | 2014-11-16 16:45:19 +0100 |
commit | 0dfe1d0d8b007ec7b7cae0715cfb2e5f4e33bad4 (patch) | |
tree | c8e5bc0ca0d0f646b37d392d9bd67f6215dec7e4 | |
parent | 526a97db7cea84ab54b7d283c888ada58a310bc4 (diff) | |
download | forums-0dfe1d0d8b007ec7b7cae0715cfb2e5f4e33bad4.tar forums-0dfe1d0d8b007ec7b7cae0715cfb2e5f4e33bad4.tar.gz forums-0dfe1d0d8b007ec7b7cae0715cfb2e5f4e33bad4.tar.bz2 forums-0dfe1d0d8b007ec7b7cae0715cfb2e5f4e33bad4.tar.xz forums-0dfe1d0d8b007ec7b7cae0715cfb2e5f4e33bad4.zip |
[ticket/13280] Output escaping for the symfony request object
PHPBB3-13280
-rw-r--r-- | phpBB/config/services.yml | 3 | ||||
-rw-r--r-- | phpBB/phpbb/controller/helper.php | 11 | ||||
-rw-r--r-- | phpBB/phpbb/path_helper.php | 6 | ||||
-rw-r--r-- | phpBB/phpbb/request/request.php | 23 | ||||
-rw-r--r-- | phpBB/phpbb/request/request_interface.php | 10 | ||||
-rw-r--r-- | phpBB/phpbb/session.php | 17 | ||||
-rw-r--r-- | phpBB/phpbb/symfony_request.php | 16 | ||||
-rw-r--r-- | tests/controller/common_helper_route.php | 32 | ||||
-rw-r--r-- | tests/mock/controller_helper.php | 3 | ||||
-rw-r--r-- | tests/mock/request.php | 21 | ||||
-rw-r--r-- | tests/pagination/pagination_test.php | 2 | ||||
-rw-r--r-- | tests/security/extract_current_page_test.php | 28 |
12 files changed, 116 insertions, 56 deletions
diff --git a/phpBB/config/services.yml b/phpBB/config/services.yml index 5003697564..975f2f7580 100644 --- a/phpBB/config/services.yml +++ b/phpBB/config/services.yml @@ -75,6 +75,7 @@ services: - @controller.provider - @ext.manager - @symfony_request + - @request - @filesystem - %core.root_path% - %core.php_ext% @@ -155,6 +156,8 @@ services: - null - %core.disable_super_globals% + # WARNING: The Symfony request does not escape the input and should be used very carefully + # prefer the phpbb request (service @request) as possible symfony_request: class: phpbb\symfony_request arguments: diff --git a/phpBB/phpbb/controller/helper.php b/phpBB/phpbb/controller/helper.php index 187e455d48..52e6947c2c 100644 --- a/phpBB/phpbb/controller/helper.php +++ b/phpBB/phpbb/controller/helper.php @@ -44,6 +44,9 @@ class helper /* @var \phpbb\symfony_request */ protected $symfony_request; + /* @var \phpbb\request\request_interface */ + protected $request; + /** * @var \phpbb\filesystem The filesystem object */ @@ -70,16 +73,18 @@ class helper * @param \phpbb\controller\provider $provider Path provider * @param \phpbb\extension\manager $manager Extension manager object * @param \phpbb\symfony_request $symfony_request Symfony Request object + * @param \phpbb\request\request_interface $request phpBB request object * @param \phpbb\filesystem $filesystem The filesystem object * @param string $phpbb_root_path phpBB root path * @param string $php_ext PHP file extension */ - public function __construct(\phpbb\template\template $template, \phpbb\user $user, \phpbb\config\config $config, \phpbb\controller\provider $provider, \phpbb\extension\manager $manager, \phpbb\symfony_request $symfony_request, \phpbb\filesystem $filesystem, $phpbb_root_path, $php_ext) + public function __construct(\phpbb\template\template $template, \phpbb\user $user, \phpbb\config\config $config, \phpbb\controller\provider $provider, \phpbb\extension\manager $manager, \phpbb\symfony_request $symfony_request, \phpbb\request\request_interface $request, \phpbb\filesystem $filesystem, $phpbb_root_path, $php_ext) { $this->template = $template; $this->user = $user; $this->config = $config; $this->symfony_request = $symfony_request; + $this->request = $request; $this->filesystem = $filesystem; $this->phpbb_root_path = $phpbb_root_path; $this->php_ext = $php_ext; @@ -153,7 +158,7 @@ class helper } } - $base_url = $this->filesystem->clean_path($base_url); + $base_url = $this->request->escape($this->filesystem->clean_path($base_url), true); $context->setBaseUrl($base_url); @@ -197,6 +202,6 @@ class helper */ public function get_current_url() { - return generate_board_url(true) . $this->symfony_request->getRequestUri(); + return generate_board_url(true) . $this->request->escape($this->symfony_request->getRequestUri(), true); } } diff --git a/phpBB/phpbb/path_helper.php b/phpBB/phpbb/path_helper.php index 936564d8b6..4a446a5d9d 100644 --- a/phpBB/phpbb/path_helper.php +++ b/phpBB/phpbb/path_helper.php @@ -154,6 +154,7 @@ class path_helper return $this->web_root_path; } + // We do not need to escape $path_info, $request_uri and $script_name because we can not find their content in the result. // Path info (e.g. /foo/bar) $path_info = $this->filesystem->clean_path($this->symfony_request->getPathInfo()); @@ -203,9 +204,12 @@ class path_helper */ if ($this->request->is_ajax() && $this->symfony_request->get('_referer')) { + // We need to escape $absolute_board_url because it can be partially concatenated to the result. + $absolute_board_url = $this->request->escape($this->symfony_request->getSchemeAndHttpHost() . $this->symfony_request->getBasePath(), true); + $referer_web_root_path = $this->get_web_root_path_from_ajax_referer( $this->symfony_request->get('_referer'), - $this->symfony_request->getSchemeAndHttpHost() . $this->symfony_request->getBasePath() + $absolute_board_url ); return $this->web_root_path = $this->phpbb_root_path . $referer_web_root_path; } diff --git a/phpBB/phpbb/request/request.php b/phpBB/phpbb/request/request.php index ea9854894c..f0f2f7e2a2 100644 --- a/phpBB/phpbb/request/request.php +++ b/phpBB/phpbb/request/request.php @@ -416,4 +416,27 @@ class request implements \phpbb\request\request_interface { return $this->input[$super_global]; } + + /** + * {@inheritdoc} + */ + public function escape($var, $multibyte) + { + if (is_array($var)) + { + $result = array(); + foreach ($var as $key => $value) + { + $this->type_cast_helper->set_var($key, $key, gettype($key), $multibyte); + $result[$key] = $this->escape($value, $multibyte); + } + $var = $result; + } + else + { + $this->type_cast_helper->set_var($var, $var, 'string', $multibyte); + } + + return $var; + } } diff --git a/phpBB/phpbb/request/request_interface.php b/phpBB/phpbb/request/request_interface.php index 3236f73990..47b3b3a4ed 100644 --- a/phpBB/phpbb/request/request_interface.php +++ b/phpBB/phpbb/request/request_interface.php @@ -142,4 +142,14 @@ interface request_interface * @return array The original array of the requested super global. */ public function get_super_global($super_global = \phpbb\request\request_interface::REQUEST); + + /** + * Escape a string variable. + * + * @param mixed $value The contents to fill with + * @param bool $multibyte Indicates whether string values may contain UTF-8 characters. + * Default is false, causing all bytes outside the ASCII range (0-127) to be replaced with question marks. + * @return string|array + */ + public function escape($value, $multibyte); } diff --git a/phpBB/phpbb/session.php b/phpBB/phpbb/session.php index 14b4c63207..dc90d942c3 100644 --- a/phpBB/phpbb/session.php +++ b/phpBB/phpbb/session.php @@ -31,10 +31,11 @@ class session var $update_session_page = true; /** - * Extract current session page - * - * @param string $root_path current root path (phpbb_root_path) - */ + * Extract current session page + * + * @param string $root_path current root path (phpbb_root_path) + * @return array + */ static function extract_current_page($root_path) { global $request, $symfony_request, $phpbb_filesystem; @@ -42,8 +43,8 @@ class session $page_array = array(); // First of all, get the request uri... - $script_name = $symfony_request->getScriptName(); - $args = explode('&', $symfony_request->getQueryString()); + $script_name = $request->escape($symfony_request->getScriptName(), true); + $args = $request->escape(explode('&', $symfony_request->getQueryString()), true); // If we are unable to get the script name we use REQUEST_URI as a failover and note it within the page array for easier support... if (!$script_name) @@ -61,8 +62,8 @@ class session // Since some browser do not encode correctly we need to do this with some "special" characters... // " -> %22, ' => %27, < -> %3C, > -> %3E - $find = array('"', "'", '<', '>'); - $replace = array('%22', '%27', '%3C', '%3E'); + $find = array('"', "'", '<', '>', '"', '<', '>'); + $replace = array('%22', '%27', '%3C', '%3E', '%22', '%3C', '%3E'); foreach ($args as $key => $argument) { diff --git a/phpBB/phpbb/symfony_request.php b/phpBB/phpbb/symfony_request.php index ad949a35f2..2931cae3cc 100644 --- a/phpBB/phpbb/symfony_request.php +++ b/phpBB/phpbb/symfony_request.php @@ -15,6 +15,10 @@ namespace phpbb; use Symfony\Component\HttpFoundation\Request; +/** + * WARNING: The Symfony request does not escape the input and should be used very carefully + * prefer the phpbb request as possible + */ class symfony_request extends Request { /** @@ -24,24 +28,12 @@ class symfony_request extends Request */ public function __construct(\phpbb\request\request_interface $phpbb_request) { - // This function is meant to sanitize the global input arrays - $sanitizer = function(&$value, $key) { - $type_cast_helper = new \phpbb\request\type_cast_helper(); - $type_cast_helper->set_var($value, $value, gettype($value), true); - }; - $get_parameters = $phpbb_request->get_super_global(\phpbb\request\request_interface::GET); $post_parameters = $phpbb_request->get_super_global(\phpbb\request\request_interface::POST); $server_parameters = $phpbb_request->get_super_global(\phpbb\request\request_interface::SERVER); $files_parameters = $phpbb_request->get_super_global(\phpbb\request\request_interface::FILES); $cookie_parameters = $phpbb_request->get_super_global(\phpbb\request\request_interface::COOKIE); - array_walk_recursive($get_parameters, $sanitizer); - array_walk_recursive($post_parameters, $sanitizer); - array_walk_recursive($server_parameters, $sanitizer); - array_walk_recursive($files_parameters, $sanitizer); - array_walk_recursive($cookie_parameters, $sanitizer); - parent::__construct($get_parameters, $post_parameters, array(), $cookie_parameters, $files_parameters, $server_parameters); } } diff --git a/tests/controller/common_helper_route.php b/tests/controller/common_helper_route.php index 859832412d..6723e3bc52 100644 --- a/tests/controller/common_helper_route.php +++ b/tests/controller/common_helper_route.php @@ -63,21 +63,21 @@ abstract class phpbb_controller_common_helper_route extends phpbb_test_case protected function generate_route_objects() { - $request = new phpbb_mock_request(); - $request->overwrite('SCRIPT_NAME', $this->get_uri(), \phpbb\request\request_interface::SERVER); - $request->overwrite('SCRIPT_FILENAME', $this->get_script_name(), \phpbb\request\request_interface::SERVER); - $request->overwrite('REQUEST_URI', $this->get_uri(), \phpbb\request\request_interface::SERVER); - $request->overwrite('SERVER_NAME', 'localhost', \phpbb\request\request_interface::SERVER); - $request->overwrite('SERVER_PORT', '80', \phpbb\request\request_interface::SERVER); + $this->request = new phpbb_mock_request(); + $this->request->overwrite('SCRIPT_NAME', $this->get_uri(), \phpbb\request\request_interface::SERVER); + $this->request->overwrite('SCRIPT_FILENAME', $this->get_script_name(), \phpbb\request\request_interface::SERVER); + $this->request->overwrite('REQUEST_URI', $this->get_uri(), \phpbb\request\request_interface::SERVER); + $this->request->overwrite('SERVER_NAME', 'localhost', \phpbb\request\request_interface::SERVER); + $this->request->overwrite('SERVER_PORT', '80', \phpbb\request\request_interface::SERVER); $this->symfony_request = new \phpbb\symfony_request( - $request + $this->request ); $this->filesystem = new \phpbb\filesystem(); $this->phpbb_path_helper = new \phpbb\path_helper( $this->symfony_request, $this->filesystem, - $this->getMock('\phpbb\request\request'), + $this->request, $phpbb_root_path, $phpEx ); @@ -130,7 +130,7 @@ abstract class phpbb_controller_common_helper_route extends phpbb_test_case */ public function test_helper_url_no_rewrite($route, $params, $is_amp, $session_id, $expected, $description) { - $this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/'); + $this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/'); $this->assertEquals($expected, $this->helper->route($route, $params, $is_amp, $session_id)); } @@ -170,7 +170,7 @@ abstract class phpbb_controller_common_helper_route extends phpbb_test_case public function test_helper_url_with_rewrite($route, $params, $is_amp, $session_id, $expected, $description) { $this->config = new \phpbb\config\config(array('enable_mod_rewrite' => '1')); - $this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/'); + $this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/'); $this->assertEquals($expected, $this->helper->route($route, $params, $is_amp, $session_id)); } @@ -210,7 +210,7 @@ abstract class phpbb_controller_common_helper_route extends phpbb_test_case public function test_helper_url_absolute($route, $params, $is_amp, $session_id, $expected, $description) { $this->config = new \phpbb\config\config(array('enable_mod_rewrite' => '0')); - $this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/'); + $this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/'); $this->assertEquals($expected, $this->helper->route($route, $params, $is_amp, $session_id, UrlGeneratorInterface::ABSOLUTE_URL)); } @@ -250,7 +250,7 @@ abstract class phpbb_controller_common_helper_route extends phpbb_test_case public function test_helper_url_relative_path($route, $params, $is_amp, $session_id, $expected, $description) { $this->config = new \phpbb\config\config(array('enable_mod_rewrite' => '0')); - $this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/'); + $this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/'); $this->assertEquals($expected, $this->helper->route($route, $params, $is_amp, $session_id, UrlGeneratorInterface::RELATIVE_PATH)); } @@ -290,7 +290,7 @@ abstract class phpbb_controller_common_helper_route extends phpbb_test_case public function test_helper_url_network($route, $params, $is_amp, $session_id, $expected, $description) { $this->config = new \phpbb\config\config(array('enable_mod_rewrite' => '0')); - $this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/'); + $this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/'); $this->assertEquals($expected, $this->helper->route($route, $params, $is_amp, $session_id, UrlGeneratorInterface::NETWORK_PATH)); } //TODO @@ -330,7 +330,7 @@ abstract class phpbb_controller_common_helper_route extends phpbb_test_case public function test_helper_url_absolute_with_rewrite($route, $params, $is_amp, $session_id, $expected, $description) { $this->config = new \phpbb\config\config(array('enable_mod_rewrite' => '1')); - $this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/'); + $this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/'); $this->assertEquals($expected, $this->helper->route($route, $params, $is_amp, $session_id, UrlGeneratorInterface::ABSOLUTE_URL)); } @@ -370,7 +370,7 @@ abstract class phpbb_controller_common_helper_route extends phpbb_test_case public function test_helper_url_relative_path_with_rewrite($route, $params, $is_amp, $session_id, $expected, $description) { $this->config = new \phpbb\config\config(array('enable_mod_rewrite' => '1')); - $this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/'); + $this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/'); $this->assertEquals($expected, $this->helper->route($route, $params, $is_amp, $session_id, UrlGeneratorInterface::RELATIVE_PATH)); } @@ -410,7 +410,7 @@ abstract class phpbb_controller_common_helper_route extends phpbb_test_case public function test_helper_url_network_with_rewrite($route, $params, $is_amp, $session_id, $expected, $description) { $this->config = new \phpbb\config\config(array('enable_mod_rewrite' => '1')); - $this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/'); + $this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $this->provider, $this->extension_manager, $this->symfony_request, $this->request, $this->filesystem, $this->root_path, 'php', dirname(__FILE__) . '/'); $this->assertEquals($expected, $this->helper->route($route, $params, $is_amp, $session_id, UrlGeneratorInterface::NETWORK_PATH)); } } diff --git a/tests/mock/controller_helper.php b/tests/mock/controller_helper.php index 9c13c309f2..ae3e7bf432 100644 --- a/tests/mock/controller_helper.php +++ b/tests/mock/controller_helper.php @@ -13,12 +13,13 @@ class phpbb_mock_controller_helper extends \phpbb\controller\helper { - public function __construct(\phpbb\template\template $template, \phpbb\user $user, \phpbb\config\config $config, \phpbb\controller\provider $provider, \phpbb\extension\manager $manager, \phpbb\symfony_request $symfony_request, \phpbb\filesystem $filesystem, $phpbb_root_path, $php_ext, $phpbb_root_path_ext) + public function __construct(\phpbb\template\template $template, \phpbb\user $user, \phpbb\config\config $config, \phpbb\controller\provider $provider, \phpbb\extension\manager $manager, \phpbb\symfony_request $symfony_request, \phpbb\request\request_interface $request, \phpbb\filesystem $filesystem, $phpbb_root_path, $php_ext, $phpbb_root_path_ext) { $this->template = $template; $this->user = $user; $this->config = $config; $this->symfony_request = $symfony_request; + $this->request = $request; $this->filesystem = $filesystem; $this->phpbb_root_path = $phpbb_root_path; $this->php_ext = $php_ext; diff --git a/tests/mock/request.php b/tests/mock/request.php index 304fcf0eaf..e7217a94a9 100644 --- a/tests/mock/request.php +++ b/tests/mock/request.php @@ -114,4 +114,25 @@ class phpbb_mock_request implements \phpbb\request\request_interface { $this->data[$super_global] = array_merge($this->data[$super_global], $values); } + + public function escape($var, $multibyte) + { + $type_cast_helper = new \phpbb\request\type_cast_helper(); + if (is_array($var)) + { + $result = array(); + foreach ($var as $key => $value) + { + $type_cast_helper->set_var($key, $key, gettype($key), $multibyte); + $result[$key] = $this->escape($value, $multibyte); + } + $var = $result; + } + else + { + $type_cast_helper->set_var($var, $var, 'string', $multibyte); + } + + return $var; + } } diff --git a/tests/pagination/pagination_test.php b/tests/pagination/pagination_test.php index d36aa11a8a..494c667198 100644 --- a/tests/pagination/pagination_test.php +++ b/tests/pagination/pagination_test.php @@ -57,7 +57,7 @@ class phpbb_pagination_pagination_test extends phpbb_template_template_test_case $request ); - $this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $provider, $manager, $symfony_request, $filesystem, '', 'php', dirname(__FILE__) . '/'); + $this->helper = new phpbb_mock_controller_helper($this->template, $this->user, $this->config, $provider, $manager, $symfony_request, $request, $filesystem, '', 'php', dirname(__FILE__) . '/'); $this->pagination = new \phpbb\pagination($this->template, $this->user, $this->helper, $phpbb_dispatcher); } diff --git a/tests/security/extract_current_page_test.php b/tests/security/extract_current_page_test.php index 5c0369f270..767b901a43 100644 --- a/tests/security/extract_current_page_test.php +++ b/tests/security/extract_current_page_test.php @@ -1,15 +1,15 @@ <?php /** - * - * This file is part of the phpBB Forum Software package. - * - * @copyright (c) phpBB Limited <https://www.phpbb.com> - * @license GNU General Public License, version 2 (GPL-2.0) - * - * For full copyright and license information, please see - * the docs/CREDITS.txt file. - * - */ +* +* This file is part of the phpBB Forum Software package. +* +* @copyright (c) phpBB Limited <https://www.phpbb.com> +* @license GNU General Public License, version 2 (GPL-2.0) +* +* For full copyright and license information, please see +* the docs/CREDITS.txt file. +* +*/ require_once dirname(__FILE__) . '/base.php'; @@ -27,8 +27,8 @@ class phpbb_security_extract_current_page_test extends phpbb_security_test_base } /** - * @dataProvider security_variables - */ + * @dataProvider security_variables + */ public function test_query_string_php_self($query_string, $expected) { global $symfony_request, $request; @@ -46,8 +46,8 @@ class phpbb_security_extract_current_page_test extends phpbb_security_test_base } /** - * @dataProvider security_variables - */ + * @dataProvider security_variables + */ public function test_query_string_request_uri($query_string, $expected) { global $symfony_request, $request; |