Look up the visitor in the block and allow list by index (65 ms -> 1 ms per request)

CHANGELOG

@@ -157,6 +157,15 @@
     are validated (no stream wrappers like phar://, a .mmdb file name)
   - Added SECURITY.md with the supported versions and how to report
     vulnerabilities
+  - Security: the check of each request against the block and allow list is
+    a lookup by index. It used to read all networks of both lists and test
+    each in PHP, for every request of every visitor: 65-70 ms with 170000
+    entries, now 1-2 ms. Addresses and networks are stored in a canonical
+    form (lower case, compressed, networks as their network address, like
+    "2001:db8:1::/48"); the component's forms do that, and the plugin
+    converts existing entries once, on the first request after the update.
+    Entries which are added to the tables by other means have to be in that
+    form as well
   - Security: the bundled MaxMind DB reader (maxmind-db/reader, now 1.14.0,
     which limits the decoding of malformed or hostile database files) is
     tracked in composer.json/composer.lock; src/Helper/Geo is generated from

sql/install.mysql.utf8.sql

@@ -21,7 +21,10 @@ CREATE TABLE IF NOT EXISTS #__bfstop_bannedip (
 -- latest one (issue #219); NULL = no attempt since the block started
 	attempts int unsigned NOT NULL DEFAULT 0,
 	last_attempt datetime NULL DEFAULT NULL,
-	PRIMARY KEY (id)
+	PRIMARY KEY (id),
+-- looked up for every request: addresses and networks are stored in
+-- canonical form (IpHelper::canonicalEntry())
+	KEY ipaddress (ipaddress)
 ) DEFAULT CHARSET=utf8mb4 DEFAULT COLLATE=utf8mb4_unicode_ci;
 
 
@@ -56,7 +59,8 @@ CREATE TABLE IF NOT EXISTS #__bfstop_allowlist (
 	id int(10) NOT NULL auto_increment,
 	ipaddress varchar(45) NOT NULL,
 	notes varchar(255) NOT NULL DEFAULT '',
-	PRIMARY KEY (id)
+	PRIMARY KEY (id),
+	KEY ipaddress (ipaddress)
 ) DEFAULT CHARSET=utf8mb4 DEFAULT COLLATE=utf8mb4_unicode_ci;
 
 

sql/install.postgresql.utf8.sql

@@ -30,6 +30,9 @@ CREATE TABLE IF NOT EXISTS "#__bfstop_bannedip" (
   "last_attempt" timestamp without time zone DEFAULT NULL,
   PRIMARY KEY ("id")
 );
