3
\$\begingroup\$

I'm new to PDO, so I was wondering if you guys could check over my PDO code for efficiency (and if the PHP can be improved)

public function ValidateUser($username = '', $password = '')
{
    $prefix = "SELECT ";
    $type = 'COUNT(id)';
    $suffix = " FROM users WHERE username = :username LIMIT 1";
    if($grabUser = $this->db->prepare($prefix.$type.$suffix))
    { 
        $grabUser->bindParam(":username", $username, PDO::PARAM_STR);
        $grabUser->execute();
        if(count($grabUser->fetchColumn()) <= 0)
        {
            return array(0,0);
        }
    }
    else {
        return array(0,0);
    }

    $type = 'salt';
    if($grabUser = $this->db->prepare($prefix.$type.$suffix))
    { 
        $grabUser->bindParam(":username", $username, PDO::PARAM_STR);
        $grabUser->execute();
        $salt = $grabUser->fetch()['salt'];
        $password = $this->core->blueHash($password, $salt);
    }
    else {
        return array(0,0);
    }


    $stmt = "SELECT COUNT(id) FROM users WHERE username = :username AND password = :password LIMIT 1";
    if($checkFinal = $this->db->prepare($stmt))
    { 
        $checkFinal->bindParam(":username", $username, PDO::PARAM_STR);
        $checkFinal->bindParam(":password", $password, PDO::PARAM_STR);
        $checkFinal->execute();
        if(!count($checkFinal->fetchColumn()) > 0)
        {
            return array(0,0);
        }
        return array(count($checkFinal->fetchColumn()),$password);
    }
    return array(0,0);
}

This code checks that the supplied username and password are correct and if so, an array is returned.

EDIT: AJAX file where ValidateUser() is used:

<?php
require_once "../../_global.php";
$core->ajaxCheck();

header("Content-Type: application/json");
$json = array();

if(USER_ID <= 0 && !LOGGED_IN)
{
    // DO ISSET CHECK :((
    $username = $core->input($_POST['name']);
    $password = $core->input($_POST['pword']);

    $valid = $users->ValidateUser($username, $password);
    if($valid[0] == 0)
    {
        $json[0] = "error";
        $json[1] = "ajax";
        $json[2] = "These details are incorrect.";
    }
    elseif($valid[0] > 0)
    {
        $_SESSION['login']['username'] = $users->GrabUserVar($users->Name2id($username),"username");
        $_SESSION['login']['hash'] = $valid[1];
        $_SESSION['login']['social'] = false;
        $json[0] = "success";
    }
}
else {
    $json[0] = "error";
    $json[1] = "ajax";
    $json[2] = "You are already signed in.";
}
echo json_encode($json);
exit;
?>

EDIT2: Code that is run on each page to check whether the session username and password hash match that of the database:

session_start();
if(isset($_SESSION['login']['username']) && isset($_SESSION['login']['hash']))
{
    $userN = $_SESSION['login']['username'];
    $userH = $_SESSION['login']['hash'];
    if($users->CheckUser($userN, $userH) > 0)
    {
        define('LOGGED_IN',true);
        define('USER_ID',$users->Name2id($userN));
        $cUser = $users->createIndividual(USER_ID);
        define('USER_NAME',$cUser->getUsername());
        define('USER_HASH',$userH);
    }
    else {
        define('LOGGED_IN',false);
        define('USER_ID',0);
        define('USER_NAME','Guest');
        define('USER_HASH',0);
        session_destroy();
    }
}
else {
    define('LOGGED_IN',false);
    define('USER_ID',0);
    define('USER_NAME','Guest');
    define('USER_HASH',0);
}
\$\endgroup\$
2
\$\begingroup\$

Efficiency

You currently perform three queries:

  • check if a user with that name exists
  • get salt for that username
  • check if user with that username and password exist

You can perform all these actions with one query:

SELECT salt, password FROM users WHERE username = :username LIMIT 1;

Then just hash the supplied password with the salt from the db, and compare the result to the password from the db. It might look something like this:

if (hash_equals($grabUser->fetch()['password'], $this->core->blueHash($password, $grabUser->fetch()['salt'])) {
    echo 'valid password';
}

Return Early

If you negate your if clauses, you can safe one level of nesting, which I think results in more readable code, because nesting decreases readability, and because it's clearer to what if clause the else clause belongs. Eg:

if(!($grabUser = $this->db->prepare($prefix.$type.$suffix))) 
{
    return array(0,0);
}

$grabUser->bindParam(":username", $username, PDO::PARAM_STR);
$grabUser->execute();
if(count($grabUser->fetchColumn()) <= 0)
{
    return array(0,0);
}

Misc

  • it's unclear why you are returning an array. It seems to always contain [1, $userPassword]. Why do you need to return the password? And wouldn't it be easier to just return true or false instead of 1 or 0 inside an array? At least I would add a PHPDoc style comment explaining what is returned.
  • I would extract some of the code to its own function to increase readability and avoid duplication. eg you could have fetchFromUser($columnName, $username).
  • I don't know what password hashing you currently use, but I would think about switching to bcrypt. It's easier to use (you don't have to manage salts, etc), and is currently considered secure.
\$\endgroup\$
12
  • \$\begingroup\$ Fantastic response, thank you! In regards to using only one query, I have split them up because I am unable to use a function such as num_rows with PDO. After looking around on Stackexchange (etc) the best method is to use COUNT() otherwise I see your point. The first part of the array should be 1 as you said; no idea why I made it count for that area, however the count is used for an AJAX request to check whether ValidateUser returned a user from the database or not. If it does, the returned password within the array is sent to the AJAX file to set a session. I might rethink this concept... \$\endgroup\$ – zuc0001 Apr 25 '15 at 9:31
  • 1
    \$\begingroup\$ You could just use the PHP function count on the result array, or empty on the result values; it definitely should not be necessary to perform three queries. And yes, I would probably rethink the sending back of the password hash; it doesn't seem necessary or a good idea (you could post that part of your code for review here as well, I'm sure someone could suggest improvements on it). And sha256 isn't all that bad, but it's really not recommended anymore, because it's way too fast. You should use bcrypt, which also uses unique per-user salts (automatically, you don't have to manage this). \$\endgroup\$ – tim Apr 25 '15 at 9:40
  • 1
    \$\begingroup\$ @zuc0001 I don't see why you need $_SESSION['login']['hash'], storing the username should be more than enough. Note also that session data might be stored in a shared temporary directory, so it should not contain sensitive information in plaintext (in this case, I would consider an unencrypted password hash plaintext). \$\endgroup\$ – tim Apr 25 '15 at 9:51
  • 1
    \$\begingroup\$ @zuc0001 you don't need to check the hash for that, the username is enough (session values are server side, so the user cannot change them directly; eg a user could not just change the username to be logged in as a different user). If the session is set, they are logged in. Storing and comparing the hash doesn't add any additional security (eg a user who highjacks the session would also pass that check). \$\endgroup\$ – tim Apr 25 '15 at 10:11
  • 1
    \$\begingroup\$ @zuc0001 Good point about storing the hash in the session. I still would not do it unencrypted though. Binding the session to an IP is great from a security perspective, but not that good regarding user experience (think eg about cellphones); a compromise might be to bind it to something like useragent. \$\endgroup\$ – tim Apr 25 '15 at 10:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.