Php Registration code efficiency

[Fabel]

Hello,

 

I hope it's ok to ask this question here. I have a registration script, but I'm not sure how to handle it efficiently and I have some questions about it. 

This is used in the page 'signup.php'. The class is called 'User'. I haven't noticed any errors or bugs. It would be very useful for me to be aware of my mistakes. 

	public function regUser($uname,$upass,$upassverify)
    {
		$new_password = password_hash($upass, PASSWORD_DEFAULT);		
		if(!password_verify($upassverify, $new_password)) {
			// passwords are not the same (I thought it would be better to do this after hashing, but maybe it doesn't matter or it's worse. I'm not sure about it) 
			$info = 'pass_err'; 
		} 
		$stmt1 = $this->db->prepare("SELECT * FROM users WHERE username=:uname");
		$stmt1->execute(array(':uname'=>$uname));

		if($stmt1->rowCount() > 0) {
			// this username has already been used 
			$info = 'user_err'; 
		}
			
		if (!$info) {
			$stmt2 = $this->db->prepare("INSERT INTO users(username,password) 
										 VALUES(:uname, :upass)");
			$stmt2->bindparam(":uname", $uname);
			$stmt2->bindparam(":upass", $new_password);            
			$stmt2->execute();
			// succesfully made an account
			$info = "success";
		}
			header("Location:/signup.php?status=".$info);
			exit();	
    }

• Am I using the prepared statements as how I should be using them? 
• Is this a safe way of handling my data or do you see vulnerabilities? 
• I'm using PRG to prevent resubmission but I want to show a 'everything is fine' or 'oh no, something went wrong' to the one who is signinup. If I now go to signup.php?status=success, i see 'eveything is fine', without actually signing up, is there a better way to do this or can I somehow prevent everyone being able to see this?

As you might have noticed in my last post, my English is not very good, sorry about that. 

Thanks,

Fabian 

Edited September 17, 2019 by Fabel

[Barand]

Several things could be improved.

1 ) Do not use "SELECT * ". Specify the columns you need, in this case all you would need is count of matching records.

2 ) Having said that, searching for the username to see if exists before adding a new user is not the way to it. I don't know the structure of your users table but you should add a unique index on the username so it will throw an error if you try to add a second with same name. It is then only necessary to insert the new record

try {
    insert new user
}
catch (exce[tion]) (
    if exception is duplicate key error
         username error
    else 
         re-throw error
)

3 ) Do not store passwords as plain text. Use password_hash() prior to storing and password_verify() when checking.

When you connect using PDO, are you setting the emulate prepares attribute to false?

Edited September 17, 2019 by Barand

[benanamen]

In addition to what @Barand said, your Method should return a Boolean. You have hard coded a redirect and have done nothing in case of failure which could be something other than a duplicate user.

As is, if you want to redirect somewhere other than what you hard coded you have to edit the Class. Classes should be closed for modification. That is known as the "Open-closed Principle" and the the "O" in the SOLID Principal of Object-Oriented Programming.

Do the redirect outside the class in the program flow.

PSEUDO Code

if ( $var->regUser($x,$y,$z) )
{
// Success
}

OR

$status = $var->regUser($x,$y,$z) ? 'Success' : 'Failed';

I would not put the password hashing in the method or class. Hashing a password is not really related to doing a DB insert query which when you get down to it, is really what you are doing. It would also mean you have to duplicate the hashing code such as the case of a password change. Pass the hashed password to the Class.

Edited September 17, 2019 by benanamen

[Fabel]

1 hour ago, Barand said:

Having said that, searching for the username to see if exists before adding a new user is not the way to it. I don't know the structure of your users table but you should add a unique index on the username so it will throw an error if you try to add a second with same name. It is then only necessary to insert the new record

I now have set 'username' to unique. Thanks! Should I make a differrence between capital letters and none capital letters? Someone makes an account with the name 'fabian' and someone else 'FaBiAn', should I give it an error, or just let it be? 

 

58 minutes ago, Barand said:

When you connect using PDO, are you setting the emulate prepares attribute to false?

I don't know what that means. I read a stackoverflow question with the answer but I still don't know how I'm supposed to use this. 

 

