From 56060caa4c44620929b6e17fe4622343750ad302 Mon Sep 17 00:00:00 2001 From: Derky Date: Thu, 14 Mar 2019 21:46:02 +0100 Subject: [PATCH 01/17] [ticket/security/235] Apply wildcard char count patch SECURITY-235 --- phpBB/phpbb/search/fulltext_native.php | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/phpBB/phpbb/search/fulltext_native.php b/phpBB/phpbb/search/fulltext_native.php index 4172e2cc4f..9a6d62f9d8 100644 --- a/phpBB/phpbb/search/fulltext_native.php +++ b/phpBB/phpbb/search/fulltext_native.php @@ -190,7 +190,7 @@ class fulltext_native extends \phpbb\search\base */ public function split_keywords($keywords, $terms) { - $tokens = '+-|()*'; + $tokens = '+-|()* '; $keywords = trim($this->cleanup($keywords, $tokens)); @@ -224,12 +224,10 @@ class fulltext_native extends \phpbb\search\base $keywords[$i] = '|'; break; case '*': - if ($i === 0 || ($keywords[$i - 1] !== '*' && strcspn($keywords[$i - 1], $tokens) === 0)) + // $i can never be 0 here since $open_bracket is initialised to false + if (strpos($tokens, $keywords[$i - 1]) !== false && ($i + 1 === $n || strpos($tokens, $keywords[$i + 1]) !== false)) { - if ($i === $n - 1 || ($keywords[$i + 1] !== '*' && strcspn($keywords[$i + 1], $tokens) === 0)) - { - $keywords = substr($keywords, 0, $i) . substr($keywords, $i + 1); - } + $keywords[$i] = '|'; } break; } @@ -264,7 +262,7 @@ class fulltext_native extends \phpbb\search\base } } - if ($open_bracket) + if ($open_bracket !== false) { $keywords .= ')'; } @@ -409,8 +407,16 @@ class fulltext_native extends \phpbb\search\base { if (strpos($word_part, '*') !== false) { - $id_words[] = '\'' . $this->db->sql_escape(str_replace('*', '%', $word_part)) . '\''; - $non_common_words[] = $word_part; + $len = utf8_strlen(str_replace('*', '', $word_part)); + if ($len >= $this->word_length['min'] && $len <= $this->word_length['max']) + { + $id_words[] = '\'' . $this->db->sql_escape(str_replace('*', '%', $word_part)) . '\''; + $non_common_words[] = $word_part; + } + else + { + $this->common_words[] = $word_part; + } } else if (isset($words[$word_part])) { From 84ea5d71481c450dfe1f4a70a10877d4469c1329 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sun, 14 Apr 2019 14:07:22 +0200 Subject: [PATCH 02/17] [ticket/security/234] Add URL validation for input fields SECURITY-234 --- phpBB/includes/acp/acp_board.php | 7 +++++-- phpBB/includes/functions_acp.php | 12 +++++++++++- phpBB/language/en/acp/common.php | 1 + 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/phpBB/includes/acp/acp_board.php b/phpBB/includes/acp/acp_board.php index f89f5535eb..5b37bb5c57 100644 --- a/phpBB/includes/acp/acp_board.php +++ b/phpBB/includes/acp/acp_board.php @@ -30,10 +30,13 @@ class acp_board function main($id, $mode) { - global $user, $template, $request; + global $user, $template, $request, $language; global $config, $phpbb_root_path, $phpEx; global $cache, $phpbb_container, $phpbb_dispatcher, $phpbb_log; + /** @var \phpbb\language\language $language Language object */ + $language = $phpbb_container->get('language'); + $user->add_lang('acp/board'); $submit = (isset($_POST['submit']) || isset($_POST['allow_quick_reply_enable'])) ? true : false; @@ -56,7 +59,7 @@ class acp_board 'legend1' => 'ACP_BOARD_SETTINGS', 'sitename' => array('lang' => 'SITE_NAME', 'validate' => 'string', 'type' => 'text:40:255', 'explain' => false), 'site_desc' => array('lang' => 'SITE_DESC', 'validate' => 'string', 'type' => 'text:40:255', 'explain' => false), - 'site_home_url' => array('lang' => 'SITE_HOME_URL', 'validate' => 'string', 'type' => 'url:40:255', 'explain' => true), + 'site_home_url' => array('lang' => 'SITE_HOME_URL', 'validate' => 'url', 'type' => 'url:40:255', 'explain' => true), 'site_home_text' => array('lang' => 'SITE_HOME_TEXT', 'validate' => 'string', 'type' => 'text:40:255', 'explain' => true), 'board_index_text' => array('lang' => 'BOARD_INDEX_TEXT', 'validate' => 'string', 'type' => 'text:40:255', 'explain' => true), 'board_disable' => array('lang' => 'DISABLE_BOARD', 'validate' => 'bool', 'type' => 'custom', 'method' => 'board_disable', 'explain' => true), diff --git a/phpBB/includes/functions_acp.php b/phpBB/includes/functions_acp.php index 9b7491305c..dd326c3db6 100644 --- a/phpBB/includes/functions_acp.php +++ b/phpBB/includes/functions_acp.php @@ -419,7 +419,7 @@ function build_cfg_template($tpl_type, $key, &$new_ary, $config_key, $vars) */ function validate_config_vars($config_vars, &$cfg_array, &$error) { - global $phpbb_root_path, $user, $phpbb_dispatcher, $phpbb_filesystem; + global $phpbb_root_path, $user, $phpbb_dispatcher, $phpbb_filesystem, $language; $type = 0; $min = 1; @@ -442,6 +442,16 @@ function validate_config_vars($config_vars, &$cfg_array, &$error) // Validate a bit. ;) (0 = type, 1 = min, 2= max) switch ($validator[$type]) { + case 'url': + $cfg_array[$config_name] = trim($cfg_array[$config_name]); + + if (!empty($cfg_array[$config_name]) && !preg_match('#^' . get_preg_expression('url') . '$#iu', $cfg_array[$config_name])) + { + $error[] = $language->lang('URL_INVALID', $language->lang($config_definition['lang'])); + } + + // no break here + case 'string': $length = utf8_strlen($cfg_array[$config_name]); diff --git a/phpBB/language/en/acp/common.php b/phpBB/language/en/acp/common.php index 0d5f6fee25..1c2253542c 100644 --- a/phpBB/language/en/acp/common.php +++ b/phpBB/language/en/acp/common.php @@ -325,6 +325,7 @@ $lang = array_merge($lang, array( 'TOTAL_SIZE' => 'Total size', 'UCP' => 'User Control Panel', + 'URL_INVALID' => 'The provided URL for the setting “%1$s” is invalid.', 'USERNAMES_EXPLAIN' => 'Place each username on a separate line.', 'USER_CONTROL_PANEL' => 'User Control Panel', From f1c2e26f0af688240f915e3d8d2aab428f0ff76f Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sun, 14 Apr 2019 14:11:36 +0200 Subject: [PATCH 03/17] [ticket/security/234] Add test for URL validation SECURITY-234 --- tests/functions_acp/validate_config_vars_test.php | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/functions_acp/validate_config_vars_test.php b/tests/functions_acp/validate_config_vars_test.php index 1182d659f0..3bd2204de9 100644 --- a/tests/functions_acp/validate_config_vars_test.php +++ b/tests/functions_acp/validate_config_vars_test.php @@ -19,10 +19,11 @@ class phpbb_functions_acp_validate_config_vars_test extends phpbb_test_case { parent::setUp(); - global $user; + global $language, $user; $user = new phpbb_mock_user(); $user->lang = new phpbb_mock_lang(); + $language = $user->lang; } /** @@ -44,6 +45,7 @@ class phpbb_functions_acp_validate_config_vars_test extends phpbb_test_case 'test_int_32' => array('lang' => 'TEST_INT', 'validate' => 'int:32'), 'test_int_32_64' => array('lang' => 'TEST_INT', 'validate' => 'int:32:64'), 'test_lang' => array('lang' => 'TEST_LANG', 'validate' => 'lang'), + 'test_url' => array('lang' => 'TEST_URL', 'validate' => 'url'), /* 'test_sp' => array('lang' => 'TEST_SP', 'validate' => 'script_path'), 'test_rpath' => array('lang' => 'TEST_RPATH', 'validate' => 'rpath'), @@ -64,6 +66,7 @@ class phpbb_functions_acp_validate_config_vars_test extends phpbb_test_case 'test_int_32' => 32, 'test_int_32_64' => 48, 'test_lang' => 'en', + 'test_url' => 'http://foobar.com', ), ), ); @@ -148,6 +151,11 @@ class phpbb_functions_acp_validate_config_vars_test extends phpbb_test_case array('test_lang' => 'this_is_no_language'), array('WRONG_DATA_LANG'), ), + array( + array('test_url' => array('lang' => 'TEST_URL', 'validate' => 'url')), + array('test_url' => 'javascript://foobar.com'), + array('URL_INVALID TEST_URL'), + ), ); } From dc5a167c429a3813d66b0ae3d14242650466cac6 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Wed, 17 Apr 2019 08:54:51 +0200 Subject: [PATCH 04/17] [ticket/security/231] Disable remote avatar functionality & add warning SECURITY-231 --- phpBB/includes/acp/acp_board.php | 3 +- phpBB/language/en/acp/board.php | 4 +-- phpBB/phpbb/avatar/manager.php | 2 +- .../data/v32x/disable_remote_avatar.php | 34 +++++++++++++++++++ 4 files changed, 39 insertions(+), 4 deletions(-) create mode 100644 phpBB/phpbb/db/migration/data/v32x/disable_remote_avatar.php diff --git a/phpBB/includes/acp/acp_board.php b/phpBB/includes/acp/acp_board.php index f89f5535eb..5d3df2629a 100644 --- a/phpBB/includes/acp/acp_board.php +++ b/phpBB/includes/acp/acp_board.php @@ -122,6 +122,7 @@ class acp_board $avatar_vars = array(); foreach ($avatar_drivers as $current_driver) { + /** @var \phpbb\avatar\driver\driver_interface $driver */ $driver = $phpbb_avatar_manager->get_driver($current_driver, false); /* @@ -730,7 +731,7 @@ class acp_board $template->assign_block_vars('options', array( 'KEY' => $config_key, 'TITLE' => (isset($user->lang[$vars['lang']])) ? $user->lang[$vars['lang']] : $vars['lang'], - 'S_EXPLAIN' => $vars['explain'], + 'S_EXPLAIN' => $vars['explain'] && !empty($l_explain), 'TITLE_EXPLAIN' => $l_explain, 'CONTENT' => $content, ) diff --git a/phpBB/language/en/acp/board.php b/phpBB/language/en/acp/board.php index eb53ac0370..9b45ffa45b 100644 --- a/phpBB/language/en/acp/board.php +++ b/phpBB/language/en/acp/board.php @@ -111,9 +111,9 @@ $lang = array_merge($lang, array( 'ALLOW_GRAVATAR' => 'Enable gravatar avatars', 'ALLOW_LOCAL' => 'Enable gallery avatars', 'ALLOW_REMOTE' => 'Enable remote avatars', - 'ALLOW_REMOTE_EXPLAIN' => 'Avatars linked to from another website.', + 'ALLOW_REMOTE_EXPLAIN' => 'Avatars linked to from another website.
Warning: Enabling this feature might allow users to check for the existence of files and services that are only accessible on the local network.', 'ALLOW_REMOTE_UPLOAD' => 'Enable remote avatar uploading', - 'ALLOW_REMOTE_UPLOAD_EXPLAIN' => 'Allow uploading of avatars from another website.', + 'ALLOW_REMOTE_UPLOAD_EXPLAIN' => 'Allow uploading of avatars from another website.
Warning: Enabling this feature might allow users to check for the existence of files and services that are only accessible on the local network.', 'ALLOW_UPLOAD' => 'Enable avatar uploading', 'AVATAR_GALLERY_PATH' => 'Avatar gallery path', 'AVATAR_GALLERY_PATH_EXPLAIN' => 'Path under your phpBB root directory for pre-loaded images, e.g. images/avatars/gallery.
Double dots like ../ will be stripped from the path for security reasons.', diff --git a/phpBB/phpbb/avatar/manager.php b/phpBB/phpbb/avatar/manager.php index 6d9604db04..a909a91042 100644 --- a/phpBB/phpbb/avatar/manager.php +++ b/phpBB/phpbb/avatar/manager.php @@ -271,7 +271,7 @@ class manager $config_name = $driver->get_config_name(); return array( - 'allow_avatar_' . $config_name => array('lang' => 'ALLOW_' . strtoupper(str_replace('\\', '_', $config_name)), 'validate' => 'bool', 'type' => 'radio:yes_no', 'explain' => false), + 'allow_avatar_' . $config_name => array('lang' => 'ALLOW_' . strtoupper(str_replace('\\', '_', $config_name)), 'validate' => 'bool', 'type' => 'radio:yes_no', 'explain' => true), ); } diff --git a/phpBB/phpbb/db/migration/data/v32x/disable_remote_avatar.php b/phpBB/phpbb/db/migration/data/v32x/disable_remote_avatar.php new file mode 100644 index 0000000000..b08833fad4 --- /dev/null +++ b/phpBB/phpbb/db/migration/data/v32x/disable_remote_avatar.php @@ -0,0 +1,34 @@ + + * @license GNU General Public License, version 2 (GPL-2.0) + * + * For full copyright and license information, please see + * the docs/CREDITS.txt file. + * + */ + +namespace phpbb\db\migration\data\v32x; + +use phpbb\db\migration\migration; + +class disable_remote_avatar extends migration +{ + static public function depends_on() + { + return array( + '\phpbb\db\migration\data\v32x\v325', + ); + } + + public function update_data() + { + return array( + array('config.update', array('allow_avatar_remote', '0')), + array('config.update', array('allow_avatar_remote_upload', '0')), + ); + } +} From 8e5a0c81ef118c7d88e10fb7d793a36e9204aea8 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sun, 21 Apr 2019 22:22:35 +0200 Subject: [PATCH 05/17] [ticket/security/233] Make smtp_password and smtp_username dynamic SECURITY-233 --- phpBB/install/schemas/schema_data.sql | 4 +- .../migration/data/v32x/smtp_dynamic_data.php | 42 +++++++++++++++++++ phpBB/phpbb/install/helper/config.php | 2 + 3 files changed, 46 insertions(+), 2 deletions(-) create mode 100644 phpBB/phpbb/db/migration/data/v32x/smtp_dynamic_data.php diff --git a/phpBB/install/schemas/schema_data.sql b/phpBB/install/schemas/schema_data.sql index 55dd72db0d..e897a7b443 100644 --- a/phpBB/install/schemas/schema_data.sql +++ b/phpBB/install/schemas/schema_data.sql @@ -269,9 +269,9 @@ INSERT INTO phpbb_config (config_name, config_value) VALUES ('smilies_per_page', INSERT INTO phpbb_config (config_name, config_value) VALUES ('smtp_auth_method', 'PLAIN'); INSERT INTO phpbb_config (config_name, config_value) VALUES ('smtp_delivery', '0'); INSERT INTO phpbb_config (config_name, config_value) VALUES ('smtp_host', ''); -INSERT INTO phpbb_config (config_name, config_value) VALUES ('smtp_password', ''); +INSERT INTO phpbb_config (config_name, config_value) VALUES ('smtp_password', '', 1); INSERT INTO phpbb_config (config_name, config_value) VALUES ('smtp_port', '25'); -INSERT INTO phpbb_config (config_name, config_value) VALUES ('smtp_username', ''); +INSERT INTO phpbb_config (config_name, config_value) VALUES ('smtp_username', '', 1); INSERT INTO phpbb_config (config_name, config_value) VALUES ('teampage_memberships', '1'); INSERT INTO phpbb_config (config_name, config_value) VALUES ('teampage_forums', '1'); INSERT INTO phpbb_config (config_name, config_value) VALUES ('topics_per_page', '25'); diff --git a/phpBB/phpbb/db/migration/data/v32x/smtp_dynamic_data.php b/phpBB/phpbb/db/migration/data/v32x/smtp_dynamic_data.php new file mode 100644 index 0000000000..aeaa3e8979 --- /dev/null +++ b/phpBB/phpbb/db/migration/data/v32x/smtp_dynamic_data.php @@ -0,0 +1,42 @@ + + * @license GNU General Public License, version 2 (GPL-2.0) + * + * For full copyright and license information, please see + * the docs/CREDITS.txt file. + * + */ + +namespace phpbb\db\migration\data\v32x; + +class smtp_dynamic_data extends \phpbb\db\migration\migration +{ + static public function depends_on() + { + return array( + '\phpbb\db\migration\data\v32x\v326rc1', + ); + } + + public function update_data() + { + return array( + array('custom', array(array($this, 'set_smtp_dynamic'))), + ); + } + + public function set_smtp_dynamic() + { + $smtp_auth_entries = [ + 'smtp_password', + 'smtp_username', + ]; + $this->sql_query('UPDATE ' . CONFIG_TABLE . ' + SET is_dynamic = 1 + WHERE ' . $this->db->sql_in_set('config_name', $smtp_auth_entries)); + } +} diff --git a/phpBB/phpbb/install/helper/config.php b/phpBB/phpbb/install/helper/config.php index fad6749019..7eb0ae3b05 100644 --- a/phpBB/phpbb/install/helper/config.php +++ b/phpBB/phpbb/install/helper/config.php @@ -330,6 +330,8 @@ class config fwrite($fp, $file_content); fclose($fp); + // Enforce 0600 permission for install config + $this->filesystem->chmod([$this->install_config_file], 0600); } /** From 71d66832c019346718e70b18923fd717775c2ca3 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sun, 21 Apr 2019 23:17:39 +0200 Subject: [PATCH 06/17] [ticket/security/233] Fix invalid INSERT INTO SECURITY-233 --- phpBB/install/schemas/schema_data.sql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/phpBB/install/schemas/schema_data.sql b/phpBB/install/schemas/schema_data.sql index e897a7b443..fada5b6cbd 100644 --- a/phpBB/install/schemas/schema_data.sql +++ b/phpBB/install/schemas/schema_data.sql @@ -269,9 +269,9 @@ INSERT INTO phpbb_config (config_name, config_value) VALUES ('smilies_per_page', INSERT INTO phpbb_config (config_name, config_value) VALUES ('smtp_auth_method', 'PLAIN'); INSERT INTO phpbb_config (config_name, config_value) VALUES ('smtp_delivery', '0'); INSERT INTO phpbb_config (config_name, config_value) VALUES ('smtp_host', ''); -INSERT INTO phpbb_config (config_name, config_value) VALUES ('smtp_password', '', 1); +INSERT INTO phpbb_config (config_name, config_value, is_dynamic) VALUES ('smtp_password', '', 1); INSERT INTO phpbb_config (config_name, config_value) VALUES ('smtp_port', '25'); -INSERT INTO phpbb_config (config_name, config_value) VALUES ('smtp_username', '', 1); +INSERT INTO phpbb_config (config_name, config_value, is_dynamic) VALUES ('smtp_username', '', 1); INSERT INTO phpbb_config (config_name, config_value) VALUES ('teampage_memberships', '1'); INSERT INTO phpbb_config (config_name, config_value) VALUES ('teampage_forums', '1'); INSERT INTO phpbb_config (config_name, config_value) VALUES ('topics_per_page', '25'); From 58f33921b56c3524e5c2b2c15bbf0dddea1ff461 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Tue, 23 Apr 2019 20:41:37 +0200 Subject: [PATCH 07/17] [ticket/16027] Force clearing of cache folder on install for functional PHPBB3-16027 --- tests/test_framework/phpbb_functional_test_case.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/test_framework/phpbb_functional_test_case.php b/tests/test_framework/phpbb_functional_test_case.php index d4856f954a..4d294fd523 100644 --- a/tests/test_framework/phpbb_functional_test_case.php +++ b/tests/test_framework/phpbb_functional_test_case.php @@ -397,6 +397,14 @@ class phpbb_functional_test_case extends phpbb_test_case global $phpbb_container; $phpbb_container->reset(); + // Purge cache to remove cached files + $phpbb_container = new phpbb_mock_container_builder(); + $phpbb_container->setParameter('core.environment', PHPBB_ENVIRONMENT); + $phpbb_container->setParameter('core.cache_dir', $phpbb_root_path . 'cache/' . PHPBB_ENVIRONMENT . '/'); + + $cache = new \phpbb\cache\driver\file; + $cache->purge(); + $blacklist = ['phpbb_class_loader_mock', 'phpbb_class_loader_ext', 'phpbb_class_loader']; foreach (array_keys($GLOBALS) as $key) From fd195fba210c8625e968ef5553e61864747c8d44 Mon Sep 17 00:00:00 2001 From: Derky Date: Thu, 25 Apr 2019 21:51:04 +0200 Subject: [PATCH 08/17] [ticket/security/235] Remove non trailing wildcards from search keywords Database indexes are only used if wildcards are used at the end. SECURITY-235 --- phpBB/phpbb/search/fulltext_native.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/phpBB/phpbb/search/fulltext_native.php b/phpBB/phpbb/search/fulltext_native.php index 9a6d62f9d8..478fe5616d 100644 --- a/phpBB/phpbb/search/fulltext_native.php +++ b/phpBB/phpbb/search/fulltext_native.php @@ -305,6 +305,11 @@ class fulltext_native extends \phpbb\search\base } } + // Remove non trailing wildcards from each word to prevent a full table scan (it's now using the database index) + $match = '#\*(?!$)\b#'; + $replace = '$1'; + $keywords = preg_replace($match, $replace, $keywords); + // set the search_query which is shown to the user $this->search_query = $keywords; From 8a73eb5f0ff912454e6479539f972081e54baa1c Mon Sep 17 00:00:00 2001 From: Derky Date: Fri, 26 Apr 2019 00:52:43 +0200 Subject: [PATCH 09/17] [ticket/security/235] Use whitespace instead of word boundary regex to remove wildcards This fixes removing the wildcard in the following search query: *.test SECURITY-235 --- phpBB/phpbb/search/fulltext_native.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phpBB/phpbb/search/fulltext_native.php b/phpBB/phpbb/search/fulltext_native.php index 478fe5616d..1925623b80 100644 --- a/phpBB/phpbb/search/fulltext_native.php +++ b/phpBB/phpbb/search/fulltext_native.php @@ -306,7 +306,7 @@ class fulltext_native extends \phpbb\search\base } // Remove non trailing wildcards from each word to prevent a full table scan (it's now using the database index) - $match = '#\*(?!$)\b#'; + $match = '#\*(?!$|\s)#'; $replace = '$1'; $keywords = preg_replace($match, $replace, $keywords); From da9910850a168f73c6b8dd8407a01f47d27ca1d8 Mon Sep 17 00:00:00 2001 From: Derky Date: Fri, 26 Apr 2019 00:56:48 +0200 Subject: [PATCH 10/17] [ticket/security/235] Only allow one wildcard in the search query to limit the database load SECURITY-235 --- phpBB/phpbb/search/fulltext_native.php | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/phpBB/phpbb/search/fulltext_native.php b/phpBB/phpbb/search/fulltext_native.php index 1925623b80..c83de75eed 100644 --- a/phpBB/phpbb/search/fulltext_native.php +++ b/phpBB/phpbb/search/fulltext_native.php @@ -310,6 +310,15 @@ class fulltext_native extends \phpbb\search\base $replace = '$1'; $keywords = preg_replace($match, $replace, $keywords); + // Only allow one wildcard in the search query to limit the database load + $match = '#\*#'; + $replace = '$1'; + $count_wildcards = substr_count($keywords, '*'); + + // Reverse the string to remove all wildcards except the first one + $keywords = strrev(preg_replace($match, $replace, strrev($keywords), $count_wildcards - 1)); + unset($count_wildcards); + // set the search_query which is shown to the user $this->search_query = $keywords; From b8368980162392bf9f97496ecec18abe2bd34fad Mon Sep 17 00:00:00 2001 From: Derky Date: Fri, 26 Apr 2019 12:08:37 +0200 Subject: [PATCH 11/17] [ticket/security/228] Add form token to login box SECURITY-228 --- phpBB/includes/functions.php | 19 +++++++++++++++++-- phpBB/index.php | 3 +++ .../styles/prosilver/template/index_body.html | 1 + .../styles/prosilver/template/login_body.html | 1 + 4 files changed, 22 insertions(+), 2 deletions(-) diff --git a/phpBB/includes/functions.php b/phpBB/includes/functions.php index e2ea7ad232..6df2ebaba7 100644 --- a/phpBB/includes/functions.php +++ b/phpBB/includes/functions.php @@ -2268,6 +2268,7 @@ function login_box($redirect = '', $l_explain = '', $l_success = '', $admin = fa global $request, $phpbb_container, $phpbb_dispatcher, $phpbb_log; $err = ''; + $form_name = 'login'; // Make sure user->setup() has been called if (!$user->is_setup()) @@ -2343,8 +2344,19 @@ function login_box($redirect = '', $l_explain = '', $l_success = '', $admin = fa trigger_error('NO_AUTH_ADMIN_USER_DIFFER'); } - // If authentication is successful we redirect user to previous page - $result = $auth->login($username, $password, $autologin, $viewonline, $admin); + // Check form key + if ($password && !check_form_key($form_name)) + { + $result = array( + 'status' => false, + 'error_msg' => 'FORM_INVALID', + ); + } + else + { + // If authentication is successful we redirect user to previous page + $result = $auth->login($username, $password, $autologin, $viewonline, $admin); + } // If admin authentication and login, we will log if it was a success or not... // We also break the operation on the first non-success login - it could be argued that the user already knows @@ -2495,6 +2507,9 @@ function login_box($redirect = '', $l_explain = '', $l_success = '', $admin = fa )); } + // Add form token for login box + add_form_key($form_name, '_LOGIN'); + $s_hidden_fields = build_hidden_fields($s_hidden_fields); $login_box_template_data = array( diff --git a/phpBB/index.php b/phpBB/index.php index 13b914abd3..5eee7723a9 100644 --- a/phpBB/index.php +++ b/phpBB/index.php @@ -211,6 +211,9 @@ if ($show_birthdays) $template->assign_block_vars_array('birthdays', $birthdays); } +// Add form token for login box +add_form_key('login', '_LOGIN'); + // Assign index specific vars $template->assign_vars(array( 'TOTAL_POSTS' => $user->lang('TOTAL_POSTS_COUNT', (int) $config['num_posts']), diff --git a/phpBB/styles/prosilver/template/index_body.html b/phpBB/styles/prosilver/template/index_body.html index b292c40eb2..239a91c580 100644 --- a/phpBB/styles/prosilver/template/index_body.html +++ b/phpBB/styles/prosilver/template/index_body.html @@ -29,6 +29,7 @@ {S_LOGIN_REDIRECT} + {S_FORM_TOKEN_LOGIN} diff --git a/phpBB/styles/prosilver/template/login_body.html b/phpBB/styles/prosilver/template/login_body.html index ef08035717..dc597af51d 100644 --- a/phpBB/styles/prosilver/template/login_body.html +++ b/phpBB/styles/prosilver/template/login_body.html @@ -33,6 +33,7 @@ {S_LOGIN_REDIRECT} + {S_FORM_TOKEN_LOGIN}
 
