From 6cc7da0c9c0fc8515aad780fba5de5b3860e5d56 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 3 Nov 2014 16:07:32 +0100 Subject: [PATCH 1/6] [ticket/13280] Properly format the current page and add sanitizer to tests PHPBB3-13280 --- phpBB/phpbb/session.php | 6 ++--- tests/security/extract_current_page_test.php | 23 +++++++++++++------- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/phpBB/phpbb/session.php b/phpBB/phpbb/session.php index 14b4c63207..a06ff9c594 100644 --- a/phpBB/phpbb/session.php +++ b/phpBB/phpbb/session.php @@ -43,7 +43,7 @@ class session // First of all, get the request uri... $script_name = $symfony_request->getScriptName(); - $args = explode('&', $symfony_request->getQueryString()); + $args = explode('&', $symfony_request->getQueryString()); // 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 +61,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/tests/security/extract_current_page_test.php b/tests/security/extract_current_page_test.php index 58dea68dc8..8e536ee461 100644 --- a/tests/security/extract_current_page_test.php +++ b/tests/security/extract_current_page_test.php @@ -37,16 +37,16 @@ class phpbb_security_extract_current_page_test extends phpbb_security_test_base )); $symfony_request->expects($this->any()) ->method('getScriptName') - ->will($this->returnValue($url)); + ->will($this->returnValue($this->sanitizer($url))); $symfony_request->expects($this->any()) ->method('getQueryString') - ->will($this->returnValue($query_string)); + ->will($this->returnValue($this->sanitizer($query_string))); $symfony_request->expects($this->any()) ->method('getBasePath') ->will($this->returnValue($server['REQUEST_URI'])); - $symfony_request->expects($this->any()) + $symfony_request->expects($this->sanitizer($this->any())) ->method('getPathInfo') - ->will($this->returnValue('/')); + ->will($this->returnValue($this->sanitizer('/'))); $result = \phpbb\session::extract_current_page('./'); $label = 'Running extract_current_page on ' . $query_string . ' with PHP_SELF filled.'; @@ -65,20 +65,27 @@ class phpbb_security_extract_current_page_test extends phpbb_security_test_base )); $symfony_request->expects($this->any()) ->method('getScriptName') - ->will($this->returnValue($url)); + ->will($this->returnValue($this->sanitizer($url))); $symfony_request->expects($this->any()) ->method('getQueryString') - ->will($this->returnValue($query_string)); + ->will($this->returnValue($this->sanitizer($query_string))); $symfony_request->expects($this->any()) ->method('getBasePath') - ->will($this->returnValue($server['REQUEST_URI'])); + ->will($this->returnValue($this->sanitizer($server['REQUEST_URI']))); $symfony_request->expects($this->any()) ->method('getPathInfo') - ->will($this->returnValue('/')); + ->will($this->returnValue($this->sanitizer('/'))); $result = \phpbb\session::extract_current_page('./'); $label = 'Running extract_current_page on ' . $query_string . ' with REQUEST_URI filled.'; $this->assertEquals($expected, $result['query_string'], $label); } + + protected function sanitizer($value) + { + $type_cast_helper = new \phpbb\request\type_cast_helper(); + $type_cast_helper->set_var($value, $value, gettype($value), true); + return $value; + } } From 6fd092b5df6f96530fec8ba1aec426b6d59b822b Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 3 Nov 2014 16:24:09 +0100 Subject: [PATCH 2/6] [ticket/13280] Correctly format user page for build_url() PHPBB3-13280 --- phpBB/includes/functions.php | 3 ++- tests/functions/build_url_test.php | 9 ++++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/phpBB/includes/functions.php b/phpBB/includes/functions.php index 1a3560dbb1..56363ed5c0 100644 --- a/phpBB/includes/functions.php +++ b/phpBB/includes/functions.php @@ -2397,7 +2397,8 @@ function build_url($strip_vars = false) global $config, $user, $phpbb_path_helper; $php_ext = $phpbb_path_helper->get_php_ext(); - $page = $user->page['page']; + // Change page back to expected format + $page = str_replace('&', '&', urldecode($user->page['page'])); // We need to be cautious here. // On some situations, the redirect path is an absolute URL, sometimes a relative path diff --git a/tests/functions/build_url_test.php b/tests/functions/build_url_test.php index 06415a424e..df178f277e 100644 --- a/tests/functions/build_url_test.php +++ b/tests/functions/build_url_test.php @@ -79,9 +79,16 @@ class phpbb_build_url_test extends phpbb_test_case { global $user, $phpbb_root_path; - $user->page['page'] = $page; + $user->page['page'] = str_replace('%2F', '/', urlencode($this->sanitizer($page))); $output = build_url($strip_vars); $this->assertEquals($expected, $output); } + + protected function sanitizer($value) + { + $type_cast_helper = new \phpbb\request\type_cast_helper(); + $type_cast_helper->set_var($value, $value, gettype($value), true); + return $value; + } } From 13b59af1ffd0af652ba0ce3bc3f2594fc448fdb5 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 3 Nov 2014 17:14:18 +0100 Subject: [PATCH 3/6] [ticket/13280] Add additional sanitizer for ampersands in server superglobal PHPBB3-13280 --- phpBB/includes/functions.php | 3 +-- phpBB/phpbb/symfony_request.php | 6 ++++++ tests/functions/build_url_test.php | 15 +++++++-------- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/phpBB/includes/functions.php b/phpBB/includes/functions.php index 56363ed5c0..dc195cebb6 100644 --- a/phpBB/includes/functions.php +++ b/phpBB/includes/functions.php @@ -2397,8 +2397,7 @@ function build_url($strip_vars = false) global $config, $user, $phpbb_path_helper; $php_ext = $phpbb_path_helper->get_php_ext(); - // Change page back to expected format - $page = str_replace('&', '&', urldecode($user->page['page'])); + $page = str_replace('&', '&', $user->page['page']); // We need to be cautious here. // On some situations, the redirect path is an absolute URL, sometimes a relative path diff --git a/phpBB/phpbb/symfony_request.php b/phpBB/phpbb/symfony_request.php index ad949a35f2..4d357a5c56 100644 --- a/phpBB/phpbb/symfony_request.php +++ b/phpBB/phpbb/symfony_request.php @@ -30,6 +30,11 @@ class symfony_request extends Request $type_cast_helper->set_var($value, $value, gettype($value), true); }; + // This function is meant for additional handling of server variables + $server_sanitizer = function(&$value, $key) { + $value = str_replace('&', '&', $value); + }; + $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); @@ -41,6 +46,7 @@ class symfony_request extends Request array_walk_recursive($server_parameters, $sanitizer); array_walk_recursive($files_parameters, $sanitizer); array_walk_recursive($cookie_parameters, $sanitizer); + array_walk_recursive($server_parameters, $server_sanitizer); parent::__construct($get_parameters, $post_parameters, array(), $cookie_parameters, $files_parameters, $server_parameters); } diff --git a/tests/functions/build_url_test.php b/tests/functions/build_url_test.php index df178f277e..5cfd1300de 100644 --- a/tests/functions/build_url_test.php +++ b/tests/functions/build_url_test.php @@ -69,6 +69,11 @@ class phpbb_build_url_test extends phpbb_test_case array('f', 'style', 't'), 'http://test.phpbb.com/viewtopic.php?', ), + array( + 'posting.php?f=2&mode=delete&p=20%22%3Cscript%3Ealert%281%29%3B%3C%2Fscript%3E', + false, + 'phpBB/posting.php?f=2&mode=delete&p=20%22%3Cscript%3Ealert%281%29%3B%3C%2Fscript%3E', + ) ); } @@ -79,16 +84,10 @@ class phpbb_build_url_test extends phpbb_test_case { global $user, $phpbb_root_path; - $user->page['page'] = str_replace('%2F', '/', urlencode($this->sanitizer($page))); + $user->page['page'] = $page; + $output = build_url($strip_vars); $this->assertEquals($expected, $output); } - - protected function sanitizer($value) - { - $type_cast_helper = new \phpbb\request\type_cast_helper(); - $type_cast_helper->set_var($value, $value, gettype($value), true); - return $value; - } } From 3986470b3c77240310f51bb101cccf180b9e4c1e Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Tue, 4 Nov 2014 16:14:11 +0100 Subject: [PATCH 4/6] [ticket/13280] Seperate server sanitizer call and add comment PHPBB3-13280 --- phpBB/phpbb/symfony_request.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/phpBB/phpbb/symfony_request.php b/phpBB/phpbb/symfony_request.php index 4d357a5c56..23e5e6e29f 100644 --- a/phpBB/phpbb/symfony_request.php +++ b/phpBB/phpbb/symfony_request.php @@ -46,6 +46,8 @@ class symfony_request extends Request array_walk_recursive($server_parameters, $sanitizer); array_walk_recursive($files_parameters, $sanitizer); array_walk_recursive($cookie_parameters, $sanitizer); + + // Run additional sanitizer for server superglobal array_walk_recursive($server_parameters, $server_sanitizer); parent::__construct($get_parameters, $post_parameters, array(), $cookie_parameters, $files_parameters, $server_parameters); From 32881dbe31a945b6d2449a3f7e1bf7c5e73cd0a6 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Tue, 4 Nov 2014 16:54:45 +0100 Subject: [PATCH 5/6] [ticket/13280] Only run sanitizer for server superglobal and modify tests PHPBB3-13280 --- phpBB/phpbb/symfony_request.php | 6 +++--- tests/security/extract_current_page_test.php | 7 ++++++- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/phpBB/phpbb/symfony_request.php b/phpBB/phpbb/symfony_request.php index 23e5e6e29f..02d22c480f 100644 --- a/phpBB/phpbb/symfony_request.php +++ b/phpBB/phpbb/symfony_request.php @@ -31,7 +31,8 @@ class symfony_request extends Request }; // This function is meant for additional handling of server variables - $server_sanitizer = function(&$value, $key) { + $server_sanitizer = function(&$value, $key) use ($sanitizer) { + $sanitizer($value, $key); $value = str_replace('&', '&', $value); }; @@ -43,11 +44,10 @@ class symfony_request extends Request 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); - // Run additional sanitizer for server superglobal + // Run special sanitizer for server superglobal array_walk_recursive($server_parameters, $server_sanitizer); parent::__construct($get_parameters, $post_parameters, array(), $cookie_parameters, $files_parameters, $server_parameters); diff --git a/tests/security/extract_current_page_test.php b/tests/security/extract_current_page_test.php index 8e536ee461..c127b69b2b 100644 --- a/tests/security/extract_current_page_test.php +++ b/tests/security/extract_current_page_test.php @@ -84,8 +84,13 @@ class phpbb_security_extract_current_page_test extends phpbb_security_test_base protected function sanitizer($value) { + // Fix for objects passed in phpunit + if (is_object($value)) + { + return $value; + } $type_cast_helper = new \phpbb\request\type_cast_helper(); $type_cast_helper->set_var($value, $value, gettype($value), true); - return $value; + return str_replace('&', '&', $value); } } From f432193e0789f0182741d1b4a38a8c4cd86dfba4 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Thu, 6 Nov 2014 13:45:09 +0100 Subject: [PATCH 6/6] [ticket/13280] Remove unneeded str_replace in build_url() PHPBB3-13280 --- phpBB/includes/functions.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phpBB/includes/functions.php b/phpBB/includes/functions.php index dc195cebb6..1a3560dbb1 100644 --- a/phpBB/includes/functions.php +++ b/phpBB/includes/functions.php @@ -2397,7 +2397,7 @@ function build_url($strip_vars = false) global $config, $user, $phpbb_path_helper; $php_ext = $phpbb_path_helper->get_php_ext(); - $page = str_replace('&', '&', $user->page['page']); + $page = $user->page['page']; // We need to be cautious here. // On some situations, the redirect path is an absolute URL, sometimes a relative path