MySQL Login -> Password always correct even if wrong?

[ChrisX930 0]

Hey Guys,

 

I'm working on a Login Script for my Unity Game.

Until now everything works fine, I can create new Accounts and stuff.

But now I've another problem.

Everytime I use a wrong password, the game thinks it's the correct password.

That shouldn't happen.

I guess the error is inside my PHP-Script:

//PHP Only
$Hostname = "";
$DBName = "";
$UserP = "";
$PasswordP = "";

//Email and Password
$Username = $_REQUEST["Username"];
$Password = $_REQUEST["Password"];

$conn = new mysqli($Hostname, $UserP, $PasswordP, $DBName);
	
	if($conn->connect_error) {
		die("Connection failed: " . $conn->connect_error);
	}

	if(!$Username || !$Password) {
		echo "Login or password can't be empty.";
	} else {
		$SQL = "SELECT * FROM accounts WHERE Username = '" . $Username . "'";
		$result_id = mysqli_query($conn, $SQL) or die ("DATABASE ERROR!");
		$total = mysqli_num_rows($result_id);
		if($total){
			$datas = mysqli_fetch_array($result_id);
			if(strcmp($Password, $datas["Password"])){
				$sql2 = "SELECT Characters FROM accounts WHERE Username = '". $Username . "'";
				$result_id2 = mysqli_query($conn, $sql2) or die ("DATABASE ERROR!");
				while($row = mysqli_fetch_array($result_id2)){
					echo $row['Characters'];
					echo ":";
					echo "Success";
				}
			} else {
			    echo "WrongPassword";
			}
		}else {
		    echo "NameDoesNotExist";
		}
	}

does anyone know how I can fix this?

[mac_gyver 471]

you need to read the documentation for any php function you are trying to use. strcmp returns a zero if the strings are the same.

 

don't even use strcmp for your passwords. you should be using php's password_hash() and password_verify() to handle your password hashing and comparison. there are examples in the php.net documentation showing how to use these.

 

next, Don't Repeat Yourself - DRY. you are already running one sql query to find and then fetch the row matching the username. don't run that query again to get a single piece of data. you already have all the data from the row, from the first time you executed the query.

 

don't loop to fetch data from a query that will at most match a single row. the while() loop code will go away anyway since you don't need to run the query the second time.

 

you need to use a prepared query to supply any data to an sql query statement to prevent sql injection.

 

you should not use or die() logic to handle sql query errors. use exceptions to handle sql errors and you should not output the raw php/mysql errors to the browser on a live site.

 

you should validate each input separately and output a unique message for each validation error.

 

you should NOT identify if the username or the password is the reason for not being able to log in. this will allow someone/bot script to repeatedly submit values to your code and first find valid usernames, then bruit force find a password. output the same generic - invalid username/password for each of these cases. you can log internally the actual reason for a login failure.

 

don't use $_REQUEST variables. if you expect data to be in $_POST variables, use $_POST. you should also detect that a post method form was submitted at all, before trying to use any $_POST variables. this will prevent php errors.

 

if the current visitor is already logged in, you should not run your login form/form processing code.

 

lastly, when the visitor is correctly authenticated, you should store the user's id in a session variable to remember that they are logged in and who they are. all other code that tests if the visitor is logged in would test if the session variable holding the user's id is set and if they need to retrieve any current data about the visitor, they would use the user id to do so. this will insure that any user data is current and up to date.

[Stielle 0]

If it is always returning 'Success', then check the return value of the comparison. That function is always returning something (-1, 0, 1, etc). In your if statement, you are checking to see if anything was returned. This will always pass. If you stayed with that function, you would need to check to see if it is 0:   if(strcmp($Password, $datas["Password"])==0)

But heed mac_guyver's advice as it was all very good.