-4
public function login($email, $password){
    $stmt = $this->pdo->prepare("SELECT 'user_id' FROM 'users' WHERE 'email'=:email AND 'password' = :password");
$stmt->bindParam(":email", $email, PDO::PARAM_STR);
$stmt->bindParam(":password", md5($password), PDO::PARAM_STR);
$stmt->execute();
$user = $stmt->fetch(PDO::FETCH_OBJ);
$count = $stmt->rowCount();

if($count >0){
 $_SESSION['user_id'] = $user->user_id;
 header('Location: home.php');
}else{
 return false;
}
}

by using md5 in password I am getting an error : Only variables should be passed by reference in D:\xammp\htdocs\twitter\core\classes\user.php on line 18

and on removing md5, I am getting error for invalid password though I am entering the correct password as in database.

  • 12
    md5 is not a secure hashing function for passwords. Use `password_hash` and `password_verify` instead. – Philipp Maurer Jan 09 '18 at 10:52
  • `$stmt->bindParam(":password", md5($password));` – urfusion Jan 09 '18 at 10:53
  • 3
    I've downvoted this question because it advocates the use of bad and irresponsible security practices. – apokryfos Jan 09 '18 at 10:59
  • 4
    @apokryfos Though on the plus side, it is using a prepared statement with bind variables; shame about the md5, but there's still hope – Mark Baker Jan 09 '18 at 11:11
  • 1
    **Do not use MD5, it is not secure and puts the user at risk.** When saving a password verifier just using a hash function is not sufficient and just adding a salt does little to improve the security. Instead use a function such as `PBKDF2`, `Rfc2898DeriveBytes`, `Argon2`, `password_hash`, `Bcrypt` or similar functions with about a 100ms duration. The point is to make the attacker spend substantial of time finding passwords by brute force. For PHP use [php Password Hashing Functions](http://php.net/manual/en/ref.password.php) – zaph Jan 09 '18 at 12:37

2 Answers2

2

Use the PHP built-in password_hash() function to encrypt your passwords.

password_hash():

creates a new password hash using a strong one-way hashing algorithm.

use it like this:

$passHash = password_hash("myPassword", PASSWORD_BCRYPT);

Note:

PASSWORD_BCRYPT - Use the CRYPT_BLOWFISH algorithm to create the hash. This will produce a standard crypt() compatible hash using the "$2y$" identifier. The result will always be a 60 character string, or FALSE on failure.

reference http://php.net/password-hash.

then change this line:

$stmt->bindParam(":password", md5($password), PDO::PARAM_STR);

to this:

$stmt->bindValue(":password", $passHash, PDO::PARAM_STR);

To verify a password you would use passsword_verify().

passsword_verify():

Verifies that the given hash matches the given password.

reference http://php.net/password-verify.

Use passsword_verify() like this:

if(password_verify('myPassword', $passHash))
{
    // the password is correct
}
else
{
    // incorrect password
}
Ivan86
  • 5,695
  • 2
  • 14
  • 30
  • You should really be recommending the built-in password_hash()/password_verify() functions rather than trying to suggest how md5 can be made more secure, because it still isn't even remotely secure – Mark Baker Jan 09 '18 at 11:10
  • @MarkBaker you are right. The question was _how to encrypt the password in mysql using md5_ so I answered to that, but I should add better examples with `password_hash()/password_verify()` and I will. – Ivan86 Jan 09 '18 at 11:19
  • @MarkBaker I think this is sufficient and understandable. Thanks for suggestions. – Ivan86 Jan 09 '18 at 11:48
  • Incidentally, this will still produce the same error as the original for the same reason. – deceze Jan 09 '18 at 11:54
  • @deceze I changed to `bindValue()`. That should do it. Thanks. – Ivan86 Jan 09 '18 at 12:05
0

As mentioned in the comment from @Philipp, md5 is not encryption and not for use with passwords - but to do what you are trying you need to set the return of md5 as a variable instead.

So change

$stmt->bindParam(":password", md5($password), PDO::PARAM_STR);

To

$md5hash=md5($password);
$stmt->bindParam(":password", $md5hash, PDO::PARAM_STR);
Professor Abronsius
  • 33,063
  • 5
  • 32
  • 46
  • The much simpler solution would be `bindValue` instead of `bindParam`… – deceze Jan 09 '18 at 11:53
  • **Do not do this, it is not secure and puts the user at risk.** BTW Philipp recommended not to use MD5 and to use `password_hash` and `password_verify` which is a secure and simple solution. – zaph Jan 09 '18 at 12:36
  • Just "for the record" - I was, in no way, advocating the use of `md5` for the storage of passwords - I was merely trying to illustrate the problem with the original approach and how it could be overcome with the approach shown. – Professor Abronsius Jan 09 '18 at 15:28