Class for working with users and passwords in a PHP CMS

Question

I'm working on CMS in PHP just for fun. Right now I'm trying to implement class for working with users/passwords. I had been reading lot of articles about hashing and as far as I understood crypt() function are the best(most secure) for now.

class Users {
    private static $key, $table, $logtable;

    public static function init($key) {
        self::$key = $key;
        self::$table = DBPREFIX.$key.'s';
        self::$logtable = self::$table.'_access_log';
    }

    public static function isLogged() {
        if(isset($_SESSION[self::$key])) {
            if(isset($_SESSION[self::$key]['uid'])) {
                $uid = intval($_SESSION[self::$key]['uid']);
                $user = DB::select()->from(self::$table)->where('id', '=', $uid)->execute()->object();
                if(count($user) > 0) {
                    return true;
                }
                else {
                    return false;
                }

            }
            else {
                return false;
            }
        }
        else {
            return false;
        }
    }

    public static function userExist($login) {
        $user = DB::select()->from(self::$table)->where('login', '=', $login)->execute()->object();
        if(count($user) > 0) {
            return true;
        }
        else {
            return false;
        }
    }

    public static function getUser($login) {
        $user = DB::select()->from(self::$table)->where('login', '=', $login)->execute()->object();
        if(count($user) > 0) {
            return $user[0];
        }
        else {
            return false;
        }
    }

    public static function generateHash($password) {
        $salt = self::generateSalt();
        $hashedPassword = crypt($password, $salt);
        return array('hashed_password' => $hashedPassword, 'salt' => $salt);
    }

    private static function generateSalt() {
        $blowfishPre = '$2y$10$';
        $blowfishEnd = '$';
        $allowedChars ='ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789./';
        $charsLength = strlen($allowedChars);
        $saltLength = 20;
        $salt = "";
        for($i=0; $i < $saltLength; $i++)
        {
            $salt .= $allowedChars[mt_rand(0,$charsLength)];
        }
        $bcrypt_salt = $blowfishPre . $salt . $blowfishEnd;
        return $bcrypt_salt;
    }

    public static function comparePasswords($input, $uid) {
        $user = DB::select()->from(self::$table)->where('id', '=', $uid)->execute()->object();
        if(count($user) > 0) {
            $user = $user[0];
            $password = $user->password;
            return crypt($input, $password) === $password;
        }
        else {
            return false;
        }
    }

    public static function userExit() {
        if(isset($_SESSION[self::$key])) {
            unset($_SESSION[self::$key]);
        }
    }

    public static function changePassword($password, $uid) {
        $newPassword = self::generateHash($password);
        if(isset($newPassword['hashed_password']) && isset($newPassword['salt'])) {
            DB::update(self::$table)->set(array('password' => $newPassword['hashed_password'], 'salt' => $newPassword['salt']))->where('id', '=', $uid)->execute();
        }
    }

    public static function updateTime($uid) {
        DB::update(self::$table)->set(array('last_login' => time()))->where('id', '=', $uid)->execute();
    }

    public static function writeAccessLog($uid, $success) {
        if($success) {
            DB::insert(self::$logtable)->set(array('admin_id' => $uid, 'date' => time(), 'success' => '1'))->execute();
        }
        else if(!$success) {
            DB::insert(self::$logtable)->set(array('admin_id' => $uid, 'date' => time(), 'success' => '0'))->execute();
        }
    }
}

Please, take a look and let me know if I doing something wrong.

Answer 1

New code (optimizations/simplifications, see comments in code for explanations):

class Users {
    private static $key, $table, $logtable;

    public static function init($key) {
        self::$key = $key;
        self::$table = DBPREFIX.$key.'s';
        self::$logtable = self::$table.'_access_log';
    }

    public static function isLogged() {
        if(isset($_SESSION[self::$key]) and isset($_SESSION[self::$key]['uid'])) { # Combined ifs
            $uid = intval($_SESSION[self::$key]['uid']);
            $user = DB::select()->from(self::$table)->where('id', '=', $uid)->execute()->object();
            return count($user) > 0; # Simplified
        }
        return false; # Removed unnecesary elses
    }

    public static function userExist($login) {
        $user = DB::select()->from(self::$table)->where('login', '=', $login)->execute()->object();
        return count($user) > 0; # Simplified
    }

    public static function getUser($login) {
        $user = DB::select()->from(self::$table)->where('login', '=', $login)->execute()->object();
        if(count($user) > 0) {
            return $user[0];
        }
        return false; # Removed unnecessary else
    }

    public static function generateHash($password) {
        $salt = self::generateSalt();
        $hashedPassword = crypt($password, $salt);
        return array('hashed_password' => $hashedPassword, 'salt' => $salt);
    }

    private static function generateSalt($blowfishPre = '$2y$10$', $blowfishEnd = '$', $allowedChars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789./', $saltLength = 20) { # Added arguments for future customization
        $charsLength = strlen($allowedChars);
        $salt = "";
        for($i=0; $i < $saltLength; $i++)
        {
            $salt .= $allowedChars[mt_rand(0,$charsLength)];
        }
        return $blowfishPre . $salt . $blowfishEnd; # Removed unnecessary variable
    }

