1
\$\begingroup\$

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();
}
\$\endgroup\$
  • \$\begingroup\$ What do you understand the purpose of (your) nonce to be? \$\endgroup\$ – AD7six Nov 9 '15 at 13:54
  • \$\begingroup\$ @AD7six Simply, I don't. Originally my thought was that the server would compare the cookie to the session variable and reject the session if it didn't match. Reading the analysis Hassan kindly provided, it's obvious to me now that that cookie can easily be stolen and used. \$\endgroup\$ – user1103 Nov 10 '15 at 8:52
  • \$\begingroup\$ ...but the original purpose was to determine whether the idle timeout of the login session had passed since the last request. The more I think about it, the more half-baked I see this idea was and that it really had nothing to do with the idea of a nonce... \$\endgroup\$ – user1103 Nov 10 '15 at 9:06
3
\$\begingroup\$

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!

\$\endgroup\$
  • \$\begingroup\$ I have updated the post a bit. \$\endgroup\$ – Hassan Althaf Nov 10 '15 at 6:23
  • \$\begingroup\$ Quote: 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. That's half-true. A data from the session is stored on the server, right - But a session always uses a random cookie to "reuse" the actual session to a user. So, in fact, the PHP standard implementation uses a cookie to reuse a session. The implentation of @user1103 is fine because it also validates if the cookie matches the session, to prevent session hi-jacking. \$\endgroup\$ – Brain Foo Long Nov 12 '15 at 11:35
  • \$\begingroup\$ @BrainFooLong Yes, but they also place in appropriate security measures. I would say, cookies must be avoided as much as possible unless you are a security expert who can securely use a cookie which is well encrypted and maintains integrity throughout. \$\endgroup\$ – Hassan Althaf Nov 12 '15 at 11:58
  • \$\begingroup\$ Yes that sounds reasonable and generally i see so also. But in this case the user asked if the code is secure, and yes, it is. He just use a random checking cookie value which enhances the security of the session, not impair. \$\endgroup\$ – Brain Foo Long Nov 12 '15 at 12:02
  • \$\begingroup\$ @BrainFooLong It may be a possibility to hack the session variable as well and if the hacker manages to hack both, then RIP. \$\endgroup\$ – Hassan Althaf Nov 12 '15 at 12:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.