diff --git a/CHANGELOG b/CHANGELOG index af9b78d..1a14bfd 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -40,6 +40,108 @@ then silently never blocked anything. IP subnet matching for block and allow list entries is now done in PHP instead of with MySQL-only SQL functions, and fixed-length time windows are computed in PHP + - Security: an emailed unblock token no longer lets a blocked IP address + bypass the block. It was accepted on any request with + view=tokenunblock (e.g. a login), was never used up by such a request + and was not checked for expiry, so anyone who got a token (e.g. by + getting their own account blocked) could keep guessing passwords from + a blocked IP. Now it only passes the unblock page of com_bfstop itself, + only while unexpired, and only if issued for a block of the requesting + IP address; the token is no longer written to the log either + - Security: behind a proxy/load balancer, the client IP is now taken from + the end of the forwarding header (the entry added by the trusted proxy) + instead of its first public entry, which a visitor could forge to evade + blocking, get other addresses blocked or impersonate an allowlisted one. + Several trusted proxies (and CIDR subnets) can be configured, the + "Forwarded" header (RFC 7239) and addresses with ports are understood, + and private client addresses reported by the proxy are accepted. Not + all proxies append to an incoming header: pick a header yours sets or + appends to + - Security: a failed login is now recorded, counted and checked against + the block threshold before the configured delay is applied, not after + it, and a request whose IP address just got (or already is) blocked is + not delayed any more. Before, a burst of parallel login attempts all + ran before the first of them was counted, and every one of them held + on to a server worker for the whole delay + - Security: usernames of failed logins which do not belong to an existing + account (and are not on the list of common usernames) are now stored, + shown, logged and mailed as an unreadable hash by default, as users + often type their password into the username field; the hash is the same + for the same input, so attempts are still counted together. The new + setting "Usernames Not Matching an Account" restores the old behaviour. + Entries stored by earlier versions stay as they are - purge them in the + failed login and username statistics views if you want them gone + - Security: unblocking by the link in the notification email now needs a + confirmation (a POST request) and only works from the IP address which + was blocked, so mail security scanners or link previews which open + every link in a message can't unblock an attacker (see com_bfstop) + - Security: the "User Block Message" with the unblock link is no longer + sent if the failed logins from the blocked IP address which led to the + block also went against other existing accounts. The link goes to the + owner of the account of the last attempt, so an attacker with an account + of their own could otherwise end every series of guesses at other + accounts with an attempt at their own, get the link and continue. + Attempts against names which don't exist (e.g. typos) don't count + - Security: anybody can make the plugin send the "User Block Message" to + a user, by failing to log in with that username from an address until it + is blocked - a way to flood somebody's inbox. The setting now has three + values: disabled; enabled only if the user has logged in from the + blocked IP address (or, for IPv6, its network) before, which is the + case for somebody locking themselves out but never for an attacker's + address; and enabled for any IP address, where a user gets at most one + unblock link at a time which can still be used (not yet expired or used + up). Note that the previous "enabled" now means the first of these; the + old behaviour is the third. Known IP addresses are recorded at + successful logins, so users get the message again after having logged + in once. The username a link was sent to is recorded with its token + - The addresses users logged in from (used for the risk score and for the + unblock message) are remembered by network for IPv6 clients, as failed + logins are, so a client whose address changes within its /64 every day + stays a known one and doesn't add a row per address; the lookup ignores + the case of the username. They are forgotten a year after the last login + from them (and the table is limited to 100000 rows, oldest first). This + maintenance, like the limit of the username statistics, runs once a day + whatever the purge age is set to + - Unblock tokens which have expired are deleted by the daily maintenance too + (before: only when somebody visited the unblock page, or by the purge by + age if one is configured). The status code of the block page is set with + http_response_code() instead of a raw "HTTP/1.0 403" header, which hard-coded + the protocol version + - The unblock page of com_bfstop answers with an error status if the link + can't be used (400 without a token, 403 from another IP address, 404 for + an unknown, expired or used token, 500 if unblocking failed), unless "Use + HTTP Error" is switched off; opening the link (a GET request) stays a 200 + for every token. The page is not cached and tells browsers not to send + its address (which contains the token) to other sites and search engines + not to index it + - Security: IPv6 clients are now tracked per network (default: /64, see + the new "IPv6 Tracking Granularity" setting) instead of per address: + failed logins are counted and blocks apply to the whole network, so an + attacker can no longer start over by switching to another address in + their /64. IPv4-mapped, NAT64, 6to4 and Teredo addresses, which embed + unrelated clients' IPv4 addresses, are still tracked individually + - Security: the block page is never cached (Cache-Control: no-store) and, + if no setting says otherwise, answers with the HTTP status 403 instead of + 200, so a caching proxy/CDN can't serve it to other visitors + - Security: only a valid IP address or subnet is written to the .htaccess + file (before, a value like "192.0.2.0/1e1" passed the backend validation + and made Apache reject the whole file); the 403 message is escaped; and + changes of the file are done while holding a lock, so that clients + getting blocked at the same time don't overwrite each other's lines + - Security: the plugin's log file is rotated at 5 MB, and the username + statistics (which aren't purged by age) are limited to 10000 usernames, + removing those with the fewest attempts first, so a visitor can't fill up + disk or database by making up usernames + - Security: the periodic maintenance (once a day) only updates its own + timestamp in the plugin's stored settings, instead of writing back all + settings as they were when the request started, which could undo a + change an administrator saved in the meantime; and it now also runs if + no purge age is configured + - Security (component): sending the test email now needs the permission to + change the settings; the .htaccess path and GeoIP database path settings + are validated (no stream wrappers like phar://, a .mmdb file name) + - Added SECURITY.md with the supported versions and how to report + vulnerabilities 1.5.2 (2024-02-18) - Note: Only component changes, no plugin changes diff --git a/SECURITY.md b/SECURITY.md index 67fb6e9..6350ec2 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -1 +1,37 @@ -With information on security vulnerabilities, please contact me via security@bfstop.de. +# Security Policy + +## Supported versions + +Security fixes are made for the latest release of the BFStop plugin +(`plg_system_bfstop`) and the BFStop component (`com_bfstop`). The two are +released together and need to be on the same major and minor version, so +please update both. + +## Reporting a vulnerability + +Please report suspected vulnerabilities by email to **security@bfstop.de** +rather than in a public issue or pull request, so that a fix can be +available before details are public. + +It helps to include: + +- the affected version(s) of plugin and component, and the Joomla! and PHP + versions, +- the relevant settings (blocking mode, proxy settings, ...), +- steps to reproduce, or a proof of concept, and what an attacker gains. + +## What is and isn't in scope + +BFStop sits in front of the login of a Joomla! site and decides which +requests to reject, so these are the kind of reports which are most useful: + +- ways around a block or the failed-login counting (for example by faking + the client address, or by reusing an unblock link), +- ways to get someone else blocked, or to make the plugin tie up the server, +- ways to read or change data (settings, blocks, logged usernames) without + the necessary permissions, and cross-site scripting in the backend views. + +Settings are changed by users with the `core.admin` permission for +`com_bfstop`. They can, among other things, choose the directory in which the +plugin writes its `.htaccess` file, so grant that permission only to people +you trust with the web server's configuration for your site. diff --git a/language/de-DE/de-DE.plg_system_bfstop.sys.ini b/language/de-DE/de-DE.plg_system_bfstop.sys.ini index 2f11ec7..658e7a8 100644 --- a/language/de-DE/de-DE.plg_system_bfstop.sys.ini +++ b/language/de-DE/de-DE.plg_system_bfstop.sys.ini @@ -27,9 +27,11 @@ PLG_SYSTEM_BFSTOP_BLOCK_DURATION_DESC="Bestimmt wie lange die IP-Sperre aufrecht PLG_SYSTEM_BFSTOP_NOTIFY_REMAINING_ATTEMPTS_LABEL="Übrige Versuche" PLG_SYSTEM_BFSTOP_NOTIFY_REMAINING_ATTEMPTS_DESC="Schaltet die Benachrichtigung über verbleibende Versuche ein; wenn aktiviert, wird im Login-Formular nach einem fehlgeschlagenen Login-Versuch eine Nachricht mit den verbleibenden Login-Versuchen angezeigt. Diese Einstellung wird nur dann berücksichtigt, wenn Sperren eingeschaltet ist (wenn es nicht eingeschaltet ist, gibt es ja auch keine 'verbleibenden' Versuche)" PLG_SYSTEM_BFSTOP_NOTIFY_BLOCKED_USER_LABEL="Benutzer-Block-Nachricht" -PLG_SYSTEM_BFSTOP_NOTIFY_BLOCKED_USER_DESC="Wenn eingeschaltet, wird an die in der Datenbank hinterlegte Email-Adresse eine Nachricht hinterlegt, wenn eine IP-Adresse gesperrt wird (sofern der Benutzername, mit dem das Login versucht wurde, ein existierender war)" +PLG_SYSTEM_BFSTOP_NOTIFY_BLOCKED_USER_DESC="Wenn eingeschaltet, erhält ein Benutzer, dessen Benutzername bei fehlgeschlagenen Logins von einer später gesperrten IP-Adresse verwendet wurde, eine Email mit einem Link zum Entsperren dieser Adresse. Es wird nichts gesendet, wenn der Benutzername nicht existiert oder wenn die fehlgeschlagenen Logins von dieser Adresse auch gegen andere existierende Konten gerichtet waren (der Link darf nicht an den Inhaber eines Kontos gehen, wenn von dort aus andere angegriffen werden). Jeder kann eine solche Email auslösen, indem er sich erfolglos als dieser Benutzer anmeldet: 'Nur wenn sich der Benutzer schon von dieser IP-Adresse aus angemeldet hat' (empfohlen) verhindert, dass dies gegen deine Benutzer eingesetzt wird; wird die Email für jede IP-Adresse gesendet, bekommt ein Benutzer höchstens einen noch verwendbaren Link auf einmal." +PLG_SYSTEM_BFSTOP_NOTIFY_BLOCKED_USER_KNOWN="eingeschaltet, nur wenn sich der Benutzer schon von dieser IP-Adresse aus angemeldet hat (empfohlen)" +PLG_SYSTEM_BFSTOP_NOTIFY_BLOCKED_USER_ANY="eingeschaltet, für jede IP-Adresse (jeder kann damit eine Email an einen Benutzer auslösen)" PLG_SYSTEM_BFSTOP_USE_HTTP_ERROR_LABEL="Verwende HTTP-Fehler" -PLG_SYSTEM_BFSTOP_USE_HTTP_ERROR_DESC="Wenn aktiviert, wird der HTTP Statuscode 403 (Forbidden) gesendet wenn die IP-Addresse eines Benutzers gesperrt ist." +PLG_SYSTEM_BFSTOP_USE_HTTP_ERROR_DESC="Wenn aktiviert (empfohlen), beantwortet bfstop Anfragen einer gesperrten IP-Adresse mit dem HTTP-Statuscode 403 (Forbidden) und die Entsperrseite des Links in der Benachrichtigungs-Email mit einem Fehlerstatus, wenn der Link nicht verwendet werden kann (400, 403, 404 oder 500). Deaktiviere dies nur, wenn dein Webserver, CDN oder deine Firewall Fehlerseiten durch eigene ersetzt und Benutzer die Meldung deshalb nie zu sehen bekommen: Sie wird dann mit dem Statuscode 200 gesendet. In beiden Fällen werden die Seiten nie zwischengespeichert." PLG_SYSTEM_BFSTOP_ENABLED="aktiviert" PLG_SYSTEM_BFSTOP_DISABLED="deaktiviert" PLG_SYSTEM_BFSTOP_NOTIFY_UNLIMITED="unbegrenzt" diff --git a/language/en-GB/en-GB.plg_system_bfstop.sys.ini b/language/en-GB/en-GB.plg_system_bfstop.sys.ini index 1bfce70..7a2b7d2 100644 --- a/language/en-GB/en-GB.plg_system_bfstop.sys.ini +++ b/language/en-GB/en-GB.plg_system_bfstop.sys.ini @@ -29,9 +29,11 @@ PLG_SYSTEM_BFSTOP_BLOCK_DURATION_DESC="Determines how long an IP address is bloc PLG_SYSTEM_BFSTOP_NOTIFY_REMAINING_ATTEMPTS_LABEL="Remaining attempts" PLG_SYSTEM_BFSTOP_NOTIFY_REMAINING_ATTEMPTS_DESC="Enables or disables notifications about remaining attempts; if enabled, the user will, after each failed login attempt, see a message in the login form about how many login attempts still are available before the IP address will be blocked. This setting is only active if blocking is enabled (if blocking is disabled, 'remaining' entries wouldn't make sense)." PLG_SYSTEM_BFSTOP_NOTIFY_BLOCKED_USER_LABEL="User Block Message" -PLG_SYSTEM_BFSTOP_NOTIFY_BLOCKED_USER_DESC="When turned on, in case of a blocked IP address, a notification will be sent to the user which tried to log in (only of course in case it is an existing username)." +PLG_SYSTEM_BFSTOP_NOTIFY_BLOCKED_USER_DESC="When turned on, a user whose username was used in failed logins from an IP address which then got blocked receives an email with a link to unblock that address. Nothing is sent if the username does not exist, or if the failed logins from that address also went against other existing accounts (the link must not go to the owner of one account if somebody is attacking others from there). Anybody can cause such an email by failing to log in as that user: 'only if the user has logged in from that IP address before' (recommended) keeps this from being used against your users; if emails are sent for any IP address, at most one link which can still be used is sent to a user at a time." +PLG_SYSTEM_BFSTOP_NOTIFY_BLOCKED_USER_KNOWN="enabled, only if the user has logged in from that IP address before (recommended)" +PLG_SYSTEM_BFSTOP_NOTIFY_BLOCKED_USER_ANY="enabled, for any IP address (anybody can make this send an email to a user)" PLG_SYSTEM_BFSTOP_USE_HTTP_ERROR_LABEL="Use HTTP Error" -PLG_SYSTEM_BFSTOP_USE_HTTP_ERROR_DESC="If enabled, bfstop will send a status code 403 (Forbidden) if the users' IP address is blocked" +PLG_SYSTEM_BFSTOP_USE_HTTP_ERROR_DESC="If enabled (recommended), bfstop answers requests from a blocked IP address with the status code 403 (Forbidden), and the unblock page of the link in the notification email with an error status when the link can't be used (400, 403, 404 or 500). Switch this off only if your web server, CDN or firewall replaces error pages with its own, so that users never get to see the message: it is then sent with status 200 instead. Either way, the pages are never cached." PLG_SYSTEM_BFSTOP_ENABLED="enabled" PLG_SYSTEM_BFSTOP_DISABLED="disabled" PLG_SYSTEM_BFSTOP_NOTIFY_UNLIMITED="unlimited" @@ -76,9 +78,9 @@ PLG_SYSTEM_BFSTOP_PROXY_FIELDSET_LABEL="Proxy / Load Balancer" PLG_SYSTEM_BFSTOP_USE_PROXY_LABEL="Behind a proxy / load balancer" PLG_SYSTEM_BFSTOP_USE_PROXY_DESC="Enable this only if your site sits behind a reverse proxy or load balancer, so that the visitor's browser never connects to Joomla! directly. When enabled, bfstop reads the visitor's IP address from the header selected below instead of from the direct TCP connection - but only for requests that actually come from the proxy IP address configured below. Leave disabled unless you have such a setup: any visitor can forge this header, so trusting it without a verified proxy IP would let an attacker get an arbitrary IP address blocked instead of their own." PLG_SYSTEM_BFSTOP_PROXY_HEADER_SOURCE_LABEL="Client IP header" -PLG_SYSTEM_BFSTOP_PROXY_HEADER_SOURCE_DESC="The HTTP header your proxy/load balancer uses to report the original client IP address. Check your proxy's documentation if unsure - X-Forwarded-For is the most common choice." +PLG_SYSTEM_BFSTOP_PROXY_HEADER_SOURCE_DESC="The HTTP header your proxy/load balancer uses to report the original client IP address. Check your proxy's documentation if unsure - X-Forwarded-For is the most common choice. The client address is taken from the end of the header, i.e. the entry added by your own proxy; entries further left are supplied by the client and ignored. Choose a header your proxy sets or appends to itself." PLG_SYSTEM_BFSTOP_PROXY_IP_LABEL="Proxy / load balancer IP address" -PLG_SYSTEM_BFSTOP_PROXY_IP_DESC="The IP address of your reverse proxy or load balancer, i.e. the address Joomla! sees as REMOTE_ADDR for every legitimate request. The client IP header above is only trusted for requests coming from this address, so that a visitor cannot bypass the proxy and spoof the header directly. Required if 'Behind a proxy / load balancer' is enabled above." +PLG_SYSTEM_BFSTOP_PROXY_IP_DESC="The IP address of your reverse proxy or load balancer, i.e. the address Joomla! sees as REMOTE_ADDR for every legitimate request. If there are several proxies in a row (e.g. a CDN in front of a load balancer), list all of them, separated by commas; subnets in CIDR notation (e.g. 10.0.0.0/8) are allowed too. The client IP header above is only trusted for requests coming from one of these addresses, so that a visitor cannot bypass the proxy and spoof the header directly. Required if 'Behind a proxy / load balancer' is enabled above." PLG_SYSTEM_BFSTOP_GROUP_NOTIFICATION_ENABLED_LABEL="User group notification" PLG_SYSTEM_BFSTOP_GROUP_NOTIFICATION_ENABLED_DESC="If enabled, the whole user group selected below will be notified in case of failed logins/banned IPs" PLG_SYSTEM_BFSTOP_USERGROUP_LABEL="User group" diff --git a/sql/install.mysql.utf8.sql b/sql/install.mysql.utf8.sql index 5937842..b005fb0 100644 --- a/sql/install.mysql.utf8.sql +++ b/sql/install.mysql.utf8.sql @@ -41,10 +41,12 @@ CREATE TABLE IF NOT EXISTS #__bfstop_unblock ( -- stores randomized tokens for unblocking an IP via an email -- to the blocked user +-- username: the user the unblock link was sent to, to limit the links per user CREATE TABLE IF NOT EXISTS #__bfstop_unblock_token ( token varchar(40) NOT NULL, block_id int(10) NOT NULL, crdate datetime NOT NULL, + username varchar(150) NULL DEFAULT NULL, PRIMARY KEY (token) ) DEFAULT CHARSET=utf8mb4 DEFAULT COLLATE=utf8mb4_unicode_ci; diff --git a/sql/install.postgresql.utf8.sql b/sql/install.postgresql.utf8.sql index f63d4fc..f5adb44 100644 --- a/sql/install.postgresql.utf8.sql +++ b/sql/install.postgresql.utf8.sql @@ -40,10 +40,12 @@ CREATE TABLE IF NOT EXISTS "#__bfstop_unblock" ( ); +-- username: the user the unblock link was sent to, to limit the links per user CREATE TABLE IF NOT EXISTS "#__bfstop_unblock_token" ( "token" varchar(40) NOT NULL, "block_id" integer NOT NULL, "crdate" timestamp without time zone NOT NULL, + "username" varchar(150) NULL DEFAULT NULL, PRIMARY KEY ("token") ); diff --git a/sql/updates/2.0.0.sql b/sql/updates/2.0.0.sql index f1c2f3a..a076ba0 100755 --- a/sql/updates/2.0.0.sql +++ b/sql/updates/2.0.0.sql @@ -17,6 +17,9 @@ ALTER TABLE `#__bfstop_unblock` CONVERT TO CHARACTER SET utf8mb4 COLLATE utf8mb4 ALTER TABLE `#__bfstop_unblock_token` CONVERT TO CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci; +-- the user an unblock link was sent to, to limit the links per user +ALTER TABLE `#__bfstop_unblock_token` ADD COLUMN username varchar(150) NULL DEFAULT NULL; + ALTER TABLE `#__bfstop_allowlist` CONVERT TO CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci; -- tables introduced in 2.0.0, see install.mysql.utf8.sql for details diff --git a/src/Extension/Bfstop.php b/src/Extension/Bfstop.php index dd828ab..0535c15 100644 --- a/src/Extension/Bfstop.php +++ b/src/Extension/Bfstop.php @@ -16,6 +16,7 @@ use Codeling\Plugin\System\Bfstop\Helper\NotifierHelper; use Codeling\Plugin\System\Bfstop\Helper\RiskHelper; use Codeling\Plugin\System\Bfstop\Helper\TokenHelper; +use Codeling\Plugin\System\Bfstop\Helper\UsernameHelper; use Joomla\CMS\Language\Text; use Joomla\CMS\Log\Log; use Joomla\CMS\Plugin\CMSPlugin; @@ -27,9 +28,16 @@ class Bfstop extends CMSPlugin implements SubscriberInterface { protected $autoloadLanguage = true; + // values of the "notifyBlockedUser" setting besides 0 (off) and 1 (only to + // users who logged in from the blocked address before) + private const NotifyBlockedAnyAddress = 2; + private LoggerHelper $logger; private NotifierHelper $notifier; private DatabaseHelper $mydb; + // the client's actual address while handling a failed login; the failed + // login entry holds the key it is tracked under instead (IPv6: its network) + private ?string $clientAddress = null; public static function getSubscribedEvents(): array { @@ -66,10 +74,10 @@ private static function endsWith($haystack, $needle) return (substr($haystack, -$length) === $needle); } - private function getUnblockLink($id) + private function getUnblockLink($id, $username) { $token = $this->mydb->getNewUnblockToken($id, - TokenHelper::getToken($this->logger)); + TokenHelper::getToken($this->logger), $username); $link = 'index.php?option=com_bfstop'. '&view=tokenunblock'. '&token='.$token; @@ -97,7 +105,7 @@ private function block($logEntry, $duration) } // if the IP address is blocked we actually shouldn't be here in // the first place I guess, but just to make sure - if ($this->mydb->isIPBlocked($logEntry->ipaddress)) + if ($this->mydb->isIPBlocked($this->clientAddress ?? $logEntry->ipaddress)) { $this->logger->log('IP '.$logEntry->ipaddress. ' is already blocked!', Log::ERROR); @@ -140,6 +148,12 @@ private function block($logEntry, $duration) $this->logger->log('htaccessPath empty, setting it to '.JPATH_ROOT, Log::INFO); $htaccessPath = JPATH_ROOT; } + // has to be found out before blocking, which marks these failed logins + // as handled + $targetedOtherAccounts = $this->getBoolParam('notifyBlockedUser', false) && + $this->mydb->hasFailedLoginsForOtherAccounts( + $this->getRealDurationFromDBDuration($this->getIntParam('checkInterval', NotifierHelper::$ONE_DAY)), + $logEntry->ipaddress, $logEntry->username, $logEntry->logtime); $id = $this->mydb->blockIP($logEntry, $duration, $usehtaccess, $htaccessPath); $this->logger->log('Inserted IP address '.$logEntry->ipaddress. @@ -151,7 +165,27 @@ private function block($logEntry, $duration) if ($this->getBoolParam('notifyBlockedUser', false)) { $userEmail = $this->mydb->getUserEmailByName($logEntry->username); - if ($userEmail != null) + if ($userEmail != null && $targetedOtherAccounts) + { + // The link unblocks this IP address, and it goes to the owner + // of the account of the last failed login. If the attempts + // which got the address blocked also went against other + // accounts, whoever caused that could just as well own this + // account - and would get to continue with the other + // accounts by following the link. + $this->logger->log("Existing user '". + $logEntry->username."' was blocked, but the failed ". + "logins from this address also targeted other accounts - ". + "not sending unblock instructions", + Log::INFO); + } + elseif ($userEmail != null && ($withheld = $this->whyNoUnblockMail($logEntry)) !== null) + { + $this->logger->log("Existing user '".$logEntry->username. + "' was blocked, but ".$withheld." - not sending unblock instructions", + Log::INFO); + } + elseif ($userEmail != null) { $this->logger->log("Existing user '". $logEntry->username. @@ -159,7 +193,7 @@ private function block($logEntry, $duration) "instructions", Log::INFO); $this->notifier->sendUnblockMail($userEmail, - $this->getUnblockLink($id)); + $this->getUnblockLink($id, $logEntry->username)); } else { @@ -171,6 +205,33 @@ private function block($logEntry, $duration) } } + /** + * Anybody can make the plugin send an unblock email to a user: by failing + * to log in with that username from an IP address until it is blocked. + * To keep that from being used to flood somebody's inbox: + * + * - by default (mode 1) the email is only sent if the user has logged in + * from the blocked IP address before - which is where somebody + * locking themselves out comes from, but never an attacker's address; + * - if it is to go to any address (mode 2), at most one email which can + * still be used is out for a user at any time. + * + * @return string|null why no email is sent, null if it may be + */ + private function whyNoUnblockMail($logEntry) + { + if ($this->getIntParam('notifyBlockedUser', 0) === self::NotifyBlockedAnyAddress) + { + return $this->mydb->hasCurrentUnblockToken($logEntry->username) + ? 'there already is an unblock link for this user which can be used' + : null; + } + return $this->mydb->hasLoggedInFrom($this->clientAddress ?? $logEntry->ipaddress, + $logEntry->username, $this->getIntParam('ipv6PrefixLength', 64)) + ? null + : 'the user has never logged in from this IP address'; + } + private function getRealDurationFromDBDuration($duration) { return ($duration <= 0) @@ -361,6 +422,25 @@ private function determineBaseDelayDuration() return $delayDuration; } + private function trackedAddress($ipAddress) + { + return IpHelper::trackedAddress($ipAddress, $this->getIntParam('ipv6PrefixLength', 64)); + } + + /** + * The form in which the username of a failed login is stored, mailed and + * logged, see UsernameHelper. + */ + private function usernameForStorage($username) + { + $mode = $this->getStringParam('unknownUsernameMode', UsernameHelper::ModeHash); + $isReadable = ($mode !== UsernameHelper::ModePlain) && + ($this->mydb->accountExists($username) || + RiskHelper::isCommonUsername($this->params, $username)); + return UsernameHelper::forStorage($username, $mode, $isReadable, + $this->getApplication()->get('secret', '')); + } + public function onUserLoginFailure($event) { $user = $event->getArgument(0); @@ -383,23 +463,23 @@ public function onUserLoginFailure($event) $username = mb_strimwidth($user['username'], 0, 150, "..."); $riskScore = RiskHelper::computeScore($this->mydb, $this->logger, $this->params, $ipAddress, $username); - $delayDuration = $this->determineDelayDuration($riskScore); - if ($delayDuration != 0) - { - sleep((int) round($delayDuration)); - } - $logEntry = new \stdClass(); $logEntry->id = null; - $logEntry->ipaddress = $ipAddress; + $this->clientAddress = $ipAddress; + $logEntry->ipaddress = $this->trackedAddress($ipAddress); $logEntry->logtime = date("Y-m-d H:i:s"); - $logEntry->username = $username; + $logEntry->username = $this->usernameForStorage($username); $logEntry->origin = $this->getApplication()->getClientId(); $this->logger->log('Failed login attempt from IP address '. $logEntry->ipaddress, Log::DEBUG); - // insert into log: + // Everything is recorded and evaluated *before* the delay below. The + // password has been checked by now, so the delay only holds back the + // response; if the attempt was only counted after the sleep, a burst + // of parallel requests would all run before the first of them + // reached the block threshold, and each of them would tie up a + // worker for the whole delay. $this->mydb->insertFailedLogin($logEntry); $this->notifyOfRemainingAttempts($logEntry, $riskScore); @@ -408,6 +488,23 @@ public function onUserLoginFailure($event) $this->notifier->failedLogin($logEntry, $maxNumber); $this->blockIfTooManyAttempts($logEntry, $riskScore); $this->accountThrottleIfNeeded($logEntry); + + $delayDuration = $this->determineDelayDuration($riskScore); + if ($delayDuration != 0) + { + // a blocked address gets nothing more out of a slow response; + // don't let it hold on to a worker (attackers could otherwise use + // the delay to exhaust the server's workers) + if ($this->mydb->isIPBlocked($ipAddress)) + { + $this->logger->log('Not delaying the response, IP address '. + $ipAddress.' is blocked', Log::DEBUG); + } + else + { + sleep((int) round($delayDuration)); + } + } } public function onUserLogin($event) @@ -423,21 +520,36 @@ public function onUserLogin($event) $info->username = $user['username']; $this->logger->log('Successful login by '.$info->username. ' from IP address '.$info->ipaddress, Log::DEBUG); - $this->mydb->successfulLogin($info); + $this->mydb->successfulLogin($info, $this->trackedAddress($info->ipaddress)); } - private function isUnblockRequest() + /** + * A blocked IP may only reach com_bfstop's unblock page, and only with an + * unexpired token that was issued for one of *its own* blocks. The pass + * has to stay this narrow: the request is the one that consumes the token, + * so it must really be the unblock view (no task, no other component). + * Letting any request with view=tokenunblock through would turn a single + * valid token - which the owner of any account can get emailed by getting + * themselves blocked - into a reusable key around the block for logins + * (e.g. option=com_users&task=user.login&view=tokenunblock&token=...). + */ + private function isUnblockRequest($blockIds) { $input = $this->getApplication()->input; - $view = $input->getString('view', ''); + if (strcmp($input->getCmd('option', ''), 'com_bfstop') != 0 || + strcmp($input->getCmd('view', ''), 'tokenunblock') != 0 || + $input->getCmd('task', '') !== '') + { + return false; + } $token = $input->getString('token', ''); - $result = (strcmp($view, "tokenunblock") == 0 && - $this->mydb->unblockTokenExists($token)); + $result = $this->mydb->unblockTokenValidForBlocks($token, $blockIds); if ($result) { - $this->logger->log('Seeing valid unblock token ('. - $token.'), letting the request pass through '. - 'to com_bfstop', + // deliberately not logging the token: it is a bearer secret and + // the log is readable in the backend + $this->logger->log('Seeing a valid unblock token for this IP '. + 'address, letting the request pass through to com_bfstop', Log::INFO); } return $result; @@ -500,21 +612,51 @@ public function onAfterInitialise($event) { return; } - $purgeAge = $this->getIntParam('deleteOld', 0); - if ($purgeAge > 0) + // periodic maintenance, at most once a day + $purgeInterval = 86400; // = 24*60*60 => one day + $lastPurge = $this->params->get('lastPurge', 0); + $now = time(); + if ($now > ($lastPurge + $purgeInterval)) { - $purgeInterval = 86400; // = 24*60*60 => one day - $lastPurge = $this->params->get('lastPurge', 0); - $now = time(); - if ($now > ($lastPurge + $purgeInterval)) + $purgeAge = $this->getIntParam('deleteOld', 0); + if ($purgeAge > 0) { $this->mydb->purgeOldEntries($purgeAge); - $this->params->set('lastPurge', $now); - $this->mydb->saveParams($this->params); } + // regardless of the purge age setting: these are not deleted by age + $this->mydb->trimUsernameStats(); + $this->mydb->pruneKnownIps(); + $this->mydb->purgeExpiredUnblockTokens(); + $this->params->set('lastPurge', $now); + $this->mydb->saveLastPurge($now); } } + /** + * The status code of the page shown to a blocked client, or null for none + * (which makes it 200). Set with http_response_code(), not as a raw + * "HTTP/1.x 403 ..." header: that would hard-code the protocol version, + * which is the web server's business (HTTP/2 doesn't even have a status line). + */ + public static function blockedResponseCode($useHttpError) + { + return $useHttpError ? 403 : null; + } + + /** + * The headers of the page shown to a blocked client. + * + * It must never be cached: a cache (browser, proxy, CDN) which kept it + * would show it to other visitors of the same URL. + */ + public static function blockedResponseHeaders() + { + return array( + 'Cache-Control: no-store, no-cache, must-revalidate, private', + 'Pragma: no-cache', + ); + } + /** * Enforcing the block happens after routing, not in onAfterInitialise: * the exemptions below look at option/view/task, which with @@ -545,7 +687,7 @@ public function onAfterRoute($event) $this->mydb->getClientString( $this->getApplication()->getClientId()), Log::INFO); - if ($this->isUnblockRequest()) + if ($this->isUnblockRequest($blockIds)) { return; } @@ -561,9 +703,14 @@ public function onAfterRoute($event) return; } $this->mydb->recordBlockedAttempt($blockIds); - if ($this->getBoolParam('useHttpError', false)) + $status = self::blockedResponseCode($this->getBoolParam('useHttpError', true)); + if ($status !== null) + { + http_response_code($status); + } + foreach (self::blockedResponseHeaders() as $header) { - header('HTTP/1.0 403 Forbidden'); + header($header); } $message = $this->params->get('blockedMessage', Text::_('PLG_SYSTEM_BFSTOP_BLOCKED_IP_MESSAGE')); diff --git a/src/Helper/DatabaseHelper.php b/src/Helper/DatabaseHelper.php index 6d4691a..dcf0d7e 100644 --- a/src/Helper/DatabaseHelper.php +++ b/src/Helper/DatabaseHelper.php @@ -10,6 +10,7 @@ defined('_JEXEC') or die; +use Joomla\CMS\Cache\CacheControllerFactoryInterface; use Joomla\CMS\Factory; use Joomla\CMS\Language\Text; use Joomla\CMS\Log\Log; @@ -22,6 +23,22 @@ class DatabaseHelper // 10 years in minutes. For all intents here sufficiently large to stand for "forever": public static $UNLIMITED_DURATION = 5256000; + // how long an emailed unblock token can be used; com_bfstop's + // TokenunblockModel::TokenValidDays must stay in sync with this + public static $UNBLOCK_TOKEN_VALID_DAYS = 3; + + // upper bound for the rows of the username statistics, which (unlike + // the failed logins) are not purged by age: an attacker can make up as + // many usernames as they like + public static $USERNAME_STATS_MAX_ROWS = 10000; + + // known IP addresses (where a user logged in from) are forgotten after + // this long without a login from them, and the table is kept below this + // number of rows, oldest first: a visitor with an account can log in from + // as many addresses as they like + public static $KNOWN_IP_MAX_AGE_DAYS = 365; + public static $KNOWN_IP_MAX_ROWS = 100000; + // how long a reverse-DNS lookup result is trusted before being redone - // keeps the (potentially slow) gethostbyaddr() call to at most once per // IP per TTL window, see issue #103 @@ -311,7 +328,10 @@ public function blockIP($logEntry, $duration, $usehtaccess, $htaccessPath) } } - public function getNewUnblockToken($id, $token) + /** + * @param string|null $username the user the link is sent to, if any + */ + public function getNewUnblockToken($id, $token, $username = null) { try { @@ -319,6 +339,7 @@ public function getNewUnblockToken($id, $token) $tokenEntry->token = $token; $tokenEntry->block_id = $id; $tokenEntry->crdate = date("Y-m-d H:i:s"); + $tokenEntry->username = $username; if (!$this->db->insertObject('#__bfstop_unblock_token', $tokenEntry)) { // maybe check if duplicate token (=PRIMARY KEY violation) and retry? @@ -334,6 +355,60 @@ public function getNewUnblockToken($id, $token) } } + /** + * Whether $token is an unexpired unblock token issued for one of the + * given blocks. Used to decide if a blocked client may reach the unblock + * page: a token that belongs to a different IP address's block, or one + * that has outlived its validity, must not work as a pass for this one. + */ + public function unblockTokenValidForBlocks($token, array $blockIds) + { + if ($token === '' || count($blockIds) === 0) + { + return false; + } + try + { + $sql = "SELECT block_id, crdate FROM #__bfstop_unblock_token WHERE token=". + $this->db->quote($token); + $this->db->setQuery($sql); + $row = $this->db->loadAssoc(); + if ($row === null || !in_array((int) $row['block_id'], $blockIds, true)) + { + return false; + } + $created = strtotime($row['crdate']); + return $created !== false && + (time() - $created) <= (self::$UNBLOCK_TOKEN_VALID_DAYS * 86400); + } + catch (\Exception $e) + { + $this->logger->log("Database exception occured: ".$e->getMessage(), Log::ERROR); + return false; + } + } + + /** + * Whether an unblock link which can still be used was sent to $username: + * one that hasn't expired and wasn't used (a used token is deleted). + */ + public function hasCurrentUnblockToken($username) + { + try + { + $this->db->setQuery("SELECT COUNT(*) FROM #__bfstop_unblock_token". + " WHERE LOWER(username) = LOWER(".$this->db->quote($username).")". + " AND crdate >= ".$this->db->quote(date("Y-m-d H:i:s", + time() - self::$UNBLOCK_TOKEN_VALID_DAYS * 86400))); + return ((int) $this->db->loadResult()) > 0; + } + catch (\Exception $e) + { + $this->logger->log("Database exception occured: ".$e->getMessage(), Log::ERROR); + return true; + } + } + public function unblockTokenExists($token) { try @@ -366,6 +441,62 @@ private function getUserEmailWhere($where) } } + /** + * Whether $login is the username or the email address of an existing + * account (Joomla! can be set up to accept either at the login). + */ + public function accountExists($login) + { + try + { + // LOWER: Joomla treats usernames case-insensitively on login + // (MySQL's default collations do, PostgreSQL's don't) + $quoted = "LOWER(".$this->db->quote($login).")"; + $this->db->setQuery("SELECT COUNT(*) FROM #__users WHERE LOWER(username) = ".$quoted. + " OR LOWER(email) = ".$quoted); + return ((int) $this->db->loadResult()) > 0; + } + catch (\Exception $e) + { + $this->logger->log("Database exception occured: ".$e->getMessage(), Log::ERROR); + return false; + } + } + + /** + * Whether the failed logins which count towards blocking $ipaddress (those + * within $interval minutes before $logtime that haven't been handled yet) + * include attempts against an existing account other than $username. + * Attempts against usernames which don't exist don't count: they can't + * get an attacker anywhere. If that can't be determined, it is assumed + * that there are. + */ + public function hasFailedLoginsForOtherAccounts($interval, $ipaddress, $username, $logtime) + { + try + { + $this->db->setQuery("SELECT DISTINCT username FROM #__bfstop_failedlogin". + " WHERE ipaddress = ".$this->db->quote($ipaddress). + " AND handled = 0 AND logtime BETWEEN ". + $this->db->quote(self::minutesBefore($logtime, $interval)). + " AND ".$this->db->quote($logtime)); + foreach ($this->db->loadColumn() as $attempted) + { + if (mb_strtolower($attempted) !== mb_strtolower($username) && + $this->accountExists($attempted)) + { + return true; + } + } + return false; + } + catch (\Exception $e) + { + $this->logger->log("Database exception occured: ".$e->getMessage(), Log::ERROR); + return true; + } + } + public function getUserEmailByID($uid) { return $this->getUserEmailWhere("id=".((int)$uid)); @@ -465,21 +596,71 @@ public function setFailedLoginHandled($info, $restrictOnUsername) } } - public function successfulLogin($info) + /** + * @param object $info ipaddress (the client's address) and username + * @param string|null $trackedAddress the key this client is tracked by (see + * IpHelper::trackedAddress(): for IPv6 its + * network), if not its address. Failed + * logins are recorded, and logins + * remembered, under this key + */ + public function successfulLogin($info, $trackedAddress = null) { - $this->setFailedLoginHandled($info, true); - $this->recordKnownIpUsername($info->ipaddress, $info->username); + $key = $trackedAddress ?? $info->ipaddress; + $handled = new \stdClass(); + $handled->ipaddress = $key; + $handled->username = $info->username; + $this->setFailedLoginHandled($handled, true); + $this->recordKnownIpUsername($key, $info->username); } - public function isKnownIpUsername($ipaddress, $username) + /** + * Whether $username has logged in successfully from $ipaddress before. + * + * Logins are remembered under the key the client is tracked by (see + * IpHelper::trackedAddress()): an IPv4 address itself, an IPv6 address by + * its network, as IPv6 clients commonly switch addresses within it. So + * is a login from any address of the network a login from this one. + */ + public function hasLoggedInFrom($ipaddress, $username, $ipv6PrefixLength) { try { - $sql = "SELECT COUNT(*) FROM #__bfstop_knownip WHERE ". - "ipaddress = ".$this->db->quote($ipaddress). - " AND username = ".$this->db->quote($username); - $this->db->setQuery($sql); - return ((int) $this->db->loadResult()) > 0; + $key = IpHelper::trackedAddress($ipaddress, $ipv6PrefixLength); + $user = "LOWER(".$this->db->quote($username).")"; + $this->db->setQuery("SELECT COUNT(*) FROM #__bfstop_knownip WHERE ". + "ipaddress IN (".$this->db->quote($key).", ".$this->db->quote($ipaddress).")". + " AND LOWER(username) = ".$user); + if (((int) $this->db->loadResult()) > 0) + { + return true; + } + if (strpos($ipaddress, ':') === false) + { + return false; + } + // IPv6: look for entries under another spelling of the address, + // or recorded at another granularity (the setting was changed) + $this->db->setQuery("SELECT ipaddress FROM #__bfstop_knownip WHERE LOWER(username) = ".$user); + $ipBin = @inet_pton($ipaddress); + foreach ($this->db->loadColumn() as $known) + { + if (strpos($known, '/') !== false) + { + $match = IpHelper::isInSubnet($ipaddress, $known); + } + else + { + $knownBin = @inet_pton($known); + $match = $knownBin !== false && + ($knownBin === $ipBin || IpHelper::trackedAddress($known, $ipv6PrefixLength) === $key); + } + if ($match) + { + return true; + } + } + return false; } catch (\Exception $e) { @@ -564,7 +745,7 @@ private function recordKnownIpUsername($ipaddress, $username) $now = date("Y-m-d H:i:s"); $sql = "SELECT id FROM #__bfstop_knownip WHERE ". "ipaddress = ".$this->db->quote($ipaddress). - " AND username = ".$this->db->quote($username); + " AND LOWER(username) = LOWER(".$this->db->quote($username).")"; $this->db->setQuery($sql); $id = $this->db->loadResult(); if ($id) @@ -631,18 +812,146 @@ public function purgeOldEntries($purgeAgeWeeks) } } - public function saveParams($params) + /** + * Remembers when the periodic maintenance last ran. + * + * 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) { try { - $query = $this->db->getQuery(true); + $where = function ($query) + { + $query->where($this->db->quoteName('type').' = '.$this->db->quote('plugin')) + ->where($this->db->quoteName('folder').' = '.$this->db->quote('system')) + ->where($this->db->quoteName('element').' = '.$this->db->quote('bfstop')); + return $query; + }; + $query = $where($this->db->getQuery(true) + ->select($this->db->quoteName('params')) + ->from($this->db->quoteName('#__extensions'))); + $this->db->setQuery($query); + $params = json_decode((string) $this->db->loadResult(), true); + if (!is_array($params)) + { + $params = array(); + } + $params['lastPurge'] = (int) $timestamp; // no table alias here: PostgreSQL doesn't allow qualified column // names in the SET clause (issue #206) - $query->update($this->db->quoteName('#__extensions')); - $query->set($this->db->quoteName('params').' = '. $this->db->quote((string)$params)); - $query->where($this->db->quoteName('element').' = '.$this->db->quote('bfstop')); + $query = $where($this->db->getQuery(true) + ->update($this->db->quoteName('#__extensions')) + ->set($this->db->quoteName('params').' = '.$this->db->quote(json_encode($params)))); $this->db->setQuery($query); $this->db->execute(); + // the plugin list (with the settings) is cached + Factory::getContainer()->get(CacheControllerFactoryInterface::class) + ->createCacheController('callback', array('defaultgroup' => 'com_plugins')) + ->clean(); + } + catch (\Exception $e) + { + $this->logger->log("Database exception occured: ".$e->getMessage(), Log::ERROR); + } + } + + /** + * Deletes the unblock tokens which can't be used any more. A used token is + * deleted when it is used, and the unblock page deletes the expired ones + * when somebody visits it - but nobody may, and the purge by age only runs + * if a purge age is configured. + */ + public function purgeExpiredUnblockTokens() + { + try + { + $this->db->setQuery('DELETE FROM #__bfstop_unblock_token WHERE crdate < '. + $this->db->quote(date("Y-m-d H:i:s", time() - self::$UNBLOCK_TOKEN_VALID_DAYS * 86400))); + $this->db->execute(); + } + catch (\Exception $e) + { + $this->logger->log("Database exception occured: ".$e->getMessage(), Log::ERROR); + } + } + + /** + * Forgets the addresses users logged in from longer ago than $maxAgeDays, + * and, oldest first, as many more as needed to keep at most $maxRows. + */ + public function pruneKnownIps($maxAgeDays = null, $maxRows = null) + { + $maxAgeDays = $maxAgeDays ?? self::$KNOWN_IP_MAX_AGE_DAYS; + $maxRows = $maxRows ?? self::$KNOWN_IP_MAX_ROWS; + try + { + $this->db->setQuery('DELETE FROM #__bfstop_knownip WHERE last_success < '. + $this->db->quote(date("Y-m-d H:i:s", time() - $maxAgeDays * 86400))); + $this->db->execute(); + for ($round = 0; $round < 1000; ++$round) + { + $this->db->setQuery('SELECT COUNT(*) FROM #__bfstop_knownip'); + $excess = ((int) $this->db->loadResult()) - $maxRows; + if ($excess <= 0) + { + return; + } + $query = $this->db->getQuery(true) + ->select($this->db->quoteName('id')) + ->from($this->db->quoteName('#__bfstop_knownip')) + ->order($this->db->quoteName('last_success').' ASC'); + $this->db->setQuery($query, 0, min($excess, 1000)); + $ids = $this->db->loadColumn(); + if (count($ids) === 0) + { + return; + } + $this->db->setQuery('DELETE FROM #__bfstop_knownip WHERE id IN ('. + implode(',', array_map('intval', $ids)).')'); + $this->db->execute(); + } + } + catch (\Exception $e) + { + $this->logger->log("Database exception occured: ".$e->getMessage(), Log::ERROR); + } + } + + /** + * Keeps the username statistics below $maxRows by deleting the entries of + * the usernames with the fewest attempts, the longest ago first. + */ + public function trimUsernameStats($maxRows = null) + { + $maxRows = $maxRows ?? self::$USERNAME_STATS_MAX_ROWS; + try + { + for ($round = 0; $round < 1000; ++$round) + { + $this->db->setQuery('SELECT COUNT(*) FROM #__bfstop_username_stats'); + $excess = ((int) $this->db->loadResult()) - $maxRows; + if ($excess <= 0) + { + return; + } + $query = $this->db->getQuery(true) + ->select($this->db->quoteName('username')) + ->from($this->db->quoteName('#__bfstop_username_stats')) + ->order($this->db->quoteName('attempts').' ASC, '.$this->db->quoteName('last_attempt').' ASC'); + $this->db->setQuery($query, 0, min($excess, 1000)); + $usernames = $this->db->loadColumn(); + if (count($usernames) === 0) + { + return; + } + $this->db->setQuery('DELETE FROM #__bfstop_username_stats WHERE username IN ('. + implode(',', array_map(array($this->db, 'quote'), $usernames)).')'); + $this->db->execute(); + } } catch (\Exception $e) { diff --git a/src/Helper/GeoHelper.php b/src/Helper/GeoHelper.php index d108ecf..ae68610 100644 --- a/src/Helper/GeoHelper.php +++ b/src/Helper/GeoHelper.php @@ -69,6 +69,13 @@ private static function lookup(LoggerHelper $logger, $dbPath, $ipaddress) { return null; } + // a plain file of the file system only: is_readable() and the reader + // would also open stream wrappers (phar://, ftp://, ...) + if (preg_match('#^[a-z][a-z0-9+.-]*://#i', $dbPath)) + { + $logger->log('GeoHelper: GeoIP database path is not a plain file name: '.$dbPath, Log::WARNING); + return null; + } if (!is_readable($dbPath)) { $logger->log('GeoHelper: GeoIP database file not readable: '.$dbPath, Log::WARNING); diff --git a/src/Helper/HtaccessHelper.php b/src/Helper/HtaccessHelper.php index 0de774f..2fa6ed9 100644 --- a/src/Helper/HtaccessHelper.php +++ b/src/Helper/HtaccessHelper.php @@ -80,7 +80,7 @@ public function checkRequirements() { $result = array( 'apacheserver' => - strstr(preg_replace("/[^a-z]+/", "", strtolower($_SERVER['SERVER_SOFTWARE'])), 'apache'), + strstr(preg_replace("/[^a-z]+/", "", strtolower($_SERVER['SERVER_SOFTWARE'] ?? '')), 'apache'), 'found' => file_exists($this->path), 'readable' => is_readable($this->path), 'writeable' => is_writeable($this->path) @@ -113,7 +113,23 @@ public function getDeniedIPs() */ public function denyIP($IP) { - return $this->addLine(self::BlockPrefix . $IP); + // the value ends up in a web server configuration file as is: a + // newline or anything but an address/subnet in it would add + // arbitrary directives (or make Apache reject the whole file, taking + // the site down) + if (!IpHelper::isValidIpOrSubnet($IP)) + { + if (!is_null($this->logger)) + { + $this->logger->log("Refusing to write invalid IP address '". + addcslashes((string) $IP, "\0..\37\177")."' to .htaccess!", Log::ERROR); + } + return false; + } + return $this->locked(function () use ($IP) + { + return $this->addLine(self::BlockPrefix . $IP); + }); } /** @@ -124,7 +140,10 @@ public function denyIP($IP) */ public function undenyIP($IP) { - return $this->removeLine(self::BlockPrefix . $IP); + return $this->locked(function () use ($IP) + { + return $this->removeLine(self::BlockPrefix . $IP); + }); } /** @@ -139,14 +158,26 @@ public function edit403Message($message) { return $this->remove403Message(); } + // a line break would start a new directive + if (preg_match('/[\x00-\x1f\x7f]/', $message)) + { + if (!is_null($this->logger)) + { + $this->logger->log("Refusing to write a 403 message containing control characters to .htaccess!", Log::ERROR); + } + return false; + } - $line = 'ErrorDocument 403 "' . $message . '"'; + $line = 'ErrorDocument 403 "' . addcslashes($message, '"\\') . '"'; - $otherLines = $this->getLines('ErrorDocument 403 ', true, true); + return $this->locked(function () use ($line) + { + $otherLines = $this->getLines('ErrorDocument 403 ', true, true); - $insertion = array_merge($this->getHeader(), array($line), $otherLines, $this->getFooter()); + $insertion = array_merge($this->getHeader(), array($line), $otherLines, $this->getFooter()); - return $this->insert($insertion); + return $this->insert($insertion); + }); } /** @@ -156,7 +187,47 @@ public function edit403Message($message) */ public function remove403Message() { - return $this->removeLine('', 'ErrorDocument 403 '); + return $this->locked(function () + { + return $this->removeLine('', 'ErrorDocument 403 '); + }); + } + + /** + * Runs a read-modify-write cycle of the .htaccess file while holding an + * exclusive lock, so that concurrent requests (several clients getting + * blocked at once is exactly when this happens) don't overwrite each + * other's changes: each of them would otherwise read the file, add its + * line and write the result back, losing the lines the others added in + * between. The lock is a file in the temporary directory, not the + * .htaccess itself, which is replaced as a whole when written. If no lock + * can be taken the change is done anyway, as before. + */ + private function locked(callable $change) + { + $lockFile = sys_get_temp_dir().'/bfstop-htaccess-'.md5($this->path).'.lock'; + $handle = @fopen($lockFile, 'c'); + if ($handle === false || !flock($handle, LOCK_EX)) + { + if (!is_null($this->logger)) + { + $this->logger->log("Could not lock '$lockFile', changing .htaccess without it", Log::WARNING); + } + if ($handle !== false) + { + fclose($handle); + } + return $change(); + } + try + { + return $change(); + } + finally + { + flock($handle, LOCK_UN); + fclose($handle); + } } /** diff --git a/src/Helper/IpHelper.php b/src/Helper/IpHelper.php index ef8b8e4..61e1976 100644 --- a/src/Helper/IpHelper.php +++ b/src/Helper/IpHelper.php @@ -43,7 +43,7 @@ class IpHelper public static function isInSubnet($ip, $subnet) { $parts = explode('/', $subnet); - if (count($parts) !== 2 || !preg_match('/^[0-9]{1,3}$/', $parts[1])) + if (count($parts) !== 2 || !preg_match('/^[0-9]{1,3}\z/', $parts[1])) { return false; } @@ -72,14 +72,180 @@ public static function isInSubnet($ip, $subnet) return (ord($ipBin[$fullBytes]) & $mask) === (ord($subnetBin[$fullBytes]) & $mask); } - private static function firstValidPublicIP($headerValue) + /** + * Whether $value is a single IP address or a subnet in CIDR notation + * (prefix length a plain number within range for the address family) - + * and nothing else: no whitespace, no further text. Used before anything + * is written to a configuration file. + */ + public static function isValidIpOrSubnet($value) + { + $parts = explode('/', (string) $value); + if (count($parts) > 2 || filter_var($parts[0], FILTER_VALIDATE_IP) === false) + { + return false; + } + if (count($parts) === 1) + { + return true; + } + return preg_match('/^[0-9]{1,3}\z/', $parts[1]) === 1 && + (int) $parts[1] <= (strpos($parts[0], ':') !== false ? 128 : 32); + } + + private static function prefixMask($bits) + { + $mask = str_repeat("\xff", intdiv($bits, 8)); + if ($bits % 8 > 0) + { + $mask .= chr((0xff << (8 - $bits % 8)) & 0xff); + } + return str_pad($mask, 16, "\x00"); + } + + /** + * The key under which failed logins and blocks of a client are tracked. + * + * For IPv4 that is the address itself. An IPv6 client usually controls a + * whole /64 (or more) and can switch to another address in it at will - + * tracking single addresses would let it start over with every request - + * so a global unicast IPv6 address is tracked as its network in CIDR + * notation (e.g. "2001:db8:1:2::/64") if $prefixLength is between 32 and + * 127. Left alone are IPv6 addresses that merely carry an IPv4 address or + * a transition mechanism's prefix (IPv4-mapped, NAT64, 6to4, Teredo): + * aggregating those would put unrelated clients, all over the world, + * under one key. + */ + public static function trackedAddress($ip, $prefixLength) + { + $prefixLength = (int) $prefixLength; + $bin = @inet_pton((string) $ip); + if ($bin === false || strlen($bin) !== 16 || $prefixLength < 32 || $prefixLength > 127 || + (ord($bin[0]) & 0xe0) !== 0x20 || // not 2000::/3 (global unicast) + substr($bin, 0, 2) === "\x20\x02" || // 6to4, 2002::/16 + substr($bin, 0, 4) === "\x20\x01\x00\x00") // Teredo, 2001::/32 + { + return $ip; + } + return inet_ntop($bin & self::prefixMask($prefixLength)).'/'.$prefixLength; + } + + /** + * Splits the trusted proxy setting into its entries. Each entry is a + * single IP address or a subnet in CIDR notation; entries may be + * separated by comma, semicolon or whitespace. A setting holding just one + * address (the only form older versions knew) keeps working unchanged. + */ + public static function parseTrustedProxies($setting) + { + return preg_split('/[\s,;]+/', (string) $setting, -1, PREG_SPLIT_NO_EMPTY); + } + + /** + * Whether $ip is one of the $trustedProxies (see parseTrustedProxies()). + * Addresses are compared in binary form, so different spellings of the + * same IPv6 address match. + */ + public static function isTrustedProxy($ip, array $trustedProxies) { - foreach (explode(',', $headerValue) as $ip) + $ipBin = @inet_pton((string) $ip); + if ($ipBin === false) { - $ip = trim($ip); // just to be safe - if (filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE) !== false) + return false; + } + foreach ($trustedProxies as $proxy) + { + if (strpos($proxy, '/') !== false) { - return $ip; + if (self::isInSubnet($ip, $proxy)) + { + return true; + } + } + elseif (@inet_pton($proxy) === $ipBin) + { + return true; + } + } + return false; + } + + /** + * The address in one entry of a forwarding header, without the quotes, + * brackets and port the various proxies decorate it with; whatever is + * left is not necessarily a valid IP address. + */ + private static function normalizeHeaderEntry($entry) + { + $entry = trim(trim($entry), '"'); + if (preg_match('/^\[([^\]]+)\](?::\d+)?$/', $entry, $m) || + preg_match('/^(\d{1,3}(?:\.\d{1,3}){3}):\d+$/', $entry, $m)) + { + return $m[1]; + } + return $entry; + } + + /** + * The addresses listed in a forwarding header, in the order they appear: + * the leftmost one is whatever the client claimed, every further one was + * appended by the proxy that handled the request next. Elements that carry + * no address (e.g. a "Forwarded" element without for=) yield ''. + */ + private static function headerAddresses($headerName, $headerValue) + { + $addresses = array(); + foreach (explode(',', $headerValue) as $element) + { + if ($headerName === 'HTTP_FORWARDED') + { + // RFC 7239: ';'-separated key=value pairs per element + $address = ''; + foreach (explode(';', $element) as $pair) + { + $keyValue = explode('=', $pair, 2); + if (count($keyValue) === 2 && strcasecmp(trim($keyValue[0]), 'for') === 0) + { + $address = $keyValue[1]; + break; + } + } + $element = $address; + } + $addresses[] = self::normalizeHeaderEntry($element); + } + return $addresses; + } + + /** + * The client address according to a forwarding header that was set by a + * trusted proxy, or null if it cannot be determined reliably. + * + * Only the entries the trusted proxies themselves added can be believed, + * and a proxy adds its entry on the right: everything to the left of that + * is whatever the client (or someone upstream) put in the header, and + * trivially forged. So the list is read from the right, skipping the + * addresses of further trusted proxies (a CDN in front of a load + * balancer, say); the first address that is not a trusted proxy is the + * client. It must never be taken from further left: with a proxy that + * appends to an incoming X-Forwarded-For, an attacker could otherwise + * pick any address - to dodge blocking, to get another visitor's + * address blocked, or to pass as an allowlisted one. + */ + public static function clientAddressFromHeader($headerName, $headerValue, array $trustedProxies) + { + $addresses = self::headerAddresses($headerName, (string) $headerValue); + for ($i = count($addresses) - 1; $i >= 0; --$i) + { + if (filter_var($addresses[$i], FILTER_VALIDATE_IP) === false) + { + // can't tell what this hop was; guessing from the entries + // further left would trust forgeable data + return null; + } + if (!self::isTrustedProxy($addresses[$i], $trustedProxies)) + { + return $addresses[$i]; } } return null; @@ -89,9 +255,11 @@ private static function firstValidPublicIP($headerValue) * Determine the client IP address. By default only REMOTE_ADDR (the * actual TCP peer address, which a visitor cannot forge) is used. A * reverse-proxy header is only consulted if the admin has explicitly - * enabled proxy support and the request's REMOTE_ADDR matches the - * configured, trusted proxy IP address - otherwise a client could bypass - * the proxy entirely and spoof the header itself. + * enabled proxy support and the request's REMOTE_ADDR is one of the + * configured, trusted proxies - otherwise a client could bypass the proxy + * entirely and spoof the header itself - and even then only the part of + * the header the trusted proxies added is believed, see + * clientAddressFromHeader(). */ public static function getAddress(LoggerHelper $logger) { @@ -109,7 +277,8 @@ public static function getAddress(LoggerHelper $logger) } $proxyIpAddress = (string) $params->get('proxyIpAddress', ''); - if ($proxyIpAddress === '' || $remoteAddr !== $proxyIpAddress) + $trustedProxies = self::parseTrustedProxies($proxyIpAddress); + if (count($trustedProxies) === 0 || !self::isTrustedProxy($remoteAddr, $trustedProxies)) { $logger->log('Proxy/load balancer usage is enabled, but the request did not originate from the '. 'configured proxy IP address (configured: "'.$proxyIpAddress.'", actual: "'.$remoteAddr. @@ -120,14 +289,14 @@ public static function getAddress(LoggerHelper $logger) $proxyHeaderSource = (string) $params->get('proxyHeaderSource', 'HTTP_X_FORWARDED_FOR'); if (array_key_exists($proxyHeaderSource, $_SERVER)) { - $ip = self::firstValidPublicIP($_SERVER[$proxyHeaderSource]); + $ip = self::clientAddressFromHeader($proxyHeaderSource, $_SERVER[$proxyHeaderSource], $trustedProxies); if ($ip !== null) { return $ip; } } $logger->log('Proxy/load balancer usage is enabled, but header "'.$proxyHeaderSource. - '" was missing or did not contain a valid public IP address, falling back to REMOTE_ADDR "'. + '" was missing or did not end in a valid client IP address, falling back to REMOTE_ADDR "'. $remoteAddr.'"!', Log::WARNING); return $remoteAddr; } diff --git a/src/Helper/LoggerHelper.php b/src/Helper/LoggerHelper.php index e54ef69..b5277cc 100644 --- a/src/Helper/LoggerHelper.php +++ b/src/Helper/LoggerHelper.php @@ -10,6 +10,7 @@ defined('_JEXEC') or die; +use Joomla\CMS\Factory; use Joomla\CMS\Log\Log; class LoggerHelper @@ -17,6 +18,10 @@ class LoggerHelper private $log_level; public const LogCategory = 'bfstop'; public const Disabled = -1; + public const LogFile = 'plg_system_bfstop.log.php'; + // the log is rotated once it exceeds this size, so it can't fill up the + // disk (a visitor can make the plugin log by failing logins) + public const MaxLogFileBytes = 5242880; public function __construct($log_level) { @@ -24,8 +29,9 @@ public function __construct($log_level) $priorities = Log::ALL; if ($log_level > self::Disabled) { + self::rotateIfTooBig(); Log::addLogger(array( - 'text_file' => 'plg_system_bfstop.log.php', + 'text_file' => self::LogFile, 'text_entry_format' => '{DATETIME} {PRIORITY} {MESSAGE}' ), @@ -34,6 +40,29 @@ public function __construct($log_level) } } + /** + * Moves the log file out of the way once it is too big, keeping one + * previous generation. The old file keeps a .php extension (the file + * names end in .log.php) as the web server must not hand out its contents. + */ + public static function rotateIfTooBig($maxBytes = self::MaxLogFileBytes) + { + try + { + $logPath = Factory::getApplication()->get('log_path'); + $file = $logPath.'/'.self::LogFile; + clearstatcache(true, $file); + if ($logPath && is_file($file) && filesize($file) > $maxBytes) + { + @rename($file, $logPath.'/'.str_replace('.log.php', '.1.log.php', self::LogFile)); + } + } + catch (\Throwable $e) + { + // never let housekeeping of the log get in the way of the request + } + } + public function isEnabled($priority = Log::ERROR) { return $priority <= $this->log_level; diff --git a/src/Helper/RiskHelper.php b/src/Helper/RiskHelper.php index 9c7ae04..67c818e 100644 --- a/src/Helper/RiskHelper.php +++ b/src/Helper/RiskHelper.php @@ -40,7 +40,7 @@ private static function knownIpScore(DatabaseHelper $db, LoggerHelper $logger, R } try { - if ($db->isKnownIpUsername($ipaddress, $username)) + if ($db->hasLoggedInFrom($ipaddress, $username, self::getIntParam($params, 'ipv6PrefixLength', 64))) { return -self::getIntParam($params, 'riskKnownIpPoints', 5); } @@ -52,22 +52,33 @@ private static function knownIpScore(DatabaseHelper $db, LoggerHelper $logger, R return 0; } - private static function commonUsernameScore(Registry $params, $username) + /** + * Whether $username is on the admin's list of common/high-value + * usernames (case-insensitive). + */ + public static function isCommonUsername(Registry $params, $username) { - if (!self::getBoolParam($params, 'riskCommonUsernameEnabled', true)) - { - return 0; - } $list = (string) $params->get('riskCommonUsernames', ''); $candidates = array_filter(array_map('trim', explode("\n", str_replace("\r", '', $list)))); foreach ($candidates as $candidate) { if ($candidate !== '' && strcasecmp($candidate, $username) === 0) { - return self::getIntParam($params, 'riskCommonUsernamePoints', 2); + return true; } } - return 0; + return false; + } + + private static function commonUsernameScore(Registry $params, $username) + { + if (!self::getBoolParam($params, 'riskCommonUsernameEnabled', true)) + { + return 0; + } + return self::isCommonUsername($params, $username) + ? self::getIntParam($params, 'riskCommonUsernamePoints', 2) + : 0; } private static function userAgentScore(Registry $params) diff --git a/src/Helper/UsernameHelper.php b/src/Helper/UsernameHelper.php new file mode 100644 index 0000000..e84e4a3 --- /dev/null +++ b/src/Helper/UsernameHelper.php @@ -0,0 +1,49 @@ +assertBlocked(); } + private function knownIps() + { + $this->db->setQuery('SELECT ipaddress FROM #__bfstop_knownip ORDER BY ipaddress'); + return $this->db->loadColumn(); + } + + private function knownIp($ip, $daysAgo) + { + $this->insert('#__bfstop_knownip', array('ipaddress' => $ip, 'username' => 'bob', + 'first_success' => self::minutesAgo($daysAgo * 1440 + 1), 'last_success' => self::minutesAgo($daysAgo * 1440))); + } + + public function testDailyMaintenancePrunesAndRemembersWhenItRan() + { + // purge age 0: nothing is purged by age, but this has to happen regardless + $this->configure(array('deleteOld' => 0, 'blockNumber' => 7)); + $this->knownIp('203.0.113.1', 400); + $this->knownIp('203.0.113.2', 1); + // unblock tokens nobody can use any more + $this->insert('#__bfstop_unblock_token', array('token' => str_repeat('ab', 20), 'block_id' => 1, + 'crdate' => self::minutesAgo(4 * 24 * 60))); + $this->insert('#__bfstop_unblock_token', array('token' => str_repeat('cd', 20), 'block_id' => 2, + 'crdate' => self::minutesAgo(60))); + $this->assertNotBlocked(); + $this->assertSame(array('203.0.113.2'), $this->knownIps()); + $this->assertSame(array(str_repeat('cd', 20)), $this->db->setQuery('SELECT token FROM #__bfstop_unblock_token')->loadColumn()); + $saved = json_decode($this->getPluginParams(), true); + $this->assertGreaterThan(time() - 60, $saved['lastPurge']); + $this->assertSame(7, $saved['blockNumber'], 'the other settings are left alone'); + } + + public function testMaintenanceRunsOnlyOncePerDay() + { + $this->configure(array('lastPurge' => time() - 3600)); + $this->knownIp('203.0.113.1', 400); + $this->assertNotBlocked(); + $this->assertSame(array('203.0.113.1'), $this->knownIps(), 'ran an hour ago already'); + } + + public function testBlockPageIsNeverCached() + { + $headers = Bfstop::blockedResponseHeaders(); + $this->assertContains('Cache-Control: no-store, no-cache, must-revalidate, private', $headers); + $this->assertContains('Pragma: no-cache', $headers); + foreach ($headers as $header) + { + $this->assertStringStartsNotWith('HTTP/', $header, 'the status is set with http_response_code()'); + } + } + + public function testBlockedResponseCode() + { + $this->assertSame(403, Bfstop::blockedResponseCode(true)); + $this->assertNull(Bfstop::blockedResponseCode(false)); + } + + public function testBlockPageIsAnErrorByDefault() + { + $this->configure(); + $this->block(); + $this->assertMatchesRegularExpression('/^STATUS: 403$/m', $this->request()); + } + + public function testErrorStatusCanBeSwitchedOff() + { + $this->configure(array('useHttpError' => 0)); + $this->block(); + $output = $this->request(); + $this->assertStringContainsString(self::BlockedMessage, $output); + $this->assertDoesNotMatchRegularExpression('/^STATUS: 403$/m', $output); + } + public function testBlockedMessageCanShowIp() { $this->configure(array('blockedMsgShowIP' => 1)); @@ -155,6 +228,37 @@ public function testValidUnblockTokenGetsThrough() $this->assertBlocked('option=com_bfstop&view=tokenunblock'); } + public function testUnblockTokenIsNoPassForOtherRequests() + { + // the token is consumed by the unblock page only; as a parameter of + // any other request (here: a login) it must not get around the block + $this->configure(); + $blockId = $this->block(); + $token = (new DatabaseHelper($this->logger))->getNewUnblockToken($blockId, str_repeat('ab', 20)); + $this->assertBlocked('option=com_users&task=user.login&view=tokenunblock&token='.$token); + $this->assertBlocked('option=com_content&view=tokenunblock&token='.$token); + $this->assertBlocked('option=com_bfstop&task=display&view=tokenunblock&token='.$token); + $this->assertNotBlocked('option=com_bfstop&view=tokenunblock&token='.$token); + } + + public function testUnblockTokenOfAnotherBlockIsNoPass() + { + $this->configure(); + $this->block(); + $otherBlockId = $this->block('203.0.113.99'); + $token = (new DatabaseHelper($this->logger))->getNewUnblockToken($otherBlockId, str_repeat('cd', 20)); + $this->assertBlocked('option=com_bfstop&view=tokenunblock&token='.$token); + } + + public function testExpiredUnblockTokenIsNoPass() + { + $this->configure(); + $blockId = $this->block(); + $this->insert('#__bfstop_unblock_token', array('token' => str_repeat('ef', 20), + 'block_id' => $blockId, 'crdate' => self::minutesAgo(4 * 24 * 60))); + $this->assertBlocked('option=com_bfstop&view=tokenunblock&token='.str_repeat('ef', 20)); + } + public function testRejectedRequestsAreCounted() { $this->configure(); diff --git a/tests/Integration/ComponentTest.php b/tests/Integration/ComponentTest.php index 8803724..2f19a34 100644 --- a/tests/Integration/ComponentTest.php +++ b/tests/Integration/ComponentTest.php @@ -120,6 +120,137 @@ public function testUnblockViaEmailToken() $this->assertSame(0, (int) $this->queryValue('SELECT COUNT(*) FROM #__bfstop_unblock_token')); } + public function testTokenUnblockNeedsConfirmationAndTheBlockedIp() + { + $ip = '203.0.113.5'; + $blockId = $this->insert('#__bfstop_bannedip', array('ipaddress' => $ip, 'crdate' => self::minutesAgo(0), 'duration' => 60), 'id'); + $this->insert('#__bfstop_unblock_token', array('token' => 'valid', 'block_id' => $blockId, 'crdate' => self::minutesAgo(60))); + $helper = new DatabaseHelper($this->logger); + $model = $this->model(TokenunblockModel::class); + $tokens = 'SELECT COUNT(*) FROM #__bfstop_unblock_token'; + + $this->assertSame(TokenunblockModel::ResultInvalid, $model->process('', true, $ip, $this->logger)); + + // opening the link (what a mail scanner does) changes nothing + $this->assertSame(TokenunblockModel::ResultConfirm, $model->process('valid', false, $ip, $this->logger)); + $this->assertTrue($helper->isIPBlocked($ip)); + $this->assertSame(1, (int) $this->queryValue($tokens)); + + // confirming from another IP address doesn't unblock either + $this->assertSame(TokenunblockModel::ResultWrongIp, $model->process('valid', true, '198.51.100.9', $this->logger)); + $this->assertTrue($helper->isIPBlocked($ip)); + $this->assertSame(1, (int) $this->queryValue($tokens)); + + // neither does an unknown token + $this->assertSame(TokenunblockModel::ResultNotFound, $model->process('unknown', true, $ip, $this->logger)); + $this->logger->errors = array(); // expected: "token not found" error + $this->assertTrue($helper->isIPBlocked($ip)); + + $this->assertSame(TokenunblockModel::ResultUnblocked, $model->process('valid', true, $ip, $this->logger)); + $this->assertFalse($helper->isIPBlocked($ip)); + $this->assertSame(0, (int) $this->queryValue($tokens)); + } + + public function testTokenUnblockHttpStatus() + { + $this->assertSame(400, TokenunblockModel::httpStatus(TokenunblockModel::ResultInvalid)); + $this->assertSame(403, TokenunblockModel::httpStatus(TokenunblockModel::ResultWrongIp)); + $this->assertSame(404, TokenunblockModel::httpStatus(TokenunblockModel::ResultNotFound)); + $this->assertSame(500, TokenunblockModel::httpStatus(TokenunblockModel::ResultFailed)); + // opening the link only asks for confirmation, whatever the token is + $this->assertSame(200, TokenunblockModel::httpStatus(TokenunblockModel::ResultConfirm)); + $this->assertSame(200, TokenunblockModel::httpStatus(TokenunblockModel::ResultUnblocked)); + } + + public function testExpiredAndUsedTokensAreNotFound() + { + $ip = '203.0.113.5'; + $blockId = $this->insert('#__bfstop_bannedip', array('ipaddress' => $ip, 'crdate' => self::minutesAgo(0), 'duration' => 60), 'id'); + $this->insert('#__bfstop_unblock_token', array('token' => 'expired', 'block_id' => $blockId, 'crdate' => self::minutesAgo(4 * 24 * 60))); + $this->insert('#__bfstop_unblock_token', array('token' => 'used', 'block_id' => $blockId, 'crdate' => self::minutesAgo(10))); + $model = $this->model(TokenunblockModel::class); + $this->assertSame(TokenunblockModel::ResultUnblocked, $model->process('used', true, $ip, $this->logger)); + $this->assertSame(TokenunblockModel::ResultNotFound, $model->process('used', true, $ip, $this->logger), 'used up'); + $this->assertSame(TokenunblockModel::ResultNotFound, $model->process('expired', true, $ip, $this->logger)); + $this->logger->errors = array(); // expected: "token not found" errors + } + + public function testTokenUnblockComparesIpv6AddressesNotSpellings() + { + $blockId = $this->insert('#__bfstop_bannedip', array('ipaddress' => '2001:db8::1', 'crdate' => self::minutesAgo(0), 'duration' => 60), 'id'); + $this->insert('#__bfstop_unblock_token', array('token' => 'valid', 'block_id' => $blockId, 'crdate' => self::minutesAgo(1))); + $model = $this->model(TokenunblockModel::class); + $this->assertSame(TokenunblockModel::ResultWrongIp, $model->process('valid', true, '2001:db8::2', $this->logger)); + $this->assertSame(TokenunblockModel::ResultUnblocked, $model->process('valid', true, '2001:0DB8:0:0:0:0:0:1', $this->logger)); + } + + public function testLogViewEscapesLogContents() + { + // the message of a log entry can contain what a visitor sent (e.g. a username) + $view = new class { + public $items; + public function escape($string) + { + return htmlspecialchars((string) $string, ENT_QUOTES, 'UTF-8'); + } + }; + $view->items = array((object) array('date' => '2026-01-01T00:00:00+00:00', + 'priority' => 'DEBUG', 'message' => 'Unknown user () blocked')); + ob_start(); + (function () + { + include getenv('COM_BFSTOP_ROOT').'/admin/tmpl/log/default_body.php'; + })->call($view); + $html = ob_get_clean(); + $this->assertStringNotContainsString('