2FA Changes

- Default interval now 30 seconds (breaking!)
- Don't add default parameters to QR code (smaller)

Fixes #22

classes/twofa.class.php

@@ -10,7 +10,7 @@ class TwoFactorAuth {
   private static $_base32lookup = array();
   private static $_supportedalgos = array('sha1', 'sha256', 'sha512', 'md5');
 
-  function __construct($issuer = null, $digits = 6, $period = 60, $algorithm = 'sha1') {
+  function __construct($issuer = null, $digits = 6, $period = 30, $algorithm = 'sha1') {
     $this->issuer = $issuer;
     $this->digits = $digits;
     $this->period = $period;
@@ -25,21 +25,17 @@ class TwoFactorAuth {
     self::$_base32lookup = array_flip(self::$_base32);
   }
 
-  /**
-   * Create a new secret
-   */
   public function createSecret($bits = 80) {
     $secret = '';
-    $bytes = ceil($bits / 5);   //We use 5 bits of each byte (since we have a 32-character 'alphabet' / BASE32)
+    $bytes = ceil($bits / 5); //We use 5 bits of each byte (since we have a 32-character 'alphabet' / BASE32)
     $rnd = random_bytes($bytes);
-    for ($i = 0; $i < $bytes; $i++)
-      $secret .= self::$_base32[ord($rnd[$i]) & 31];  //Mask out left 3 bits for 0-31 values
+    for ($i = 0; $i < $bytes; $i++) {
+      $secret .= self::$_base32[ord($rnd[$i]) & 31]; //Mask out left 3 bits for 0-31 values
+    }
     return $secret;
   }
 
-  /**
-   * Calculate the code with given secret and point in time
-   */
+  // Calculate the code with given secret and point in time
   public function getCode($secret, $time = null) {
     $secretkey = $this->base32Decode($secret);
 
@@ -52,42 +48,20 @@ class TwoFactorAuth {
     return str_pad($value % pow(10, $this->digits), $this->digits, '0', STR_PAD_LEFT);
   }
 
-  /**
-   * Check if the code is correct. This will accept codes starting from ($discrepancy * $period) sec ago to ($discrepancy * period) sec from now
-   */
+  // Check if the code is correct. This will accept codes starting from now +/- ($discrepancy * period) seconds
   public function verifyCode($secret, $code, $discrepancy = 1, $time = null) {
     $result = false;
     $timetamp = $this->getTime($time);
 
-    // To keep safe from timing-attachs we iterate *all* possible codes even though we already may have verified a code is correct
-    for ($i = -$discrepancy; $i <= $discrepancy; $i++)
-      $result |= $this->codeEquals($this->getCode($secret, $timetamp + ($i * $this->period)), $code);
+    // Always iterate all possible codes to prevent timing-attacks
+    for ($i = -$discrepancy; $i <= $discrepancy; $i++) {
+      $result |= hash_equals($this->getCode($secret, $timetamp + ($i * $this->period)), $code);
+    }
 
     return (bool)$result;
   }
 
-  /**
-   * Timing-attack safe comparison of 2 codes (see http://blog.ircmaxell.com/2014/11/its-all-about-time.html)
-   */
-  private function codeEquals($safe, $user) {
-    if (function_exists('hash_equals')) {
-      return hash_equals($safe, $user);
-    } else {
-      // In general, it's not possible to prevent length leaks. So it's OK to leak the length. The important part is that
-      // we don't leak information about the difference of the two strings.
-      if (strlen($safe)===strlen($user)) {
-        $result = 0;
-        for ($i = 0; $i < strlen($safe); $i++)
-          $result |= (ord($safe[$i]) ^ ord($user[$i]));
-        return $result === 0;
-      }
-    }
-    return false;
-  }
-
-  /**
-   * Get data-uri of QRCode
-   */
+  // Get data-uri of QRCode
   public function getQRCodeImageAsDataUri($label, $secret, $size = 300) {
 
     if (exec('which qrencode')) {
@@ -120,16 +94,14 @@ class TwoFactorAuth {
     return (int)floor($time / $this->period) + ($offset * $this->period);
   }
 
-  /**
-   * Builds a string to be encoded in a QR code
-   */
+  // Builds a string to be encoded in a QR code
   public function getQRText($label, $secret) {
-    return 'otpauth://totp/' . rawurlencode($label)
-      . '?secret=' . rawurlencode($secret)
-      . '&issuer=' . rawurlencode($this->issuer)
-      . '&period=' . intval($this->period)
-      . '&algorithm=' . rawurlencode(strtoupper($this->algorithm))
-      . '&digits=' . intval($this->digits);
+    $QRText = 'otpauth://totp/'.rawurlencode($label).'?secret='.rawurlencode($secret);
+    $QRText .= ($this->issuer) ? '&issuer='.rawurlencode($this->issuer) : '';
+    $QRText .= ($this->period != 30) ? '&period='.intval($this->period) : '';
+    $QRText .= ($this->algorithm != 'sha1') ? '&algorithm='.rawurlencode(strtoupper($this->algorithm)) : '';
+    $QRText .= ($this->digits != 6) ? '&digits='.intval($this->digits) : '';
+    return $QRText;
   }
 
   private function base32Decode($value) {
@@ -145,8 +117,9 @@ class TwoFactorAuth {
     $blocks = trim(chunk_split(substr($buffer, 0, $length - ($length % 8)), 8, ' '));
 
     $output = '';
-    foreach (explode(' ', $blocks) as $block)
+    foreach (explode(' ', $blocks) as $block) {
       $output .= chr(bindec(str_pad($block, 8, 0, STR_PAD_RIGHT)));
+    }
 
     return $output;
   }

sections/user/edit.php

@@ -27,7 +27,7 @@ $DB->query("
   WHERE m.ID = '".db_string($UserID)."'");
 list($Username, $TwoFactor, $Email, $IRCKey, $Paranoia, $Info, $Avatar, $StyleID, $StyleURL, $SiteOptions, $UnseededAlerts, $DownloadAlt, $Class, $InfoTitle) = $DB->next_record(MYSQLI_NUM, array(4, 9));
 
-$TwoFA = new TwoFactorAuth(SITE_NAME);
+$TwoFA = new TwoFactorAuth();
 
 $Email = apc_exists('DBKEY') ? DBCrypt::decrypt($Email) : '[Encrypted]';