Another PHP login handler

Question

I wrote this class to handle login sessions for a framework I'm writing for educational purposes.

My major areas of concern:

• Style

  I know my style is a little contrary to most coding conventions. Is it difficult to read?

• The nonce

  If the login is valid, the session checker punts a random hash to a cookie to compare with the same hash stored in a session variable on the next request. Is this a proper nonce or just nonsense?

• bcrypt

  Plain and simple - Did I do it right?

• PDO

  I just started using PDO. Do my prepared statements look right?

• Security

  From a security standpoint, is this a disaster waiting to happen? Is it reasonably resistant to hijacking and injection? Am I handling passwords responsibly?

session.php:

define("SESS_NAME","PHPSESSNAME");
define("COOK_TIMEOUT",1500);
define("COOK_DOMAIN","localhost");
define("COOK_PATH","/");
define("COOK_SECURE",FALSE);
define("COOK_HTTP",TRUE);

class Session {

    public $is_logged;
    public $error;
    private $useragent;

    public function __construct(){
        $this->useragent = $_SERVER["HTTP_USER_AGENT"];
        session_name(SESS_NAME);
        session_set_cookie_params(COOK_TIMEOUT,COOK_PATH,COOK_DOMAIN,COOK_SECURE,COOK_HTTP);
        session_start();
    }

    public function set_session($user){
        if(isset($_SESSION['user'])){
            $this->error = "Session Already Set";
            return false;
        }
        $nonce = $this->gen_nonce();
        $_SESSION['user'] = $user;
        $_SESSION['nonce'] = $nonce;
        $_SESSION['useragent'] = $this->useragent;
        setcookie('nuo',$nonce,time()+COOK_TIMEOUT);
        header("refresh:0");
    }

    public function check_session(){
        if(!isset($_SESSION['useragent']) ||
                !isset($_SESSION['nonce'])
            ){
            $this->error = "Session Verification Failed";
            return false;
        }
        if(($this->useragent != $_SESSION['useragent']) ||
                ($_COOKIE['nuo'] != $_SESSION['nonce'])
            ){
            $this->killsess();
            $this->error = "Session Verification Failed";
            return false;
        }
        $newnonce = $this->gen_nonce();
        $_SESSION['nonce'] = $newnonce;
        setcookie('nuo',$newnonce,time()+COOK_TIMEOUT);
        $this->is_logged = true;
        return true;
    }

    private function gen_nonce(){
        $string = "";
        for($i = 0; $i < 256; $i++){
            $string .= chr(rand(0,255));
        }
        return md5($string);
    }

    private function killsess(){
        foreach($_SESSION as $key =>$id ){
            unset($_SESSION[$key]);
        }
        setcookie('nuo','',time() - 42000);
        setcookie(SESS_NAME,'',time() - 42000);
        session_destroy();
    }

};

Authorization is handled by another class that uses PHP's password hashing functions.

bcrypt.php:

class Bcrypt {

    public $error;  
    private $dbhand;
    private $crypt_cost = 10;

    public function __construct($dbhand){
        if(!($dbhand instanceof PDO)) die('Database Object Invalid');
        $this->dbhand = $dbhand;
        if(CRYPT_BLOWFISH !== 1) die("Blowfish not supported");
    }

    public function create_user($user,$pass){
        if($this->check_user_exists($user)){
            $this->error = "User Already Exists";
            return false;
        }
        if(empty($hash = $this->hash_pass($pass))){
            $this->error = "Password Hash Failed";
            return false;
        }
        $stmt = $this->dbhand->prepare("INSERT INTO users VALUES(NULL,?,?);");
        if(!$stmt->execute(array(0=>$user,1=>$hash))){
            $this->error = "Database Error: {$stmt->error}";
            return false;
        }
        return true;
    }

    public function set_pass($user,$pass){
        if(!$this->check_user_exists($user)){
            $this->error = "User Does Not Exist";
            return false;
        }
        if(!empty($hash = $this->hash_pass($pass))){
            $stmt = $this->dbhand->prepare("UPDATE users SET pass = ? WHERE user = ?");
            if($stmt->execute(array(0=>$hash,1=>$user))){
                return true;
            }
        }
        $this->error = "Set Password Failed";
        return false;
    }

