Is this user login secure?

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!@#$%^&*()-=+_<>';

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:

• What other possibilities are there? Shouldn't that elseif just be an else?

• Why == instead of ===?

• Pick a style for use of whitespace and stick with it.

StackExchange Code Review Q#48146, answer score: 5