Security review fixes (component): settings validation, escaping, permissions, token hashes

admin/language/de-DE/de-DE.com_bfstop.ini

@@ -54,6 +54,9 @@ COM_BFSTOP_SETTINGS_VERSION_UNKNOWN="unbekannt"
 COM_BFSTOP_HEADING_IPINFO="Informationen zur IP-Adresse %s"
 COM_BFSTOP_SUBMENU_ALLOWLIST="Ausnahme-Liste"
 COM_BFSTOP_HEADING_ALLOWLIST="Brute Force Stop: Ausnahme-Liste"
+COM_BFSTOP_ALLOWLIST_REMOVED="Aus der Positivliste entfernt."
+COM_BFSTOP_ALLOWLIST_REMOVE_FAILED="Die Einträge konnten nicht aus der Positivliste entfernt werden."
+COM_BFSTOP_ALLOWLIST_NOTHING_REMOVED="Es wurde nichts ausgewählt."
 COM_BFSTOP_ALLOWLIST_DELETE_CONFIRM="Sind Sie sicher, dass Sie die ausgewählte(n) IP-Adresse(n) löschen wollen?"
 COM_BFSTOP_BLOCK_UNLIMITED="dauerhaft"
 COM_BFSTOP_BLOCK_5MINUTES="5 Minuten"
@@ -120,6 +123,7 @@ COM_BFSTOP_SETTINGS_UNKNOWN_USERNAME_MODE_LABEL="Benutzernamen ohne passendes Ko
 COM_BFSTOP_SETTINGS_UNKNOWN_USERNAME_MODE_DESC="Benutzer tippen oft ihr Passwort in das Benutzernamenfeld. Mit 'Hash' wird der Benutzername eines fehlgeschlagenen Logins nur dann so wie eingegeben gespeichert (angezeigt, protokolliert und per E-Mail gesendet), wenn er zu einem bestehenden Konto gehört oder in der Liste der häufigen Benutzernamen steht; alles andere wird durch einen unlesbaren Wert ersetzt, der für dieselbe Eingabe immer gleich ist, sodass Versuche damit zusammen gezählt werden. 'Wie eingegeben speichern' lässt jeden eingegebenen Benutzernamen lesbar, speichert dann aber auch Passwörter, die ins Benutzernamenfeld getippt wurden."
 COM_BFSTOP_SETTINGS_UNKNOWN_USERNAME_MODE_HASH="Hash (empfohlen)"
 COM_BFSTOP_SETTINGS_UNKNOWN_USERNAME_MODE_PLAIN="Wie eingegeben speichern"
+COM_BFSTOP_SETTINGS_EMAIL_INVALID="Keine gültige E-Mail-Adresse: %s"
 COM_BFSTOP_SETTINGS_HTACCESS_PATH_INVALID="Der .htaccess-Pfad muss ein existierendes Verzeichnis im Dateisystem des Servers sein."
 COM_BFSTOP_SETTINGS_GEOIP_DB_PATH_INVALID="Der Pfad der GeoIP-Datenbank muss der Name einer MaxMind-Datenbankdatei (Endung .mmdb) im Dateisystem des Servers sein."
 COM_BFSTOP_SETTINGS_IPV6_PREFIX_LABEL="Genauigkeit bei IPv6"

admin/language/en-GB/en-GB.com_bfstop.ini

@@ -54,6 +54,9 @@ COM_BFSTOP_SETTINGS_VERSION_UNKNOWN="unknown"
 COM_BFSTOP_HEADING_IPINFO="Information for IP Address %s"
 COM_BFSTOP_SUBMENU_ALLOWLIST="Allow List"
 COM_BFSTOP_HEADING_ALLOWLIST="Brute Force Stop: Allow List"
+COM_BFSTOP_ALLOWLIST_REMOVED="Removed from the allow list."
+COM_BFSTOP_ALLOWLIST_REMOVE_FAILED="Could not remove the entries from the allow list."
+COM_BFSTOP_ALLOWLIST_NOTHING_REMOVED="Nothing was selected."
 COM_BFSTOP_ALLOWLIST_DELETE_CONFIRM="Are you sure you want to delete the marked IP-Address(es)?"
 COM_BFSTOP_BLOCK_UNLIMITED="unlimited"
 COM_BFSTOP_BLOCK_5MINUTES="5 minutes"