    public function change_pass($user,$oldpass,$newpass){
        if($this->check_pass($user,$oldpass)){
            if($this->set_pass($user,$newpass))
                return true;
        }
        $this->error = "Password Change Failed";
        return false;
    }

    public function check_pass($user,$pass){
        if(!empty($hash = $this->get_hash($user))){
            if(password_verify($pass,$hash)){
                $this->check_hash($user,$pass,$hash);
                return true;
            }
        }
        $this->error = "Password Verification Failed";
        return false;
    }

    private function check_user_exists($user){
        $stmt = $this->dbhand->prepare("SELECT COUNT(user) FROM users WHERE user = ?");
        $stmt->execute(array(0=>$user));
        $result = $stmt->fetchAll();
        if($result[0][0] > 0){
            return true;
        }       
        return false;
    }

    private function hash_pass($pass){
        if(!empty($hash = password_hash($pass,PASSWORD_BCRYPT,array('cost'=>$this->crypt_cost)))){
            return $hash;
        }
        return false;
    }

    private function check_hash($user,$pass,$hash){
        if(password_needs_rehash($hash,PASSWORD_BCRYPT,array('cost'=>$this->crypt_cost))){
            $this->set_pass($user,$pass);
        }
    }   

    private function get_hash($user){
        $stmt = $this->dbhand->prepare("SELECT pass FROM users WHERE user = ?");
        if(!$stmt->execute(array(0=>$user)))
            return false;
        $return = $stmt->fetchAll();
        return $return[0]['pass'];
    }

};

Implementation is simple:

require_once('bcrypt.php');
require_once('session.php');
$dbhand = new PDO('blahblah');
$bcryptobj = new Bcrypt($dbhand);
$sessobj = new Session;
if(!empty($_POST['userlogin'])){
  if($bcryptobj->check_pass($_POST['username'],$_POST['password'])){
    $sessobj->set_session($_POST['username']);
  }
}
else {
  $sessobj->check_session();
}

Answer 1

Let's begin with your style.

1. Naming

   As per the unofficial PHP standards constructed by PHP majors, you should name your classes in StudlyCaps.

   i.e. for usersession you'd name it UserSession.

   Moreover, for properties and methods naming, you should use camelCase.

   i.e. for $is_logged should be written as $isLogged.

   Also, looking at $dbhand, not only is it not camelCase but it is also very shortly written. It is recommended to avoid abbreviations or shortforms in naming. So, you would name it: $databaseHandler

2. Syntax format style

   Classes:

   As per the standard, this is the way you write a class:

   <?php
   
   class ClassName
   {
       // methods and properties here.
   }
   

   Methods

   This is the right way to write a method as per the standards:

   public function methodName()
   {
       //code
   }
   

   The nonce

   No, no, no! Never use a Cookie! Cookies are stored in the client-side and is never safe to do so! A SESSION would be much more safer to use in your implementation.

   What you can do is, store the random hash in a database table, with the users id along with it, and that way, every time the user makes a request, the database can be checked, and the request can be continued. That way, you can even log users out by just deleting entries from the database giving you more control.

   I also see a few methods in the bcrypt class that need to be moved away, I have talked about it in the bcrypt section.

   bcrypt

   Firstly, as per the naming standards, you should name it: BCrypt.

   No, you violated the "Single Responsibility Principle" which states that a single class may have only a single responsibility.

   Your BCrypt class handles the responsibility of holding some properties you wish to use for the bcrypt algorithm as well as some Users related stuff. This is a big no!

   Your BCrypt class could be something like this to abstract the implementation:

   <?php
   
   class BCrypt
   {
       private $cost = 10;
   
       public function hash($plainText)
       {
           // implementation.
       }
   
       public function verifyHash($plainText, $cipherText)
       {
          // implementation.
       }
   }
   

   Something like that could be used to make your component much more robust and reusable. In case you changed the algorithm, you only need to alter this class.

   The user related methods, such as login, could be moved on to a class called UserService which handles the login, etc and the Sessions checking etc could be moved onto a class called UserSessionsService which could be used to create, verify and destroy sessions.

   For the random md5() hash you create, you can create a new class called RandomKeyGenerator and use that to add methods such as generating a new random key which could then be hashed and use for the session.

   Prepared statements

   It looks like your prepared statements are alright. Good job!

   Security

   I'm not a security guru but everything looks fine other than the Cookie which I mentioned above, and well done on using BCrypt!