Secure Login?

[lszanto]

I've been working on my own script and I just want to make sure that this login function is secure and can't be hacked by sql injections or other methods, please leave some constructive criticism.

 

Login Function -

<?php

function login($username, $password) {
//Make query.
$sql = "SELECT * FROM users WHERE username='$username' AND password='$password'";

//Turn query into result and arrays.
$result = mysql_fetch_array(mysql_query($sql));

//Turn results into arrays.
$ruser = $result['username'];
$rpass = $result['password'];

//Check if they match.
if($username == $ruser && $password == $rpass) {
	//Session time.
	$_SESSION['username'] = $ruser;
	$_SESSION['rank'] = $result['rank'];

	//Redirect.
	echo "<script> window.location = 'home.php'; </script>";
} else {
	//Show results.
	echo "Your login was incorrect, please try again.";
}
}

?>

 

Call to login function -

		<?php

		//If login.
		if(isset($_POST['username']) && isset($_POST['password'])) {
			//Make password.
			$letters = str_split($_POST['password']);
			$first = $letters[0];
			$first = sha1($first);
			$full = $first . sha1($_POST['password']);

			//Attempt login.
			login($_POST['username'], $full);
		}

		?>

[trq]

Well, you never run any checks or escape any data the user has inputted so yes, it is open to attack. Take a look at mysql_real_escape_string, you must always escape / validate user inputted data.

 

Also, you never check your query succeeds before attempting to use it. This can easily generate errors, and errors can lead to hack attempts.

 

Better would be something like....

 

<?php

function login($username, $password) {
  // escape user input.
  $uname = mysql_real_escape_string($username);
  $upass = mysql_real_escape_string($password);
  $sql = "SELECT * FROM users WHERE username = '$uname' AND `password` = '$upass'";

  if ($result = mysql_query($sql)) { // check the query succeeds.
    if (mysql_num_rows($result)) { // check you have a match.
      $row = mysql_fetch_assoc($result);
      $_SESSION['username'] = $row['ruser'];
      $_SESSION['rank'] = $row['rank'];
      return true;
    }
  }
  // if we make it to here, login failed.
  return false;
}

// by not echoing from within the function you make it more reusable.
if (login('foo','bar')) {
  echo "<script> window.location = 'home.php'; </script>";
} else {
  echo "Your login was incorrect, please try again.";
}
?>

[lszanto]

First thing is if the data is encrypted with sha1() before checked in a query why do I need to escape the string, its not gonna be in the form they entered it but in an encrypted form and if the login works, the login function redirects the user to home.php and if not just leaves the login error but it does the checking inside the function.

[trq]

Knowhere in your calling code do you encrypt the $_POST['username'] variable. Without escaping the data you are wide open to sql injection.

 

And yes, I know how your code works. Display a failure message or redirecting on success. I was merely pointing out that it is allot more portable to simply return a boolean true/false from functions and let the calling code deal with those types of things.