Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Fix login API for users with @ in their usernames

Browse source 
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
This commit is contained in:
Gergő Tisza 2016-09-13 23:25:49 +00:00
parent 47d437a6c9
commit af37a4c77d
3 changed files with 12 additions and 8 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

10
includes/user/BotPassword.php
View file

@ -411,11 +411,13 @@ class BotPassword implements IDBAccessObject {
*/ */
public static function canonicalizeLoginData( $username, $password ) { public static function canonicalizeLoginData( $username, $password ) {
$sep = BotPassword::getSeparator(); $sep = BotPassword::getSeparator();
if ( strpos( $username, $sep ) !== false ) { // the strlen check helps minimize the password information obtainable from timing
// the separator is not valid in usernames so this must be a bot login if ( strlen( $password ) >= 32 && strpos( $username, $sep ) !== false ) {
return [ $username, $password, 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 ) { } elseif ( strlen( $password ) > 32 && strpos( $password, $sep ) !== false ) {
// the strlen check helps minimize the password information obtainable from timing
$segments = explode( $sep, $password ); $segments = explode( $sep, $password );
$password = array_pop( $segments ); $password = array_pop( $segments );
$appId = implode( $sep, $segments ); $appId = implode( $sep, $segments );

7
tests/phpunit/includes/api/ApiLoginTest.php
View file

@ -231,10 +231,11 @@ class ApiLoginTest extends ApiTestCase {
$centralId = CentralIdLookup::factory()->centralIdFromLocalUser( $user->getUser() ); $centralId = CentralIdLookup::factory()->centralIdFromLocalUser( $user->getUser() );
$this->assertNotEquals( 0, $centralId, 'sanity check' ); $this->assertNotEquals( 0, $centralId, 'sanity check' );
$password = 'ngfhmjm64hv0854493hsj5nncjud2clk';
$passwordFactory = new PasswordFactory(); $passwordFactory = new PasswordFactory();
$passwordFactory->init( RequestContext::getMain()->getConfig() ); $passwordFactory->init( RequestContext::getMain()->getConfig() );
// A is unsalted MD5 (thus fast) ... we don't care about security here, this is test only // 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 = wfGetDB( DB_MASTER );
$dbw->insert( $dbw->insert(
@ -255,7 +256,7 @@ class ApiLoginTest extends ApiTestCase {
$ret = $this->doApiRequest( [ $ret = $this->doApiRequest( [
'action' => 'login', 'action' => 'login',
'lgname' => $lgName, 'lgname' => $lgName,
'lgpassword' => 'foobaz', 'lgpassword' => $password,
] ); ] );
$result = $ret[0]; $result = $ret[0];
@ -270,7 +271,7 @@ class ApiLoginTest extends ApiTestCase {
'action' => 'login', 'action' => 'login',
'lgtoken' => $token, 'lgtoken' => $token,
'lgname' => $lgName, 'lgname' => $lgName,
'lgpassword' => 'foobaz', 'lgpassword' => $password,
], $ret[2] ); ], $ret[2] );
$result = $ret[0]; $result = $ret[0];

3
tests/phpunit/includes/user/BotPasswordTest.php
View file

@ -244,8 +244,9 @@ class BotPasswordTest extends MediaWikiTestCase {
return [ return [
[ 'user', 'pass', false ], [ 'user', 'pass', false ],
[ 'user', 'abc@def', false ], [ 'user', 'abc@def', false ],
[ 'legacy@user', 'pass', false ],
[ 'user@bot', '12345678901234567890123456789012', [ 'user@bot', '12345678901234567890123456789012',
[ 'user@bot', '12345678901234567890123456789012', false ] ], [ 'user@bot', '12345678901234567890123456789012', true ] ],
[ 'user', 'bot@12345678901234567890123456789012', [ 'user', 'bot@12345678901234567890123456789012',
[ 'user@bot', '12345678901234567890123456789012', true ] ], [ 'user@bot', '12345678901234567890123456789012', true ] ],
[ 'user', 'bot@12345678901234567890123456789012345', [ 'user', 'bot@12345678901234567890123456789012345',