Is This Login Script Secure?

[happypete]

I can across this simple/clean login script, can anyone tell me if there are any vunerabilities, is it secure - I intent to remove the bits about registering and just use if for the login part?

 

https://code.google.com/p/php-pdo-secure-login-script-example/source/browse/branches/index.php

 

<?php
session_start();
/**
* Table
CREATE TABLE IF NOT EXISTS `users` (
 `id` int(11) NOT NULL AUTO_INCREMENT,
 `username` varchar(45) DEFAULT NULL,
 `pass_hash` varchar(255) DEFAULT NULL,
 `pass_salt` varchar(255) DEFAULT NULL,
 PRIMARY KEY (`id`)
) ENGINE=InnoDB  DEFAULT CHARSET=latin1 AUTO_INCREMENT=0 ;
*/
//DB Stuff
define('DBHOST','localhost');
define('DBNAME','yourdb');
define('DBUSER','root');
define('DBPASS','');
//End Config:---

//Open a PDO Database connection
try {
    $db = new PDO("mysql:host=".DBHOST.";dbname=".DBNAME, DBUSER, DBPASS);
    $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    $db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
}catch (Exception $e){
    die('Cannot connect to mySQL server.');
}

class Login{
    public $db;
    public $user;
    public $pass;
    public $error;
    // sha512
    public $algo = '$6';
    // Cost parameter, 25k iterations
    public $cost = '$rounds=25000$';
    function __construct(PDO $db){
		    $this->db = $db;
		    $this->global_salt = sha1($_SERVER['HTTP_HOST']);
    }
    /**
	 * Return a random seed for the mt_rand function
	 */
    function make_seed(){
		    list($usec, $sec) = explode(' ', microtime());
		    return (float) $sec + ((float) $usec * 100000);
    }
    /**
	 * Return a random unique salt for new created hash/crypt function salts
	 */
    function unique_salt(){
		    $salt = null;
		    mt_srand($this->make_seed());
		    for($i=0;$i < mt_rand(1,10);$i++){
				    $salt = sha1($this->global_salt.$salt.mt_rand().uniqid().microtime(true));
		    }
		    return substr($salt,0,16);
    }
    /**
	 * Hash a given password and store parts in:
	 * $this->salt = a unique 16 byte salt
	 * $this->hash = The full crypted hash sting including algo/cost/salt/crytedpassword
	 * $this->full_salt = Just algo/cost/salt section, the first 33 bytes
	 * $this->hashed_password = Just crytedpassword section, proceeding bytes after first 33 bytes
	 *
	 */
    function hash($password){
		    $this->salt = $this->unique_salt();
		    $this->full_hash = crypt($password, $this->algo.$this->cost.$this->salt);
		    $this->full_salt = substr($this->full_hash, 0, 33);
		    $this->hashed_password = substr($this->full_hash, 33);
		    return $this->full_hash;
    }
    /**
	 * Method to validate the given crypto hash against the given password
	 */
    function check_password($hash, $salt, $password){
		    $hash = ($this->algo.$this->cost.$salt.'$'.$hash);
		    if($hash == crypt($password, substr($hash, 0, 33))){
				    //Regenerate new hash and salt for given password
				    $this->update_keys();
				    $this->status = true;
				    $_SESSION['logged_in']=true;
				    return true;
		    }else{
				    $this->status = false;
				    return false;
		    }
    }
    /**
	 * Set error
	 */
    function set_error($type,$value){
		    $this->error[$type]=$value;
    }
    /**
	 * Output error
	 */
    function error($type){
		    echo (isset($this->error[$type]))?$this->error[$type]:null;
    }
    /**
	 * Logout and regenirate session and redirect to index
	 */
    static function logout(){
		    unset($_SESSION['logged_in']);
		    session_regenerate_id(true);
		    exit(header('Location: ./index.php'));
    }
    function anti_brute($intval){
		    if(!isset($_SESSION['access_time'])){
				    $_SESSION['access_time']=time();
		    }else{
				    $t = time()-$_SESSION['access_time'];
				    if($t <= $intval){
						    $this->set_error('global','Time violation');
						    $_SESSION['access_time']=time();
						    return true;
				    }
				    $_SESSION['access_time']=time();
				    return false;
		    }
    }
    function process_login(){
		    if($_SERVER['REQUEST_METHOD']=='POST'){
				    $this->user   = (isset($_SESSION['userParam']) && isset($_POST[$_SESSION['userParam']]))?$_POST[$_SESSION['userParam']]:null;
				    $this->pass   = (isset($_SESSION['passParam']) && isset($_POST[$_SESSION['passParam']]))?$_POST[$_SESSION['passParam']]:null;
				    $this->create = (isset($_SESSION['createParam']) && isset($_POST[$_SESSION['createParam']]))?$_POST[$_SESSION['createParam']]:null;
				    $cont = true;
				    if($this->user == null || strlen($this->user) <= 2){$this->set_error('user','Please enter a username!'); $cont=false;}
				    if($this->pass == null || strlen($this->pass) <= 2){$this->set_error('pass','Please enter a password!'); $cont=false;}

				    if($cont==true){
						    //Alls good continue
						    if($this->create != null && $this->create=='1'){
								    //Check user for new account
								    if($this->check_user()==true){$this->set_error('user','Username already taken.');return;}
								    //Create account
								    $this->create_account();
						    }else{
								    //Stop really fast request 2 seconds
								    if($this->anti_brute(2)==false){
										    //Attempt to login
										    $this->check_login();
								    }
						    }
				    }else{
						    //Error with form
						    $this->set_error('global','Please fill in login form!');
				    }
		    }
    }
    function check_user(){
		    $sql = 'SELECT 1 FROM users WHERE username=:username';
		    $statement = $this->db->prepare($sql);
		    $statement->bindParam(':username', $this->user, PDO::PARAM_STR);
		    $statement->execute();
		    $result = $statement->fetch(PDO::FETCH_ASSOC);
		    if(!empty($result)){return true;}else{return false;}
    }
    function check_login(){
		    $sql = 'SELECT pass_hash, pass_salt FROM users WHERE username=:username';
		    $statement = $this->db->prepare($sql);
		    $statement->bindParam(':username', $this->user, PDO::PARAM_STR);
		    $statement->execute();
		    $result = $statement->fetch(PDO::FETCH_ASSOC);
		    $this->check_password($result['pass_hash'], $result['pass_salt'], $this->pass);
    }
    function create_account(){
		    //Create new account
		    $this->hash($this->pass);
		    $sql = 'INSERT into users (username, pass_hash, pass_salt) VALUES (:username, :pass_hash, :pass_salt)';
		    $statement = $this->db->prepare($sql);
		    $statement->bindParam(':username', $this->user, PDO::PARAM_STR);
		    $statement->bindParam(':pass_hash', $this->hashed_password, PDO::PARAM_STR);
		    $statement->bindParam(':pass_salt', $this->salt, PDO::PARAM_STR);
		    $statement->execute();
		    $this->status = true;
		    $_SESSION['logged_in']=true;
    }
    function update_keys(){
		    //Update account password hash & salt
		    $this->hash($this->pass);
		    $sql = 'UPDATE users SET pass_hash=:pass_hash, pass_salt=:pass_salt WHERE username=:username';
		    $statement = $this->db->prepare($sql);
		    $statement->bindParam(':username', $this->user, PDO::PARAM_STR);
		    $statement->bindParam(':pass_hash', $this->hashed_password, PDO::PARAM_STR);
		    $statement->bindParam(':pass_salt', $this->salt, PDO::PARAM_STR);
		    $statement->execute();
		    $this->status = true;
		    $_SESSION['logged_in']=true;
    }
}//END Login class
//Logout handler
if(isset($_GET['logout'])){ Login::logout(); }
$login = new Login($db);
//Login handler
$login->process_login();
//Debug
echo '<pre>';
print_r($login);
echo '</pre>';

