From 2c1e550b6f652e2a649a82c1e67fd8a423f1c28e Mon Sep 17 00:00:00 2001 From: Ori Livneh Date: Tue, 23 Feb 2016 11:16:31 -0800 Subject: [PATCH] Revert "Log multiple IPs using the same session or the same user account" This reverts commit f22549a60539c9aa5c5390c8417c984ba8eef5b2. Per T125455#2054194. Bug: T125455 Change-Id: Ic2049381e98586e91974fc5b47d9e857a73414a4 --- includes/DefaultSettings.php | 22 ----- includes/Setup.php | 5 - includes/session/SessionManager.php | 91 ------------------- .../includes/session/SessionManagerTest.php | 71 --------------- 4 files changed, 189 deletions(-) diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php index da8eed5652..933767356d 100644 --- a/includes/DefaultSettings.php +++ b/includes/DefaultSettings.php @@ -2310,28 +2310,6 @@ $wgSessionHandler = null; */ $wgPHPSessionHandling = 'enable'; -/** - * The number of different IPs in the same session within a period of $wgSuspiciousIpExpiry - * that should cause warnings to be logged. This is meant more for debugging errors in the - * authentication system than for detecting abuse. - * @since 1.27 - */ -$wgSuspiciousIpPerSessionLimit = 2; - -/** - * Like $wgSuspiciousIpPerSessionLimit but over all requests from the same user within - * $wgSuspiciousIpExpiry, whether they are in the same session or not. - * @since 1.27 - */ -$wgSuspiciousIpPerUserLimit = 5; - -/** - * Time in seconds to remember IPs for, for the purposes of $wgSuspiciousIpPerSessionLimit and - * $wgSuspiciousIpPerUserLimit. - * @since 1.27 - */ -$wgSuspiciousIpExpiry = 600; - /** * If enabled, will send MemCached debugging information to $wgDebugLogFile */ diff --git a/includes/Setup.php b/includes/Setup.php index 47fb73e143..189855e82c 100644 --- a/includes/Setup.php +++ b/includes/Setup.php @@ -827,10 +827,5 @@ if ( !defined( 'MW_NO_SESSION' ) && !$wgCommandLineMode ) { wfDebug( "Fully initialised\n" ); $wgFullyInitialised = true; -// T125455 -if ( !defined( 'MW_NO_SESSION' ) && !$wgCommandLineMode ) { - MediaWiki\Session\SessionManager::singleton()->checkIpLimits(); -} - Profiler::instance()->scopedProfileOut( $ps_extensions ); Profiler::instance()->scopedProfileOut( $ps_setup ); diff --git a/includes/session/SessionManager.php b/includes/session/SessionManager.php index 9ca3bbc42d..1aab12a895 100644 --- a/includes/session/SessionManager.php +++ b/includes/session/SessionManager.php @@ -24,7 +24,6 @@ namespace MediaWiki\Session; use Psr\Log\LoggerInterface; -use Psr\Log\LogLevel; use BagOStuff; use CachedBagOStuff; use Config; @@ -1074,96 +1073,6 @@ final class SessionManager implements SessionManagerInterface { self::$globalSessionRequest = null; } - /** - * Do a sanity check to make sure the session is not used from many different IP addresses - * and store some data for later sanity checks. - * FIXME remove this once SessionManager is considered stable - * @private For use in Setup.php only - * @param Session $session Defaults to the global session. - */ - public function checkIpLimits( Session $session = null ) { - $session = $session ?: self::getGlobalSession(); - - try { - $ip = $session->getRequest()->getIP(); - } catch ( \MWException $e ) { - return; - } - if ( $ip === '127.0.0.1' || \IP::isConfiguredProxy( $ip ) ) { - return; - } - $now = time(); - - // Record (and possibly log) that the IP is using the current session. - // Don't touch the stored data unless we are adding a new IP or re-adding an expired one. - // This is slightly inaccurate (when an existing IP is seen again, the expiry is not - // extended) but that shouldn't make much difference and limits the session write frequency - // to # of IPs / $wgSuspiciousIpExpiry. - $data = $session->get( 'SessionManager-ip', [] ); - if ( - !isset( $data[$ip] ) - || $data[$ip] < $now - ) { - $data[$ip] = time() + $this->config->get( 'SuspiciousIpExpiry' ); - foreach ( $data as $key => $expires ) { - if ( $expires < $now ) { - unset( $data[$key] ); - } - } - $session->set( 'SessionManager-ip', $data ); - - $logger = \MediaWiki\Logger\LoggerFactory::getInstance( 'session-ip' ); - $logLevel = count( $data ) >= $this->config->get( 'SuspiciousIpPerSessionLimit' ) - ? LogLevel::WARNING : ( count( $data ) === 1 ? LogLevel::DEBUG : LogLevel::INFO ); - $logger->log( - $logLevel, - 'Same session used from {count} IPs', - [ - 'count' => count( $data ), - 'ips' => $data, - 'session' => $session->getId(), - 'user' => $session->getUser()->getName(), - 'persistent' => $session->isPersistent(), - ] - ); - } - - // Now do the same thing globally for the current user. - // We are using the object cache and assume it is shared between all wikis of a farm, - // and further assume that the same name belongs to the same user on all wikis. (It's either - // that or a central ID lookup which would mean an extra SQL query on every request.) - if ( $session->getUser()->isLoggedIn() ) { - $userKey = 'SessionManager-ip:' . md5( $session->getUser()->getName() ); - $data = $this->store->get( $userKey ) ?: []; - if ( - !isset( $data[$ip] ) - || $data[$ip] < $now - ) { - $data[$ip] = time() + $this->config->get( 'SuspiciousIpExpiry' ); - foreach ( $data as $key => $expires ) { - if ( $expires < $now ) { - unset( $data[$key] ); - } - } - $this->store->set( $userKey, $data, $this->config->get( 'SuspiciousIpExpiry' ) ); - $logger = \MediaWiki\Logger\LoggerFactory::getInstance( 'session-ip' ); - $logLevel = count( $data ) >= $this->config->get( 'SuspiciousIpPerUserLimit' ) - ? LogLevel::WARNING : ( count( $data ) === 1 ? LogLevel::DEBUG : LogLevel::INFO ); - $logger->log( - $logLevel, - 'Same user had sessions from {count} IPs', - [ - 'count' => count( $data ), - 'ips' => $data, - 'session' => $session->getId(), - 'user' => $session->getUser()->getName(), - 'persistent' => $session->isPersistent(), - ] - ); - } - } - } - /**@}*/ } diff --git a/tests/phpunit/includes/session/SessionManagerTest.php b/tests/phpunit/includes/session/SessionManagerTest.php index d3d85092a0..fe2c3b765d 100644 --- a/tests/phpunit/includes/session/SessionManagerTest.php +++ b/tests/phpunit/includes/session/SessionManagerTest.php @@ -3,7 +3,6 @@ namespace MediaWiki\Session; use AuthPlugin; -use MediaWiki\Logger\LoggerFactory; use MediaWikiTestCase; use Psr\Log\LogLevel; use User; @@ -1677,74 +1676,4 @@ class SessionManagerTest extends MediaWikiTestCase { ], $logger->getBuffer() ); $logger->clearBuffer(); } - - /** - * @dataProvider provideCheckIpLimits - */ - public function testCheckIpLimits( $ip, $sessionData, $userData, $logLevel1, $logLevel2 ) { - $this->setMwGlobals( [ - 'wgSuspiciousIpPerSessionLimit' => 5, - 'wgSuspiciousIpPerUserLimit' => 10, - 'wgSuspiciousIpExpiry' => 600, - 'wgSquidServers' => [ '11.22.33.44' ], - ] ); - $manager = new SessionManager(); - $logger = $this->getMock( '\Psr\Log\LoggerInterface' ); - $this->setLogger( 'session-ip', $logger ); - $request = new \FauxRequest(); - $request->setIP( $ip ); - - $session = $manager->getSessionForRequest( $request ); - /** @var SessionBackend $backend */ - $backend = \TestingAccessWrapper::newFromObject( $session )->backend; - $data = &$backend->getData(); - $data = [ 'SessionManager-ip' => $sessionData ]; - $backend->setUser( User::newFromName( 'UTSysop' ) ); - $manager = \TestingAccessWrapper::newFromObject( $manager ); - $manager->store->set( 'SessionManager-ip:' . md5( 'UTSysop' ), $userData ); - - $logger->expects( $this->exactly( isset( $logLevel1 ) + isset( $logLevel2 ) ) )->method( 'log' ); - if ( $logLevel1 ) { - $logger->expects( $this->at( 0 ) )->method( 'log' )->with( $logLevel1, - 'Same session used from {count} IPs', $this->isType( 'array' ) ); - } - if ( $logLevel2 ) { - $logger->expects( $this->at( isset( $logLevel1 ) ) )->method( 'log' )->with( $logLevel2, - 'Same user had sessions from {count} IPs', $this->isType( 'array' ) ); - } - - $manager->checkIpLimits( $session ); - } - - public function provideCheckIpLimits() { - $future = time() + 1000; - $past = time() - 1000; - return [ - // DEBUG log for first new IP - [ '1.2.3.4', [], [], LogLevel::DEBUG, LogLevel::DEBUG ], - // no log for same IP - [ '1.2.3.4', [ '1.2.3.4' => $future ], [ '1.2.3.4' => $future ], - null, null ], - [ '1.2.3.4', [], [ '1.2.3.4' => $future ], - LogLevel::DEBUG, null ], - // INFO log for second new IP - [ '1.2.3.4', [ '10.20.30.40' => $future ], [ '10.20.30.40' => $future ], - LogLevel::INFO, LogLevel::INFO ], - // WARNING above $wgSuspiciousIpPerSessionLimit - [ '1.2.3.4', array_fill_keys( range( 1, 5 ), $future ), - array_fill_keys( range( 1, 5 ), $future ), LogLevel::WARNING, LogLevel::INFO ], - // WARNING above $wgSuspiciousIpPerUserLimit - - [ '1.2.3.4', array_fill_keys( range( 1, 2 ), $future ), - array_fill_keys( range( 1, 12 ), $future ), LogLevel::INFO, LogLevel::WARNING ], - // expired keys ignored - [ '1.2.3.4', [ '1.2.3.4' => $past ], [ '1.2.3.4' => $past ], - LogLevel::DEBUG, LogLevel::DEBUG ], - [ '1.2.3.4', array_fill_keys( range( 1, 5 ), $past ), - array_fill_keys( range( 1, 5 ), $past ), LogLevel::DEBUG, LogLevel::DEBUG ], - // special IPs are ignored - [ '127.0.0.1', [], [], null, null ], - [ '11.22.33.44', [], [], null, null ], - ]; - } } -- 2.20.1