Several bugs / comments

[RobThree]

verifyCode discrepancy bug

verifyCode()'s 3rd argument, $discrepancy is specified in units of 30s (so a discrepancy of 1 calculates codes for -1, 0, 1 timeslices e.g. your device's time and the server can be off +/- about 45 seconds in both directions on average or 1m30s worst-case). A $discrepancy of 2 should calculate for -2, -1, 0, 1, 2. However, the loop iterator $i is added by increments of 1. The code in the verifyCode() method is currently:

$calculatedCode = $this->getCode($secret, $currentTimeSlice + $i);

It should read:

$calculatedCode = $this->getCode($secret, $currentTimeSlice + ($i * 30));

getQRCodeGoogleUrl url encoding bug

The method getQRCodeGoogleUrl() encodes the string incorrectly:

$urlencoded = urlencode('otpauth://totp/'.$name.'?secret='.$secret.'');
return 'https://chart.googleapis.com/chart?chs=200x200&chld=M|0&cht=qr&chl='.$urlencoded.'';

Should read:

$url = 'otpauth://totp/'.urlencode($name).'?secret='.urlencode($secret);
return 'https://chart.googleapis.com/chart?chs=200x200&chld=M|0&cht=qr&chl='.urlencode($url);

See KeyUriFormat for what should be encoded how. (Also: an Issuer value is "STRONGLY RECOMMENDED"' this should be easy to add/implement. I would also add support for the Algorithm, Digits, Counter and Period values which also should be easy to add/implement (even though the current Google Authenticator implementations still ignore some of them).

createSecret uses non-cryptographically secure RNG

Not exactly a bug but why not use a cryptographically secure RNG?

public function createSecret($secretLength = 16)
{
    $validChars = $this->_getBase32LookupTable();
    unset($validChars[32]);

    $secret = '';
    for ($i = 0; $i < $secretLength; $i++) {
        $secret .= $validChars[array_rand($validChars)];
    }
    return $secret;
}

Should/could be:

public function CreateSecret($length = 16) {
    $validChars = $this->_getBase32LookupTable();
    $secret = '';
    $rnd = openssl_random_pseudo_bytes($length);
    for ($i = 0; $i < $length; $i++)
        $secret .= $validChars[ord($rnd[$i]) & 31];  //Mask out left 3 bits for 0-31 values
    return $secret;
}

The left 3 bits are masked out resulting in values ranging from 0-31. Because we require a nice even 5 bits we can simply mask them out. I'm no security expert but should we need values in a range of 0-100 we can't use a nice 'whole' number of bits which usually results in people using a modulo 100 operation intruducing a bias towards some numbers. This should not be the case in this code since we require an 'exact' amount of 5 bits and simply discard the rest.

Also; there's no more need to unset the last element of the array (the padding char) since it is simply never referred to.

Boobies!

The getCode() method mentions a nipple in the comments; this should probably read nibble ;-)

Others

• I haven't gotten around to the _base32Decode() method but that looks overly complex too (which is just a gut-feel, I won't know until I have had time to examine it more closely).
• The _base32Encode() method is never used and should be removed.
• Furthermore I'd suggest a new method getQRCodeImage($name, $secret, ...) that returns a data-uri so can easily embed an image in a page:

echo '<img src="' . $ga->getQRCodeImage('Test', $secret) . '">';

This method would return a string like: data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAG8AAABvAQMAAADYCwwjAAAABlBMVEX///8AAABVwtN+AAABHklEQVQ4jdXTu7GGIBAF4OMQkEkDztAGmS1pA1dtQFsiow1mbEAzAse9i859RCzpz5h8BLB7ZIFPXIZoS7F1iiiKbKG2ZE9SMyrY72vfTWnnE2q4BXs4VckZGFBHqFXT/yIL5H5Xt/P3236BvBofTfgLs0DjadbdFDC+RxXZajSJbmcpyTTe3jo2nKSX2Xig70b+rUEmYGdHi+9+GiqRO81hgu4+igTUAa7fnl6mCZ3hJD0tQSa0PYNdCIOGyNblZBav3qrKBL+r/hkHVBDXFDhJzh8ijedbroaI3o6KzDvJHuCWZfKLnR0GnjXIzPPLd1FsdQXz/PKw013HJcUvdzXPOIjMSQZ19FEmV5UfwL5qmXl+OXkfx+eiMj9vfQNOb/aNopC1ZAAAAABJRU5ErkJggg==

Example

You could use PHPQRCode for this. The method would look something like this: (Refer to this post for more info)

public function getQRCodeImage($name, $secret, $size = 3, $margin = 4, $level = 'L') {
    require_once 'phpqrcode.php';

    ob_start();
    QRCode::png($this->getQRText($name, $secret), null, constant('QR_ECLEVEL_'.$level), $size, $margin);
    $result = base64_encode(ob_get_contents());
    ob_end_clean();
    return 'data:image/png;base64,'.$result;
}

This method refers to getQRText() which I simple refactored out of getQRCodeGoogleUrl to it's own method so both getQRCodeGoogleUrl and getQRText can use it.

private function getQRText($name, $secret) {
    return 'otpauth://totp/'.urlencode($name).'?secret='.urlencode($secret);
}