If I understand it correctly, you also suggest that my code should look like this:

			try 
			{
				$stmt2 = $this->db->prepare("INSERT INTO users(username,password) 
											 VALUES(:uname, :upass)");
				$stmt2->bindparam(":uname", $uname);
				$stmt2->bindparam(":upass", $new_password);            
				$stmt2->execute();
			}
			catch 
			{
				if(){
				} else {
				} 	
			}

is that true? 

[benanamen]

Mysql is case insensitive by default. Doesnt matter how Fabian is cased.

[Fabel]

13 minutes ago, benanamen said:

if ( $var->regUser($x,$y,$z) )  
{  
 // Success
}

 

The errors I want to return will make the $var->regUser true, without succes. If I would do it this way, how then could I trow the different errors?

 

Btw, the unique username solves the resubmission problem as well. Thanks

[benanamen]

Just how many different errors do you expect? The registration (insert query) is either successful (true) or it fails (false).

duplicate error = false
other error = false
no error = true

You do not want to output system error messages to the user. In the case of a duplicate username, you do not want to specify that the username is already used. That would open you up to a User Enumeration Attack.

Edited September 17, 2019 by benanamen

[Barand]

14 minutes ago, benanamen said:

Mysql is case insensitive by default. Doesnt matter how Fabian is cased.

It depends on the collation setting for the column.

[Fabel]

I really want to thank you for the information! User Enumeration attack, the SOLID principal, et cetera. It's very valuable to me. 

I updated my code and it works smoothly. 

	public function regUser($uname,$upass,$upassverify)
    {
		$new_password = password_hash($upass, PASSWORD_DEFAULT);		
		if(!password_verify($upassverify, $new_password)) {
			// passwords are not the same (I thought it would be better to do this after hashing, but maybe it doesn't matter or it's worse. I'm not sure about it) 
			return false;  
		} 
		
		$stmt1 = $this->db->prepare("SELECT username FROM users WHERE username=:uname");
		$stmt1->execute(array(':uname'=>$uname));
			
		try 
		{
			$stmt2 = $this->db->prepare("INSERT INTO users(username,password) 
										 VALUES(:uname, :upass)");
			$stmt2->bindparam(":uname", $uname);
			$stmt2->bindparam(":upass", $new_password);            
			$stmt2->execute();
			// succesfully made an account
			return true; 
		} 
		catch (PDOException $e)
		{
			return false;
		}
    }

my final question for today: should I hash the passwords before comparing them or after? 

[Barand]

Just compare the two posted strings to make sure they entered it correctly, hash the password and save to the user table.

You don't need password_verify at this stage - you use that when they log in with the username/password to verify at that stage that the entered password matches the stored (hashed) password.

By default, PDO only emulates prepared statements. To use real prepared statements you need to turn off that default setting. My connection code is

    $db = new PDO(  <db credentials> );
    $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);           // tell PDO to throw exceptions on error
    $db->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_ASSOC);       
    $db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);                   // stop emulating prepared statements

 

[Barand]

1 hour ago, benanamen said:

you do not want to specify that the username is already used.

Agreed, you should let the user keep trying to register until eventually, in desperation, they try a different user name. At that point, when it works, they realize that the problem was a duplicate username. But at least, you didn't tell them.

[benanamen]

45 minutes ago, Barand said:

Agreed, you should let the user keep trying to register until eventually, in desperation, they try a different user name. At that point, when it works, they realize that the problem was a duplicate username. But at least, you didn't tell them.

 

Assuming Sarcasm....

So are you saying you are OK with explicitly verifying 50% of a valid system login to an attacker? So instead of just saying "Username Invalid " you want to say "Congratulations, that exact username is in the database. Now you just need to guess the password that goes with it"?

[Psycho]

1 hour ago, Barand said:

2 hours ago, benanamen said:

You do not want to output system error messages to the user. In the case of a duplicate username, you do not want to specify that the username is already used. That would open you up to a User Enumeration Attack.

Agreed, you should let the user keep trying to register until eventually, in desperation, they try a different user name. At that point, when it works, they realize that the problem was a duplicate username. But at least, you didn't tell them.

Just to add some clarity here. @benanamen is correct in that you don't want to create a system that allows a malicious user to easily ascertain usernames from your system - specifically in mass. And, @Barand is correct that it makes no sense on a registration page to NOT tell a user you could not create their account because they chose a user ID that is already in use. The problem to solve is to prevent a malicious user from farming the system to create an inventory of all your users through automation. The malicious users could then iterate through all the users trying different common passwords until they get a match.

If this is important, there are various solutions that can be employed:

1. CAPTCHA or some other means that requires human interaction

2. Slow them down. Introduce a delay of a few seconds or more in the registration process which would make the time to get a full list lengthy even with automation. Easy to implement and would not be noticed by users. (as long as it is not excessive)

3. Keep a log of requests by IP, session, or some other means. If those attempts exceed a threshold you set then either prevent new requests or introduce an even longer delay. More difficult to implement.

There are other ways (such as using analytics) to programatically detect malicious submissions. But, you need to determine the risks to your application and the costs associated with any potential data breach in order to weight how much effort to invest.

EDIT:

10 minutes ago, benanamen said:

So are you saying you are OK with explicitly verifying 50% of a valid system login to an attacker? So instead of just saying "Username Invalid " you want to say "Congratulations, that exact username is in the database. Now you just need to guess the password that goes with it"?

This is a registration page where a user is creating an account - not an authentication page. You should never tell a user the reason you could not authenticate them (i.e. username not found or password wrong). But, that is not what this was about

Edited September 17, 2019 by Psycho

[Psycho]

OK, I need to correct myself I was mixing up some of the techniques. I went back and reviewed a training session about account management on Pluralsight (great training material).

Troy Hunt (the author) recommends the following approach to prevent account enumeration:

Upon submitting a form to register an account provide the user a common message along the lines of "Your registration has been processed, an email has been sent to complete your account". They would get this message in the case of a successful registration or a duplicate username/email.

1. If the registration was successful, the user receives an email to confirm the account

2. If there was a duplicate, send an email to the user that the account was already registered.

Of course, this requires a more complicated process of user registration.

[Barand]

Would the email also contain a token to confirm receipt?

[Psycho]

2 hours ago, Barand said:

Would the email also contain a token to confirm receipt?

Correct, the typical Click Here to confirm your account (in the case of #1).

Here's a page on his site where he announced the training module on Pluralsight. He has a mock conversation between himself and a developer creating an account management system. It is a pretty entertaining read. However, Pluralsight is a paid resource. You can register for free and get a free trial of all their contentl. Not sure if a CC is required.