PHP secure login script

I was just wondering how secure my code looked and if I'm overlooking any serious mistakes. Any suggestions/critiques are welcome.

This is my relevant login script.

login.php

``
Not all fields were entered";
} elseif (!preg_match('~^[a-z0-9_.-]+$~i', $user)) {
$error = "Usernames must be all lowercase. They can only contain letters, numbers, periods, hyphens, and underscores";
} else {
$stmt = $con->prepare("SELECT pass, user FROM members WHERE user= (?) LIMIT 1");
$stmt->bind_param('s', $user);

$stmt->execute();

$result = $stmt->get_result();
$num = $result->num_rows;
$row = $result->fetch_array(MYSQLI_ASSOC);

if (password_verify($pass, $row['pass']) && $row['user'] == $user) {
$stmt = $con->prepare("UPDATE members SET logins`=logins + 1 WHERE user= (?)");
$stmt->bind_param('s', $user);

$stmt->execute();

$result = $stmt->get_result();

$_SESSION['user'] = $user;
$_SESSION['canary'] = time();
$_SESSION['HTTP_USER_AGENT'] != md5($_SERVER['HTTP_USER_AGENT']);

$stmt = $con->prepare("SELECT logins FROM members WHERE user=(?)");
$stmt->bind_param('s', $user);

$stmt->execute();

$result = $stmt->get_result();
$num = $result->num_rows;
$row = $result->fetch_array(MYSQLI_ASSOC);

if ($row['logins'] == 1) {
queryMysql("INSERT INTO leave_type (user, hours, leave_start, leave_end, leave_reason, leave_type)
VALUES ('$user', '24.00', '2016-01-08T06:30:00', '2016-01-09T06:30:00', 'Example Leave', 'Annual')
");
die("setTimeout(function () {
window.location.href='FullscreenForm.php'; // the redirect goes here
}, 100); // 5 seconds

Security

Generally, your code looks secure. Your authentication doesn't seem to contain any giant logic flaws, you mostly use prepared statements, you use bcrypt, etc.

Unused Security Mechanisms and pieces of code

Your login does contain quite a bit of code that doesn't seem to be used, which makes it more complex than necessary, which makes it harder to maintain, and thus harder to verify its security.

For example, you have a remember_me cookie, which indicates that you have a remember_me functionality, which would need to be checked for security. But you don't actually have that functionality, just the cookie.

Another example is the failedLogins table. It suggests that you have some form of bruteforce detection / account blocking code. But again, it doesn't seem to be used and thus adds complexity and further attack surfaces.

Yet another example is the odd setcookie that sets $_COOKIE[session_name()] as session_name(). It doesn't seem all that harmful, but also doesn't seem to serve any purpose.

Because of all these additional pieces of code that don't do anything, it's hard to see at first glance what pieces of code actually do do something. For example, is the user agent check real, or just party implemented as some of the functionality above? [It's real - at least after the request after the initial login - , but it takes extra effort to see that]

Possible future SQL Injection

For the most part, you use prepared statements, which is great. But then all of a sudden, you have this:

queryMysql("INSERT INTO leave_type (user, hours, leave_start, leave_end, leave_reason, leave_type)
    VALUES ('$user', '24.00', '2016-01-08T06:30:00', '2016-01-09T06:30:00', 'Example Leave', 'Annual')
");

Now, you currently do have a regex on the username which would make it impossible to exploit this. But what if your requirements change? What if you want to allow Bill O'Harris to sign up as well? You might change that regex, and then be vulnerable.

You should really always use prepared statements, not only most of the time.

Possible future XSS

Same problem as above, if the regex changes and ' is allowed, this is vulnerable to XSS:

window.location.href='members.php?view=$user'; // the redirect goes here
    }, 0); " .

Misc

• Even though remember_me is currently unused, you should set it as httpOnly in case it is used in the future.

• You regenerate the session ever X minutes, which is good, but it is more important to regenerate it when the session state changes, especially on login (to avoid session fixation with some less common php.ini settings)

• It's good to use htmlspecialchars on PHP_SELF, but you should ideally use it when actually echoing PHP_SELF. If you handle other values like this as well, it will be very difficult to determine which values are safe and which are not (eg is $user safe?).

• It doesn't seem that you have any CSRF protection. Even login pages do need it (to easily avoid exploitation of XSS in the user area, and to avoid information leakages by force-login a user into an attackers account).

Other

Structure

You should introduce functions to make your code easier to read, test and reuse. You might also want to add classes, such as a User class. Functions may be User:getByName(), User:increaseLoginCount(), User:firstLogin(), redirectFirstLogin(), redirectLogin(), failedLogin(), checkSessionUserAgent(), etc.

You should also return early / add guard clauses to reduce nesting (eg if (!isset($_POST['user'])) { return; }).

Naming

Try to be consistent with your naming.

For example, why is the same thing called user when using POST, but username when using GET?

Formatting

• Your indentation is inconsistent (2 vs 3 vs 4 spaces), as is your use of spaces (sometimes, assignments are aligned, sometimes they are not).

• You don't need () around ?, it just makes the queries more difficult to read.

Misc

• Why is the cookie delete check outside of the big if? It's confusing, and you need to check isset($_POST['user']) twice because of it.

• The $row['user'] == $user check is confusing and not necessary. You just selected $row['user'] based on $user, so it will always be equal.

• "clever" code is never good. I would need to look up / test what $x != md5($x) does to understand it. It looks like a bug to me.

• remember_me doesn't seem to do anything.

• What's $hide?

• Do you really need to count the number of logins? It seems to add two unnecessary queries and quite a bit of unnecessary complexity. You could just add a isFirstLogin field to the members table and select that when selecting pass and user.

• You never do anything with failedlogins.

StackExchange Code Review Q#125202, answer score: 3