    public static function comparePasswords($input, $uid) {
        $user = DB::select()->from(self::$table)->where('id', '=', $uid)->execute()->object();
        if(count($user) > 0) {
            $user = $user[0];
            $password = $user->password;
            return crypt($input, $password) === $password;
        }
        return false; # Removed unnecessary else
    }

    public static function userExit() {
        if(isset($_SESSION[self::$key])) {
            unset($_SESSION[self::$key]);
        }
    }

    public static function changePassword($password, $uid) {
        $newPassword = self::generateHash($password);
        if(isset($newPassword['hashed_password']) && isset($newPassword['salt'])) {
            DB::update(self::$table)->set(array('password' => $newPassword['hashed_password'], 'salt' => $newPassword['salt']))->where('id', '=', $uid)->execute();
        }
    }

    public static function updateTime($uid) {
        DB::update(self::$table)->set(array('last_login' => time()))->where('id', '=', $uid)->execute();
    }

    public static function writeAccessLog($uid, $success) {
        DB::insert(self::$logtable)->set(array('admin_id' => $uid, 'date' => time(), 'success' => $success ? '1' : '0'))->execute(); # Inlined success var.
    }
}

Answer 2

You really should drop all of your hash encryption implementation unless you are specifically needing to implement encryption algorithms not supported by PHP's password_hash() and password_verify() which I would strongly recommend as basis for most PHP password hashing implementations. You are using Blowfish here which is supported by these methods, so it seems unclear what value all this code is adding. Also, please read the crypt() manual page in PHP where it clearly recommends using these wrapper functions for password hashing use cases.

---

You should embrace a style where you exit codepaths as early as possibly to minimize the number of nested conditionals and additional code paths you have in your code. Your following code:

public static function isLogged() {
    if(isset($_SESSION[self::$key])) {
        if(isset($_SESSION[self::$key]['uid'])) {
            $uid = intval($_SESSION[self::$key]['uid']);
            $user = DB::select()->from(self::$table)->where('id', '=', $uid)->execute()->object();
            if(count($user) > 0) {
                return true;
            }
            else {
                return false;
            }

        }
        else {
            return false;
        }
    }
    else {
        return false;
    }
}

Could be simplified down to:

public static function isLogged() {
    if(
        !isset($_SESSION[self::$key]) ||
        !isset($_SESSION[self::$key]['uid'] ||
        !ctype_digit($_SESSION[self::$key]['uid'])
    ) {
        return false;
    }
    $uid = (int)$_SESSION[self::$key]['uid'];
    $user = DB::select()->from(self::$table)->where('id', '=', $uid)->execute()->object();
    return (count($user) > 0);
}

Note that I added ctype_digit() validation here to make sure it is safe to cast value in session to integer without potential edge case issues. Though really since you control $_SESSION, you really should not have to be using this type juggling at all. If you expect that value to be an integer write it as such and get rid of type uncertainty here.

Also note I just directly returning results of conditional evaluation. This style can clean up some of your return code.

---

You have a couple of security problems as well (that would go away immediately with conversion to password_hash()/password_verify().

• You really should be using random_bytes() for PHP 7+ as basis for salt generation. (password_hash() does this automatically for you based on PHP version)
• You probably should not be hard-coding your hash "cost" in a string. This is obfuscated from code reader. Consider configuring at application or class level. Adaptive hashing algorithms should be periodically adjusted in terms of cost factor such that you can occasionally update your computation cost as hardware changes are made. You need to make sure that brute forcing your passwords remains computationally impractical. This may mean you change work factors and even algorithms over time. I would expect you don't want these critical settings tucked away in a string in a method somewhere. You also may need to tweak these work factors to find one that make sense for your application. Cost factor 10 for Blowfish may be a little low as a default for modern hardware depending on the password sizes you are allowing (which is unclear here).
• Your password comparison function is not safe against timing attacks. You should eliminate early code exits for this method and always perform a hash calculation. If you don't use suggestion for changing to password_verify() for the password comparison, you should at least use hash_equals() which is a basically a timing attack safe wrapper around crypt().

For example:

public static function comparePasswords($input, $uid) {
    $password = '';
    $user = DB::select()->from(self::$table)->where('id', '=', $uid)->execute()->object();
    if(count($user) > 0) {
        $password = $user->password;
    }
    return hash_equals($input, $password);
}

• Building on previous points, and assuming you are now considering treating the algorithm and cost factor/iterations as configuration values that can change over time (without changing this code), you may need to address the use case of re-hashing an existing password to an updated encryption configuration. The only time you have access to the actual valid password value is during a successful login attempt. If you detect that an old algorithm was used for the password hash (using password_needs_rehashing()) then this is your only opportunity to update to new hash if your configuration has changed. You should consider this in your design.

---

The design seems questionable with regard to the static interface. This init() function seems very odd in that what you are doing is trying to, in essence, create state on this static interface. If this is the case, should this really be a static implementation or should you be working with concrete instances?

I think you don't have a clear differentiation between what behaviors might exist in a Users class that operates against a collection of user records in a database (i.e. like a factory/provider), versus what might need to exist in a concrete User class instantiated from a DB record. This shows up in your code by the fact that many of your methods to get user information from a database based on a user id. Why should you have to keep doing this over and over again over what may be the course of several calls to determine if a user exists and/or comparing password. Perhaps password comparison and password changing methods should live on a concrete class, where you already have property of user available. For simple read use cases, you should not have to query the DB more than once for the user id in question throughout code execution.