My old script: Exploitable holes in login script
Have I improved?
I've wrote this script today, by listening to many many suggestions from my last login script thread above.
How secure is my script? What would you suggest me to change in my script to make it more secure? Any problems? (not 100% tester, but I haven't seen any bugs so far).
Member.class.php:
/**
* Member.class
*
* Handling everything related to members
*
* @Author Jony <[email protected]> <www.driptone.com>
**/
Class Member
{
/**
* Properties
**/
private $pdo = null;
private $db;
private $hasher;
/**
* Connect to the database
**/
function __construct()
{
$this->db = new Database();
$this->hasher = new Hash();
$this->pdo = $this->db->connect();
}
/**
* Method login
*
* Logs the user in securly
*
* @param username The entered username
* @param password The entered password
* @param ip Client's ip
*
* @return boolean (Login successed or not).
**/
public function login($username, $password, $ip)
{
// Query to fetch from login attempts
$query = $this->pdo->prepare("SELECT * FROM login_attempts WHERE ip_address = :ip");
$query->execute(array(":ip" => $ip));
// Fetching from login attempts.
$fetch = $query->fetch(PDO::FETCH_ASSOC);
// If the username exists in the database..
if ($this->db->countRows("users", array("username" => $username)) == 1)
{
// Hashed password using our hashing class.
$password = $this->hasher->secureHash($password);
// Checking if user is blocked.
if ($fetch['blocked'] != 1)
{
// Checking if user has used less than 6 attempts.
if ($fetch['attempts'] < 6)
// Checking if the entered username and hashed password are matching the ones in the table.
if ($this->db->countRows("users", array("username" => $username, "password" => $password)) == 1)
{
// Match found, clear login history.
$this->clearHistory($ip);
// Everything works fine, return true.
return true;
}
else
{
// Updating attempts count (+1), if not exists, it will insert it.
$this->updateAttempts($ip);
// Throwing an error.
throw new exception ("Details are incorrect.");
// Not working, return false.
return false;
}
}
else
{
// Updating attempts count (+1), if not exists, it will insert it.
$this->updateAttempts($ip);
// Throwing error..
throw new exception ("You are blocked!");
}
}
else
{
// Updating attempts count (+1), if not exists, it will insert it.
$this->updateAttempts($ip);
// Throwing error again..
throw new exception ("You are blocked.");
}
}
else
{
// Checking if attempts count is less than 6
if ($fetch['attempts'] > 6)
{
// Updating attempts count (+1), if not exists, it will insert it.
$this->updateAttempts($ip);
// Throw an error..
throw new exception ("You're blocked.");
}
else
{
// Updating attempts count (+1), if not exists, it will insert it..
$this->updateAttempts($ip);
//Throwing an error again.
throw new exception ("Details are incorrect.");
}
// Updating attempts count (+1), if not exists, it will insert it.
$this->updateAttempts($ip);
// Not working, return false..
return false;
}
}
/**
* Method loginCookie
*
* Validating the remember me cookie
*
* @param hash The value of the cookie (Hashed name).
* @return boolean (If hash matching, works).
**/
public function loginCookie($hash)
{
// Checking if there is a row with the same hash as the remember me cookie.
if ($this->db->countRows("users", array("hashed_name" => $hash)) == 1)
{
// Worked, return true.
return true;
}
else
{
// Didn't work, throw an error and return false..
throw new exception ("An error has occured.");
return false;
}
}
/**
* Method getNameAfterHash
*
* Gets the name of the hashed name (for cookie).
*
* @param hash The hashed name.
* @return String the name.
**/
public function getNameAfterHash($hash)
{
// Checks if the hash is available..
if ($this->db->countRows("users", array("hashed_name" => $hash)) == 1)
{
/**
* Fetching from the table, basically fetching the column `username`
**/
$query = $this->pdo->prepare("SELECT * FROM users WHERE hashed_name = :hash");
$query->execute(array(":hash" => $hash));
$fetch = $query->fetch(PDO::FETCH_ASSOC);
// Returnign the fetches column value.
return $fetch['username'];
}
else
{
// Not working to be honest, throw error.
throw new exception ("ERROR!");
}
}
/**
* Method logout
*
* Pretty useless, but it checks if the name is in the database.
*
* @param name Session name.
* @return boolean
**/
public function logout($name)
{
// Checking if there is a row with that username.
if ($this->db->countRows("users", array("username" => $name)) == 1)
{
// Working
return true;
}
else
{
// Not working.
return false;
}
}
/**
* Method register
*
* Just for testing
**/
public function register($name, $password)
{
$query = $this->pdo->prepare("SELECT * FROM users WHERE username = :user");
$query->execute(array(":user" => $name));
if (!$query->rowCount())
{
$hashedName = $this->hasher->secureHash($name);
$password = $this->hasher->secureHash($password);
$this->db->insert("users", array("username" => $name, "password" => $password, "hashed_name" => $hashedName));
}
else
{
throw new exception("User exists");
}
}
/**
* Method clearLimits
*
* Clear user login fails attempts count
*
* @param ip The IP address of the client.
**/
public function clearLimits($ip)
{
// Query to fetch from login_attempts table.
$query = $this->pdo->prepare("SELECT * FROM login_attempts WHERE ip_address = :ip");
$query->execute(array(":ip" => $ip));
// Fetching from login_attempts
$fetch = $query->fetch(PDO::FETCH_ASSOC);
/* If fails count is less than 6, block for 10 minutes. */
if ($fetch['attempts'] < 6)
{
$time = "10 MINUTE";
}
/* If fails count is less than 11, block for 1 hour. */
else if ($fetch['attempts'] < 11)
{
$time = "1 HOUR";
}
/* If fails count is less than 21, block for 1 day. */
else if ($fetch['attempts'] < 21)
{
$time = "1 DAY";
}
/* If fails count is greater than 21, block for 1 week. */
else if ($fetch['attempts'] > 21)
{
$time = "7 DAY";
}
// Updating the results now, setting INTERVAL as @var Time.
$update = $this->pdo->prepare("UPDATE login_attempts SET blocked = 0, date = NOW() WHERE ip_address = :ip AND date < DATE_SUB(NOW(), INTERVAL $time)");
$update->execute(array(":ip" => $ip));
// This is the default delete query, it will occur only for ips that their date didnt update for 1 day atleast.
$delete = $this->pdo->prepare("DELETE FROM login_attempts WHERE date < DATE_SUB(NOW(), INTERVAL 1 DAY) AND ip_address = :ip");
}
/**
* Method clearHistory
*
* Clears ALL login history off an ip, deletes his row.
*
* @param ip The clients ip
**/
private function clearHistory($ip)
{
// Deleting process...
$delete = $this->pdo->prepare("DELETE FROM login_attempts WHERE ip_address = :ip");
$delete->execute(array(":ip" => $ip));
}
/**
* Method updateAttempts
*
* Checks if theres a row of attempts, if not, insert it. Else add +1 to attempts.
*
* @param ip The clients ip
**/
private function updateAttempts($ip)
{
// Checking if row exists.
if ($this->db->countRows("login_attempts", array("ip_address" => $ip)) == 1)
{
// Add +1 to attempts and update time to NOW().
$update = $this->pdo->prepare("UPDATE login_attempts SET attempts = attempts + 1, date = NOW() WHERE ip_address = :ip");
$update->execute(array(":ip" => $ip));
}
else
{
// Not found, insert new row.
$this->db->insert("login_attempts", array("ip_address" => $ip, "attempts" => 1));
}
}
}
Hash.class.php:
/**
* Hash.class
*
* Hashing passwords/names
*
* @Author Jony <[email protected]> <www.driptone.com>
**/
Class Hash
{
private $globalSalt = "F926E3GGmv6Iy3kYj411Sq6J4A8L885co168UK4I5q1128chk685dTny21518s2";
public function secureHash($toHash)
{
$salt = hash("SHA512", $toHash."".$this->globalSalt);
return self::hashName($salt);
}
private static function hashName($salt)
{
$salt = hash("SHA512", $salt);
$final = hash("SHA512", $salt);
return hash("SHA512", $final);
}
}
login.php:
<?php
/**
* - LOGIN
* - The login page.
**/
include ("includes/config.inc.php");
/**
* Creating objects.
**/
$member = new Member();
$hasher = new Hash();
$member->clearLimits($_SERVER['REMOTE_ADDR']);
if (!isset($_SESSION['user']))
{
if(!isset($_COOKIE['remember_me']))
{
if (isset($_POST['submit']))
{
try
{
if ($member->login($_POST['username'], $_POST['password'], $_SERVER['REMOTE_ADDR']))
{
if (isset($_POST['remember']))
{
setcookie("remember_me", $hasher->secureHash($_POST['username']), time()+604800);
}
$_SESSION['user'] = $username;
session_regenerate_id();
header("Location: index.php");
}
}
catch (exception $t)
{
echo $t->getMessage();
}
}
}
else
{
try
{
if ($member->loginCookie($_COOKIE['remember_me']))
{
$_SESSION['user'] = $member->getNameAfterHash($_COOKIE['remember_me']);
header("Location: index.php");
}
}
catch (exception $e)
{
echo $e->getMessage();
}
}
}
else
{
header("Location: index.php");
}
?>
<html>
<form action="login.php" method="POST">
<input type="text" name="username">
<input type="password" name="password">
<input type="checkbox" name="remember">
<input type="submit" name="submit">
</form>
</html>
index.php including the logout method:
include ("includes/config.inc.php");
$member = new Member();
if (isset($_SESSION['user']))
{
if(isset($_GET['logout']))
{
try
{
if ($member->logout($_SESSION['user']))
{
setcookie("remember_me", "", time()-1000000);
unset($_SESSION['user']);
session_regenerate_id();
header("Location: login.php");
}
}
catch (exception $r)
{
echo $r->getMessage();
}
}
echo "hello, ". $_SESSION['user'],
'
<a href="?logout">Do you wish to log out</a>?
';
}
else
{
header("Location: login.php");
}
