PHPGangsta / GoogleAuthenticator

PHP class to generate and verify Google Authenticator 2-factor authentication
http://phpgangsta.de/4376
BSD 2-Clause "Simplified" License
2.26k stars 698 forks source link

Can't verify code from device #61

Closed karabone closed 5 years ago

karabone commented 5 years ago

Hello. I has been using this library for many months on my website and it worked alright for a long time, but we changed hosting-provider and we faced the problem. Users reported that they can't log in with 2fa or can't set 2fa on their accounts.

I started researching. Codes generated on my device (Iphone 8 with latest Google Authenticator App from AppStore) and codes on server are different in ~80-90% of my tries, at that script returns successful results pretty chaotically. For example, i can get 20 false results after calling verifyCode() and then get 1-3 true results in a row or get only 1 true result after 30 tries.

Everywhere on the web i can see advice to sync time in GA app, but, first of all, there is no such settings on iOS GA app no more (i guess that app does it authomaticly). Secondly, i checked time on my server with command date and it's normal UTC time that fully equal with UTC time i checked with Google.

I added var_dump($calculatedCode) inside forloop in method verifyCode(), and i get a strange result after calling verifyCode method with code from my device:

string(6) "695201" string(6) "781292" string(6) "000000" string(6) "419982" string(6) "774585" bool(false)

It's 5 codes (with discrepancy = 2) generated by php script, and sometimes it prints one or couple 000000 codes, but sometimes all 5 codes contain numbers.

Also, i see that error in backend log:

PHP Warning: unpack(): Type N: not enough input, need 4, have 0 in /html/system/libs/ga.class.php on line 83

It appears every time when method getCode() returns 000000 code and i guess it's because of some wrong data put in unpack() func:

In getCode(): // Unpak binary value $value = unpack('N', $hashpart);

What's the reason of that? How can i solve that problem?

I tried to use another lib https://github.com/Dolondro/google-authenticator but i faced absolutly same problem with same errors in backend log.

PHPGangsta commented 5 years ago

That 000000 looks suspicious.

Which version of PHP are you using on the new server, which on the old server? Can you compare the modules/extensions on both servers? (php -m or in phpinfo())

Not sure if it might be a problem with pack(), hash_hmac() or unpack() somehow...

Can you insert some more var_dump()s in the function, for example check $result after the hash_hmac() call, and $time after pack().

Are you using the correct Base32 class, which should have been installed via composer https://github.com/Dolondro/google-authenticator/blob/master/composer.json#L12

karabone commented 5 years ago

Which version of PHP are you using on the new server, which on the old server? Can you compare the modules/extensions on both servers? (php -m or in phpinfo())

Unfortunately, i can't give exact info about php version on old server because it has been deleted summer 2018 and i just postponed this problem for a long time.

Now it's:

php -v PHP 5.6.37-1+ubuntu16.04.1+deb.sury.org+1 (cli) Copyright (c) 1997-2016 The PHP Group Zend Engine v2.6.0, Copyright (c) 1998-2016 Zend Technologies with Zend OPcache v7.0.6-dev, Copyright (c) 1999-2016, by Zend Technologies

It's seems like it was php 5.6.* too, maybe litle older (5.6.35 or 5.6.36) on old server.

php -m [PHP Modules] bcmath bz2 calendar Core ctype curl date dom ereg exif fileinfo filter ftp gd gettext gmp hash iconv igbinary intl json libxml mbstring mcrypt memcache memcached mhash msgpack mysql mysqli mysqlnd openssl pcntl pcre PDO pdo_mysql pdo_pgsql pdo_sqlite pgsql Phar posix readline Reflection session shmop SimpleXML sockets SPL sqlite3 standard sysvmsg sysvsem sysvshm tidy tokenizer wddx xml xmlreader xmlwriter xsl Zend OPcache zip zlib zmq

[Zend Modules] Zend OPcache

Can you insert some more var_dump()s in the function, for example check $result after the hash_hmac() call, and $time after pack().

I put inside getCode() method var_dump() after every modification of any variables.