@@ -172,6 +175,7 @@ COM_BFSTOP_SETTINGS_UNKNOWN_USERNAME_MODE_LABEL="Usernames Not Matching an Accou
 COM_BFSTOP_SETTINGS_UNKNOWN_USERNAME_MODE_DESC="Users often type their password into the username field. With 'Hash', the username of a failed login is only stored (and shown, logged and mailed) as typed if it belongs to an existing account or is on the list of common usernames; anything else is replaced by an unreadable value, which is still the same for the same input, so attempts against it are counted together. 'Store as typed' keeps every entered username readable, but then also stores passwords typed into the username field."
 COM_BFSTOP_SETTINGS_UNKNOWN_USERNAME_MODE_HASH="Hash (recommended)"
 COM_BFSTOP_SETTINGS_UNKNOWN_USERNAME_MODE_PLAIN="Store as typed"
+COM_BFSTOP_SETTINGS_EMAIL_INVALID="Not a valid email address: %s"
 COM_BFSTOP_SETTINGS_HTACCESS_PATH_INVALID="The .htaccess path has to be an existing directory of the server's file system."
 COM_BFSTOP_SETTINGS_GEOIP_DB_PATH_INVALID="The GeoIP database path has to be the name of a MaxMind database file (ending in .mmdb) in the server's file system."
 COM_BFSTOP_SETTINGS_IPV6_PREFIX_LABEL="IPv6 Tracking Granularity"

admin/rules/emaillist.php

@@ -0,0 +1,39 @@
+<?php
+/*
+ * @package BFStop Component (com_bfstop) for Joomla!
+ * @author Bernhard Froehler
+ * @copyright (C) Bernhard Froehler
+ * @license GNU/GPLv3 http://www.gnu.org/licenses/gpl-3.0.html
+**/
+
+defined('_JEXEC') or die;
+
+use Joomla\CMS\Form\Form;
+use Joomla\CMS\Form\FormRule;
+use Joomla\CMS\Language\Text;
+use Joomla\Registry\Registry;
+
+/**
+ * Email addresses separated by semicolons, as the notification setting takes
+ * them; the value ends up in the recipients of mails sent by the plugin.
+ */
+class JFormRuleEmaillist extends FormRule
+{
+	public function test(SimpleXMLElement $element, $value, $group = null, Registry $input = null, Form $form = null)
+	{
+		$value = trim((string) $value);
+		if ($value === '')
+		{
+			return true;
+		}
+		foreach (explode(';', $value) as $address)
+		{
+			if (filter_var(trim($address), FILTER_VALIDATE_EMAIL) === false)
+			{
+				return new UnexpectedValueException(Text::sprintf('COM_BFSTOP_SETTINGS_EMAIL_INVALID',
+					htmlspecialchars(trim($address), ENT_QUOTES, 'UTF-8')));
+			}
+		}
+		return true;
+	}
+}

admin/src/Controller/AllowlistController.php

@@ -37,7 +37,7 @@ public function remove()
 			return;
 		}
 		$logger = LogHelper::getLogger();
-		$input = Factory::getApplication()->input;
+		$input = Factory::getApplication()->getInput();
 		$ids = $input->post->get('cid', array(), 'array');
 		ArrayHelper::toInteger($ids);
 		$model = $this->getModel('allowlist');

admin/src/Controller/BlocklistController.php

@@ -36,7 +36,7 @@ function unblock()
 			return;
 		}
 		$logger = LogHelper::getLogger();
-		$input = Factory::getApplication()->input;
+		$input = Factory::getApplication()->getInput();
 		$ids = $input->post->get('cid', array(), 'array');
 		$model = $this->getModel('blocklist');
 		$message = $model->unblock($ids, $logger);

admin/src/Controller/DisplayController.php

@@ -18,12 +18,13 @@
 use Joomla\CMS\Language\Text;
 use Joomla\CMS\MVC\Controller\BaseController;
 use Joomla\CMS\Router\Route;
