I'm creating a login system and I have been reading a lot about the security measures needed to prevent session hijacking, fixation, and injection attacks, etc. I'm definitely not a security expert - I pieced together a lot of this with help from posts on this site and various others. These two Stack Overflow posts were particularly helpful: Link Link 2
After reading those, I ended up making a lot of changes and improvements to my system, but I'm not sure I'm protected against everything. Below is what I've come up with so far. I suspect some of it will be right, some of it will be wrong, and some may be superfluous. Here's the basic flow of what happens:
<?php
// convert password to hash
function passwordHash($string) {
return password_hash($string, PASSWORD_DEFAULT);
}
// compare password to hash
function passwordVerify($string, $hash) {
return password_verify($string, $hash);
}
// authenticate login password
function login($submitted_password, $password, $username) {
global $link;
if(passwordVerify($submitted_password, $password)) {
ini_set('session.use_trans_sid', FALSE);
ini_set('session.entropy_file', '/dev/urandom');
ini_set('session.hash_function', 'whirlpool');
ini_set('session.use_only_cookies', TRUE);
ini_set('session.cookie_httponly', TRUE);
ini_set('session.cookie_lifetime', 1200);
ini_set('session.cookie_secure', TRUE);
session_start();
$link->query("SELECT id, password, username, user_level FROM users WHERE username = :username");
$link->bind(':username', $username);
$link->execute();
$row = $link->getOneRow();
$link->closeStream();
$id = $row['id'];
$username = $row['username'];
$user_level = $row['user_level'];
$_SESSION['userID'] = $id;
$_SESSION['username'] = $username;
$_SESSION['user_level'] = $user_level;
$_SESSION['user_ip'] = $_SERVER['REMOTE_ADDR'];
$_SESSION['HTTP_USER_AGENT'] = $_SERVER['HTTP_USER_AGENT'];
// store in db to use in page_protect()
$user_ip = $_SERVER['REMOTE_ADDR'];
$useragent_hash = passwordHash($_SESSION['HTTP_USER_AGENT']);
$link->query("UPDATE users SET user_ip = :user_ip, useragent_hash = :useragent_hash WHERE id = :id");
$link->bind(':user_ip', $user_ip);
$link->bind(':useragent_hash', $useragent_hash);
$link->bind(':id', $id);
$link->execute();
$link->closeStream();
header("Location: dashboard.php");
exit();
} else {
$error = "Username or password error"; // password fails
}
}
// function called by every page requiring you to be logged in
function page_protect() {
global $link;
if (session_status() == PHP_SESSION_NONE) {
session_start();
}
if(!isset($_SESSION['user_ip']) || $_SESSION['user_ip'] != $_SERVER['REMOTE_ADDR']) {
logout();
exit();
}
// referenced in question #5 below
if (!isset($_SESSION['HTTP_USER_AGENT']) || $_SERVER['HTTP_USER_AGENT'] != $_SESSION['HTTP_USER_AGENT']) {
logout();
exit();
}
// check that IP address/useragent hash stored in db at login match current session variables
$link->query("SELECT useragent_hash, user_ip FROM users WHERE username = :username");
$link->bind(':username', $_SESSION['username']);
$link->execute();
$row = $link->getOneRow();
$link->closeStream();
$user_ip = $row['user_ip'];
$useragent_hash = $row['useragent_hash'];
if($_SESSION['user_ip'] != $user_ip || !passwordVerify($_SESSION['HTTP_USER_AGENT'], $useragent_hash)) {
logout();
exit();
}
session_regenerate_id(true);
}
function logout() {
if (session_status() == PHP_SESSION_NONE) {
session_start();
}
unset($_SESSION);
session_unset();
session_destroy();
header("Location: index.php");
exit();
}
?>
1) Am I missing anything or does anything look wrong?
2) I'm not sure I'm using session_regenerate_id() in a good spot. I know it needs to be called periodically, so I figured every time a protected page is called would be good.
3) All of the ini_set's in #4 only appear before the session_start() in the login function. However, the top of each page protected with page_protect() has a session_start(). Should it be preceded by all of the same ini_set's on every page, or once they're set during the initial login, they stay set?
4) Should I remove this line? ini_set('session.cookie_lifetime', 1200); That would require the user to login again after 20 minutes. I think it would be good to log the user out after 20 minutes of inactivity, but not after 20 minutes of moving around the site.
5) In page_protect, when I check the ip and user agent hash, should I use && instead of ||? If both conditions are met, something is definitely amiss.
Any help is greatly appreciated.