` public function getCode($secret, $timeSlice = null) { if ($timeSlice === null) { $timeSlice = floor(time() / 30); }

    echo "<br/><br/>Calling getCode()<br/>";

    echo "<br/><br/>secret dump:<br/>";

    var_dump($secret);

    echo "<br/><br/>timeSlice dump:<br/>";

    var_dump($timeSlice);

    $secretkey = $this->_base32Decode($secret);

    echo "<br/><br/>secretkey dump:<br/>";

    var_dump($secretkey);

    // Pack time into binary string
    $time = chr(0).chr(0).chr(0).chr(0).pack('N*', $timeSlice);

    echo "<br/><br/>time dump:<br/>";

    var_dump($time);

    // Hash it with users secret key
    $hm = hash_hmac('SHA1', $time, $secretkey, true);

    echo "<br/><br/>hm dump:<br/>";

    var_dump($hm);

    // Use last nipple of result as index/offset
    $offset = ord(substr($hm, -1)) & 0x0F;
    // grab 4 bytes of the result
    $hashpart = substr($hm, $offset, 4);

    echo "<br/><br/>offset dump:<br/>";

    var_dump($offset);

    echo "<br/><br/>hashpart dump:<br/>";

    var_dump($hashpart);

    // Unpak binary value
    $value = unpack('N', $hashpart);

    echo "<br/><br/>value dump:<br/>";

    var_dump($value);

    $value = $value[1];

    echo "<br/><br/>value dump:<br/>";

    var_dump($value);

    // Only 32 bits
    $value = $value & 0x7FFFFFFF;

    echo "<br/><br/>value dump:<br/>";

    var_dump($value);

    $modulo = pow(10, $this->_codeLength);

    echo "<br/><br/>modulo dump:<br/>";

    var_dump($modulo);

    return str_pad($value % $modulo, $this->_codeLength, '0', STR_PAD_LEFT);
}`

You can see 2 examples of my tries (1 successful and 1 unsuccessful):

Successful try:

Calling getCode()

secret dump: string(16) "LPASU75O4PH6FPQG"

timeSlice dump: float(51766341)

secretkey dump: string(10) "[�*����"

time dump: string(8) "�E"

hm dump: string(20) "]�����NNL�y� ��"

offset dump: int(7)

hashpart dump: string(4) "NL"

value dump: array(1) { [1]=> int(1310524748) }

value dump: int(1310524748)

value dump: int(1310524748)

modulo dump: int(1000000)

Calculated code with currentTimeSlice + i = 51766341:

string(6) "524748"

Calling getCode()

secret dump: string(16) "LPASU75O4PH6FPQG"

timeSlice dump: float(51766342)

secretkey dump: string(10) "[�*����"

time dump: string(8) "�F"

hm dump: string(20) "a2�W��^�&��l����ө�k"

offset dump: int(11)

hashpart dump: string(6) "���ө�"

value dump: array(1) { [1]=> int(3472345043) }

value dump: int(3472345043)

value dump: int(1324861395)

modulo dump: int(1000000)

Calculated code with currentTimeSlice + i = 51766342:

string(6) "861395"

Calling getCode()

secret dump: string(16) "LPASU75O4PH6FPQG"

timeSlice dump: float(51766343)

secretkey dump: string(10) "[�*����"

time dump: string(8) "�G"

hm dump: string(20) "9� �p�bt@��E/E*"

offset dump: int(10)

hashpart dump: string(4) "t@�"

value dump: array(1) { [1]=> int(1946370228) }

value dump: int(1946370228)

value dump: int(1946370228)

modulo dump: int(1000000)

Calculated code with currentTimeSlice + i = 51766343:

string(6) "370228" bool(true)

Unsuccessful try with 000000 code:

Calling getCode()

secret dump: string(16) "IAR23PCWGFVDL5EA"

timeSlice dump: float(51766348)

secretkey dump: string(10) "@#��V1j5�"

time dump: string(8) "�L"

hm dump: string(20) "���Z>��@P ��G�@���"

offset dump: int(1)

hashpart dump: string(6) "��Z>��"

value dump: array(1) { [1]=> int(4023147070) }

value dump: int(4023147070)

value dump: int(1875663422)

modulo dump: int(1000000)

Calculated code with currentTimeSlice + i = 51766348:

string(6) "663422"

Calling getCode()

secret dump: string(16) "IAR23PCWGFVDL5EA"

timeSlice dump: float(51766349)

secretkey dump: string(10) "@#��V1j5�"

time dump: string(8) "�M"

hm dump: string(20) "-~r-8�O���{�uB���7"

offset dump: int(7)

hashpart dump: string(4) "O���"

value dump: array(1) { [1]=> int(1335796469) }

value dump: int(1335796469)

value dump: int(1335796469)

modulo dump: int(1000000)

Calculated code with currentTimeSlice + i = 51766349:

string(6) "796469"

Calling getCode()

secret dump: string(16) "IAR23PCWGFVDL5EA"

timeSlice dump: float(51766350)

secretkey dump: string(10) "@#��V1j5�"

time dump: string(8) "�N"

hm dump: string(20) "�Je��ɒ�x:ۘ/��ٜ"

offset dump: int(9)

hashpart dump: string(5) "��"

value dump: array(1) { [1]=> int(2815428119) }

value dump: int(2815428119)

value dump: int(667944471)

