Problems with PHP login code validation and password_verify function

I’ve been using BCRYPT and the default cost of 10 and which seems sufficient. So basically, I need code that will input the member’s username and unhashed password, then retrieve the record by unique username (I’m working on validation), access the hashed password, and compare (password_verify is supposed to automatically hash before comparison I read). Then if passwords match, log the user in.

Correct? And this should all be done using Prepared Statements?

I suggest using PASSWORD_DEFAULT since it uses the next available hashing algorithm. One day, Bcrypt will become deprecated and it’s going to be a mess having to migrate all of your code to use the next hashing algorithm.

Then you’re reading wrong. password_verify() doesn’t hash. It returns a Boolean based on the given arguments. If you read somewhere to “re-hash” before using password_verify then that’s something completely different.

Correct. That’s all you need. And yes. Use prepared statements to avoid exploits as much as possible.

1 Like

I would say the “hard” part is that you need to make a workflow that re-hashes all your old passwords. Changing your configuration value, or having a quick search for all the places the constant is used, or letting PHP change the default algo is a minor issue, but at least i would use PASSWORD_DEFAULT so everything is auto updated.

2 Likes

Okay cool. So this will always work then? Even if something changes in PHP?

I meant that there doesn’t appear a need to hash the user entered password before checking it as password_verify’s first argument is a plain text password.

I’m now working with object oriented prepared statements. Is this good?

Okay thanks for the tip!

Yes. PASSWORD_DEFAULT will always default to the next available hashing algorithm. I know BCRYPT will eventually become deprecated since they’ve done that to MCRYPT. There will always be newer and better hashing algorithms so PASSWORD_DEFAULT will most likely default to those.

It should be. Even if you’re using procedural code, if you know that you’re using prepared statements then that’s all you need to worry about. Just remember that stuffing raw variables into the query while using prepared statements “doesn’t” mean you’re using prepared statements properly. It’s equivalent to just not using prepared statements at all.

Doing something like this is a big no no

$prepare = $db->prepare("SELECT * FROM users WHERE username = $username");

Anything that is written like this is just asking to get hacked. There are multiple ways that people will write this. They will either do something like "SELECT ... WHERE username = '$username'", 'SELECT ... WHERE username = '.$username.'' or any of the like with variants of concatenation. This is a big no no. Don’t do it because this defeats the purpose of using prepared statements. The whole point of using prepared statements is to bind the parameterized placeholder with a variable so that PHP doesn’t execute the query as literal SQL. Rather, it’ll treat the string as a junk string and not literal SQL. You don’t do that if you are stuffing raw variables into the query because there is no parameterized placeholders to bind. This is extremely dangerous.

2 Likes

Okay, what about this:

if (count($errors) == 0) {
  	$password = password_hash($password_1, PASSWORD_DEFAULT);//encrypt the password before saving in the database

      $stmt = $db->prepare("INSERT INTO members (member_name, member_password, member_email) VALUES (?, ?, ?)");
      $stmt->bind_param("sss", $member_name, $member_password, $member_email);

	$member_name = "$username";
	$member_password = "$password";
	$member_email = "$email";
	$stmt->execute();

  	// $_SESSION['username'] = $username;
  	header('location: thanks.html');
  }

It finally works (my original issue was the password not being validated and ANY password entered with an existing username would work). That’s now fixed. But now I’m having validation issues (e-mail and username are not being validated and can be registered, even if they exist). Here’s the complete code, I must be missing something simple:

<?php
session_start();

require_once "Auth.php";
require_once "Util.php";

$auth = new Auth();
$util = new Util();

require_once "authCookieSessionValidate.php";

// initializing variables
$username = "";
$email    = "";
$errors = array(); 

// connect to the database
$db = mysqli_connect('localhost', xxxxxxxxxxxxx);

