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.
React
Typescript
Django
Laravel
Facebook
Microsoft
Google
Alibaba
D3
Tencent