{S_HIDDEN_FIELDS}
From 2353ad11f2afe213e407842a9a9e1533c0de57b0 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Fri, 26 Apr 2019 23:39:51 +0200 Subject: [PATCH 12/17] [ticket/security/235] Update search native tests SECURITY-235 --- tests/search/native_test.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/search/native_test.php b/tests/search/native_test.php index 29d0d0a8d3..0e6f719cef 100644 --- a/tests/search/native_test.php +++ b/tests/search/native_test.php @@ -70,7 +70,7 @@ class phpbb_search_native_test extends phpbb_search_test_case 'ba*az', 'all', true, - array('\'ba%az\''), + array(4), array(), array(), ), @@ -78,7 +78,7 @@ class phpbb_search_native_test extends phpbb_search_test_case 'ba*z', 'all', true, - array('\'ba%z\''), + array(), // <= 3 chars after removing * array(), array(), ), @@ -86,7 +86,7 @@ class phpbb_search_native_test extends phpbb_search_test_case 'baa* baaz*', 'all', true, - array('\'baa%\'', '\'baaz%\''), + array('\'baa%\'', 4), array(), array(), ), @@ -94,7 +94,7 @@ class phpbb_search_native_test extends phpbb_search_test_case 'ba*z baa*', 'all', true, - array('\'ba%z\'', '\'baa%\''), + array('\'baa%\''), // baz is <= 3 chars, only baa* is left array(), array(), ), From 1843e4f6b4e63b43de7da3fce0f7dd4c235853a9 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sat, 27 Apr 2019 13:52:37 +0200 Subject: [PATCH 13/17] [prep-release-3.2.6] Fix expected data in avatar manager test --- tests/avatar/manager_test.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/avatar/manager_test.php b/tests/avatar/manager_test.php index 0a64bfd69f..d1e907b53d 100644 --- a/tests/avatar/manager_test.php +++ b/tests/avatar/manager_test.php @@ -185,7 +185,7 @@ class phpbb_avatar_manager_test extends \phpbb_database_test_case $avatar_settings = $this->manager->get_avatar_settings($this->avatar_foobar); $expected_settings = array( - 'allow_avatar_' . get_class($this->avatar_foobar) => array('lang' => 'ALLOW_' . strtoupper(get_class($this->avatar_foobar)), 'validate' => 'bool', 'type' => 'radio:yes_no', 'explain' => false), + 'allow_avatar_' . get_class($this->avatar_foobar) => array('lang' => 'ALLOW_' . strtoupper(get_class($this->avatar_foobar)), 'validate' => 'bool', 'type' => 'radio:yes_no', 'explain' => true), ); $this->assertEquals($expected_settings, $avatar_settings); From 37e5457dcfd9c9b3a07c1ec8e248f6b648235386 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sat, 27 Apr 2019 14:34:33 +0200 Subject: [PATCH 14/17] [prep-release-3.2.6] Update to 3.2.6 version --- build/build.xml | 4 ++-- phpBB/includes/constants.php | 2 +- phpBB/install/phpbbcli.php | 2 +- phpBB/install/schemas/schema_data.sql | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/build/build.xml b/build/build.xml index 0dc59455c8..d1232a97fb 100644 --- a/build/build.xml +++ b/build/build.xml @@ -2,9 +2,9 @@ - + - + diff --git a/phpBB/includes/constants.php b/phpBB/includes/constants.php index 98b60166a3..b22f003ad3 100644 --- a/phpBB/includes/constants.php +++ b/phpBB/includes/constants.php @@ -28,7 +28,7 @@ if (!defined('IN_PHPBB')) */ // phpBB Version -@define('PHPBB_VERSION', '3.2.6-RC1'); +@define('PHPBB_VERSION', '3.2.6'); // QA-related // define('PHPBB_QA', 1); diff --git a/phpBB/install/phpbbcli.php b/phpBB/install/phpbbcli.php index c87a9a83ad..f8683d8f0f 100755 --- a/phpBB/install/phpbbcli.php +++ b/phpBB/install/phpbbcli.php @@ -23,7 +23,7 @@ if (php_sapi_name() !== 'cli') define('IN_PHPBB', true); define('IN_INSTALL', true); define('PHPBB_ENVIRONMENT', 'production'); -define('PHPBB_VERSION', '3.2.6-RC1'); +define('PHPBB_VERSION', '3.2.6'); $phpbb_root_path = __DIR__ . '/../'; $phpEx = substr(strrchr(__FILE__, '.'), 1); diff --git a/phpBB/install/schemas/schema_data.sql b/phpBB/install/schemas/schema_data.sql index fada5b6cbd..4d9adfd3d4 100644 --- a/phpBB/install/schemas/schema_data.sql +++ b/phpBB/install/schemas/schema_data.sql @@ -279,7 +279,7 @@ INSERT INTO phpbb_config (config_name, config_value) VALUES ('tpl_allow_php', '0 INSERT INTO phpbb_config (config_name, config_value) VALUES ('upload_icons_path', 'images/upload_icons'); INSERT INTO phpbb_config (config_name, config_value) VALUES ('upload_path', 'files'); INSERT INTO phpbb_config (config_name, config_value) VALUES ('use_system_cron', '0'); -INSERT INTO phpbb_config (config_name, config_value) VALUES ('version', '3.2.6-RC1'); +INSERT INTO phpbb_config (config_name, config_value) VALUES ('version', '3.2.6'); INSERT INTO phpbb_config (config_name, config_value) VALUES ('warnings_expire_days', '90'); INSERT INTO phpbb_config (config_name, config_value) VALUES ('warnings_gc', '14400'); From bec047586a09ec0bf4a8214edb66c452e2489851 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sat, 27 Apr 2019 14:41:50 +0200 Subject: [PATCH 15/17] [prep-release-3.2.6] Add migration to 3.2.6 --- phpBB/phpbb/db/migration/data/v32x/v326.php | 39 +++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 phpBB/phpbb/db/migration/data/v32x/v326.php diff --git a/phpBB/phpbb/db/migration/data/v32x/v326.php b/phpBB/phpbb/db/migration/data/v32x/v326.php new file mode 100644 index 0000000000..2d511b9ed8 --- /dev/null +++ b/phpBB/phpbb/db/migration/data/v32x/v326.php @@ -0,0 +1,39 @@ + +* @license GNU General Public License, version 2 (GPL-2.0) +* +* For full copyright and license information, please see +* the docs/CREDITS.txt file. +* +*/ + +namespace phpbb\db\migration\data\v32x; + +class v326 extends \phpbb\db\migration\migration +{ + public function effectively_installed() + { + return phpbb_version_compare($this->config['version'], '3.2.6', '>='); + } + + static public function depends_on() + { + return array( + '\phpbb\db\migration\data\v32x\v326rc1', + '\phpbb\db\migration\data\v32x\disable_remote_avatar', + '\phpbb\db\migration\data\v32x\smtp_dynamic_data', + ); + + } + + public function update_data() + { + return array( + array('config.update', array('version', '3.2.6')), + ); + } +} From d54c43ae8f711e38d54ea4f1c0cfcb120e20a4ac Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sun, 28 Apr 2019 09:27:54 +0200 Subject: [PATCH 16/17] [prep-release-3.2.6] Update changelog for 3.2.6 --- phpBB/docs/CHANGELOG.html | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/phpBB/docs/CHANGELOG.html b/phpBB/docs/CHANGELOG.html index 37710b30a6..e8dcc2be2b 100644 --- a/phpBB/docs/CHANGELOG.html +++ b/phpBB/docs/CHANGELOG.html @@ -50,6 +50,7 @@
  1. Changelog
      +
    • Changes since 3.2.6-RC1
    • Changes since 3.2.5
    • Changes since 3.2.5-RC1
    • Changes since 3.2.4
      • @@ -136,6 +137,23 @@
      +

      Changes since 3.2.6-RC1

      +

      Bug

      + +

      Security Issue

      +
        +
      • [SECURITY-231] - Remote avatar functionality allows checking for files and ports on local network
        • +
      • [SECURITY-235] - Fulltext native search can be used to cause long execution times
        • +
      +

      Hardening

      +
        +
      • [SECURITY-228] - Require form token in login_box
        • +
      • [SECURITY-233] - SMTP auth data shouldn't be cached
        • +
      • [SECURITY-234] - Main website URL can be set to JS URL in Admin Control Panel
        • +
      +

      Changes since 3.2.5

      Bug

        From 2575b499a38ccf2480d5da9d5c566f47a9e2d824 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sun, 28 Apr 2019 18:15:33 +0200 Subject: [PATCH 17/17] [prep-release-3.2.6] Update Changelog and add missing preg_match --- phpBB/docs/CHANGELOG.html | 2 +- phpBB/phpbb/avatar/driver/upload.php | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/phpBB/docs/CHANGELOG.html b/phpBB/docs/CHANGELOG.html index e8dcc2be2b..c6f05ca309 100644 --- a/phpBB/docs/CHANGELOG.html +++ b/phpBB/docs/CHANGELOG.html @@ -151,7 +151,7 @@
        • [SECURITY-228] - Require form token in login_box
        • [SECURITY-233] - SMTP auth data shouldn't be cached
          • -
        • [SECURITY-234] - Main website URL can be set to JS URL in Admin Control Panel
          • +
        • [SECURITY-234] - Main website URL in Admin Control Panel should not support JS URLs

        Changes since 3.2.5

        diff --git a/phpBB/phpbb/avatar/driver/upload.php b/phpBB/phpbb/avatar/driver/upload.php index 77b44754ac..a012bb15b6 100644 --- a/phpBB/phpbb/avatar/driver/upload.php +++ b/phpBB/phpbb/avatar/driver/upload.php @@ -148,7 +148,8 @@ class upload extends \phpbb\avatar\driver\driver // Do not allow specifying the port (see RFC 3986) or IP addresses // remote_upload() will do its own check for allowed filetypes - if (preg_match('@^(http|https|ftp)://[^/:?#]+:[0-9]+[/:?#]@i', $url) || + if (!preg_match('#^(http|https|ftp)://(?:(.*?\.)*?[a-z0-9\-]+?\.[a-z]{2,4}|(?:\d{1,3}\.){3,5}\d{1,3}):?([0-9]*?).*?\.('. implode('|', $this->allowed_extensions) . ')$#i', $url) || + preg_match('@^(http|https|ftp)://[^/:?#]+:[0-9]+[/:?#]@i', $url) || preg_match('#^(http|https|ftp)://(?:(?:\d{1,2}|1\d\d|2[0-4]\d|25[0-5])\.){3}(?:\d{1,2}|1\d\d|2[0-4]\d|25[0-5])#i', $url) || preg_match('#^(http|https|ftp)://(?:(?:(?:[\dA-F]{1,4}:){6}(?:[\dA-F]{1,4}:[\dA-F]{1,4}|(?:(?:\d{1,2}|1\d\d|2[0-4]\d|25[0-5])\.){3}(?:\d{1,2}|1\d\d|2[0-4]\d|25[0-5])))|(?:::(?:[\dA-F]{1,4}:){0,5}(?:[\dA-F]{1,4}(?::[\dA-F]{1,4})?|(?:(?:\d{1,2}|1\d\d|2[0-4]\d|25[0-5])\.){3}(?:\d{1,2}|1\d\d|2[0-4]\d|25[0-5])))|(?:(?:[\dA-F]{1,4}:):(?:[\dA-F]{1,4}:){4}(?:[\dA-F]{1,4}:[\dA-F]{1,4}|(?:(?:\d{1,2}|1\d\d|2[0-4]\d|25[0-5])\.){3}(?:\d{1,2}|1\d\d|2[0-4]\d|25[0-5])))|(?:(?:[\dA-F]{1,4}:){1,2}:(?:[\dA-F]{1,4}:){3}(?:[\dA-F]{1,4}:[\dA-F]{1,4}|(?:(?:\d{1,2}|1\d\d|2[0-4]\d|25[0-5])\.){3}(?:\d{1,2}|1\d\d|2[0-4]\d|25[0-5])))|(?:(?:[\dA-F]{1,4}:){1,3}:(?:[\dA-F]{1,4}:){2}(?:[\dA-F]{1,4}:[\dA-F]{1,4}|(?:(?:\d{1,2}|1\d\d|2[0-4]\d|25[0-5])\.){3}(?:\d{1,2}|1\d\d|2[0-4]\d|25[0-5])))|(?:(?:[\dA-F]{1,4}:){1,4}:(?:[\dA-F]{1,4}:)(?:[\dA-F]{1,4}:[\dA-F]{1,4}|(?:(?:\d{1,2}|1\d\d|2[0-4]\d|25[0-5])\.){3}(?:\d{1,2}|1\d\d|2[0-4]\d|25[0-5])))|(?:(?:[\dA-F]{1,4}:){1,5}:(?:[\dA-F]{1,4}:[\dA-F]{1,4}|(?:(?:\d{1,2}|1\d\d|2[0-4]\d|25[0-5])\.){3}(?:\d{1,2}|1\d\d|2[0-4]\d|25[0-5])))|(?:(?:[\dA-F]{1,4}:){1,6}:[\dA-F]{1,4})|(?:(?:[\dA-F]{1,4}:){1,7}:)|(?:::))#i', $url)) {