Is this admin area secure enough? V2

Question

This is a response to my previous question, Is this admin area secure enough?.

There were some very helpful answers there, for which I am very grateful. So I went back to the drawing board and here is my new question, because I'm new to the security side of things and I'm not sure if I've understood or covered all of the loop holes correctly.

---

The problems I'm trying to protect the system against are:

• Man in the Middle attacks
• SQL injection attacks
• CSRF attacks
• XSS attacks
• Click jacks
• Brute force attacks

Notes

• Passwords are stored with this implementation of bcrypt
• The entire site will use the https:// protocol
• Flags session.cookie_secure and session.cookie_httponly will be set
• PHP's PDO object is used in all SQL queries, all inputs are parameterized
• Any user input variables will be sanitized
• Every page will start with a x-frame-options: deny http header
• Every restricted page will start with an authorization script (see fig. 1)
• All forms will only be authorised if the set variable $_SESSION["auth_key"] matches the key sent with the form (see fig. 1)

Fig.1: Authorization & Login Script

I hope this diagram is mostly self-explanatory. However, I will try to clarify certain functions.

• check token: A token that is generated once a user logs in, stored in $_SESSION['auth_token'] and sent with any form the user submits.
• check session: If the user has a valid token, pull their details from the database using the PHPSESSID (the name is required to welcome back the user). If the session id is not found or the token is invalid, their session has expired and they need to login again.
• regen. session: Once the user is logged in successfully, give them a new session id (session_regenerate_id()) and update the database with the new session id. Set the $_SESSION['auth_token'] = md5(mt_rand(1,10000).$username);. The username is unique so this should generate a unique auth token so they may submit their own forms. I'm not sure if I've implemented this correctly - please see the example here
• check login: Simply checks the bcrypted password matches the bcrypted input, and usernames match.
• fail attempts: This side is added to slow down brute force attacks; first a captcha is added, then the IP is temporarily blocked and the user/account holder is sent an email of notification.
• I will add more detail if necessary/correct these details if there are any mistakes

Implementation (so far):

Restricted page: starts with requirement of authenticate.php

<?php require_once "authenticate.php"; ?>

<p>Welcome <?php echo $_SESSION["guest"]; ?>!
<a href="index.php?action=logout">Logout</a></p>

<p><a href="index.php?token=<?php echo $_SESSION["auth_token"] ?>">Try again.</a></p>

Authenticate.php: redirects to login.php if not authenticated
Note: I left encryption out (while testing) because bcrypt is unavailable in my version of PHP (5.2.17) so I'm looking for either SHA256 or SHA512.

<?php
    session_start();
    // load database abstraction layer
    require_once "../../dal.php";
    unset($_SESSION["login_errors"]);

function loginError($str) {
    if (isset($_SESSION["login_errors"]))
        $_SESSION["login_errors"] = "" . $_SESSION["login_errors"] . "; " . $str;
    else
        $_SESSION["login_errors"] = $str;
}
function hasValidToken() {
    return (isset($_SESSION["auth_token"]) && isset($_GET["token"])
    && $_SESSION["auth_token"] == $_GET["token"]);
}
function hasValidSession() {
    return (session_id()? hasOpenSession() : false);
}
function hasOpenSession() {
    $sql = "SELECT * FROM tbl_store_admin WHERE php_sesskey=?;";
    $data = array(session_id());
    return (dbRowsCount($sql, $data) == 1);
}
function updateUserSession() {
    $sql = "UPDATE tbl_store_admin SET php_sesskey=? WHERE username=?";
    $data = array(session_id(), $_SESSION["uid"]);
    dbQuery($sql, $data);
    return (dbRowsAffected() == 1);
}
function hasLoggedIn() {
    if (isset($_POST["uid"]) && isset($_POST["key"])) {
        $uid = htmlspecialchars($_POST["uid"]);
        $key = htmlspecialchars($_POST["key"]);
        return (getUser($uid, $key));
    } else {
        return false;
    }
}
function wrongCredentials() {
    if (isset($_SESSION["login_attempts"])) {
        $_SESSION["login_attempts"] = $_SESSION["login_attempts"]+1;
    } else {
        $_SESSION["login_attempts"] = 1;
    }
    logout();
    loginError("Bad Credentials");
    return false;
}
function getUser($uid, $key) {
    $sql = "SELECT * FROM tbl_store_admin WHERE username=? AND keycode=? LIMIT 1;";
    $data = array($uid, $key);
    $rows = dbRowsCount($sql, $data);
    if ($rows == 1) {
        dbQuery($sql, $data);
        $user = dbFetch();
        // store data in session
        $_SESSION["uid"] = $user["username"];
        $_SESSION["guest"] = $user["nickname"];
        return true;
    } else {
        return wrongCredentials();
    }
}
function logout() {
    if (isset($_SESSION["auth_token"])) {
        unset($_SESSION["auth_token"]);
    }
    if (isset($_SESSION["uid"])) {
        $sql = "UPDATE tbl_store_admin SET php_sesskey=NULL WHERE username=?;";
        $data = array($_SESSION["uid"]);
        dbQuery($sql, $data);
        unset($_SESSION["uid"]);
    }
    if (isset($_SESSION["guest"])) {
        unset($_SESSION["guest"]);
    }
}
function hasAuthenticated() {
    if (hasValidToken()) {
        if (hasValidSession()) {
            return true;
        } else {
            logout();
            return false;
        }
    } else {
        logout();
        return hasLoggedIn();
    }
}
function regenerateSession() {
    session_regenerate_id();
    updateUserSession();
    $token = "" . mt_rand(1,10000) . $_SESSION["uid"];
    $_SESSION["auth_token"] = md5($token);
}
if (isset($_GET["action"]) && $_GET["action"]=="logout") {
    logout();
}
if (!hasAuthenticated()) {
    header('Location: login.php');
} else {
    regenerateSession();
}