//Check login status
if(isset($_SESSION['logged_in']) && $_SESSION['logged_in']==true){
    //Logged in
    echo '<a href="?logout">Logout</a>';
}else{
    //Not Logged In
    //Show login form & create uniqie parrams for user/pass/create post keys
    $_SESSION['userParam']   = sha1(uniqid().microtime(true));
    $_SESSION['passParam']   = sha1(uniqid().microtime(true));
    $_SESSION['createParam'] = sha1(uniqid().microtime(true));
?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<title>Secure Login</title>
</head>
<body>
<h1>Secure Login Example</h1>
<h3>Please login:</h3>
<?php $login->error('global'); ?>
<form method="POST" action="">
 <label for="user">Username :  </label>
 <input type="text" name="<?=$_SESSION['userParam'];?>" size="29"> <?php $login->error('user'); ?>
 <br />
 <label for="pass">Password :  </label>
 <input type="text" name="<?=$_SESSION['passParam'];?>" size="29"> <?php $login->error('pass'); ?>
 <br />
 <input type="submit" value="Login">  and create my account:<input type="checkbox" name="<?=$_SESSION['createParam'];?>" value="1">
</form>
</body>
</html>
<?php } ?>

[Christian F.]

Some quick comments:

• You don't need to salt the rand () functions, and even if you wanted to using the time is a bad method to seed it. (In fact, it's already seeded based upon some time or more random methods.)
  
• Using the site's host for a global salt isn't he best option, much better to create a truly random string with a lot higher entropy. If you want a global salt in addition to the individual salts.
  
• Your method to create the individual salts is not good, for the above reasons. I'd recommend using mcrypt_create_iv () or some other equivalent function.
  In either case, make it simple: Don't try to be smart about it, all you'll end up doing is making it more complex and easier to poke a hole in. Better to use industry standard functions, as a lot of people (smarter and more knowledgeable than either of us) has spent a lot of time perfecting those functions.
  
• Extracting and creating the salts inside the hash () method isn't what I'd expect. Based upon the name all I'd expect it to do was to hash the string given by it in the parameter, and return the hashed result.
  
• Why create the full hash (again) in the check_password () method, just to select only a subset from it?
  
• Your anti-brute functions doesn't work if client doesn't accept cookies, or just drop the session cookie (or ID). Never involve the client in stuff like this, but keep it all server-side using some (semi-)permanent storage.
  
• Other than this you seem to have done things a bit more complicated than what they need to be, on a general basis. So I'd advice having a second look over your code, and see if you can't find a way to make the code a bit simpler.

 

There's this old quote from Antoine de Saint-Exupery, which I always try to adhere to:

Perfection is achieved, not when there is nothing more to add, but when there is nothing left to take away.

This is true in programming as it is (often) true in literature, and elsewhere in life.

 

Anyway, besides the above points you seem to have thought quite well on this before starting. You've selected a good encryption routine, and you have all of the basic security in place. It's well commented, and (thankfully) indented properly too. So thanks for foiling this forum's attempts at destroying indentation.

All in all, this is a very good first implementation. You've just forgot/overlooked a few details, and fallen into the all too common trap of slightly overcomplicating things. Which is easily fixable.

 

PS: I reckon you've read this article about secure login systems already, but if you haven't I do recommend it. Should fill out a few of the details you might be a bit uncertain about.

Edited December 4, 2012 by Christian F.