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?

• elclanrs: The indentation doesn't look too good, make sure to convert tabs to spaces when copy/pasting if it looks correct in your editor.
• AlucardTheRipper: Use the build in PHP Functions for password hashing. php.net/manual/en/function.password-hash.php or if you don't have PHP 5.5 use the backport github.com/ircmaxell/password_compat
• DominikM: I would select only user only by username and then check password in php code. You can find some discussion about this topic at stackoverflow.com/questions/5978539/…
• Lutcikaur: elclanrs fixed a few spaces in editor, but copy/pasting here messed a bit more of it up. AlucardTheRipper I'm pretty sure mine work adequately, but I'll look into using the built in functions over built in hash. @DominikM did just that. Looks cleaner and I think its faster. Thank you.

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";

• Alexandru Guzinschi: You mix PHP4 and PHP5 code. var was deprecated in PHP5 and __constructor was introduced in PHP5. Decide if you are writing PHP4 or PHP5 code and stick to that.
• Andreas Finne: As of PHP 5.1.3 it is no longer deprecated. If you declare a property using var instead of one of public, protected, or private, then PHP 5 will treat the property as if it had been declared as public. But you are correct in saying that it has a PHP4 smell to it. Source
• Alexandru Guzinschi: Supporting var declaration can be stopped or changed at any time and the property declared it will be public. So it is better to consider var deprecated and let it die in peace.
• Andreas Finne: Edited code to change var to private as pointed out by @AlexandruG.
• Alexandru Guzinschi: It is the implementer's decision what to do when an error occurs, not the library itself. So you should not interrupt the process when something has gone haywire, but throw an exception or return something, so that the implementer can decide what to do next. ( see exit from your example )

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, '2ドルa$' . $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'] ) ) );
 }