PHP MySQL login

Question

I'm trying to learn PHP/MySQL and the likes, so I've been reading tutorials for PHP login systems. My current iteration is based heavily on one from this website and contains the accepted answer for random salts here. This is my first thing I've done in MySQL, and my first attempt at PHP besides a tiny Tic-Tac-Toe game.

config.php:

<?php
    //set off all error for security purposes
    error_reporting(E_ALL);


    //define some contstant
    define( "DB_DSN", "mysql:host=localhost;dbname=gryp" );
    define( "DB_USERNAME", "root" );
    define( "DB_PASSWORD", "" );
    define( "CLS_PATH", "class" );

    //include the classes
    include_once( CLS_PATH . "/user.php" );

    ?>

user.php:

<?php

     class Users {
         public $username = null;
         public $password = null;
         public $salt = null;

         public function __construct( $data = array() ) {
             if( isset( $data['username'] ) ) $this->username = mysql_real_escape_string( htmlspecialchars( strip_tags( $data['username'] ) ) );
             if( isset( $data['password'] ) ) $this->password = mysql_real_escape_string( htmlspecialchars( strip_tags( $data['password'] ) ) );

         }

         public function storeFormValues( $params ) {
            //store the parameters
            $this->__construct( $params ); 
         }

         public function userLogin() {
             $success = false;
             try{
                $con = new PDO( DB_DSN, DB_USERNAME, DB_PASSWORD ); 
                $con->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );

                $sql = "SELECT * FROM users WHERE username = :username LIMIT 1";
                $fetch = $con->prepare( $sql );
                $fetch->bindValue( "username", $this->username, PDO::PARAM_STR );
                $fetch->execute();
                $row = $fetch->fetch(PDO::FETCH_ASSOC);

                if($row){
                    $this->salt=$row['salt'];
                    if ( hash("sha256", $this->password . $this->salt) == $row['password'])
                    {
                        $success = true;
                        $sql = "UPDATE users SET lastlogin=NOW() WHERE username=:username";
                        $fetch = $con->prepare($sql);
                        $fetch->bindValue( "username", $this->username, PDO::PARAM_STR );
                        $fetch->execute();
                    }
                }

                $con = null;
                return $success;
             }catch (PDOException $e) {
                 echo $e->getMessage();
                 return $success;
             }
         }

         public function register() {
            $correct = false;
                try {
                    $con = new PDO( DB_DSN, DB_USERNAME, DB_PASSWORD );
                    $con->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );

                    $this->salt = $this->unique_md5();

                    $sql = "INSERT INTO users(username, password,salt,registerdate) VALUES(:username, :password,:salt,NOW())";

                    $fetch = $con->prepare( $sql );


            $fetch->bindValue( "username", $this->username, PDO::PARAM_STR );
                $fetch->bindValue( "password", hash("sha256", $this->password . $this->salt), PDO::PARAM_STR );
                $fetch->bindValue( "salt", $this->salt, PDO::PARAM_STR );
                $fetch->execute();
                return "Registration Successful <br/> <a href='index.php'>Login Now</a>";
            }catch( PDOException $e ) {
                return $e->getMessage();
            }
     }

    public function unique_md5() {
        mt_srand(microtime(true)*100000 + memory_get_usage(true));
        return md5(uniqid(mt_rand(), true));
    }

 }

?>

Table structure:

# name        type        collation       null    default extra       
1 userID      int(11)                 No  None    AUTO_INCREMENT  
2 username    varchar(50) latin1_swedish_ci   No  None        
3 password    varbinary(250)              No  None        
4 salt        varbinary(32)               No  None        
5 registerdate    datetime                No  None        
6 lastlogin   datetime                No  None

I think that my input is sanitized, that I'm safe from SQL injections, and that I'm safe from XSS attacks. But before I move on with what I'm doing and learn more, I figure that it's better to assume my code is insecure and ask for help, than to assume it is secure and find out it isn't.

I feel that

$this->salt=$stmt->fetchColumn(3); 

shouldn't be what I'm doing. Also that I have 3 queries for login, which seems wasteful. But that was the least I could do it in.

Is my code going in the right direction? What can I do better?

Answer 1

Depending on the size of your application, it could be a good idea to abstract away the user information to a DAO (data access object). This object would contain the methods needed for handling user information.

For instance like the following:

class UserDAO {

