Changeset 501

Timestamp:

Nov 16, 2014 11:07:01 AM (10 years ago)

Author:

anonymous

Message:

Optimizing auth and csrf token.

Location:

trunk/lib

Files:

• App.inc.php (modified) (6 diffs)
• Auth_SQL.inc.php (modified) (13 diffs)

trunk/lib/App.inc.php

@@ -80 +80 @@
         'ssl_enabled' => false,
 
-        // Use CSRF tokens.
+        // Use CSRF tokens. See notes in the getCSRFToken() method.
         'csrf_token_enabled' => true,
+        // Form tokens will expire after this duration, in seconds.
+        'csrf_token_timeout' => 259200, // 259200 seconds = 3 days.
         'csrf_token_name' => 'csrf_token',
-        'csrf_token_timeout' => 86400, // In seconds. This causes form tokens to be unusable after this duration. This might only cause problems when opening forms in multiple tabs left open beyond the timeout duration. But usually their session will timeout first, and they'll receive new tokens when they load the form again..
 
         // HMAC signing method
@@ -1018 +1019 @@
         // This token can be validated upon form submission with $app->verifyCSRFToken() or $app->requireValidCSRFToken()
         if ($this->getParam('csrf_token_enabled') && $include_csrf_token) {
-            printf('<input type="hidden" name="%s" value="%s" />', $this->getParam('csrf_token_name'), $this->recycleCSRFToken());
+            printf('<input type="hidden" name="%s" value="%s" />', $this->getParam('csrf_token_name'), $this->getCSRFToken());
         }
     }
@@ -1041 +1042 @@
 
     /*
-    * Generate a csrf_token, saving it to the session and returning its value.
+    * Generate a csrf_token if it doesn't exist or is expired, save it to the session and return its value.
+    * Otherwise just return the current token.
+    * Details on the synchronizer token pattern:
     * https://www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF)_Prevention_Cheat_Sheet#General_Recommendation:_Synchronizer_Token_Pattern
-    * @access   public
-    * @return   string  The new csrf_token.
-    * @author   Quinn Comendant <quinn@strangecode.com>
-    * @version  1.0
-    * @since    15 Nov 2014 17:53:51
-    */
-    public function generateCSRFToken()
-    {
-        return $_SESSION['_app'][$this->_ns]['csrf_tokens'][] = addSignature(time(), null, 64);
-    }
-
-    /*
-    * Update the csrf_token to a new value if it hasn't been set yet or has expired.
-    * Save the previous csrf_token in the session to ensure continuity of currently open sessions.
     *
     * @access   public
-    * @return   string The current csrf_token
+    * @return   string The new or current csrf_token
     * @author   Quinn Comendant <quinn@strangecode.com>
     * @version  1.0
     * @since    15 Nov 2014 17:57:17
     */
