PHP Login and Registration System Security

Question

I've followed many tutorials to make my user system as secure as possible. I don't know what else I might've missed. The website will be released to hundreds of thousands of users. I am clueless to what security flaw I missed and I'll appreciate it if any of you advanced users out there can give me feedback on how to improve.

Registration

function validateDate($date, $format = 'Y-m-d H:i:s'){
$d = DateTime::createFromFormat($format, $date);
return $d && $d->format($format) == $date;
 }
 if (isset($_POST['register'])) {
if (empty($_POST['date'])) {
 $error = '1';
 $dateError = "Please enter a birthday date.";
} else {
 if ((validateDate($_POST['date'], 'm/d/Y')) == FALSE) {
 $error = '1';
 $dateError = "Please enter a valid birthday date.";
 }
}
if (empty($_POST['username'])) {
 $error = '1';
 $usernameError = "Please enter an username.";
} else
if (strlen($_POST['username']) < 3) {
 $error = '1';
 $usernameError = "Username must have at least 3 characters.";
} else
if (!preg_match("/^[a-z0-9\d_]{3,16}$/i", $_POST['username'])) {
 $error = '1';
 $usernameError = "Username can only contain alphabets, underscore characters and numbers.";
} else {
 $sql = "SELECT * FROM users WHERE username=?";
 $stmt = $mysql->prepare($sql);
 $stmt->bind_param('s', $_POST['username']);
 $stmt->execute();
 $result = $stmt->get_result();
 $results = $result->num_rows;
 if ($results >= 1) {
 $error = '1';
 $usernameError = "Username is already in use.";
 }
}
if (!empty($_POST["name"])) {
 if (!preg_match("^(\s)*[A-Za-z]+((\s)?((\'|\-|\.)?([A-Za-z])+))*(\s)*$^", $_POST['name'])) {
 $error = '1';
 $nameError = "Name can only contain alphabets and spaces.";
 }
}
if (!filter_var($_POST['email'], FILTER_VALIDATE_EMAIL)) {
 $error = '1';
 $emailError = "Please enter a valid email address.";
} else {
 $xsql = "SELECT * FROM users WHERE email=?";
 $xstmt = $mysql->prepare($xsql);
 $xstmt->bind_param('s', $_POST['email']);
 $xstmt->execute();
 $xresult = $xstmt->get_result();
 $xresults = $xresult->num_rows;
 if ($xresults >= 1) {
 $error = '1';
 $emailError = "Email is already in use.";
 }
}
if (empty($_POST['password'])) {
 $error = '1';
 $passError = "Please enter a password.";
}
else {
 if (strlen($_POST['password']) < 6) {
 $error = '1';
 $passError = "Password must have at least 6 characters.";
 }
}
if ($error == '0') {
 $name = $_POST['name'];
 $name = substr($name, 0, 40);
 $username = $_POST['username'];
 $username = substr($username, 0, 16);
 $date = $_POST['date'];
 $email = $_POST['email'];
 $password = $_POST['password'];
 $trn_date = date("Y-m-d H:i:s");
 $ip = '';
 if (getenv('HTTP_CLIENT_IP')) {
 $ip = getenv('HTTP_CLIENT_IP');
 }
 else
 if (getenv('HTTP_X_FORWARDED_FOR')) {
 $ip = getenv('HTTP_X_FORWARDED_FOR');
 }
 else
 if (getenv('HTTP_X_FORWARDED')) {
 $ip = getenv('HTTP_X_FORWARDED');
 }
 else
 if (getenv('HTTP_FORWARDED_FOR')) {
 $ip = getenv('HTTP_FORWARDED_FOR');
 }
 else
 if (getenv('HTTP_FORWARDED')) {
 $ip = getenv('HTTP_FORWARDED');
 }
 else
 if (getenv('REMOTE_ADDR')) {
 $ip = getenv('REMOTE_ADDR');
 }
 else {
 $ip = '127.0.0.1';
 }
 $activation = md5($email . time());
 $sql = "INSERT into `users` (username, name, password, email, birthday, status, activation, ip, trn_date, theme, picture, views, currency, publicprofile, showaboutyourself, showviews, changedecimal, sortby, apiscrape, badge, sortfeed, who_can_pm_me, who_can_notify_me, google_auth_code) VALUES (?, ?, ?, ?, ?, ?, '0', ?, ?, ?, 'light', 'default.png', '1', 'USD', '1', '1', '1', '0', 'latestadded', 'coinmarketcap', 'USER', 'relevance', 'all', 'all', ?)";
 $options = ['cost' => 15, ];
 $pdw = password_hash($password, PASSWORD_BCRYPT, $options);
 $stmt = $mysql->prepare($sql);
 $stmt->bind_param('sssssssss', $username, $name, $pdw, $email, $date, $activation, $ip, $trn_date, $secret);
 $stmt->execute();
 // log them in after registration
 $sql = "SELECT id FROM `users` WHERE email=?";
 $stmt = $mysql->prepare($sql);
 $stmt->bind_param('s', $email);
 $stmt->execute();
 $result = $stmt->get_result();
 $row = $result->fetch_assoc();
 $userid = $row["id"];
 $usersession = substr("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ", mt_rand(0, 50) , 1) . substr(md5(time()) , 1) . bin2hex(openssl_random_pseudo_bytes(25));
 $now = time();
 $expires = time() + 86400;
 date_default_timezone_set('Europe/Amsterdam');
 $date = date('d-m-Y');
 $sql = "INSERT INTO sessions (SESSION_ID, SESSION_USER_ID, SESSION_IP, SESSION_EXPIRES, SESSION_DATE, SESSION_ACTIVE, SESSION_AGENT) VALUES (?,?,?,?,?,'1',?)";
 $stmt = $mysql->prepare($sql);
 $stmt->bind_param('ssssss', $usersession, $userid, $ip, $expires, $date, $_SERVER['HTTP_USER_AGENT']);
 $stmt->execute();
 setcookie("MySite_Session_ID", $usersession, $expires, '/');
 header("Location: /");
 die();
}

Login

if (isset($_POST['login'])) {
usleep(500000); // Slow down brute-forcers
$error = '0';
$email = $_POST['email'];
$password = $_POST['password'];
if (empty($email)) {
 $error = '1';
 $userError = "Please enter your email.";
}
if (empty($password)) {
 $error = '1';
 $passError = "Please enter your password.";
}
if ($error == '0') {
 $sql = "SELECT id,password,status FROM `users` WHERE email=?";
 $stmt = $mysql->prepare($sql);
 $stmt->bind_param('s', $email);
 $stmt->execute();
 $result = $stmt->get_result();
 $results = $result->num_rows;
 if ($results >= 1) {
 $row = $result->fetch_assoc();
 if ($row['status'] != 0) {
 if (password_verify($password, $row["password"])) {
 // Password success, log them in
 $userid = $row["id"];
 $useragent = $_SERVER['HTTP_USER_AGENT'];
 $usersession = substr("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ", mt_rand(0, 50) , 1) . substr(md5(time()) , 1) . bin2hex(openssl_random_pseudo_bytes(25));
 $now = time();
 $expires = time() + 86400;
 if (isset($_POST['rememberme'])) {
 $expires = time() + 15768000;
 }
 date_default_timezone_set('Europe/Amsterdam');
 $date = date('d-m-Y');
 $sql = "INSERT INTO sessions (SESSION_ID, SESSION_USER_ID, SESSION_IP, SESSION_EXPIRES, SESSION_DATE, SESSION_ACTIVE, SESSION_AGENT) VALUES (?,?,?,?,?,'1',?)";
 $stmt = $mysql->prepare($sql);
 $stmt->bind_param('ssssss', $usersession, $userid, $_SERVER['REMOTE_ADDR'], $expires, $date, $_SERVER['HTTP_USER_AGENT']);
 $stmt->execute();
 setcookie("MySite_Session_ID", $usersession, $expires, '/');
 header("Location: /");
 } else {
 $passError = "Invalid password.";
 }
 } else {
 $userError = 'Your account has not been activated. Please check your email.';
 }
 }
}
}

Logout

$mysql = mysqli_connect('localhost', 'root', '');
mysqli_select_db($mysql,"mysite");
$sql = "DELETE FROM sessions WHERE SESSION_ID=?";
$stmt = $mysql->prepare($sql);
$stmt->bind_param('s', $_COOKIE['MySite_Session_ID']);
$stmt->execute();
setcookie ('MySite_Session_ID', "", 1);
setcookie ('MySite_Session_ID', false);
unset($_COOKIE['MySite_Session_ID']);
header("Location: /");
exit();

Check if they're logged in

ini_set('display_errors', 1);
ini_set('display_startup_errors', 1);
error_reporting(E_ALL);
$mysql = mysqli_connect('localhost', 'root', '');
mysqli_select_db($mysql, "mysite");
date_default_timezone_set('Europe/Amsterdam');
if (isset($_COOKIE['MySite_Session_ID'])) {
$usersession = $_COOKIE['MySite_Session_ID'];
$sql = "SELECT SESSION_USER_ID,SESSION_EXPIRES FROM sessions WHERE SESSION_ID = ? AND SESSION_ACTIVE = '1' AND SESSION_AGENT = ?";
$stmt = $mysql->prepare($sql);
$stmt->bind_param('ss', $usersession, $_SERVER['HTTP_USER_AGENT']);
$stmt->execute();
$dasdasresult = $stmt->get_result();
$results = $dasdasresult->num_rows;
if ($results >= 1) {
 // Is logged in
 $row = $dasdasresult->fetch_assoc();
 $userid = $row["SESSION_USER_ID"];
 $expiretime = $row["SESSION_EXPIRES"];
 if ($expiretime > time()) {
 // Session did not expire
 $sql = "SELECT * from users WHERE id=?";
 $stmt = $mysql->prepare($sql);
 $stmt->bind_param('s', $userid);
 $stmt->execute();
 $dasdasresultx = $stmt->get_result();
 $results = $dasdasresultx->num_rows;
 if ($results >= 1) {
 // User exists
 $row = $dasdasresultx->fetch_assoc();
 $username = $row["username"];
 $name = $row["name"];
 $userid = $row["id"];
 $email = $row["email"];
 $loggedin = 1;
 } else {
 // User does not exist
 header("Location: /login");
 exit();
 }
 } else {
 // Session expired
 $sql = "UPDATE sessions SET SESSION_ACTIVE = '0' WHERE SESSION_ID = ?";
 $stmt = $mysql->prepare($sql);
 $stmt->bind_param('s', $usersession);
 $stmt->execute();
 header("Location: /login");
 exit();
 }
} else {
 header("Location: /login");
 exit();
}
} else {
 header("Location: /login");
 exit();
}

• engineercoding: Might not see it that quickly, but have you thought about CSRF?
• R Records: Hmm? Where do you mean?
• engineercoding: Read up on CSRF here: owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF). For more information or implementations use Google ;p
• R Records: Thank you. Yes I did take this in consideration, but not everywhere. I only put it in pages where I use GET, but not where I use POST because I believe they can only do a POST request by submitting a form, right? I tried submitting the form via URL and it never worked.
• engineercoding: Programmatically everyone can do a POST request