?>

Login.php: A simple login form

<?php
session_start();
function getRequestURL() {
    if (isset($_SESSION['HTTP_REFERER'])) {
        return $_SESSION['HTTP_REFERER'];
    }
    return "index.php?token=".$_SESSION["auth_token"];
}
?>

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
   "http://www.w3.org/TR/html4/loose.dtd">

<html>
<head>
<meta http-equiv="X-Frame-Options" content="deny">

<title>Admin Login</title>

<style type="text/css">
.errorbox {
 position:absolute;
 top:0px;
 left:0px;
 width:100%;
 height:100px;
 background-color:#FFAAAA;
 border:1px solid #FF0000;
}
.loginbox {
 width:600px;
 height:300px;
 border:5px solid #000000;
 margin:100px auto;
 
border-radius:10px;
 -moz-border-radius:10px;
 -webkit-border-radius:10px;
 background-color:#CCCCCC;
 
box-shadow:4px 4px 2px #999999;
 -moz-box-shadow:4px 4px 2px #999999;
 -webkit-box-shadow:4px 4px 2px #999999;
}
.formbox {
 width:400px;
 height:200px;
 margin:0px auto;
}
.label {
 width:120px;
 height:40px;
 text-align:right;
 line-height:40px;
 float:left;
 font-weight:bold;
}
.textinput {
 height:40px;
 width:260px;
 font-size:30px;
 line-height:40px;
}
.submitbtn {
 height:50px;
 width:100px;
 font-size:30px;
 line-height:40px;
}
</style>
</head>

<body style="font-family: Arial;font-size:20px;">

<?php
if (isset($_SESSION["login_errors"])) {
?>
    <div class="errorbox">
        Error: <?php echo ($_SESSION["login_errors"]); ?>
    </div>
<?php
}
?>

<div class="loginbox">
<h1 align="center">Admin Area</h1>

<div class="formbox">
<form action="<?php echo getRequestURL(); ?>" method="POST">
<p><big>
<div class="label">Name:</div>&nbsp;
<input class="textinput" type="text" name="uid"/><br>
<div class="label">Password:</div>&nbsp;
<input class="textinput" type="password" name="key"/><br>
</big></p>
<p align="right"><input class="submitbtn" type="submit" value="Log In"/></p>
</form>
</div>

</div>

</body>
</html>

Answer 1

Overall. This is much better. But I still have some comments --

Clickjacking. Your diagram on preventing clickjacking is awfully vague. I presume you are doing client-side framebusting (using Javascript that runs on the client). Be warned that this is extremely error-prone and most people who write their own framebusting code end up with something that is subtly broken. One academic paper surveyed the top 500 most widely used sites (in 2010), and found that every single site that used framebusting, implemented it in a way that was insecure. The attacks were subtle and non-obvious, so the developers probably thought their code was fine -- when actually it was flawed.

I suggest that you read the following paper, which outlines approaches that are flawed and also describes how to do framebusting properly:

• Busting frame busting: a study of clickjacking vulnerabilities at popular sites. Gustav Rydstedt, Elie Bursztein, Dan Boneh, and Collin Jackson. W2SP 2010.

Workflow enforcement. I did not understand your discussion about ensuring that checkout.php shouldn't be accessed before review.php. It is not clear to me why you list this as a security property, or what security value this adds. It does not sound like a security issue to me. Also, your mechanism may break having multiple copies of your site open in multiple tabs.

If your concern is about forceful browsing attacks, I would suggest simply ensuring that you use proper CSRF defense at all places in the site, including on the checkout.php page and all form targets on that page.

CSRF defense. Note that you need to use a CSRF token to protect all side-effecting actions. If your site is properly architected, this means that a CSRF token should be used to protect all POST requests.

CSRF tokens. You use mt_rand() to generate the CSRF token. This is not secure and represents bad practice. You need to use a cryptographically strong PRNG (e.g., read from /dev/urandom), to ensure that the attacker cannot predict this value. mt_rand() is not cryptographically strong. See, e.g., Is a rand from glibc's rand secure for a login key?, What are the requirements for a random number generator to a be safe to use in cryptography?, Is a rand from /dev/urandom secure for a login key?.

Also, you are calling mt_rand(5,15), which means your random number is from the range 5..15. In other words, you have less than 4 bits of randomness. This is totally inadequate and completely broken. Hopefully that was a typo, or maybe I misunderstood.

Failed logins. You ban the user for one month after 10 failed login attempts. This is probably not such a great idea, because it makes it too easy for "griefers" to lock someone out of their account for a month. I suggest reading the following questions on this site: Why do sites implement locking after 3 failed password attempts?, Is denying login after incorrect attempts ineffective?, Appropriate strategy for preventing brute forcing of logins?, How do I secure my login page?, Why re-verify with CAPTCHA on failed form entry?.

Cookies. Make sure you set the secure flag on all cookies (so that they will only be sent back over HTTPS connections). I suggest that you enable HTTP Strict Transport Security, to prevent sslstrip-style attacks.

General comment. A lot of the stuff you are building is bog-standard. In the future you might consider choosing a web application development framework that provides support for these mechanisms (e.g., secure session management, CSRF defense, authorization checks). Also I encourage you to look at OWASP's resources on web application security. They cover a lot of the topics you're asking about pretty well.