jenkins-bot has submitted this change and it was merged.
Change subject: Fix login API for users with @ in their usernames
..
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
---
M includes/user/BotPassword.php
M tests/phpunit/includes/api/ApiLoginTest.php
M tests/phpunit/includes/user/BotPasswordTest.php
3 files changed, 12 insertions(+), 8 deletions(-)
Approvals:
Anomie: Looks good to me, approved
jenkins-bot: Verified
diff --git a/includes/user/BotPassword.php b/includes/user/BotPassword.php
index 0bbe12e..eae57f4 100644
--- a/includes/user/BotPassword.php
+++ b/includes/user/BotPassword.php
@@ -411,11 +411,13 @@
*/
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 487ab84..97681eb 100644
--- a/tests/phpunit/includes/api/ApiLoginTest.php
+++ b/tests/phpunit/includes/api/ApiLoginTest.php
@@ -231,10 +231,11 @@
$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 @@
$ret = $this->doApiRequest( [
'action' => 'login',
'lgname' => $lgName,
- 'lgpassword' => 'foobaz',
+ 'lgpassword' => $password,
] );
$result = $ret[0];
@@ -270,7 +271,7 @@
'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 d637704..cb27fde 100644
--- a/tests/phpunit/includes/user/BotPasswordTest.php
+++ b/tests/phpunit/includes/user/BotPasswordTest.php
@@ -244,8 +244,9 @@
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',
--
To view, visit https://gerrit.wikimedia.org/r/310462
To