modulo dump: int(1000000)

Calculated code with currentTimeSlice + i = 51766350:

string(6) "944471"

Calling getCode()

secret dump: string(16) "IAR23PCWGFVDL5EA"

timeSlice dump: float(51766351)

secretkey dump: string(10) "@#��V1j5�"

time dump: string(8) "�O"

hm dump: string(20) "^� �ʞH{)����b���:"

offset dump: int(10)

hashpart dump: string(8) "����b���"

value dump: array(1) { [1]=> int(4108579827) }

value dump: int(4108579827)

value dump: int(1961096179)

modulo dump: int(1000000)

Calculated code with currentTimeSlice + i = 51766351:

string(6) "096179"

Calling getCode()

secret dump: string(16) "IAR23PCWGFVDL5EA"

timeSlice dump: float(51766352)

secretkey dump: string(10) "@#��V1j5�"

time dump: string(8) "�P"

hm dump: string(20) " �y�%� ��8X���.^"

offset dump: int(14)

hashpart dump: string(3) "�.^"

value dump: bool(false)

value dump: NULL

value dump: int(0)

modulo dump: int(1000000)

Calculated code with currentTimeSlice + i = 51766352:

string(6) "000000" bool(false)

keykrussha commented 5 years ago

Got same problem. Googled over all the internet. No clues what could casue this. Added samevar_dump($calculatedCode) and called verifyCode with $discrepancy = 20:

string(6) "000000" string(6) "891727" string(6) "796682" string(6) "000000" string(6) "297465" string(6) "535683" string(6) "000000" string(6) "000000" string(6) "064081" string(6) "295109" string(6) "305938" string(6) "000000" string(6) "473683" string(6) "903349" string(6) "171135" string(6) "497309" string(6) "420920" string(6) "568475" string(6) "000000" string(6) "000000" string(6) "402166"

Generated zeros are accompainged with the same warning: Warning: unpack(): Type N: not enough input, need 4, have 0 in /var/www/gauth/GoogleAuthenticator.php on line 81

What is more. Sometimes the calculated code equals to what's generated in Google Authenticator android app. Sometimes not. Almost half of codes generated are wrong, half correct. Sometimes there are no zero-codes. Sometimes there are. More or less, always different. Once upon a time I've even managed to get no warnings and no zero-codes in a sequence =D

So, the code calculation is inconsistent due to... I can't even guess what. The only two inputs are VERY consistent: constant secret key and timestamp which is always of same format, but different value. Maybe it is PHP's type-determination problem? Some timestamps are treated as floats, some as integers while calculated? For example. Just guesses.

I've tested this python implementaion: https://github.com/tadeck/onetimepass Generates correct codes, same as android app. 100% stable. So, that's not something machine- or OS- related.

Linux 4.4.0-144-generic #170~14.04.1-Ubuntu
PHP 5.5.9-1ubuntu4.27
Zend Engine v2.5.0, Copyright (c) 1998-2014 Zend Technologies
    with Zend OPcache v7.0.3, Copyright (c) 1999-2014, by Zend Technologies
keykrussha commented 5 years ago

Workaround

Took a time to look at @karabone var_dumps. An unseccessful case when 000000 code is generated:

`hm dump: string(20) " �y�%�� ��8X���.^"

offset dump: int(14)

hashpart dump: string(3) "�.^"`

The hm string has less than 20 printed symbols. And the substring extracted symbols from 15th position instead of 14 for some reason. So, hashpart is invalid. Obviously, strings and\or string functions in PHP do something wrong with certain binary sequences. Life's to short to dig into PHP... hm... features, so I've just made a workaround using hex representation of hash.

UPD Managed to find the root cause of such behavior. See the next comment for a real fix. Try the following workaround only if you can't change PHP configuration.

In function getCode:

// last arg: When set to TRUE, outputs raw binary data. FALSE outputs lowercase hexits.
// hex string would be more stable in PHP I guess.
$hm = hash_hmac('SHA1', $time, $secretkey, false);

// last nibble is the last hex symbol now. Just turn it to decimal.
$offset = hexdec(substr($hm, -1));

// as each byte is 2 hex symbols, multiply 'substr' args by 2
// turn resulting hex into binary for compliance with further code
$hashpart = hex2bin(substr($hm, $offset * 2, 8));
keykrussha commented 5 years ago

Solution found! Set mbstring.func_overload to 0 in your php configuration if you don't really need it in your project.

I had mbstring.func_overload=7 in php.ini mb_substr() was called instead of substr() every time. https://www.php.net/manual/en/mbstring.overload.php

karabone commented 5 years ago

keykrussha, You are my savior! 1st solution is working! 2st is not, but my problem is solved!