Security and correctness fixes from a Codex-assisted review

adm/style/banhammer_body.html

@@ -2,10 +2,6 @@
 
 <h1>{L_ACP_BH_SETTINGS}</h1>
 
-<!-- IF S_SAVED -->
-	<div class="successbox">{L_SETTINGS_SAVED}</div>
-<!-- ENDIF -->
-
 <form id="acp_board" method="post" action="{U_ACTION}">
 	<fieldset>
 		<dl>

config/services.yml

@@ -37,6 +37,8 @@ services:
             - '@language'
             - '@request'
             - '@user'
+            - '%core.root_path%'
+            - '%core.php_ext%'
     phpbbmodders.banhammer.cron.restriction_expiry:
         class: phpbbmodders\banhammer\cron\task\restriction_expiry
         arguments:

controller/admin_controller.php

@@ -126,20 +126,68 @@ public function display_options()
 	 */
 	protected function set_options()
 	{
+		$move_group = $this->request->variable('move_group', 0);
+		$restrict_group = $this->request->variable('restrict_group', 0);
+
+		$this->validate_group($move_group);
+		$this->validate_group($restrict_group);
+
 		$this->config->set('bh_ban_email', $this->request->variable('ban_email', 0));
 		$this->config->set('bh_ban_ip', $this->request->variable('ban_ip', 0));
 		$this->config->set('bh_del_avatar', $this->request->variable('del_avatar', 0));
 		$this->config->set('bh_del_privmsgs', $this->request->variable('del_privmsgs', 0));
 		$this->config->set('bh_del_posts', $this->request->variable('del_posts', 0));
 		$this->config->set('bh_del_profile', $this->request->variable('del_profile', 0));
 		$this->config->set('bh_del_signature', $this->request->variable('del_signature', 0));
-		$this->config->set('bh_group_id', $this->request->variable('move_group', 0));
-		$this->config->set('bh_restrict_group_id', $this->request->variable('restrict_group', 0));
+		$this->config->set('bh_group_id', $move_group);
+		$this->config->set('bh_restrict_group_id', $restrict_group);
 		$this->config->set('bh_sfs_api_key', $this->request->variable('sfs_api_key', '', true));
 		$this->config->set('bh_sfs_allow_http', $this->request->variable('sfs_allow_http', 0));
 		$this->config->set('bh_ban_time', $this->request->variable('ban_time', 0));
 	}
 
+	/**
+	 * Reject a submitted group id that isn't a valid choice: one of the
+	 * special groups excluded from the dropdown (get_groups() only hides
+	 * them client-side, a crafted submission can still send their id), or a
+	 * founder-managed group the current admin isn't allowed to assign users
+	 * into (phpBB's own acp_users.php enforces the same rule).
+	 *
+	 * @param int $group_id
+	 * @return void
+	 * @access private
+	 */
+	private function validate_group($group_id)
+	{
+		if (!$group_id)
+		{
+			return;
+		}
+
+		$sql = 'SELECT group_name, group_founder_manage
+			FROM ' . GROUPS_TABLE . '
+			WHERE group_id = ' . (int) $group_id;
+		$result = $this->db->sql_query($sql);
+		$row = $this->db->sql_fetchrow($result);
+		$this->db->sql_freeresult($result);
+
+		if (!$row || in_array($row['group_name'], $this->ignored_groups(), true) || ($this->user->data['user_type'] != USER_FOUNDER && $row['group_founder_manage']))
+		{
+			trigger_error($this->user->lang['FORM_INVALID'] . adm_back_link($this->u_action), E_USER_WARNING);
+		}
+	}
+
+	/**
+	 * Special groups no admin should be moving/restricting users into.
+	 *
+	 * @return array
+	 * @access private
+	 */
+	private function ignored_groups()
+	{
+		return array('BOTS', 'GUESTS', 'REGISTERED', 'REGISTERED_COPPA', 'NEWLY_REGISTERED', 'ADMINISTRATORS', 'GLOBAL_MODERATORS');
+	}
+
 	/**
 	 * function to return groups that are allowed
 	 */
@@ -149,9 +197,9 @@ private function get_groups($group_selected)
 
 		// Don't display any of the default groups
 		// highly doubt an admin would want to ban someone into a default group
-		$ignore_groups = array('BOTS', 'GUESTS', 'REGISTERED', 'REGISTERED_COPPA', 'NEWLY_REGISTERED', 'ADMINISTRATORS', 'GLOBAL_MODERATORS');
+		$ignore_groups = $this->ignored_groups();
 