Answer 1

Security

• make sure your site is running over HTTPS.
• add httponly parameter to setcookie()
• you'd like to check whether openssl_random_pseudo_bytes()'s result was cryprographically strong, using the function's 2nd parameter

Some random nitpicks

$usersession = substr("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ", mt_rand(0, 50) , 1) . substr(md5(time()) , 1)

This code is rather silly and could be safely discarded.

 "Name can only contain alphabets and spaces.";

Suppose your site won't be overly popular in Ireland.
Yours, Patrick O'Neal

if (getenv('HTTP_CLIENT_IP')) {

Apart from the fact that registration IP is rather useless as is, the code is one of old PHP users' superstitions and should never be used. Any IP related stuff should be taken from REMOTE_ADDR only, not from various HTTP headers.

if (empty($email)) {

It makes no sense to use empty() for the previously defined variable. if (!$email) { would be enough.

 $sql = "SELECT id FROM `users` WHERE email=?";

mysqli_insert_id()

ini_set('display_errors', 1);

Should be 0 on a live site

"UPDATE sessions SET SESSION_ACTIVE = '0' WHERE SESSION_ID = ?";

I don't see much point in the SESSION_ACTIVE field, as it's just duplicating the timeout

setcookie ('MySite_Session_ID', "", 1);
setcookie ('MySite_Session_ID', false);

There should be only one call and parameter mach ones with setting a session cookie, otherwise it won't be removed

unset($_COOKIE['MySite_Session_ID']);

this call is useless at the end of the script

$xsql = "SELECT * FROM users WHERE email=?";
$xstmt = $mysql->prepare($xsql);
$xstmt->bind_param('s', $_POST['email']);
$xstmt->execute();
$xresult = $xstmt->get_result();
$xresults = $xresult->num_rows;

Did you notice that you are repeating this kind of code all the time? There is one programming concept which is often underestimated by PHP users. It is called user-defined functions. What about calling a code like this:

$sql = "SELECT count(*) FROM users WHERE email=?";
$xresults = getSQLResult($mysql, $sql, $_POST['email']);

• R Records: Thank you so much for the detailed reply, I appreciate your time. I did almost everything you said except the first one with the generating the usersession. It works fine and I don't see anything wrong with it?
• Your Common Sense: It's just useless
• I wrestled a bear once.: REMOTE_ADDR comes from headers as well. perhaps you should explain exactly why that header is safe as opposed to others.
• Simon Forsberg: @Iwrestledabearonce. A) Does it really come from a header? webmasters.stackexchange.com/questions/49962/… B) It can be faked but is significantally more difficult to fake as the browser doesn't send it. (See link A) stackoverflow.com/q/5092563/1310566
• I wrestled a bear once.: @YourCommonSense and SimonForsberg - I used Google to find this very helpful video to explain how the server gets the IP from the packet headers. It is not difficult to fake with low level languages but its generally pointless to do so as that would cause the server to send the response to the wrong machine. youtube.com/watch?v=PpsEaqJV_A0

Answer 2

All code must be formatted and indented properly, otherwise you scare off potential readers. There are enough other things to worry about when reviewing code, so don't distract your readers.

So instead of this:

function abc() {
 if (condition) {
if (condition2) {
 action();
}
 }
 }

The code must look like:

function abc() {
 if (condition) {
 if (condition2) {
 action();
 }
 }
}

So when you say you beautified your code, did you actually do it, or did your IDE do it? It should always be the IDE for basic things like indentation and spacing. The first few lines of your code look very inconsistent in this regard.

• Your Common Sense: Surely you meant this as a comment, aren't you?
• Roland Illig: No, I didn't. As a reviewer I expect a certain minimum quality of the code. I'm not willing to review obfuscated code, and I consider improper indentation to be exactly that. See also goto fail.
• R Records: Obfuscated code? I beautified the code and put it in a code tag. What else needs to be done?
• Roland Illig: I updated my answer. I hope it is clearer now.