From 585a3ed863c267f36584a3fb9a0cf35f6a2e4c2d Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sun, 18 Sep 2016 01:14:23 +0200 Subject: [ticket/14789] Add link hashes and form tokens to all acp links/buttons This will further harden the ACP security by adding link hashes to links and form tokens to forms that did not have these yet and result in modified settings or write action on the filesystem or database. These few links and forms were still relying on the global ACP protection, mainly due to them not posing further risks of compromising data. After this change these will now also be properly protected against tampering. PHPBB3-14789 --- phpBB/includes/acp/acp_database.php | 8 ++++++++ phpBB/includes/acp/acp_icons.php | 18 +++++++++++++++-- phpBB/includes/acp/acp_language.php | 7 ++++++- phpBB/includes/acp/acp_modules.php | 30 +++++++++++++++++++++++------ phpBB/includes/acp/acp_permission_roles.php | 9 +++++++-- phpBB/includes/acp/acp_profile.php | 29 +++++++++++++++++++++++++--- phpBB/includes/acp/acp_reasons.php | 9 +++++++-- phpBB/includes/acp/acp_search.php | 18 ++++++++++++++++- 8 files changed, 111 insertions(+), 17 deletions(-) diff --git a/phpBB/includes/acp/acp_database.php b/phpBB/includes/acp/acp_database.php index 9666ac5b6e..16655ff4cb 100644 --- a/phpBB/includes/acp/acp_database.php +++ b/phpBB/includes/acp/acp_database.php @@ -39,6 +39,14 @@ class acp_database $action = request_var('action', ''); $submit = (isset($_POST['submit'])) ? true : false; + $form_key = 'acp_database'; + add_form_key($form_key); + + if ($submit && !check_form_key($form_key)) + { + trigger_error($user->lang['FORM_INVALID'] . adm_back_link($this->u_action), E_USER_WARNING); + } + $template->assign_vars(array( 'MODE' => $mode )); diff --git a/phpBB/includes/acp/acp_icons.php b/phpBB/includes/acp/acp_icons.php index 9265415dd1..e9bc02d88b 100644 --- a/phpBB/includes/acp/acp_icons.php +++ b/phpBB/includes/acp/acp_icons.php @@ -40,6 +40,15 @@ class acp_icons $action = (isset($_POST['edit'])) ? 'edit' : $action; $action = (isset($_POST['import'])) ? 'import' : $action; $icon_id = request_var('id', 0); + $submit = $request->is_set_post('submit', false); + + $form_key = 'acp_icons'; + add_form_key($form_key); + + if ($submit && !check_form_key($form_key)) + { + trigger_error($user->lang['FORM_INVALID'] . adm_back_link($this->u_action), E_USER_WARNING); + } $mode = ($mode == 'smilies') ? 'smilies' : 'icons'; @@ -811,6 +820,11 @@ class acp_icons case 'move_up': case 'move_down': + if (!check_link_hash($request->variable('hash', ''), 'acp_icons')) + { + trigger_error($user->lang['FORM_INVALID'] . adm_back_link($this->u_action), E_USER_WARNING); + } + // Get current order id... $sql = "SELECT {$fields}_order as current_order FROM $table @@ -928,8 +942,8 @@ class acp_icons 'EMOTION' => (isset($row['emotion'])) ? $row['emotion'] : '', 'U_EDIT' => $this->u_action . '&action=edit&id=' . $row[$fields . '_id'], 'U_DELETE' => $this->u_action . '&action=delete&id=' . $row[$fields . '_id'], - 'U_MOVE_UP' => $this->u_action . '&action=move_up&id=' . $row[$fields . '_id'] . '&start=' . $pagination_start, - 'U_MOVE_DOWN' => $this->u_action . '&action=move_down&id=' . $row[$fields . '_id'] . '&start=' . $pagination_start, + 'U_MOVE_UP' => $this->u_action . '&action=move_up&id=' . $row[$fields . '_id'] . '&start=' . $pagination_start . '&hash=' . generate_link_hash('acp_icons'), + 'U_MOVE_DOWN' => $this->u_action . '&action=move_down&id=' . $row[$fields . '_id'] . '&start=' . $pagination_start . '&hash=' . generate_link_hash('acp_icons'), )); if (!$spacer && !$row['display_on_posting']) diff --git a/phpBB/includes/acp/acp_language.php b/phpBB/includes/acp/acp_language.php index 3888a411f0..bddc2be9cb 100644 --- a/phpBB/includes/acp/acp_language.php +++ b/phpBB/includes/acp/acp_language.php @@ -244,6 +244,11 @@ class acp_language break; case 'install': + if (!check_link_hash($request->variable('hash', ''), 'acp_language')) + { + trigger_error($user->lang['FORM_INVALID'] . adm_back_link($this->u_action), E_USER_WARNING); + } + $lang_iso = request_var('iso', ''); $lang_iso = basename($lang_iso); @@ -423,7 +428,7 @@ class acp_language 'ISO' => htmlspecialchars($lang_ary['iso']), 'LOCAL_NAME' => htmlspecialchars($lang_ary['local_name'], ENT_COMPAT, 'UTF-8'), 'NAME' => htmlspecialchars($lang_ary['name'], ENT_COMPAT, 'UTF-8'), - 'U_INSTALL' => $this->u_action . '&action=install&iso=' . urlencode($lang_ary['iso'])) + 'U_INSTALL' => $this->u_action . '&action=install&iso=' . urlencode($lang_ary['iso']) . '&hash=' . generate_link_hash('acp_language')) ); } } diff --git a/phpBB/includes/acp/acp_modules.php b/phpBB/includes/acp/acp_modules.php index 55ea26b9d3..9d14614417 100644 --- a/phpBB/includes/acp/acp_modules.php +++ b/phpBB/includes/acp/acp_modules.php @@ -46,6 +46,9 @@ class acp_modules $user->add_lang('acp/modules'); $this->tpl_name = 'acp_modules'; + $form_key = 'acp_modules'; + add_form_key($form_key); + // module class $this->module_class = $mode; @@ -119,6 +122,11 @@ class acp_modules trigger_error($user->lang['NO_MODULE_ID'] . adm_back_link($this->u_action . '&parent_id=' . $this->parent_id), E_USER_WARNING); } + if (!check_link_hash($request->variable('hash', ''), 'acp_modules')) + { + trigger_error($user->lang['FORM_INVALID'] . adm_back_link($this->u_action . '&parent_id=' . $this->parent_id), E_USER_WARNING); + } + $sql = 'SELECT * FROM ' . MODULES_TABLE . " WHERE module_class = '" . $db->sql_escape($this->module_class) . "' @@ -150,6 +158,11 @@ class acp_modules trigger_error($user->lang['NO_MODULE_ID'] . adm_back_link($this->u_action . '&parent_id=' . $this->parent_id), E_USER_WARNING); } + if (!check_link_hash($request->variable('hash', ''), 'acp_modules')) + { + trigger_error($user->lang['FORM_INVALID'] . adm_back_link($this->u_action . '&parent_id=' . $this->parent_id), E_USER_WARNING); + } + $sql = 'SELECT * FROM ' . MODULES_TABLE . " WHERE module_class = '" . $db->sql_escape($this->module_class) . "' @@ -273,6 +286,11 @@ class acp_modules if ($submit) { + if (!check_form_key($form_key)) + { + trigger_error($user->lang['FORM_INVALID'] . adm_back_link($this->u_action . '&parent_id=' . $this->parent_id), E_USER_WARNING); + } + if (!$module_data['module_langname']) { trigger_error($user->lang['NO_MODULE_LANGNAME'] . adm_back_link($this->u_action . '&parent_id=' . $this->parent_id), E_USER_WARNING); @@ -460,12 +478,12 @@ class acp_modules 'S_ACP_MODULE_MANAGEMENT' => ($this->module_class == 'acp' && ($row['module_basename'] == 'modules' || $row['module_langname'] == 'ACP_MODULE_MANAGEMENT')) ? true : false, 'U_MODULE' => $this->u_action . '&parent_id=' . $row['module_id'], - 'U_MOVE_UP' => $url . '&action=move_up', - 'U_MOVE_DOWN' => $url . '&action=move_down', + 'U_MOVE_UP' => $url . '&action=move_up&hash=' . generate_link_hash('acp_modules'), + 'U_MOVE_DOWN' => $url . '&action=move_down&hash=' . generate_link_hash('acp_modules'), 'U_EDIT' => $url . '&action=edit', 'U_DELETE' => $url . '&action=delete', - 'U_ENABLE' => $url . '&action=enable', - 'U_DISABLE' => $url . '&action=disable') + 'U_ENABLE' => $url . '&action=enable&hash=' . generate_link_hash('acp_modules'), + 'U_DISABLE' => $url . '&action=disable&hash=' . generate_link_hash('acp_modules')) ); } while ($row = $db->sql_fetchrow($result)); @@ -484,8 +502,8 @@ class acp_modules 'U_EDIT' => $url . '&action=edit', 'U_DELETE' => $url . '&action=delete', - 'U_ENABLE' => $url . '&action=enable', - 'U_DISABLE' => $url . '&action=disable') + 'U_ENABLE' => $url . '&action=enable&hash=' . generate_link_hash('acp_modules'), + 'U_DISABLE' => $url . '&action=disable&hash=' . generate_link_hash('acp_modules')) ); } $db->sql_freeresult($result); diff --git a/phpBB/includes/acp/acp_permission_roles.php b/phpBB/includes/acp/acp_permission_roles.php index be4ab4676a..0796b36fef 100644 --- a/phpBB/includes/acp/acp_permission_roles.php +++ b/phpBB/includes/acp/acp_permission_roles.php @@ -366,6 +366,11 @@ class acp_permission_roles case 'move_up': case 'move_down': + if (!check_link_hash($request->variable('hash', ''), 'acp_permission_roles')) + { + trigger_error($user->lang['FORM_INVALID'] . adm_back_link($this->u_action), E_USER_WARNING); + } + $sql = 'SELECT role_order FROM ' . ACL_ROLES_TABLE . " WHERE role_id = $role_id"; @@ -440,8 +445,8 @@ class acp_permission_roles 'U_EDIT' => $this->u_action . '&action=edit&role_id=' . $row['role_id'], 'U_REMOVE' => $this->u_action . '&action=remove&role_id=' . $row['role_id'], - 'U_MOVE_UP' => $this->u_action . '&action=move_up&role_id=' . $row['role_id'], - 'U_MOVE_DOWN' => $this->u_action . '&action=move_down&role_id=' . $row['role_id'], + 'U_MOVE_UP' => $this->u_action . '&action=move_up&role_id=' . $row['role_id'] . '&hash=' . generate_link_hash('acp_permission_roles'), + 'U_MOVE_DOWN' => $this->u_action . '&action=move_down&role_id=' . $row['role_id'] . '&hash=' . generate_link_hash('acp_permission_roles'), 'U_DISPLAY_ITEMS' => ($row['role_id'] == $display_item) ? '' : $this->u_action . '&display_item=' . $row['role_id'] . '#assigned_to') ); diff --git a/phpBB/includes/acp/acp_profile.php b/phpBB/includes/acp/acp_profile.php index 8c7691538c..485f849f51 100644 --- a/phpBB/includes/acp/acp_profile.php +++ b/phpBB/includes/acp/acp_profile.php @@ -53,6 +53,9 @@ class acp_profile $error = array(); $s_hidden_fields = ''; + $form_key = 'acp_profile'; + add_form_key($form_key); + if (!$field_id && in_array($action, array('delete','activate', 'deactivate', 'move_up', 'move_down', 'edit'))) { trigger_error($user->lang['NO_FIELD_ID'] . adm_back_link($this->u_action), E_USER_WARNING); @@ -161,6 +164,11 @@ class acp_profile case 'activate': + if (!check_link_hash($request->variable('hash', ''), 'acp_profile')) + { + trigger_error($user->lang['FORM_INVALID'] . adm_back_link($this->u_action), E_USER_WARNING); + } + $sql = 'SELECT lang_id FROM ' . LANG_TABLE . " WHERE lang_iso = '" . $db->sql_escape($config['default_lang']) . "'"; @@ -201,6 +209,11 @@ class acp_profile case 'deactivate': + if (!check_link_hash($request->variable('hash', ''), 'acp_profile')) + { + trigger_error($user->lang['FORM_INVALID'] . adm_back_link($this->u_action), E_USER_WARNING); + } + $sql = 'UPDATE ' . PROFILE_FIELDS_TABLE . " SET field_active = 0 WHERE field_id = $field_id"; @@ -230,6 +243,11 @@ class acp_profile case 'move_up': case 'move_down': + if (!check_link_hash($request->variable('hash', ''), 'acp_profile')) + { + trigger_error($user->lang['FORM_INVALID'] . adm_back_link($this->u_action), E_USER_WARNING); + } + $sql = 'SELECT field_order FROM ' . PROFILE_FIELDS_TABLE . " WHERE field_id = $field_id"; @@ -579,6 +597,11 @@ class acp_profile if (!sizeof($error)) { + if (!check_form_key($form_key)) + { + trigger_error($user->lang['FORM_INVALID'] . adm_back_link($this->u_action), E_USER_WARNING); + } + if (($step == 3 && (sizeof($this->lang_defs['iso']) == 1 || $save)) || ($action == 'edit' && $save)) { $this->save_profile_field($cp, $field_type, $action); @@ -735,12 +758,12 @@ class acp_profile 'FIELD_TYPE' => $profile_field->get_name(), 'L_ACTIVATE_DEACTIVATE' => $user->lang[$active_lang], - 'U_ACTIVATE_DEACTIVATE' => $this->u_action . "&action=$active_value&field_id=$id", + 'U_ACTIVATE_DEACTIVATE' => $this->u_action . "&action=$active_value&field_id=$id" . '&hash=' . generate_link_hash('acp_profile'), 'U_EDIT' => $this->u_action . "&action=edit&field_id=$id", 'U_TRANSLATE' => $this->u_action . "&action=edit&field_id=$id&step=3", 'U_DELETE' => $this->u_action . "&action=delete&field_id=$id", - 'U_MOVE_UP' => $this->u_action . "&action=move_up&field_id=$id", - 'U_MOVE_DOWN' => $this->u_action . "&action=move_down&field_id=$id", + 'U_MOVE_UP' => $this->u_action . "&action=move_up&field_id=$id" . '&hash=' . generate_link_hash('acp_profile'), + 'U_MOVE_DOWN' => $this->u_action . "&action=move_down&field_id=$id" . '&hash=' . generate_link_hash('acp_profile'), 'S_NEED_EDIT' => $s_need_edit) ); diff --git a/phpBB/includes/acp/acp_reasons.php b/phpBB/includes/acp/acp_reasons.php index 3d7ccf422c..bd40a88138 100644 --- a/phpBB/includes/acp/acp_reasons.php +++ b/phpBB/includes/acp/acp_reasons.php @@ -282,6 +282,11 @@ class acp_reasons case 'move_up': case 'move_down': + if (!check_link_hash($request->variable('hash', ''), 'acp_reasons')) + { + trigger_error($user->lang['FORM_INVALID'] . adm_back_link($this->u_action), E_USER_WARNING); + } + $sql = 'SELECT reason_order FROM ' . REPORTS_REASONS_TABLE . " WHERE reason_id = $reason_id"; @@ -383,8 +388,8 @@ class acp_reasons 'U_EDIT' => $this->u_action . '&action=edit&id=' . $row['reason_id'], 'U_DELETE' => (!$other_reason) ? $this->u_action . '&action=delete&id=' . $row['reason_id'] : '', - 'U_MOVE_UP' => $this->u_action . '&action=move_up&id=' . $row['reason_id'], - 'U_MOVE_DOWN' => $this->u_action . '&action=move_down&id=' . $row['reason_id']) + 'U_MOVE_UP' => $this->u_action . '&action=move_up&id=' . $row['reason_id'] . '&hash=' . generate_link_hash('acp_reasons'), + 'U_MOVE_DOWN' => $this->u_action . '&action=move_down&id=' . $row['reason_id'] . '&hash=' . generate_link_hash('acp_reasons')) ); } $db->sql_freeresult($result); diff --git a/phpBB/includes/acp/acp_search.php b/phpBB/includes/acp/acp_search.php index abb8301507..2f68949834 100644 --- a/phpBB/includes/acp/acp_search.php +++ b/phpBB/includes/acp/acp_search.php @@ -54,6 +54,13 @@ class acp_search global $config, $phpbb_root_path, $phpbb_admin_path, $phpEx; $submit = (isset($_POST['submit'])) ? true : false; + $form_key = 'acp_search'; + add_form_key($form_key); + + if ($submit && !check_form_key($form_key)) + { + trigger_error($user->lang['FORM_INVALID'] . adm_back_link($this->u_action), E_USER_WARNING); + } $search_types = $this->get_search_types(); @@ -232,7 +239,7 @@ class acp_search function index($id, $mode) { - global $db, $user, $auth, $template, $cache; + global $db, $user, $auth, $template, $cache, $request; global $config, $phpbb_root_path, $phpbb_admin_path, $phpEx; $action = request_var('action', ''); @@ -244,6 +251,15 @@ class acp_search $this->state = array(); $this->save_state(); } + $submit = $request->is_set_post('submit', false); + + $form_key = 'acp_search'; + //add_form_key($form_key); + + if (!check_form_key($form_key) && in_array($action, array('delete', 'create'))) + { + trigger_error($user->lang['FORM_INVALID'] . adm_back_link($this->u_action), E_USER_WARNING); + } if ($action) { -- cgit v1.2.1 From 72f6241aa2c6d129c8c49380d84fd915d589aa6c Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sun, 18 Sep 2016 11:47:18 +0200 Subject: [ticket/14789] Add form tokens to tests and uncomment add_form_key PHPBB3-14789 --- phpBB/includes/acp/acp_search.php | 2 +- tests/functional/search/base.php | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/phpBB/includes/acp/acp_search.php b/phpBB/includes/acp/acp_search.php index 2f68949834..f15a75e9a1 100644 --- a/phpBB/includes/acp/acp_search.php +++ b/phpBB/includes/acp/acp_search.php @@ -254,7 +254,7 @@ class acp_search $submit = $request->is_set_post('submit', false); $form_key = 'acp_search'; - //add_form_key($form_key); + add_form_key($form_key); if (!check_form_key($form_key) && in_array($action, array('delete', 'create'))) { diff --git a/tests/functional/search/base.php b/tests/functional/search/base.php index 1d37d748df..d41e3ec925 100644 --- a/tests/functional/search/base.php +++ b/tests/functional/search/base.php @@ -75,6 +75,8 @@ abstract class phpbb_functional_search_base extends phpbb_functional_test_case protected function create_search_index() { $this->add_lang('acp/search'); + $crawler = self::request('GET', 'adm/index.php?i=acp_search&mode=index&sid=' . $this->sid); + $form_values = $crawler->selectButton('Delete index')->form()->getValues(); $crawler = self::request( 'POST', 'adm/index.php?i=acp_search&mode=index&sid=' . $this->sid, @@ -82,6 +84,8 @@ abstract class phpbb_functional_search_base extends phpbb_functional_test_case 'search_type' => $this->search_backend, 'action' => 'create', 'submit' => true, + 'form_token' => $form_values['form_token'], + 'creation_time' => $form_values['creation_time'], ) ); $this->assertContainsLang('SEARCH_INDEX_CREATED', $crawler->text()); @@ -90,6 +94,8 @@ abstract class phpbb_functional_search_base extends phpbb_functional_test_case protected function delete_search_index() { $this->add_lang('acp/search'); + $crawler = self::request('GET', 'adm/index.php?i=acp_search&mode=index&sid=' . $this->sid); + $form_values = $crawler->selectButton('Delete index')->form()->getValues(); $crawler = self::request( 'POST', 'adm/index.php?i=acp_search&mode=index&sid=' . $this->sid, @@ -97,6 +103,8 @@ abstract class phpbb_functional_search_base extends phpbb_functional_test_case 'search_type' => $this->search_backend, 'action' => 'delete', 'submit' => true, + 'form_token' => $form_values['form_token'], + 'creation_time' => $form_values['creation_time'], ) ); $this->assertContainsLang('SEARCH_INDEX_REMOVED', $crawler->text()); -- cgit v1.2.1