-    public function recycleCSRFToken()
-    {
-        if (!isset($_SESSION['_app'][$this->_ns]['csrf_tokens'])) {
-            // No token yet; generate one and return it.
-            $_SESSION['_app'][$this->_ns]['csrf_tokens'] = array();
-            $return = $this->generateCSRFToken();
-        }
-        if (removeSignature(end($_SESSION['_app'][$this->_ns]['csrf_tokens'])) + $this->getParam('csrf_token_timeout') < time()) {
-            // Newest token is expired; prune array of tokens and generate new token.
-            // We'll save the 10-most-recent tokens. This allows the user to submit up to 5 forms saved in previously opened tabs with expired tokens (loading a form will prune one token, and submitting a form will prune one token, thus 10 = 5).
-            $_SESSION['_app'][$this->_ns]['csrf_tokens'] = array_slice($_SESSION['_app'][$this->_ns]['csrf_tokens'], -10, 10);
-            $return = $this->generateCSRFToken();
+    public function getCSRFToken()
+    {
+        if (!isset($_SESSION['_app'][$this->_ns]['csrf_token']) || (removeSignature($_SESSION['_app'][$this->_ns]['csrf_token']) + $this->getParam('csrf_token_timeout') < time())) {
+            // No token, or token is expired; generate one and return it.
+            return $_SESSION['_app'][$this->_ns]['csrf_token'] = addSignature(time(), null, 64);
         }
         // Current token is not expired; return it.
-        $return = end($_SESSION['_app'][$this->_ns]['csrf_tokens']);
-        $app =& App::getInstance();
-        return $return;
+        return $_SESSION['_app'][$this->_ns]['csrf_token'];
     }
 
@@ -1093 +1073 @@
     * @since    15 Nov 2014 18:06:55
     */
-    public function verifyCSRFToken($csrf_token)
-    {
-        $app =& App::getInstance();
+    public function verifyCSRFToken($user_submitted_csrf_token)
+    {
 
         if (!$this->getParam('csrf_token_enabled')) {
-            $app->logMsg(sprintf('%s method called, but csrf_token_enabled=false', __FUNCTION__), LOG_ERR, __FILE__, __LINE__);
-            return false;
-        }
-        if ('' == trim($csrf_token)) {
-            $app->logMsg(sprintf('Empty string failed CSRF verification.', null), LOG_NOTICE, __FILE__, __LINE__);
-            return false;
-        }
-        if (!verifySignature($csrf_token, null, 64)) {
-            $app->logMsg(sprintf('Input failed CSRF verification (invalid signature in %s).', $csrf_token), LOG_WARNING, __FILE__, __LINE__);
-            return false;
-        }
-        $this->recycleCSRFToken();
-        if (!in_array($csrf_token, $_SESSION['_app'][$this->_ns]['csrf_tokens'])) {
-            $app->logMsg(sprintf('Input failed CSRF verification (%s not in %s).', $csrf_token, getDump($_SESSION['_app'][$this->_ns]['csrf_tokens'])), LOG_WARNING, __FILE__, __LINE__);
-            return false;
-        }
-        // $app->logMsg(sprintf('Verified token %s is in %s', $csrf_token, getDump($_SESSION['_app'][$this->_ns]['csrf_tokens'])), LOG_DEBUG, __FILE__, __LINE__);
+            $this->logMsg(sprintf('%s method called, but csrf_token_enabled=false', __FUNCTION__), LOG_ERR, __FILE__, __LINE__);
+            return true;
+        }
+        if ('' == trim($user_submitted_csrf_token)) {
+            $this->logMsg(sprintf('Empty string failed CSRF verification.', null), LOG_NOTICE, __FILE__, __LINE__);
+            return false;
+        }
+        if (!verifySignature($user_submitted_csrf_token, null, 64)) {
+            $this->logMsg(sprintf('Input failed CSRF verification (invalid signature in %s).', $user_submitted_csrf_token), LOG_WARNING, __FILE__, __LINE__);
+            return false;
+        }
+        $csrf_token = $this->getCSRFToken();
+        if ($user_submitted_csrf_token != $csrf_token) {
+            $this->logMsg(sprintf('Input failed CSRF verification (%s not in %s).', $user_submitted_csrf_token, $csrf_token), LOG_WARNING, __FILE__, __LINE__);
+            return false;
+        }
+        $this->logMsg(sprintf('Verified CSRF token %s is in %s', $user_submitted_csrf_token, $csrf_token), LOG_DEBUG, __FILE__, __LINE__);
         return true;
     }
@@ -1123 +1102 @@
     *
     * @access   public
-    * @param    string  $csrf_token The token to compare with the session token.
+    * @param    string  $user_submitted_csrf_token The user-submitted token to compare with the session token.
     * @param    string  $message    Optional message to display to the user (otherwise default message will display). Set to an empty string to display no message.
     * @param    int    $type    The type of message: MSG_NOTICE,
@@ -1134 +1113 @@
     * @since    15 Nov 2014 18:10:17
     */
-    public function requireValidCSRFToken($csrf_token, $message=null, $type=MSG_NOTICE, $file=null, $line=null)
-    {
-        $app =& App::getInstance();
-
-        if (!$this->verifyCSRFToken($csrf_token)) {
-            $message = isset($message) ? $message : _("Something went wrong; please try again.");
-            $app->raiseMsg($message, $type, $file, $line);
-            $app->dieBoomerangURL();
+    public function requireValidCSRFToken($message=null, $type=MSG_NOTICE, $file=null, $line=null)
+    {
+        if (!$this->verifyCSRFToken(getFormData($this->getParam('csrf_token_name')))) {
+            $message = isset($message) ? $message : _("Sorry, the form token expired. Please try again.");
+            $this->raiseMsg($message, $type, $file, $line);
+            $this->dieBoomerangURL();
         }
     }

trunk/lib/Auth_SQL.inc.php

@@ -32 +32 @@
 class Auth_SQL {
 
-    // Available encryption types for class Auth_SQL.
+    // Available hash types for class Auth_SQL.
     const ENCRYPT_PLAINTEXT = 1;
     const ENCRYPT_CRYPT = 2;
@@ -68 +68 @@
         'db_login_table' => 'user_login_tbl',
 
-        // The type of encryption to use for passwords stored in the db_table. Use one of the Auth_SQL::ENCRYPT_* types specified above.
-        // Hardened password hashes rely on the same key/salt being used to compare encryptions.
+        // The type of hash to use for passwords stored in the db_table. Use one of the Auth_SQL::ENCRYPT_* types specified above.
+        // Hardened password hashes rely on the same key/salt being used to compare hashs.
         // Be aware that when using one of the hardened types the App signing_key or $more_salt below cannot change!
-        'encryption_type' => self::ENCRYPT_MD5,
+        'hash_type' => self::ENCRYPT_MD5,
+        'encryption_type' => null, // Backwards misnomer compatibility.
+
+        // Automatically update stored user hashes when the user next authenticates if the hash type changes (requires user_tbl with populated userpass_hashtype column).
+        'hash_type_autoupdate' => true,
 
         // The URL to the login script.
@@ -257 +261 @@
     public function setParam($params)
     {
+        $app =& App::getInstance();
+
         if (isset($params['match_remote_ip_exempt_usernames'])) {
             $params['match_remote_ip_exempt_usernames'] = array_map('strtolower', $params['match_remote_ip_exempt_usernames']);
@@ -263 +269 @@
             $params['login_abuse_exempt_usernames'] = array_map('strtolower', $params['login_abuse_exempt_usernames']);
         }
-        if (isset($params['encryption_type']) && version_compare(PHP_VERSION, '5.5.0', '<') && in_array($params['encryption_type'], array(self::ENCRYPT_PASSWORD_BCRYPT, self::ENCRYPT_PASSWORD_DEFAULT))) {
+        if (isset($params['encryption_type'])) {
+            // Backwards misnomer compatibility.
+            $params['hash_type'] = $params['encryption_type'];
+        }
+        if (isset($params['hash_type']) && version_compare(PHP_VERSION, '5.5.0', '<') && in_array($params['hash_type'], array(self::ENCRYPT_PASSWORD_BCRYPT, self::ENCRYPT_PASSWORD_DEFAULT))) {
             // These hash types require the password_* userland lib in PHP < 5.5.0
             $pw_compat_lib = 'vendor/ircmaxell/password-compat/lib/password.php';
@@ -269 +279 @@
                 include_once $pw_compat_lib;
             } else {
-                $app =& App::getInstance();
-                $app->logMsg(sprintf('Encryption type %s requires password-compat lib in PHP < 5.5.0; falling back to ENCRYPT_SHA1', $params['encryption_type']), LOG_ERR, __FILE__, __LINE__);
-                $params['encryption_type'] = self::ENCRYPT_SHA1;
-            }
+                $app->logMsg(sprintf('Hash type %s requires password-compat lib in PHP < 5.5.0; falling back to ENCRYPT_SHA1_HARDENED', $params['hash_type']), LOG_ERR, __FILE__, __LINE__);
+                $params['hash_type'] = self::ENCRYPT_SHA1_HARDENED;
+            }
+        }
+        if (isset($params['hash_type']) && !in_array($params['hash_type'], array(self::ENCRYPT_PLAINTEXT, self::ENCRYPT_CRYPT, self::ENCRYPT_SHA1, self::ENCRYPT_SHA1_HARDENED, self::ENCRYPT_MD5, self::ENCRYPT_MD5_HARDENED, self::ENCRYPT_PASSWORD_BCRYPT, self::ENCRYPT_PASSWORD_DEFAULT))) {
+            $app->logMsg(sprintf('Invalid hash type %s; falling back to ENCRYPT_SHA1_HARDENED', $params['hash_type']), LOG_ERR, __FILE__, __LINE__);
+            $params['hash_type'] = self::ENCRYPT_SHA1_HARDENED;
         }
         if (isset($params) && is_array($params)) {
@@ -393 +406 @@
         }
 
-        if ($this->verifyPassword($password, $user_data['userpass'])) {
+        $old_hash_type = isset($user_data['userpass_hashtype']) && !empty($user_data['userpass_hashtype']) ? $user_data['userpass_hashtype'] : $this->getParam('hash_type');
+        if ($this->verifyPassword($password, $user_data['userpass'], $old_hash_type)) {
             $app->logMsg(sprintf('Authentication successful for %s (user_id=%s)', $username, $user_data['user_id']), LOG_INFO, __FILE__, __LINE__);
             unset($user_data['userpass']); // Avoid revealing the encrypted password in the $user_data.
+            if ($this->getParam('hash_type_autoupdate') && $old_hash_type != $this->getParam('hash_type')) {
+                // Let's update user's password hash to new type (just run setPassword with this authenticated password
).
+                $this->setPassword($user_data['user_id'], $password);
+                $app->logMsg(sprintf('User %s password hash type updated from %s to %s', $username, $old_hash_type, $this->getParam('hash_type')), LOG_INFO, __FILE__, __LINE__);
+            }
             return $user_data;
         }
@@ -835 +854 @@
      *
      */
-    public function encryptPassword($password, $salt=null)
+    public function encryptPassword($password, $salt=null, $hash_type=null)
     {
         $app =& App::getInstance();
@@ -841 +860 @@
         $password = (string)$password;
 
-        // Existing password hashes rely on the same key/salt being used to compare encryptions.
+        // Existing password hashes rely on the same key/salt being used to compare hashs.
         // Don't change this (or the value applied to signing_key) unless you know existing hashes or signatures will not be affected!
         $more_salt = 'B36D18E5-3FE4-4D58-8150-F26642852B81';
 
-        switch ($this->_params['encryption_type']) {
+        $hash_type = isset($hash_type) && !empty($hash_type) ? $hash_type : $this->getParam('hash_type');
+
+        switch ($hash_type) {
         case self::ENCRYPT_PLAINTEXT :
             $encrypted_password = $password;
@@ -886 +907 @@
 
         default :
-            $app->logMsg(sprintf('Authentication encrypt type specified is unrecognized: %s', $this->_params['encryption_type']), LOG_NOTICE, __FILE__, __LINE__);
+            $app->logMsg(sprintf('Unknown hash type: %s', $hash_type), LOG_WARNING, __FILE__, __LINE__);
             return false;
-            break;
         }
 
         // In case our hashing function returns 'false' or another empty value, bail out.
         if ('' == trim((string)$encrypted_password)) {
-            $app->logMsg(sprintf('Invalid password hash returned; check yo crypto!', null), LOG_ALERT, __FILE__, __LINE__);
+            $app->logMsg(sprintf('Invalid password hash returned ("%s") for hash type %s; check yo crypto!', $encrypted_password, $hash_type), LOG_ALERT, __FILE__, __LINE__);
             return false;
         }
@@ -910 +930 @@
     * @since    15 Nov 2014 21:37:28
     */
-    public function verifyPassword($password, $encrypted_password)
-    {
-        switch ($this->_params['encryption_type']) {
+    public function verifyPassword($password, $encrypted_password, $hash_type=null)
+    {
+        $app =& App::getInstance();
+
+        $hash_type = isset($hash_type) && !empty($hash_type) ? $hash_type : $this->getParam('hash_type');
+
+        switch ($hash_type) {
         case self::ENCRYPT_CRYPT :
             return $this->encryptPassword($password, $encrypted_password) == $encrypted_password;
@@ -928 +952 @@
             return password_verify($password, $encrypted_password);
         }
-    }
-
-    /**
-     *
-     */
-    public function setPassword($user_id=null, $password)
+
+        $app->logMsg(sprintf('Unknown hash type: %s', $hash_type), LOG_WARNING, __FILE__, __LINE__);
+        return false;
+    }
+
+    /**
+     *
+     */
+    public function setPassword($user_id=null, $password, $hash_type=null)
     {
         $app =& App::getInstance();
@@ -943 +970 @@
         $user_id = isset($user_id) ? $user_id : $this->get('user_id');
 
-        // Get old password.
-        $qid = $db->query("
-            SELECT userpass
-            FROM " . $this->_params['db_table'] . "
-            WHERE " . $this->_params['db_primary_key'] . " = '" . $db->escapeString($user_id) . "'
-        ");
-        if (!list($old_encrypted_password) = mysql_fetch_row($qid)) {
-            $app->logMsg(sprintf('Cannot set password for nonexistent user_id %s', $user_id), LOG_WARNING, __FILE__, __LINE__);
-            return false;
-        }
-
-        // Compare old with new to ensure we're actually *changing* the password.
-        if ($this->verifyPassword($password, $old_encrypted_password)) {
-            $app->logMsg(sprintf('Not setting password: new is the same as old.', null), LOG_INFO, __FILE__, __LINE__);
-            return null;
-        }
+        // New hash type.
+        $hash_type = isset($hash_type) ? $hash_type : $this->getParam('hash_type');
 
         // Save the hash method used if a table exists for it.
-        $userpass_hashtype = '';
+        $userpass_hashtype_clause = '';
         if ($db->columnExists($this->_params['db_table'], 'userpass_hashtype', false)) {
-            $userpass_hashtype = ", userpass_hashtype = '" . $db->escapeString($this->getParam('encryption_type')) . "'";
+            $userpass_hashtype_clause = ", userpass_hashtype = '" . $db->escapeString($hash_type) . "'";
         }
 
@@ -969 +982 @@
         $db->query("
             UPDATE " . $this->_params['db_table'] . "
-            SET userpass = '" . $db->escapeString($this->encryptPassword($password)) . "'
-            $userpass_hashtype
+            SET userpass = '" . $db->escapeString($this->encryptPassword($password, null, $hash_type)) . "'
+            $userpass_hashtype_clause
             WHERE " . $this->_params['db_primary_key'] . " = '" . $db->escapeString($user_id) . "'
         ");
 
         if (mysql_affected_rows($db->getDBH()) != 1) {
-            $app->logMsg(sprintf('Failed to update password for user_id %s', $user_id), LOG_WARNING, __FILE__, __LINE__);
+            $app->logMsg(sprintf('Failed to update password for user_id %s (no affected rows)', $user_id), LOG_WARNING, __FILE__, __LINE__);
             return false;
         }