From b49a04f796b9ad8533c53fee541132a4c2214909 Mon Sep 17 00:00:00 2001 From: ArthurHoaro Date: Sat, 9 Feb 2019 16:44:48 +0100 Subject: Rewrite IP ban management This adds a dedicated manager class to handle all ban interactions, which is instantiated and handled by LoginManager. IPs are now stored in the same format as the datastore, through FileUtils. Fixes #1032 #587 --- application/security/BanManager.php | 213 ++++++++++++++++++++++++++++++++++ application/security/LoginManager.php | 100 +++------------- 2 files changed, 227 insertions(+), 86 deletions(-) create mode 100644 application/security/BanManager.php (limited to 'application/security') diff --git a/application/security/BanManager.php b/application/security/BanManager.php new file mode 100644 index 00000000..68190c54 --- /dev/null +++ b/application/security/BanManager.php @@ -0,0 +1,213 @@ +trustedProxies = $trustedProxies; + $this->nbAttempts = $nbAttempts; + $this->banDuration = $banDuration; + $this->banFile = $banFile; + $this->logFile = $logFile; + $this->readBanFile(); + } + + /** + * Handle a failed login and ban the IP after too many failed attempts + * + * @param array $server The $_SERVER array + */ + public function handleFailedAttempt($server) + { + $ip = $this->getIp($server); + // the IP is behind a trusted forward proxy, but is not forwarded + // in the HTTP headers, so we do nothing + if (empty($ip)) { + return; + } + + // increment the fail count for this IP + if (isset($this->failures[$ip])) { + $this->failures[$ip]++; + } else { + $this->failures[$ip] = 1; + } + + if ($this->failures[$ip] >= $this->nbAttempts) { + $this->bans[$ip] = time() + $this->banDuration; + logm( + $this->logFile, + $server['REMOTE_ADDR'], + 'IP address banned from login: '. $ip + ); + } + $this->writeBanFile(); + } + + /** + * Remove failed attempts for the provided client. + * + * @param array $server $_SERVER + */ + public function clearFailures($server) + { + $ip = $this->getIp($server); + // the IP is behind a trusted forward proxy, but is not forwarded + // in the HTTP headers, so we do nothing + if (empty($ip)) { + return; + } + + if (isset($this->failures[$ip])) { + unset($this->failures[$ip]); + } + $this->writeBanFile(); + } + + /** + * Check whether the client IP is banned or not. + * + * @param array $server $_SERVER + * + * @return bool True if the IP is banned, false otherwise + */ + public function isBanned($server) + { + $ip = $this->getIp($server); + // the IP is behind a trusted forward proxy, but is not forwarded + // in the HTTP headers, so we allow the authentication attempt. + if (empty($ip)) { + return false; + } + + // the user is not banned + if (! isset($this->bans[$ip])) { + return false; + } + + // the user is still banned + if ($this->bans[$ip] > time()) { + return true; + } + + // the ban has expired, the user can attempt to log in again + if (isset($this->failures[$ip])) { + unset($this->failures[$ip]); + } + unset($this->bans[$ip]); + logm($this->logFile, $server['REMOTE_ADDR'], 'Ban lifted for: '. $ip); + + $this->writeBanFile(); + return false; + } + + /** + * Retrieve the IP from $_SERVER. + * If the actual IP is behind an allowed reverse proxy, + * we try to extract the forwarded IP from HTTP headers. + * + * @param array $server $_SERVER + * + * @return string|bool The IP or false if none could be extracted + */ + protected function getIp($server) + { + $ip = $server['REMOTE_ADDR']; + if (! in_array($ip, $this->trustedProxies)) { + return $ip; + } + return getIpAddressFromProxy($server, $this->trustedProxies); + } + + /** + * Read a file containing banned IPs + */ + protected function readBanFile() + { + $data = FileUtils::readFlatDB($this->banFile); + if (isset($data['failures']) && is_array($data['failures'])) { + $this->failures = $data['failures']; + } + + if (isset($data['bans']) && is_array($data['bans'])) { + $this->bans = $data['bans']; + } + } + + /** + * Write the banned IPs to a file + */ + protected function writeBanFile() + { + return FileUtils::writeFlatDB( + $this->banFile, + [ + 'failures' => $this->failures, + 'bans' => $this->bans, + ] + ); + } + + /** + * Get the Failures (for UT purpose). + * + * @return array + */ + public function getFailures() + { + return $this->failures; + } + + /** + * Get the Bans (for UT purpose). + * + * @return array + */ + public function getBans() + { + return $this->bans; + } +} diff --git a/application/security/LoginManager.php b/application/security/LoginManager.php index 1ff3d0be..0b0ce0b1 100644 --- a/application/security/LoginManager.php +++ b/application/security/LoginManager.php @@ -20,8 +20,8 @@ class LoginManager /** @var SessionManager Session Manager instance **/ protected $sessionManager = null; - /** @var string Path to the file containing IP bans */ - protected $banFile = ''; + /** @var BanManager Ban Manager instance **/ + protected $banManager; /** @var bool Whether the user is logged in **/ protected $isLoggedIn = false; @@ -35,17 +35,21 @@ class LoginManager /** * Constructor * - * @param array $globals The $GLOBALS array (reference) * @param ConfigManager $configManager Configuration Manager instance * @param SessionManager $sessionManager SessionManager instance */ - public function __construct(& $globals, $configManager, $sessionManager) + public function __construct($configManager, $sessionManager) { - $this->globals = &$globals; $this->configManager = $configManager; $this->sessionManager = $sessionManager; - $this->banFile = $this->configManager->get('resource.ban_file', 'data/ipbans.php'); - $this->readBanFile(); + $this->banManager = new BanManager( + $this->configManager->get('security.trusted_proxies', []), + $this->configManager->get('security.ban_after'), + $this->configManager->get('security.ban_duration'), + $this->configManager->get('resource.ban_file', 'data/ipbans.php'), + $this->configManager->get('resource.log') + ); + if ($this->configManager->get('security.open_shaarli') === true) { $this->openShaarli = true; } @@ -157,31 +161,6 @@ class LoginManager return true; } - /** - * Read a file containing banned IPs - */ - protected function readBanFile() - { - if (! file_exists($this->banFile)) { - return; - } - include $this->banFile; - } - - /** - * Write the banned IPs to a file - */ - protected function writeBanFile() - { - if (! array_key_exists('IPBANS', $this->globals)) { - return; - } - file_put_contents( - $this->banFile, - "globals['IPBANS'], true) . ";\n?>" - ); - } - /** * Handle a failed login and ban the IP after too many failed attempts * @@ -189,34 +168,7 @@ class LoginManager */ public function handleFailedLogin($server) { - $ip = $server['REMOTE_ADDR']; - $trusted = $this->configManager->get('security.trusted_proxies', []); - - if (in_array($ip, $trusted)) { - $ip = getIpAddressFromProxy($server, $trusted); - if (! $ip) { - // the IP is behind a trusted forward proxy, but is not forwarded - // in the HTTP headers, so we do nothing - return; - } - } - - // increment the fail count for this IP - if (isset($this->globals['IPBANS']['FAILURES'][$ip])) { - $this->globals['IPBANS']['FAILURES'][$ip]++; - } else { - $this->globals['IPBANS']['FAILURES'][$ip] = 1; - } - - if ($this->globals['IPBANS']['FAILURES'][$ip] >= $this->configManager->get('security.ban_after')) { - $this->globals['IPBANS']['BANS'][$ip] = time() + $this->configManager->get('security.ban_duration', 1800); - logm( - $this->configManager->get('resource.log'), - $server['REMOTE_ADDR'], - 'IP address banned from login' - ); - } - $this->writeBanFile(); + $this->banManager->handleFailedAttempt($server); } /** @@ -226,13 +178,7 @@ class LoginManager */ public function handleSuccessfulLogin($server) { - $ip = $server['REMOTE_ADDR']; - // FIXME unban when behind a trusted proxy? - - unset($this->globals['IPBANS']['FAILURES'][$ip]); - unset($this->globals['IPBANS']['BANS'][$ip]); - - $this->writeBanFile(); + $this->banManager->clearFailures($server); } /** @@ -244,24 +190,6 @@ class LoginManager */ public function canLogin($server) { - $ip = $server['REMOTE_ADDR']; - - if (! isset($this->globals['IPBANS']['BANS'][$ip])) { - // the user is not banned - return true; - } - - if ($this->globals['IPBANS']['BANS'][$ip] > time()) { - // the user is still banned - return false; - } - - // the ban has expired, the user can attempt to log in again - logm($this->configManager->get('resource.log'), $server['REMOTE_ADDR'], 'Ban lifted.'); - unset($this->globals['IPBANS']['FAILURES'][$ip]); - unset($this->globals['IPBANS']['BANS'][$ip]); - - $this->writeBanFile(); - return true; + return ! $this->banManager->isBanned($server); } } -- cgit v1.2.3