Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

When it come to security I try to be to better as possible but I don't have the knowledge.

According to what I read on-line my following code should be good but I could use some of your comment/critic/fixes

Here is a simple class just to example how I would do a login. Does it look secure enough?

class UserClass
{
private $dbCon = null;
public $Error = '';
public function __construct(PDO $dbCon)
{
    $this->dbCon = $dbCon;
}
public function login($Email,$Password,$RegisterCustomerSession = FALSE)
{
    $GetSalt = $this->dbCon->prepare('SELECT id,salt,hashPass FROM `customer` WHERE `email` = :Email');
    $GetSalt -> bindValue(':Email',$Email);
    $GetSalt -> execute();
    if($GetSalt -> rowCount() == 0)
    {
        $this->Error = "No customer is registered with that email";
        return false;
    }
    elseif($GetSalt->rowCount()>0)
    {
        $CustomerInfo = $GetSalt->fetch(PDO::FETCH_ASSOC);
        if(sha1($Password.$CustomerInfo['salt'])==$CustomerInfo['hashPass'])
        {
            if($RegisterCustomerSession)
                self::RegisterAllCustomerSession($CustomerInfo['id']);
            return true;
        }
        else
        {
            $this->Error = "Invalid Password";
            return false;
        }
    }
}
public function SetPassword($CustomerId,$Password)
{
    $Salt = self::CreateSalt(16);
    $HashPass = sha1($Password.$Salt);

    $SetPasswordAndSalt = $this->dbCon->prepare('UPDATE `customer` SET `hashPass` = :HashPass,`salt` = :Salt WHERE `id` = :CustomerId;');
    $SetPasswordAndSalt -> bindValue(':CustomerId',$CustomerId);
    $SetPasswordAndSalt -> bindValue(':Salt',$Salt);
    $SetPasswordAndSalt -> bindValue(':HashPass',$HashPass);
    try{
        $SetPasswordAndSalt ->execute();
        return true;
    }catch(PDOException $e){echo $e->getMessage(); return false; }

}
private function CreateSalt($HowLong = 16)
{
    $CharStr = 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789!@#$%^&*()-=+_<>';
    $ReturnStr = '';
    for($i = 0;$i<$HowLong;$i++)
    {
        $ReturnStr .= $CharStr{mt_rand(0,77)};
    }
    return $ReturnStr;
}
private function RegisterAllCustomerSession($CustomerId)
{
    // some code.
}
}
share|improve this question

1 Answer 1

up vote 4 down vote accepted

I'm not sufficiently familiar with PDO to say more on your database access than that you don't seem to have any injection vulnerabilities. However, there are a few things which I would do differently.

Salt entropy

There are two things which strike me as odd about your salt generation. Firstly, using a base-77 encoding. If you configure your database correctly then you can use the full 8 bits of each byte of salt. That's more efficient, avoids possible bugs with generating a number in the wrong range (how sure are you that strlen($CharStr) === 78?), and is closer to the assumptions made when analysing salted hashing.

Secondly, while mt_rand is better than rand it's not a cryptographic PRNG. The best portable secure PRNG in PHP is openssl_random_pseudo_bytes; if you're not planning to deploy to Windows then you could also get good entropy from /dev/random.

Hash

SHA is not generally recommended as a hash for passwords. The current conventional wisdom is that you want password hashing to be slow, and should use either bcrypt or scrypt. If you insist on SHA then you should use it as a component of PBKDF2.

Account existence oracle

There are two schools of thought on telling people "That username doesn't exist". The usability argument is that it's preferable to tell someone that they got their username wrong. The security argument is that you should always say "Either your username doesn't exist or you got your password wrong" to prevent people identifying accounts which do exist and then trying to brute-force their passwords. (Anti-brute-forcing techniques is a separate issue which I'm not going to address in detail).

If you favour the security argument over the usability argument then you need to avoid telling people indirectly that the username doesn't exist. That means that if $GetSalt -> rowCount() === 0 you should still do a hashing operation to avoid a quicker page load which leaks information.

Minor style points

if($GetSalt -> rowCount() == 0)
    ...
elseif($GetSalt->rowCount()>0)

I see three things wrong here:

  1. What other possibilities are there? Shouldn't that elseif just be an else?
  2. Why == instead of ===?
  3. Pick a style for use of whitespace and stick with it.
share|improve this answer
    
Thanks! For your answer this is a very complete answer I couldn't ask for more. also Sorry for my ignorance but :P you missed me for the first paragraph of salt entropy,. "Firstly, using a base-77 encoding. If you configure your database correctly then you can use the full 8 bits of each byte of salt. " huh ? Then " how sure are you that strlen($CharStr) === 78? " Im not sure what you mean again. –  Nicolas Racine Apr 25 at 17:12
    
CreateSalt has a set of permitted bytes which the salt can use. I'm saying that you should allow it to use all 256 possible values for each byte. –  Peter Taylor Apr 25 at 17:16
    
Ohhh!, Okay I Think I understand. Again sorry for my ignorance. How can put some byte that are not associated with a char in a string :S –  Nicolas Racine Apr 25 at 17:19
    
If you get your random data from either of the routes I suggested then you don't need to. If you get it from mt_rand(0, 255) then chr. –  Peter Taylor Apr 25 at 17:20
    
Thanks! Question resolved! –  Nicolas Racine Apr 25 at 17:21

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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