Please critique this PHP login script

Question

A site I developed was recently compromised, most likely by a brute force or Rainbow Table attack. The original log-in script did not have a SALT, passwords were stored in MD5.

Below is an updated script, complete with SALT and IP address banning. In addition, it will send a Mayday email and SMS and disable the account should the same IP address or account attempt 4 failed log-ins. Please look it over and let me know what could be improved, what is missing, and what is just plain strange.

<?php
    //Start session
    session_start();
    //Include DB config
    include $_SERVER['DOCUMENT_ROOT'] . '/includes/pdo_conn.inc.php';

    //Error message array
    $errmsg_arr = array();
    $errflag = false;

    //Function to sanitize values received from the form. Prevents SQL injection
    function clean($str) {
        $str = @trim($str);
        if(get_magic_quotes_gpc()) {
            $str = stripslashes($str);
        }
        return $str;
    }

    //Define a SALT, the one here is for demo
    define('SALT', '63Yf5QNA');

    //Sanitize the POST values
    $login = clean($_POST['login']);
    $password = clean($_POST['password']);
    //Encrypt password
    $encryptedPassword = md5(SALT . $password);
    //Input Validations
    //Obtain IP address and check for past failed attempts
    $ip_address = $_SERVER['REMOTE_ADDR'];
    $checkIPBan = $db->prepare("SELECT COUNT(*) FROM ip_ban WHERE ipAddr = ? OR login = ?");
    $checkIPBan->execute(array($ip_address, $login));
    $numAttempts = $checkIPBan->fetchColumn();
    //If there are 4 failed attempts, send back to login and temporarily ban IP address
    if ($numAttempts == 1) {
        $getTotalAttempts = $db->prepare("SELECT attempts FROM ip_ban WHERE ipAddr = ? OR login = ?");
        $getTotalAttempts->execute(array($ip_address, $login));
        $totalAttempts = $getTotalAttempts->fetch();
        $totalAttempts = $totalAttempts['attempts'];
        if ($totalAttempts >= 4) {
            //Send Mayday SMS
            $to = "[email protected]";
            $subject = "Banned Account - $login";
            $mailheaders = 'From: [email protected]' . "\r\n";
            $mailheaders .= 'Reply-To: [email protected]' . "\r\n";
            $mailheaders .= 'MIME-Version: 1.0' . "\r\n";
            $mailheaders .= 'Content-type: text/html; charset=iso-8859-1' . "\r\n";
            $msg = "<p>IP Address - " . $ip_address . ", Username - " . $login . "</p>";
            mail($to, $subject, $msg, $mailheaders);
            $setAccountBan = $db->query("UPDATE ip_ban SET isBanned = 1 WHERE ipAddr = '$ip_address'");
            $setAccountBan->execute();
            $errmsg_arr[] = 'Too Many Login Attempts';
            $errflag = true;    
        }
    }
    if($login == '') {
        $errmsg_arr[] = 'Login ID missing';
        $errflag = true;
    }
    if($password == '') {
        $errmsg_arr[] = 'Password missing';
        $errflag = true;
    }

    //If there are input validations, redirect back to the login form
    if($errflag) {
        $_SESSION['ERRMSG_ARR'] = $errmsg_arr;
        session_write_close();
        header('Location: http://somewhere.com/login.php');
        exit();
    }

    //Query database
    $loginSQL = $db->prepare("SELECT password FROM user_control WHERE username = ?");
    $loginSQL->execute(array($login));
    $loginResult = $loginSQL->fetch();

    //Compare passwords
    if($loginResult['password'] == $encryptedPassword) {
        //Login Successful
        session_regenerate_id();
        //Collect details about user and assign session details
        $getMemDetails = $db->prepare("SELECT * FROM user_control WHERE username = ?");
        $getMemDetails->execute(array($login));
        $member = $getMemDetails->fetch();
        $_SESSION['SESS_MEMBER_ID'] = $member['user_id'];
        $_SESSION['SESS_USERNAME'] = $member['username'];
        $_SESSION['SESS_FIRST_NAME'] = $member['name_f'];
        $_SESSION['SESS_LAST_NAME'] = $member['name_l'];
        $_SESSION['SESS_STATUS'] = $member['status'];
        $_SESSION['SESS_LEVEL'] = $member['level'];
        //Get Last Login
        $_SESSION['SESS_LAST_LOGIN'] = $member['lastLogin'];
        //Set Last Login info
        $updateLog = $db->prepare("UPDATE user_control SET lastLogin = DATE_ADD(NOW(), INTERVAL 1 HOUR), ip_addr = ? WHERE user_id = ?");
        $updateLog->execute(array($ip_address, $member['user_id']));
        session_write_close();
        //If there are past failed log-in attempts, delete old entries
        if ($numAttempts > 0) {
            //Past failed log-ins from this IP address. Delete old entries
            $deleteIPBan = $db->prepare("DELETE FROM ip_ban WHERE ipAddr = ?");
            $deleteIPBan->execute(array($ip_address));
        }
        if ($member['level'] != "3" || $member['status'] == "Suspended") {
            header("location: http://somewhere.com");
        } else {
            header('Location: http://somewhere.com');
        }
        exit();
    } else {
        //Login failed. Add IP address and other details to ban table
        if ($numAttempts < 1) {
        //Add a new entry to IP Ban table
        $addBanEntry = $db->prepare("INSERT INTO ip_ban (ipAddr, login, attempts) VALUES (?,?,?)");
        $addBanEntry->execute(array($ip_address, $login, 1));
        } else {
            //increment Attempts count 
            $updateBanEntry = $db->prepare("UPDATE ip_ban SET ipAddr = ?, login = ?, attempts = attempts+1 WHERE ipAddr = ? OR login = ?");
            $updateBanEntry->execute(array($ip_address, $login, $ip_address, $login));
        }
        header('Location: http://somewhere.com/login.php');
        exit();
    }