  private $dbh;

//$dbh is the dbhandle you get from new PDO(…)
function __construct( $dbh ) {
  $this->dbh = $dbh;
}

function save( $user ) {
  if ( !isset( $user->id ) || $user->id == 0 ) {
    $this->insert( $user );
  } else {
    $this->update( $user );
  }
}

function getByUsername( $username ) {
  $stmt = $this->dbh->prepare( "SELECT * FROM user WHERE username = ?" );
  if ( !$stmt ) {
    echo 'Error in fetch query';
    die();
  }
  $stmt->setFetchMode( PDO::FETCH_CLASS, "User" );
  $stmt->execute( array( $username ) );
  $user = $stmt->fetch();
  return $user
}

private function insert( $user ) {
  $stmt = $this->dbh->prepare( "INSERT INTO
                        user (username, email, password)
                        VALUES (:username, :email, :password)" );
  if ( !$stmt ) {
    echo 'Error in saving query.';
    die();
  }
  $stmt->bindParam( ':username', $user->username );
  $stmt->bindParam( ':email', $user->email );
  $stmt->bindParam( ':password', $user->password );
  if ( !$stmt->execute() ) {
    echo 'Error saving user data. ';
    echo $stmt->errorCode();
    print_r( $stmt->errorInfo() );
    die();
  }
  $user->id = $this->dbh->lastInsertId();
}

private function update( $user ) {
  $stmt = $this->dbh->prepare( "UPDATE user
      SET username = :username,
          email = :email,
          password = :password
      WHERE
          id = :id" );
  if ( !$stmt ) {
    echo 'Error in update query';
    die();
  }
  $stmt->bindParam( ':username', $user->username );
  $stmt->bindParam( ':email', $user->email );
  $stmt->bindParam( ':password', $user->password );
  $stmt->bindParam( ':id', $user->id );
  if ( !$stmt->execute() ) {
    echo 'Error updating user';
    echo $stmt->errorCode();
    print_r( $stmt->errorInfo() );
    die();
  }

The the login code on your php page would just be:

    $dao = DAOFactory::getDAO("user");
    if ($user = $dao->getByUsername($this->username)) {
      if (Utils::checkPassword($password, $user->password)) {
        $_SESSION['username'] = $user->username;
        $user->last_login = date("Y-m-d H:i:s");
        $dao->save($user);
        header('Location: info.php');
        return;
      }
    }
    $this->errors[] = "Invalid username or password";

Answer 2

For hashing passwords, I use the code below. Note, I need to support running on older versions of PHP, so that is why I have the different checks for available features.

static function hashPassword($pwd) {
  $salt = self::secure_rand(16);
  $salt = str_replace('+', '.', base64_encode($salt));
  if (CRYPT_BLOWFISH == 1) {
    $salt = substr($salt, 0, 22);
    $cost = "07";
    $hash = crypt($pwd, '$2a$' . $cost . '$' . $salt);
    return $hash;
  } else {
    $salt = substr($salt, 0, 8);
    $hash = crypt($pwd, '$1$' . $salt . '$');
    return $hash;
  }
}
private static function secure_rand($length) {
  if(function_exists('openssl_random_pseudo_bytes')) {
    $rnd = openssl_random_pseudo_bytes($length, $strong);
    if ($strong === TRUE)
      return $rnd;
  }
  $sha =''; $rnd ='';
  for ($i=0; $i<$length; $i++) {
    $sha = hash('md5',$sha.mt_rand());
    $char = mt_rand(0,62);
    $rnd .= chr(hexdec($sha[$char].$sha[$char+1]));
  }
  return $rnd;
}

The valid password is checked by fetching the user by username from database, and then comparing the entered password with the encrypted password using the crypt function.

static function checkPassword($pwd, $hash) {
  return $hash == crypt($pwd, $hash);
}

I have these methods in a Utils class.

Answer 3

For logging in and registering it does what it should. You may want to consider beyond the act of logging in and registering though.

1. If this user object is something that will be used after someone has logged in to do other things, the password and salt are probably not going to be something you need to keep track of beyond the login request.
2. Keeping item 1 in mind, I would restructure this to use a static method to do the login and registration since those are things that are typically self contained. So instead of passing the $data array into the construct you would do Users::userLogin($data)
3. I'm not a huge fan of calling the construct from other methods. When you resort to that I think it is a sign that you need to make a change. The construct is intended to run once, if you need to use it a second time you should move the logic into it's own method. If you kept your code "as is", I would swap the construct for storeFromValues like this:

---

public function __construct( $data = array() ) {
   $this->storeFormValues( $data ); 
 }

 public function storeFormValues( $params ) {
 //store the parameters
     if( isset( $params['username'] ) ) $this->username = mysql_real_escape_string( htmlspecialchars( strip_tags( $params['username'] ) ) );
     if( isset( $params['password'] ) ) $this->password = mysql_real_escape_string( htmlspecialchars( strip_tags( $params['password'] ) ) );
  }