if ($isLoggedIn) {
    header('Location: dashboard.php');
}
// REGISTER USER
if (isset($_POST['reg_user'])) {
  // receive all input values from the form
  $username = mysqli_real_escape_string($db, $_POST['username']);
  $email = mysqli_real_escape_string($db, $_POST['email']);
  $password_1 = mysqli_real_escape_string($db, $_POST['password_1']);
  $password_2 = mysqli_real_escape_string($db, $_POST['password_2']);

  // form validation: ensure that the form is correctly filled ...
  // by adding (array_push()) corresponding error unto $errors array
  if (empty($username)) { array_push($errors, "Username is required!"); }
  if (empty($email)) { array_push($errors, "E-mail is required!"); }
  if (empty($password_1)) { array_push($errors, "Password is required!"); }
  if ($password_1 != $password_2) {
	array_push($errors, "The two passwords do not match!");
  }
  // first check the database to make sure 
  // a user does not already exist with the same username and/or email
  $user_check_query = "SELECT * FROM members WHERE member_username='$username' OR member_email='$email' LIMIT 1";
  $result = mysqli_query($db, $user_check_query);
  $user = mysqli_fetch_assoc($result);
  
  if ($user) { // if user exists
    if ($user['name'] == $username) {
      array_push($errors, "Username already taken!");
    }

    if ($user['email'] == $email) {
      array_push($errors, "E-mail already taken!");
    }
  }

  // Finally, register user if there are no errors in the form
  if (count($errors) == 0) {
  	$password = password_hash($password_1, PASSWORD_DEFAULT);//encrypt the password before saving in the database

      $stmt = $db->prepare("INSERT INTO members (member_name, member_password, member_email) VALUES (?, ?, ?)");
      $stmt->bind_param("sss", $member_name, $member_password, $member_email);

	$member_name = "$username";
	$member_password = "$password";
	$member_email = "$email";
	$stmt->execute();

  	// $_SESSION['username'] = $username;
  	header('location: thanks.html');
  }
}
?>

Thanks!

Off Topic:

@vexdarkness: when you post code on the forums, you need to format it so it will display correctly. (I’ve edited your last post for you.)

You can highlight your code, then use the </> button in the editor window, or you can place three backticks ``` (top left key on US/UK keyboards) on a line above your code, and three on a line below your code. I find this approach easier, but unfortunately some European and other keyboards don’t have that character.

1 Like

Thanks! I noticed the improper formatting and was actually editing my post at the same time as you. lol

1 Like

Then you have to set a proper index onto your database column: UNIQUE

https://dev.mysql.com/doc/refman/5.7/en/create-index.html

I can’t just query the database to see if anything matches? My DB is, and will remain, really small.

Got it, but is this safe:


  // first check the database to make sure 
  // a user does not already exist with the same username and/or email
  $user_check_query = "SELECT * FROM members WHERE member_name='$username' OR member_email='$email' LIMIT 1";
  $result = mysqli_query($db, $user_check_query);
  $user = mysqli_fetch_assoc($result);
  
  if ($user) { // if user exists
    if ($user['member_name'] == $username) {
      array_push($errors, "Username already taken!");
    }

    if ($user['member_email'] == $email) {
      array_push($errors, "E-mail already taken!");
    }
  }

No, someone might delete your database. You are still not using Prepared Statements, read again:

And then:

And if you want to stay with MySQLi, you can still do it:

https://www.php.net/manual/en/mysqli.prepare.php

1 Like

Do not do that. @chorn already told you what to do in Post #31.

Set a unique constraint on the DB column, attempt the insert and capture the duplicate error if any.

2 Likes

Show us the code you tried, and someone will help show what was wrong with it.

In here

  $user_check_query = "SELECT * FROM members WHERE 
member_name='$username' OR member_email='$email' LIMIT 1";

what’s the idea of the LIMIT 1? If you have one user with the same username, and another with the same email address, you’ll only show one of your error messages. I’m not sure that being so specific with the error messages is quite the right way as it gives a potential hacker a bit more information than they need. “Email address already taken” is a synonym for “you’ve found a valid email address, only the password to guess now.”

And here

$password_1 = mysqli_real_escape_string($db, $_POST['password_1']);
$password_2 = mysqli_real_escape_string($db, $_POST['password_2']);

is there any point in calling that function when you’re going to use prepared statements and password_hash() later on? I can’t think of one. It might even confuse password_verify(), I don’t know. But it seems a waste of effort.

2 Likes

Okay cool. Do I need to create a new DB or can I make this change to the existing one?

I got it working, thanks! :slight_smile:

It’s not my script. It’s an open source snippet I’ve been working with. And I was told to use a DB constraint anyway, and plan to remove that entire segment as I’m to understand it’s not secure.

Only needs to be done via the current database. If you’re using a GUI for looking through the database then just set the username and email as a UNIQUE key. That’s all you need to do. Then in PHP, you check to see if it returns a duplicate key or not using the error code 1062. Then that’s it.

Okay I’ll do some more research on that error. :smiley:

I wouldn’t want to display the actual error to them. They wouldn’t know what it even means. I’d put it in an if statement then display a custom error such as Please pick a different username. Or something like that. Displaying the actual duplicate message is a security risk because SQL errors tend to give more information than you really need. And again, it’s pretty useless for the user since they don’t know what it means.

1 Like

Agreed. I’m trying something like this:

if (mysql_errno() == 1062) {
    $message = "error etc";
}

I guess after the prepared statement insert routine?