?>

EDIT

Okay, here is my attempt at a random Salt. First, create the salt to be inserted into the table:

define('SALT_LENGTH', 15);
function createSalt()
{
    $key = '!@#$%^&*()_+=-{}][;";/?<>.,';
    $salt = substr(hash('sha512',uniqid(rand(), true).$key.microtime()), 0, SALT_LENGTH);
    return $salt;
}
$salt = createSalt()
//More prep for entering into table...

Then generate the hash with random salt:

$hash = hash('sha256', $salt . $pw); //$pw is the cleaned user submitted password

When the user logs in, compare the stored hash using the stored randomly generated salt:

$loginHash = hash('sha256', $dbSalt . $pw);
if ($loginHash == $dbHash) {
    //Logged in
} else {
    //Failed
}

How does that look?

Answer 1

Ok, here's a few:

1. You shouldn't be using md5 anymore. You can, but there are better methods (Such as sha512 used with the hash() function).

2. I would use a MUCH longer static salt as well. I suggest at least 64 characters (after all, it's minimal overhead to compute on write, but much more difficult to guess).

3. I would also add a dynamic (random) salt as well. Generate a new one for each user, and store it along side the password hash (it's common to separate them by a : character). This way, even if your static salt is compromised, a new rainbow table would need to be generated (or at least iterated against) for each password in your db...

4. Don't trust or operate based upon IP address for anything non-temporal. Most ISPs use a form of NAT where more than one user will be seen from a single IP (and this will only become more prevalent with the exhaustion of the IPv4 namespace). If you want to rate-limit or temporarily block IP addresses, fine. But don't ban on them...

5. Your clean() function should either check for a string first, or force it to be a string: ($str = is_string($str) ? trim($str) : (string) $str;). It doesn't prevent sql injection at all. But the stripslashes call is necessary (exactly how you have it) to allow the code to work on servers that have magic_quotes_gpc set (which will attempt to escape quotes for you)... So keep that around.

6. Format your code better. Create functions to handle related tasks. That way you don't have 75 lines of procedural code to look through to figure out what's going on. Better yet, wrap it in a class and move the common tasks (db access, etc) into their own classes. And remember to indent properly. Readability is king, so don't take shortcuts...

Edit: As far as how to validate the password, you fetch the salted hash first, then re-compute the hash with the stored salt. (The makeSaltedHash function I show below makes additional use of something called Key Stretching.

function validatePassword($password, $hash) {
    list($oldHash, $salt) = explode(':', $hash, 2);
    $newHash = makeSaltedHash($password, $salt);
    return $hash == $newHash;
}

function makeSaltedHash($password, $salt = '') {
    if (empty($salt)) {
        $salt = makeRandomSalt(mt_rand(64, 128));
    }
    $hash = hash('sha512', $password . $salt . SALT);
    for ($i = 0; $i < 50; $i++) {
        $hash = hash('sha512', $password . $salt . SALT . $hash);
    }
    return $hash . ':' . $salt;
}

function makeRandomSalt($length = 64) {
    $salt = '';
    for ($i = 0; $i < $lenght; $i++) {
        $salt .= chr(mt_rand(33, 126));
    }
    return $salt;
}

Answer 2

Two tips from me:

• Don't use stripslashes or magic quotes to prevent SQL injection. Use PDO parameters. No exceptions.
• Use a different salt for every user. The salt is no secret, store it in the DB along with the user record. Using the same salt for everyone makes your database more attackable.

Unrelated, but often overlooked: Do not limit password length for your users. I've seen too many web sites that impose an arbitrary limit (like 12 characters) on password length, but then have laughable complexity rules ("at least one upper, one lower, a digit and a special character but not '<', '>'" or such nonsense). This is very hostile, avoid it.