-		$sql = 'SELECT group_name, group_id, group_type
+		$sql = 'SELECT group_name, group_id, group_type, group_founder_manage
 			FROM ' . GROUPS_TABLE . '
 			WHERE ' . $this->db->sql_in_set('group_name', $ignore_groups, true) . '
 			ORDER BY group_name ASC';
@@ -161,6 +209,13 @@ private function get_groups($group_selected)
 		$s_group_options = "<option value='0'$selected>&nbsp;{$this->user->lang['NO_GROUP']}&nbsp;</option>";
 		while ($row = $this->db->sql_fetchrow($result))
 		{
+			// Same rule as phpBB's own acp_users.php: don't offer a group a
+			// non-founder isn't allowed to manage.
+			if ($this->user->data['user_type'] != USER_FOUNDER && $row['group_founder_manage'])
+			{
+				continue;
+			}
+
 			$selected = ($row['group_id'] == $group_selected) ? ' selected="selected"' : '';
 			$group_name = ($row['group_type'] == GROUP_SPECIAL) ? $this->user->lang['G_' . $row['group_name']] : $row['group_name'];
 			$s_group_options .= "<option value='{$row['group_id']}'$selected>$group_name</option>";

controller/ban_domain_controller.php

@@ -35,26 +35,38 @@ class ban_domain_controller
 	/** @var \phpbb\user */
 	protected $user;
 
+	/** @var string phpBB root path */
+	protected $root_path;
+
+	/** @var string phpEx */
+	protected $php_ext;
+
 	/**
 	* Constructor
 	*
 	* @param \phpbb\auth\auth			$auth		Auth object
 	* @param \phpbb\language\language	$language	Language object
 	* @param \phpbb\request\request		$request	Request object
 	* @param \phpbb\user					$user		User object
+	* @param string							$root_path	phpBB root path
+	* @param string							$php_ext	PHP file extension
 	* @access public
 	*/
 	public function __construct(
 		\phpbb\auth\auth $auth,
 		\phpbb\language\language $language,
 		\phpbb\request\request $request,
-		\phpbb\user $user
+		\phpbb\user $user,
+		$root_path,
+		$php_ext
 	)
 	{
 		$this->auth		= $auth;
 		$this->language	= $language;
 		$this->request	= $request;
 		$this->user		= $user;
+		$this->root_path	= $root_path;
+		$this->php_ext		= $php_ext;
 	}
 
 	/**
@@ -93,6 +105,11 @@ public function handle()
 		{
 			$reason = $this->request->variable('bh_reason', '', true);
 
+			if (!function_exists('user_ban'))
+			{
+				include($this->root_path . 'includes/functions_user.' . $this->php_ext);
+			}
+
 			$success = user_ban('email', $ban_pattern, 0, '', false, $reason);
 
 			if (!$success)

cron/task/restriction_expiry.php

@@ -67,7 +67,7 @@ public function run()
 	{
 		$this->config->set('bh_restrict_last_run', time(), false);
 
-		$sql = 'SELECT restrict_id, user_id, original_group_id
+		$sql = 'SELECT restrict_id, user_id, original_group_id, restrict_group_id
 			FROM ' . $this->restrict_table . '
 			WHERE restrict_until > 0
 				AND restrict_until <= ' . time();
@@ -86,17 +86,20 @@ public function run()
 			return;
 		}
 
-		if (!function_exists('group_user_add') || !function_exists('group_user_del'))
+		if (!function_exists('group_user_del') || !function_exists('group_user_attributes'))
 		{
 			include($this->root_path . 'includes/functions_user.' . $this->php_ext);
 		}
 
-		$restrict_group_id = (int) $this->config['bh_restrict_group_id'];
-
 		foreach ($expired as $row)
 		{
 			$user_id = (int) $row['user_id'];
 			$original_group_id = (int) $row['original_group_id'];
+			// The group actually applied when this restriction was created,
+			// not the current ACP setting: an admin may have changed it
+			// since, and removing the wrong (or no) group would strand the
+			// user in whatever group they were actually restricted into.
+			$restrict_group_id = (int) $row['restrict_group_id'];
 
 			if ($restrict_group_id)
 			{
@@ -105,7 +108,11 @@ public function run()
 
 			if ($original_group_id)
 			{
-				group_user_add($original_group_id, array($user_id), false, false, true);
+				// Not group_user_add(..., true): the user was never removed
+				// from their original group while restricted, so it would
+				// see them as already a member and return GROUP_USERS_EXIST
+				// before reaching the code that sets the default group.
+				group_user_attributes('default', $original_group_id, array($user_id));
 			}
 
 			$this->db->sql_query('DELETE FROM ' . $this->restrict_table . ' WHERE restrict_id = ' . (int) $row['restrict_id']);

event/banhammer_listener.php

@@ -133,10 +133,14 @@ public function add_mcp_queue_banhammer_link($event)
 
 		if ($post_info['user_type'] != USER_FOUNDER && $target_user_id != $this->user->data['user_id'] && $target_user_id > 0)
 		{
+			// Deliberately not 'bh' => 1: that shortcut jumps straight to the
+			// confirmation step with none of the ban options set (permanent,
+			// no email/IP ban, no deletions, no group move, no SFS report),
+			// silently ignoring the ACP-configured defaults. Link to the
+			// profile page instead, which shows the real options form.
 			$params = array(
 				'mode'	=> 'viewprofile',
 				'u'		=> $target_user_id,
-				'bh'	=> 1,
 			);
 
 			$template_vars['S_MCP_SHOW_BANHAMMER'] = true;
@@ -234,7 +238,7 @@ public function do_ban_hammer_stuff($event)
 				}
 
 				$this->template->assign_vars(array(
-					'BH_STYLE'		=> (($bh_result == 'success') ? 'green' : '#a92c2c') . '; color: white;"',
+					'BH_STYLE'		=> (($bh_result == 'success') ? 'green' : '#a92c2c') . '; color: white;',
 					'BH_MESSAGE'	=> $bh_message,
 				));
 			}
@@ -341,6 +345,11 @@ public function do_ban_hammer_stuff($event)
 			$message .= ($hidden_fields['sfs_report'] && $curl_exists)			? $this->user->lang['BH_SUBMIT_SFS'] . '<br>' : '';
 
 			confirm_box(false, $message, build_hidden_fields($hidden_fields));
+
+			// confirm_box(false, ...) above only returns instead of exiting
+			// when the request was actually a cancellation (POST 'cancel'),
+			// in which case we must not fall through to the ban below.
+			return;
 		}
 
 		// We have a user to ban.
@@ -538,6 +547,11 @@ public function do_restrict_stuff($event)
 			$message .= ($length) ? '<br><br>' . $this->user->lang('BH_RESTRICT_FOR', $length) : '<br><br>' . $this->user->lang['BH_RESTRICT_PERM'];
 
 			confirm_box(false, $message, $hidden_fields);
+
+			// confirm_box(false, ...) above only returns instead of exiting
+			// when the request was actually a cancellation (POST 'cancel'),
+			// in which case we must not fall through to the restriction below.
+			return;
 		}
 
 		if (!function_exists('group_user_add'))
@@ -554,6 +568,7 @@ public function do_restrict_stuff($event)
 		$sql_ary = array(
 			'user_id'			=> $user_id,
 			'original_group_id'	=> $original_group_id,
+			'restrict_group_id'	=> $restrict_group_id,
 			'restrict_until'	=> $restrict_until,
 		);
 		$sql = 'INSERT INTO ' . $this->restrict_table . ' ' . $this->db->sql_build_array('INSERT', $sql_ary);
@@ -605,6 +620,15 @@ public function undo_bh_group($event)
 
 			if ($group_id)
 			{
+				// The ban and restrict groups can be configured to be the
+				// same group. A restricted (not banned) user deliberately
+				// sits in it, so leave their membership alone while the
+				// restriction is still active instead of undoing it here.
+				if ((int) $this->config['bh_restrict_group_id'] === (int) $this->config['bh_group_id'] && $this->active_restriction($this->user->data['user_id']) !== null)
+				{
+					return;
+				}
+
 				// Remove the user from the banned group set in the ACP
 				if (!function_exists('group_user_del'))
 				{
@@ -619,31 +643,22 @@ private function bh_del_privmsgs()
 	{
 		$user_id = $this->user_id;
 
-		// Get private messages
-		$sql = 'SELECT msg_id, author_id FROM ' . PRIVMSGS_TABLE . "
-				WHERE author_id = $user_id";
-		$result = $this->db->sql_query($sql);
-
-		$privmsgs_ary = array();
-		while ($row = $this->db->sql_fetchrow($result))
+		// phpBB's own bulk PM cleanup (used when deleting a user account
+		// entirely): correctly adjusts recipients' unread/new counts,
+		// removes attachments and notifications, and anonymizes already-
+		// delivered sent messages instead of deleting them out from under
+		// their recipients. A hand-rolled DELETE here previously left all
+		// of that bookkeeping inconsistent.
+		if (!function_exists('phpbb_delete_users_pms'))
 		{
-			$privmsgs_ary[] = $row['msg_id'];
+			include($this->root_path . 'includes/functions_privmsgs.' . $this->php_ext);
 		}
-		$this->db->sql_freeresult($result);
+		phpbb_delete_users_pms(array($user_id));
 
-		if (!empty($privmsgs_ary))
-		{
-			// And now close eventual reports.
-			$sql = 'UPDATE ' . REPORTS_TABLE . '
-					SET report_closed = 1
-					WHERE ' . $this->db->sql_in_set('pm_id', $privmsgs_ary);
-			$this->db->sql_query($sql);
-		}
-
-		$this->db->sql_query('DELETE FROM ' . PRIVMSGS_TABLE .			" WHERE author_id = $user_id");
+		// The account itself isn't deleted, only its own folder structure
+		// and rules, which don't affect any other user.
 		$this->db->sql_query('DELETE FROM ' . PRIVMSGS_FOLDER_TABLE .	" WHERE user_id = $user_id");
 		$this->db->sql_query('DELETE FROM ' . PRIVMSGS_RULES_TABLE .	" WHERE user_id = $user_id");
-		$this->db->sql_query('DELETE FROM ' . PRIVMSGS_TO_TABLE .		" WHERE user_id = $user_id OR author_id = $user_id");
 	}
 
 	private function bh_del_posts()
@@ -672,6 +687,24 @@ private function bh_del_posts()
 		}
 		$this->db->sql_freeresult($result);
 
+		// m_ban alone doesn't grant delete rights in every forum; only the
+		// extension's own explicit permission does. Without it, only touch
+		// posts in forums the acting moderator could delete in anyway.
+		if (!$this->auth->acl_get('m_banhammer_del_posts_all'))
+		{
+			foreach ($posts as $post_id => $post_row)
+			{
+				if (!$this->auth->acl_get('m_delete', (int) $post_row['forum_id']))
+				{
+					// Only gates $posts: the report-closing loop below only
+					// ever reads $topics through a $posts[$post_id] lookup,
+					// so leaving a stale count here for a topic we're no
+					// longer touching has no effect.
+					unset($posts[$post_id]);
+				}
+			}
+		}
+
 		// And now handle the reports.
 		$sql = 'SELECT report_id, post_id, report_closed
 				FROM ' . REPORTS_TABLE . '
@@ -729,7 +762,10 @@ private function bh_del_posts()
 		$this->db->sql_query('DELETE FROM ' . FORUMS_WATCH_TABLE . " WHERE user_id = $user_id");
 		$this->db->sql_query('DELETE FROM ' . MODERATOR_CACHE_TABLE . " WHERE user_id = $user_id");
 		$this->db->sql_query('DELETE FROM ' . NOTIFICATIONS_TABLE .	" WHERE user_id = $user_id");
-		$this->db->sql_query('DELETE FROM ' . POLL_VOTES_TABLE . " WHERE vote_user_id = $user_id");
+		// Poll votes are deliberately left alone, same as phpBB's own
+		// user_delete() (which doesn't touch POLL_VOTES_TABLE either):
+		// removing them here without decrementing poll_option_total would
+		// corrupt the poll's totals and let the user vote again later.
 		$this->db->sql_query('DELETE FROM ' . TOPICS_POSTED_TABLE . " WHERE user_id = $user_id");
 		$this->db->sql_query('DELETE FROM ' . TOPICS_TRACK_TABLE . " WHERE user_id = $user_id");
 		$this->db->sql_query('DELETE FROM ' . TOPICS_WATCH_TABLE . " WHERE user_id = $user_id");
@@ -744,12 +780,14 @@ private function get_file($url)
 		curl_setopt($ch, CURLOPT_URL, $url);
 		curl_setopt($ch, CURLOPT_TIMEOUT, 5);
 		curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, 5);
-		curl_exec($ch);
+		$response = curl_exec($ch);
 		$httpcode = curl_getinfo($ch, CURLINFO_HTTP_CODE);
 		curl_close($ch);
 
-		// if nothing is returned (SFS is down)
-		if ($httpcode != 200)
+		// curl_exec() returns false on a transport failure (e.g. the
+		// connection dropped after headers were already sent), which the
+		// HTTP code alone would not catch.
+		if ($response === false || $httpcode != 200)
 		{
 			return false;
 		}