+-- looked up for every request: addresses and networks are stored in canonical
+-- form (IpHelper::canonicalEntry())
+CREATE INDEX IF NOT EXISTS "#__bfstop_bannedip_ipaddress" ON "#__bfstop_bannedip" ("ipaddress");
 
 
 CREATE TABLE IF NOT EXISTS "#__bfstop_unblock" (
@@ -56,6 +59,7 @@ CREATE TABLE IF NOT EXISTS "#__bfstop_allowlist" (
   "notes" varchar(255) NOT NULL DEFAULT '',
   PRIMARY KEY ("id")
 );
+CREATE INDEX IF NOT EXISTS "#__bfstop_allowlist_ipaddress" ON "#__bfstop_allowlist" ("ipaddress");
 
 
 CREATE TABLE IF NOT EXISTS "#__bfstop_knownip" (

sql/updates/2.0.0.sql

@@ -61,3 +61,9 @@ INSERT INTO `#__bfstop_username_stats` (username, attempts, first_attempt, last_
 
 -- speeds up the per-username (account-level) throttle query
 ALTER TABLE `#__bfstop_failedlogin` ADD INDEX username_logtime (username, logtime);
+
+-- every request looks up the visitor's address in the block and allow list;
+-- addresses and networks are stored in canonical form (the plugin brings
+-- existing entries into it once, IpHelper::canonicalEntry())
+ALTER TABLE `#__bfstop_bannedip` ADD INDEX ipaddress (ipaddress);
+ALTER TABLE `#__bfstop_allowlist` ADD INDEX ipaddress (ipaddress);

src/Extension/Bfstop.php

@@ -717,6 +717,17 @@ public function onAfterInitialise($event)
 		{
 			return;
 		}
+		// lookups of the block and allow list need the addresses in canonical
+		// form, which entries of earlier versions are not in: once, before the
+		// first lookup of this version (every request has one, later). The
+		// installer script can't do this reliably - the class of the previous
+		// version may be loaded still
+		if (!$this->getBoolParam('addressesCanonical', false))
+		{
+			$this->mydb->canonicalizeStoredAddresses();
+			$this->mydb->savePluginSetting('addressesCanonical', 1);
+			$this->params->set('addressesCanonical', 1);
+		}
 		// periodic maintenance, at most once a day
 		$purgeInterval = 86400; // = 24*60*60 => one day
 		$lastPurge = $this->params->get('lastPurge', 0);

src/Helper/DatabaseHelper.php

@@ -206,37 +206,36 @@ public function getFormattedFailedList($ipAddress, $curTime, $interval)
 
 	/**
 	 * Loads the rows of $table (aliased t, needs id and ipaddress columns)
-	 * which match the given IP address, either literally or as part of a
-	 * stored IPv4/IPv6 subnet (CIDR notation). Only the literal match is done
-	 * in SQL; subnet entries are matched in PHP (IpHelper::isInSubnet), which
-	 * works the same on every database - the previous SQL implementation
-	 * relied on MySQL-only functions (INET_ATON, INET6_ATON, LOCATE, REGEXP,
-	 * ...), see issue #206, and was the source of #117, #134 and #142.
+	 * which contain the given IP address: the address itself or a network
+	 * (CIDR notation) it is part of. Addresses and networks are stored in
+	 * canonical form (IpHelper::canonicalEntry()), so the entries which can
+	 * match are known beforehand (IpHelper::lookupCandidates()), and the query
+	 * is a lookup by index. It used to read every network of the table and test
+	 * each in PHP, on every request of every visitor - slow once a few
+	 * thousand IPv6 clients were blocked, and a way for them to slow the site
+	 * down (the SQL was MySQL-only at first, see issue #206, and the source
+	 * of #117, #134 and #142).
 	 */
 	private function loadMatchingEntries($ipaddress, $table, $additionalWhere, $action)
 	{
+		$candidates = IpHelper::lookupCandidates($ipaddress);
+		if (count($candidates) === 0)
+		{
+			return array();
+		}
 		try
 		{
-			// LOWER: IPv6 addresses may be stored in upper case; MySQL's
-			// default collations compare case-insensitively, PostgreSQL doesn't
-			$sql = "SELECT t.id, t.ipaddress FROM ".$table." t WHERE ".
-				"((t.ipaddress NOT LIKE '%/%' AND LOWER(t.ipaddress) = LOWER(".$this->db->quote($ipaddress)."))".
-				" OR t.ipaddress LIKE '%/%')".
+			$sql = "SELECT t.id, t.ipaddress FROM ".$table." t WHERE t.ipaddress IN (".
+				implode(', ', array_map(array($this->db, 'quote'), $candidates)).")".
 				$additionalWhere;
 			$this->db->setQuery($sql);
-			$entries = array();
-			foreach ($this->db->loadObjectList() as $entry)
+			$entries = $this->db->loadObjectList();
+			foreach ($entries as $entry)
 			{
-				if (strpos($entry->ipaddress, '/') !== false &&
-					!IpHelper::isInSubnet($ipaddress, $entry->ipaddress))
-				{
-					continue;
-				}
 				$this->logger->log($action." because of entry: ".
 					"id=".$entry->id.", ".
 					"ipaddress=".$entry->ipaddress,
 					Log::DEBUG);
-				$entries[] = $entry;
 			}
 			return $entries;
 		}
@@ -247,6 +246,57 @@ private function loadMatchingEntries($ipaddress, $table, $additionalWhere, $acti
 		}
 	}
 
+	/**
+	 * Brings the addresses in the block and allow list into canonical form
+	 * (IpHelper::canonicalEntry()), which lookups depend on: entries written by
+	 * earlier versions, or by hand, may be spelled differently ("2001:DB8::1",
+	 * "203.0.113.5/24"). Entries which are no address or network are left alone.
+	 *
+	 * @return int the number of entries changed
+	 */
+	public function canonicalizeStoredAddresses()
+	{
+		$changed = 0;
+		try
+		{
+			foreach (array('#__bfstop_bannedip', '#__bfstop_allowlist') as $table)
+			{
+				$lastId = 0;
+				do
+				{
+					$query = $this->db->getQuery(true)
+						->select($this->db->quoteName(array('id', 'ipaddress')))
+						->from($this->db->quoteName($table))
+						->where($this->db->quoteName('id').' > '.(int) $lastId)
+						->order($this->db->quoteName('id').' ASC');
+					$this->db->setQuery($query, 0, 5000);
+					$rows = $this->db->loadObjectList();
+					foreach ($rows as $row)
+					{
+						$lastId = (int) $row->id;
+						$canonical = IpHelper::canonicalEntry($row->ipaddress);
+						if ($canonical !== null && $canonical !== $row->ipaddress)
+						{
+							$update = $this->db->getQuery(true)
+								->update($this->db->quoteName($table))
+								->set($this->db->quoteName('ipaddress').' = '.$this->db->quote($canonical))
+								->where($this->db->quoteName('id').' = '.(int) $row->id);
+							$this->db->setQuery($update);
+							$this->db->execute();
+							++$changed;
+						}
+					}
+				}
+				while (count($rows) === 5000);
+			}
+		}
+		catch (\Exception $e)
+		{
+			$this->logger->log("Database exception occured: ".$e->getMessage(), Log::ERROR);
+		}
+		return $changed;
+	}
+
 	/**
 	 * IDs of all currently active blocks matching the given IP address,
 	 * whether as single address or as part of a blocked IPv4/IPv6 subnet.
@@ -330,7 +380,7 @@ public function blockIP($logEntry, $duration, $usehtaccess, $htaccessPath)
 		try
 		{
 			$blockEntry = new \stdClass();
-			$blockEntry->ipaddress = $logEntry->ipaddress;
+			$blockEntry->ipaddress = IpHelper::canonicalEntry($logEntry->ipaddress) ?? $logEntry->ipaddress;
 			$blockEntry->crdate = date("Y-m-d H:i:s");
 			$blockEntry->duration = $duration;
 			if (!$this->db->insertObject('#__bfstop_bannedip', $blockEntry, 'id'))
@@ -854,13 +904,22 @@ public function purgeOldEntries($purgeAgeWeeks)
 
 	/**
 	 * Remembers when the periodic maintenance last ran.
+	 */
+	public function saveLastPurge($timestamp)
+	{
+		$this->savePluginSetting('lastPurge', (int) $timestamp);
+	}
+
+	/**
+	 * Stores one value in the plugin's settings (bookkeeping of the plugin,
+	 * not set on the settings page).
 	 *
 	 * Only this one value is changed in the plugin's stored settings, which
 	 * are read from the database right now: writing back the settings the
 	 * request was started with would undo whatever an administrator saved in
 	 * the meantime.
 	 */
-	public function saveLastPurge($timestamp)
+	public function savePluginSetting($name, $value)
 	{
 		try
 		{
@@ -880,7 +939,7 @@ public function saveLastPurge($timestamp)
 			{
 				$params = array();
 			}
-			$params['lastPurge'] = (int) $timestamp;
+			$params[$name] = $value;
 			// no table alias here: PostgreSQL doesn't allow qualified column
 			// names in the SET clause (issue #206)
 			$query = $where($this->db->getQuery(true)

src/Helper/IpHelper.php

@@ -93,14 +93,65 @@ public static function isValidIpOrSubnet($value)
 			(int) $parts[1] <= (strpos($parts[0], ':') !== false ? 128 : 32);
 	}
 
-	private static function prefixMask($bits)
+	private static function prefixMask($bits, $length = 16)
 	{
 		$mask = str_repeat("\xff", intdiv($bits, 8));
 		if ($bits % 8 > 0)
 		{
 			$mask .= chr((0xff << (8 - $bits % 8)) & 0xff);
 		}
-		return str_pad($mask, 16, "\x00");
+		return str_pad($mask, $length, "\x00");
+	}
+
+	/**
+	 * The one spelling in which addresses and networks are stored in the
+	 * block and allow list: an address in the compressed lower-case form of
+	 * inet_ntop(), a network as its masked network address and prefix length
+	 * ("2001:db8:1:2::/64", "203.0.113.0/24"). Lookups compare these strings
+	 * (see lookupCandidates()), so every writer has to use this form.
+	 *
+	 * @return string|null null if $value is neither an address nor a network
+	 */
+	public static function canonicalEntry($value)
+	{
+		$value = trim((string) $value);
+		if (!self::isValidIpOrSubnet($value))
+		{
+			return null;
+		}
+		$parts = explode('/', $value);
+		$bin = inet_pton($parts[0]);
+		if (count($parts) === 1)
+		{
+			return inet_ntop($bin);
+		}
+		$bits = (int) $parts[1];
+		return inet_ntop($bin & self::prefixMask($bits, strlen($bin))).'/'.$bits;
+	}
+
+	/**
+	 * All the entries (in canonical form) of the block or allow list which
+	 * contain $ip: the address itself and its network for every prefix length.
+	 * Fetching the rows whose address is one of these finds what the address is
+	 * blocked or allowed by, with an index and without looking at the other
+	 * entries - how many there are depends on what visitors made the plugin
+	 * block, and a request from any visitor has to look.
+	 *
+	 * @return string[] empty if $ip is not an address
+	 */
+	public static function lookupCandidates($ip)
+	{
+		$bin = @inet_pton(trim((string) $ip));
+		if ($bin === false)
+		{
+			return array();
+		}
+		$candidates = array(inet_ntop($bin));
+		for ($bits = 0; $bits <= strlen($bin) * 8; ++$bits)
+		{
+			$candidates[] = inet_ntop($bin & self::prefixMask($bits, strlen($bin))).'/'.$bits;
+		}
+		return $candidates;
 	}
 
 	/**
@@ -125,7 +176,9 @@ public static function trackedAddress($ip, $prefixLength)
 			substr($bin, 0, 2) === "\x20\x02" ||       // 6to4, 2002::/16
 			substr($bin, 0, 4) === "\x20\x01\x00\x00")  // Teredo, 2001::/32
 		{
-			return $ip;
+			// in the form the block list holds it in, whatever way the web
+			// server spells the address (garbage is returned as it is)
+			return self::canonicalEntry($ip) ?? $ip;
 		}
 		return inet_ntop($bin & self::prefixMask($prefixLength)).'/'.$prefixLength;
 	}

tests/Integration/ComponentTest.php

@@ -422,4 +422,16 @@ public function testEditViewsRegisterTheirStylesheetsAsRealFiles()
 			$this->assertStringEndsWith('/edit.css', $uri, $view);
 		}
 	}
+
+	public function testEntriesOfTheEditFormsAreStoredInCanonicalForm()
+	{
+		$blockId = $this->saveNew(BlockModel::class,
+			array('ipaddress' => '203.0.113.77/24', 'crdate' => '2026-01-02', 'duration' => '0'));
+		$allowId = $this->saveNew(AllowModel::class, array('ipaddress' => '2001:DB8::ABCD', 'notes' => ''));
+		$this->assertSame('203.0.113.0/24', $this->queryValue('SELECT ipaddress FROM #__bfstop_bannedip WHERE id='.$blockId));
+		$this->assertSame('2001:db8::abcd', $this->queryValue('SELECT ipaddress FROM #__bfstop_allowlist WHERE id='.$allowId));
+		$helper = new DatabaseHelper($this->logger);
+		$this->assertTrue($helper->isIPBlocked('203.0.113.200'));
+		$this->assertTrue($helper->isIPOnAllowList('2001:db8::ABCD'));
+	}
 }