When the getQRCodeImage() would be implemented/added you'll gain the following benefits:

• Solves Possible security problem
• No more reliant on 3rd parties (granted, Google isn't down often but it CAN be; also see previous point: proxies/caches/whatevers could cause problems or even be an extra attack vector (MITM))
• Saves at least one more HTTP request (the page is a bit bigger in terms of bytes but a roundtrip to a 3rd party is saved)
• The image is harder to 'capture' for MITM's (not impossible) and not saved in browser caches, url histories etc.

Hope this helps.

[PHPGangsta]

Hi Rob,

I wrote this a long time ago, let's see:

Your first suggestion is not correct I think. $currentTimeSlice is a "slice", not a timestamp. This slice is a "range" of 30 seconds, see line 91, the current time is devided by 30. That's why "just" $i is added and not $i*30. So I think the current code is correct.

Your second comment would be correct if the otpauth://-URL would be directly used somewhere. But here it is added as a GET-Parameter to the Google-URL, that's why it has to be completely urlencoded. In your version you end up having two question marks in the resulting URL-string. You are right regarding the additional parameter "Issuer" that should be added. The Counter is just used if the type is hotp, but we are doing totp here.

Your version of createSecret() adds an addition external dependency on OpenSSL. Not all systems have it installed. There is a nice Pull Request from Steve, he is only using openssl_random_pseudo_bytes() if it exists, otherwise he falls back to array_rand(), which I would say is a better solution: https://github.com/PHPGangsta/GoogleAuthenticator/pull/10

Good catch regarding the "nipple" ;-)

You are right, _base32Encode() is not used in the current version of the code, I think it was used in one of the first (internal) versions before putting it on GitHub. It can be removed.

I'm not sure if I want to have an external dependency on PHPQRCode. It's of cause a good idea not to use Google and external HTTP requests, but it adds an external dependency to a library not being on GitHub as far as I can see (it's on sourceforge).

Thank you for your suggestions! I would like to hear from you!

Do you want to send a pull request regarding the "nipple" and "base32Encode()", or should I do the necessary changes?

Michael

[RobThree]

Your first suggestion is not correct I think. $currentTimeSlice is a "slice", not a timestamp. This slice is a "range" of 30 seconds, see line 91, the current time is devided by 30. That's why "just" $i is added and not $i*30. So I think the current code is correct.

Looking over this again I see you are correct. Must've misread it or had one too many beer :stuck_out_tongue_winking_eye:

Your second comment would be correct if the otpauth://-URL would be directly used somewhere. But here it is added as a GET-Parameter to the Google-URL, that's why it has to be completely urlencoded. In your version you end up having two question marks in the resulting URL-string.

urlencode should always be used on single values. The parameters are all, and each, a single value that requires encoding. However, you then send the entire url as as single value (parameter) to Google. Because the entire URL is a single value it requires URLEncoding (again). You should be able to test/reproduce this quite easily (use a (nonsensical) name like test & testing = ?foo? for example). Your method should display incorrectly in the authenticator app (if it doesn't it may compensate for this (common) mistake; try other apps too!).

You are right regarding the additional parameter "Issuer" that should be added. The Counter is just used if the type is hotp, but we are doing totp here.

I never implemented totp either so I left that one out too. It was just a suggestion (though I would also consider Algorithm and Digits) as I mentioned.

Your version of createSecret() adds an addition external dependency on OpenSSL. Not all systems have it installed.

That is a good, fair, point. I would, however, consider allowing a "pluggable" random-bytes-provider so people can easily provide the function with their own provider of choice (much like I support different QR code providers by providing a simple interface. I am by no means very skilled in PHP (I'm more of a .Net kinda-guy) so I wasn't aware that openssl_random_pseudo_bytes() isn't some "built-in BCL-sort-of functionality" but relies on Cryptography Extensions being installed. Update: Implemented it :)

There is a nice Pull Request from Steve, he is only using openssl_random_pseudo_bytes() if it exists, otherwise he falls back to array_rand(), which I would say is a better solution:

This could very well be. As said: I'm not much of a PHP guy so all I can say is that, to me, it at least looks better than the current implementation. (Edit: However, I have commented on that PR as well :stuck_out_tongue_closed_eyes: )

Good catch regarding the "nipple" ;-)

A dirty mind is a joy forever :stuck_out_tongue_winking_eye:

You are right, _base32Encode() is not used in the current version of the code, I think it was used in one of the first (internal) versions before putting it on GitHub. It can be removed.

Yay! :stuck_out_tongue_winking_eye:

I'm not sure if I want to have an external dependency on PHPQRCode. It's of cause a good idea not to use Google and external HTTP requests, but it adds an external dependency to a library not being on GitHub as far as I can see (it's on sourceforge).

As said: why not give people a choice. It's part of good SOLID design :stuck_out_tongue_winking_eye:

Thank you for your suggestions! I would like to hear from you!

You're welcome!

Do you want to send a pull request regarding the "nipple" and "base32Encode()", or should I do the necessary changes?

By all means go ahead; I'm currently not in a position to do (well tested) PR's.

Rob

[PHPGangsta]

I guess all comments/ideas are either fixed/implemented by you or me ;-)