From af37a4c77dc7fbea200d761f112e27479e747063 Mon Sep 17 00:00:00 2001
From: =?utf8?q?Gerg=C5=91=20Tisza?= <gtisza@wikimedia.org>
Date: Tue, 13 Sep 2016 23:25:49 +0000
Subject: [PATCH] Fix login API for users with @ in their usernames

An @ in the username caused the password to be treated as a bot password,
but apparently some real usernames still contain it. Try both logins
instead. Security considerations are the same as for the other bot
password syntax: the length check makes sure we do not provide any
information on a timing side channel about the password unless it is
extremely long.

Change-Id: I58f42544a08c3208c41f54cfae932632d9c5affa
---
 includes/user/BotPassword.php                   | 10 ++++++----
 tests/phpunit/includes/api/ApiLoginTest.php     |  7 ++++---
 tests/phpunit/includes/user/BotPasswordTest.php |  3 ++-
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/includes/user/BotPassword.php b/includes/user/BotPassword.php
index 0bbe12e995..eae57f40dc 100644
--- a/includes/user/BotPassword.php
+++ b/includes/user/BotPassword.php
@@ -411,11 +411,13 @@ class BotPassword implements IDBAccessObject {
 	 */
 	public static function canonicalizeLoginData( $username, $password ) {
 		$sep = BotPassword::getSeparator();
-		if ( strpos( $username, $sep ) !== false ) {
-			// the separator is not valid in usernames so this must be a bot login
-			return [ $username, $password, false ];
+		// the strlen check helps minimize the password information obtainable from timing
+		if ( strlen( $password ) >= 32 && strpos( $username, $sep ) !== false ) {
+			// the separator is not valid in new usernames but might appear in legacy ones
+			if ( preg_match( '/^[0-9a-w]{32,}$/', $password ) ) {
+				return [ $username, $password, true ];
+			}
 		} elseif ( strlen( $password ) > 32 && strpos( $password, $sep ) !== false ) {
-			// the strlen check helps minimize the password information obtainable from timing
 			$segments = explode( $sep, $password );
 			$password = array_pop( $segments );
 			$appId = implode( $sep, $segments );
diff --git a/tests/phpunit/includes/api/ApiLoginTest.php b/tests/phpunit/includes/api/ApiLoginTest.php
index 487ab84d04..97681eb76f 100644
--- a/tests/phpunit/includes/api/ApiLoginTest.php
+++ b/tests/phpunit/includes/api/ApiLoginTest.php
@@ -231,10 +231,11 @@ class ApiLoginTest extends ApiTestCase {
 		$centralId = CentralIdLookup::factory()->centralIdFromLocalUser( $user->getUser() );
 		$this->assertNotEquals( 0, $centralId, 'sanity check' );
 
+		$password = 'ngfhmjm64hv0854493hsj5nncjud2clk';
 		$passwordFactory = new PasswordFactory();
 		$passwordFactory->init( RequestContext::getMain()->getConfig() );
 		// A is unsalted MD5 (thus fast) ... we don't care about security here, this is test only
-		$passwordHash = $passwordFactory->newFromPlaintext( 'foobaz' );
+		$passwordHash = $passwordFactory->newFromPlaintext( $password );
 
 		$dbw = wfGetDB( DB_MASTER );
 		$dbw->insert(
@@ -255,7 +256,7 @@ class ApiLoginTest extends ApiTestCase {
 		$ret = $this->doApiRequest( [
 			'action' => 'login',
 			'lgname' => $lgName,
-			'lgpassword' => 'foobaz',
+			'lgpassword' => $password,
 		] );
 
 		$result = $ret[0];
@@ -270,7 +271,7 @@ class ApiLoginTest extends ApiTestCase {
 			'action' => 'login',
 			'lgtoken' => $token,
 			'lgname' => $lgName,
-			'lgpassword' => 'foobaz',
+			'lgpassword' => $password,
 		], $ret[2] );
 
 		$result = $ret[0];
diff --git a/tests/phpunit/includes/user/BotPasswordTest.php b/tests/phpunit/includes/user/BotPasswordTest.php
index d63770418c..cb27fde1e6 100644
--- a/tests/phpunit/includes/user/BotPasswordTest.php
+++ b/tests/phpunit/includes/user/BotPasswordTest.php
@@ -244,8 +244,9 @@ class BotPasswordTest extends MediaWikiTestCase {
 		return [
 			[ 'user', 'pass', false ],
 			[ 'user', 'abc@def', false ],
+			[ 'legacy@user', 'pass', false ],
 			[ 'user@bot', '12345678901234567890123456789012',
-				[ 'user@bot', '12345678901234567890123456789012', false ] ],
+				[ 'user@bot', '12345678901234567890123456789012', true ] ],
 			[ 'user', 'bot@12345678901234567890123456789012',
 				[ 'user@bot', '12345678901234567890123456789012', true ] ],
 			[ 'user', 'bot@12345678901234567890123456789012345',
-- 
2.20.1