+use Joomla\Database\DatabaseInterface;
 
 class DisplayController extends BaseController
 {
 	function display($cachable = false, $urlparams = false)
 	{
-		$input = Factory::getApplication()->input;
+		$input = Factory::getApplication()->getInput();
 		$view = $input->getCmd('view', 'blocklist');
 
 		$pluginInstalled = $this->checkWhetherPluginInstalled();
@@ -42,7 +43,7 @@ function warnIfAdminUserExists()
 	{
 		try
 		{
-			$db = Factory::getDbo();
+			$db = Factory::getContainer()->get(DatabaseInterface::class);
 			// LOWER: also catch "Admin" etc. on PostgreSQL, which (unlike
 			// MySQL's default collations) compares case-sensitively
 			$query = "SELECT COUNT(*) FROM #__users u WHERE LOWER(u.username)='admin'";
@@ -101,7 +102,7 @@ function checkWhetherPluginInstalled()
 	{
 		try
 		{
-			$db = Factory::getDbo();
+			$db = Factory::getContainer()->get(DatabaseInterface::class);
 			$query = "SELECT manifest_cache,enabled FROM #__extensions WHERE name='plg_system_bfstop'";
 			$db->setQuery($query);
 			$plugin = $db->loadObject();

admin/src/Controller/FailedloginlistController.php

@@ -34,7 +34,7 @@ function purge()
 			$this->setRedirect($listUrl, Text::_('COM_BFSTOP_NOT_AUTHORISED'), 'error');
 			return;
 		}
-		$age = Factory::getApplication()->input->post->getInt('age', 0);
+		$age = Factory::getApplication()->getInput()->post->getInt('age', 0);
 		if ($age < 1)
 		{
 			$this->setRedirect($listUrl, Text::_('COM_BFSTOP_FAILEDLOGIN_PURGE_INVALID_AGE'), 'error');

admin/src/Controller/HtblockController.php

@@ -57,7 +57,7 @@ public function save($key = null, $urlVar = null)
 		$htaccess = new HtaccessHelper($htaccessPath, null);
 		$model = $this->getModel('block');
 		$form = $model->getForm(null, false);
-		$input = Factory::getApplication()->input;
+		$input = Factory::getApplication()->getInput();
 		$data  = $input->post->get('jform', array(), 'array');
 		$validData = $model->validate($form, $data);
 		if ($validData === false)
@@ -66,16 +66,11 @@ public function save($key = null, $urlVar = null)
 			$msg = "";
 			foreach ($errors as $error)
 			{
-				if ($error instanceof \Exception)
-				{
-					$msg .= $error->getMessage();
-				}
-				else
-				{
-					$msg .= $error;
-				}
+				// the messages have been escaped where they were made (they may
+				// contain what was typed in), see IpValidateHelper
+				$msg .= ($error instanceof \Exception) ? $error->getMessage() : $error;
 			}
-			$this->returnToFormWithMessage($data['ipaddress'], $msg);
+			$this->returnToFormWithMessage(is_string($data['ipaddress'] ?? null) ? $data['ipaddress'] : '', $msg);
 			return false;
 		}
 		$ipaddress = $validData['ipaddress'];

admin/src/Controller/HtblocklistController.php

@@ -36,7 +36,7 @@ function unblock()
 			return;
 		}
 		$logger = LogHelper::getLogger();
-		$input = Factory::getApplication()->input;
+		$input = Factory::getApplication()->getInput();
 		$ips = $input->post->get('cid', array(), 'array');
 		$model = $this->getModel('htblocklist');
 		$message = $model->unblock($ips, $logger);

admin/src/Controller/UsernamestatsController.php

@@ -34,7 +34,7 @@ function purge()
 			$this->setRedirect($listUrl, Text::_('COM_BFSTOP_NOT_AUTHORISED'), 'error');
 			return;
 		}
-		$age = Factory::getApplication()->input->post->getInt('age', 0);
+		$age = Factory::getApplication()->getInput()->post->getInt('age', 0);
 		if ($age < 1)
 		{
 			$this->setRedirect($listUrl, Text::_('COM_BFSTOP_USERNAMESTATS_PURGE_INVALID_AGE'), 'error');

admin/src/Helper/IpValidateHelper.php

@@ -16,6 +16,15 @@
 
 class IpValidateHelper
 {
+	/**
+	 * Joomla shows the messages it is given as HTML, so what was typed into
+	 * a form must not be put into one as it is.
+	 */
+	private static function e($text)
+	{
+		return htmlspecialchars((string) $text, ENT_QUOTES, 'UTF-8');
+	}
+
 	public static function validIPRange($address)
 	{
 		$ip = $address;
@@ -31,18 +40,18 @@ public static function validIPRange($address)
 			// up (e.g. the web server's configuration)
 			if (preg_match('/^[0-9]{1,3}\z/', $subnet) !== 1 || (int) $subnet > $maxBits)
 			{
-				Factory::getApplication()->enqueueMessage(Text::sprintf('COM_BFSTOP_IP_INVALID_SUBNET', $subnet), 'warning');
+				Factory::getApplication()->enqueueMessage(Text::sprintf('COM_BFSTOP_IP_INVALID_SUBNET', self::e($subnet)), 'warning');
 				return false;
 			}
 		}
 		if (!filter_var($ip, FILTER_VALIDATE_IP))
 		{
-			Factory::getApplication()->enqueueMessage(Text::sprintf('COM_BFSTOP_IP_INVALID_ADDRESS', $ip), 'warning');
+			Factory::getApplication()->enqueueMessage(Text::sprintf('COM_BFSTOP_IP_INVALID_ADDRESS', self::e($ip)), 'warning');
 			return false;
 		}
 		if (!filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE))
 		{
-			Factory::getApplication()->enqueueMessage(Text::sprintf('COM_BFSTOP_IP_PRIVATE_OR_RESERVED', $ip), 'warning');
+			Factory::getApplication()->enqueueMessage(Text::sprintf('COM_BFSTOP_IP_PRIVATE_OR_RESERVED', self::e($ip)), 'warning');
 		}
 		return true;
 	}
@@ -86,7 +95,7 @@ public static function matchesCurrentIP($range)
 		$result = self::cidrMatch($curIP, $range);
 		if ($result)
 		{
-			Factory::getApplication()->enqueueMessage(Text::sprintf('COM_BFSTOP_IP_BLOCKS_USER', $range, $curIP), 'warning');
+			Factory::getApplication()->enqueueMessage(Text::sprintf('COM_BFSTOP_IP_BLOCKS_USER', self::e($range), self::e($curIP)), 'warning');
 		}
 		return $result;
 	}

admin/src/Helper/LinkHelper.php

@@ -16,7 +16,7 @@ class LinkHelper
 {
 	public static function getIpInfoLink($ipaddress)
 	{
-		$input = Factory::getApplication()->input;
+		$input = Factory::getApplication()->getInput();
 		$menuId = $input->get('Itemid', 0, 'INTEGER');
 		$link = 'index.php?option=com_bfstop&Itemid='.$menuId.'&view=ipinfo&ipaddress='.rawurlencode($ipaddress);
 		return $link;

admin/src/Helper/LogHelper.php

@@ -19,8 +19,10 @@ class LogHelper
 {
 	public static function getLogger()
 	{
+		// false if the plugin is disabled: the page of an unblock link and
+		// the lists still work then
 		$plugin = PluginHelper::getPlugin('system', 'bfstop');
-		$params = new Registry($plugin->params);
+		$params = new Registry($plugin ? $plugin->params : null);
 		$loglevel = $params->get('logLevel', Log::ERROR);
 		return new LoggerHelper($loglevel);
 	}

admin/src/Helper/ParamHelper.php

@@ -11,12 +11,13 @@
 defined('_JEXEC') or die;
 
 use Joomla\CMS\Factory;
+use Joomla\Database\DatabaseInterface;
 
 class ParamHelper
 {
 	public static function get($name, $column, $defaultValue)
 	{
-		$db = Factory::getDbo();
+		$db = Factory::getContainer()->get(DatabaseInterface::class);
 		$sql = "SELECT $column FROM #__extensions WHERE name = 'plg_system_bfstop'";
 		$db->setQuery($sql);
 		$rawSettings = $db->loadResult();

admin/src/Helper/UnblockHelper.php

@@ -47,6 +47,7 @@ public static function unblockDB($db, $ids, $source, $logger)
 		$unblockDate = date('Y-m-d H:i:s');
 		foreach ($ids as $id)
 		{
+			$curResult = false;
 			try
 			{
 				$id = (int) $id;

admin/src/Helper/VersionHelper.php

@@ -11,6 +11,7 @@
 defined('_JEXEC') or die;
 
 use Joomla\CMS\Factory;
+use Joomla\Database\DatabaseInterface;
 
 class VersionHelper
 {
@@ -31,7 +32,7 @@ public static function checkSameMajorMinor($version1, $version2)
 	// treat that as "not installed", not as an empty version string).
 	public static function getInstalledVersions()
 	{
-		$db = Factory::getDbo();
+		$db = Factory::getContainer()->get(DatabaseInterface::class);
 
 		$query = "SELECT manifest_cache FROM #__extensions WHERE name='plg_system_bfstop'";
 		$db->setQuery($query);

admin/src/Model/AllowlistModel.php

@@ -10,7 +10,7 @@
 
 defined('_JEXEC') or die;
 
-use Joomla\CMS\Factory;
+use Joomla\CMS\Language\Text;
 use Joomla\CMS\Log\Log;
 use Joomla\CMS\MVC\Model\ListModel;
 
@@ -28,7 +28,7 @@ public function __construct($config = array())
 
 	protected function getListQuery()
 	{
-		$db = Factory::getDbo();
+		$db = $this->getDatabase();
 		$query = $db->getQuery(true);
 		$query->select('a.id, a.ipaddress, a.notes');
 		$query->from('#__bfstop_allowlist a');
@@ -47,9 +47,14 @@ protected function populateState($ordering = null, $direction = null)
 
 	public function remove($ids, $logger)
 	{
+		$ids = array_values(array_filter(array_map('intval', (array) $ids)));
+		if (count($ids) === 0)
+		{
+			return Text::_('COM_BFSTOP_ALLOWLIST_NOTHING_REMOVED');
+		}
 		try
 		{
-			$db = Factory::getDbo();
+			$db = $this->getDatabase();
 			$query = $db->getQuery(true);
 			$conditions = array(
 				$db->quoteName('id').' IN ('.implode(", ", $ids).')'
@@ -58,10 +63,12 @@ public function remove($ids, $logger)
 			$query->where($conditions);
 			$db->setQuery($query);
 			$db->execute();
+			return Text::_('COM_BFSTOP_ALLOWLIST_REMOVED');
 		}
 		catch (\RuntimeException $e)
 		{
 			$logger->log($e->getMessage(), Log::ERROR);
+			return Text::_('COM_BFSTOP_ALLOWLIST_REMOVE_FAILED');
 		}
 	}
 }

admin/src/Model/BlocklistModel.php

@@ -11,7 +11,6 @@
 defined('_JEXEC') or die;
 
 use Codeling\Component\Bfstop\Administrator\Helper\UnblockHelper;
-use Joomla\CMS\Factory;
 use Joomla\CMS\Language\Text;
 use Joomla\CMS\MVC\Model\ListModel;
 
@@ -33,7 +32,7 @@ public function __construct($config = array())
 
 	protected function getListQuery()
 	{
-		$db = Factory::getDbo();
+		$db = $this->getDatabase();
 		$query = $db->getQuery(true);
 		$query->select('b.id, b.ipaddress, b.crdate, b.duration, b.attempts, b.last_attempt, u.crdate as unblocked');
 		$query->from('#__bfstop_bannedip b left join #__bfstop_unblock u on b.id=u.block_id');
@@ -58,7 +57,7 @@ protected function populateState($ordering = null, $direction = null)
 
 	public function unblock($ids, $logger)
 	{
-		if (UnblockHelper::unblockDB(Factory::getDbo(), $ids, 0, $logger))
+		if (UnblockHelper::unblockDB($this->getDatabase(), $ids, 0, $logger))
 		{
 			return Text::_("COM_BFSTOP_UNBLOCK_SUCCESS");
 		}