Problem with MySQL Database linked PHP login form

RossBryan

Hey people, Im new to PHP and MySQL and have bumped into a little problem trying to get my username and password verified. Most of the script is functional for example not granting access if the both/one of the login fields are empty.

But, it actually seems to allow the user to login if they have entered text into both the username and password fields regardless of whether these (username/password) are stored in the corresponding login database.

Here is the main script:

<html> 
<body> 

<form action="index3.php?login=yes" method="POST"> 
Username:<input type="text" name="user"><br /> 
Password:<input type="password" name="pass"><br/> 
<input type="submit" name="login" value="login" ><p> 
</form> 

<?php 

$user=$_POST['user']; 
$pass=$_POST['pass']; 
$login=$_POST['login']; 

function denied() 
    { 
    echo '<h3><span style= "color:red"> Access Denied!!! </span></h3><br><br>'; 
    } 

function granted ($user) //function with username parameter retrieved 
    { 
    echo '<h3><span style= "color:green"> Access Granted!!! </span></h3>'; 
    echo 'Welcome, ' . $user; 
    } 

if($login=='login') 
{ 
$con= include_once "mysql_connect.php"; 

$query = "SELECT id FROM login WHERE user='$user' AND pass='$pass' "; 
echo $query; 
$result = mysql_query($query) or die ("ERROR IN SQL STATEMENT: ".mysql_error()); 
$row = mysql_fetch_assoc($result); 


    if (empty($user) || empty($pass)) 
    { 
    denied(); 
    die("<br>Please fill out user login fields carefully....<br>"); 
    } 

    if ($result!=1) 
    { 
    granted($user); 
    } 

    else 
    { 
    denied (); 
    } 
} 

?> 

</body> 
</html>

And the following is external scripting associated with the the above:

<?php 

$db_host = "localhost"; 
$db_username = "root"; 
$db_pass = "rhianna"; 
$db_name = "login"; 

@mysql_connect("$db_host", "$db_username", "$db_pass") or die ("Could not connect to MySQL"); 
@mysql_select_db("$db_name") or die ("No $db_name Database "); 

?>

Any ideas of what the problem is and how i could possibly resolve this issue?

For some reason i believe it could be something to do with the password not being verified correctly once retrieved from the database, maybe in the $return variable but im not entirely sure, just a guess. Even if it is that i wouldn't know another way of going about fixing it.

leatherback

Adjusted the logic for you, and modified a little. Try this:


if($login=='login')
  {
  if(empty($user) || empty($pass))
    {
    denied();
    die("<br>Please fill out user login fields carefully....<br>");
    }
  else
    {
    $con= include_once "mysql_connect.php";

$query = "SELECT id FROM login WHERE user='$user' AND pass='$pass' limit 1";
echo $query;
$result = mysql_query($query) or die ("ERROR IN SQL STATEMENT: ".mysql_error());
$row = mysql_fetch_assoc($result);

if(mysql_num_rows($result) == 1)
  {
  granted($user);
  }        
else
  {
  denied ();
  }
}
  }

RossBryan

Thanks did just that and its working fine now.

Thanks for taking the time to look and my post and advise me, appreciate it loads.

johanafm

There are some other things you definitely need to correct as well.

Since you don't sanitize your input, you are vulnerable to SQL injection. Also, since you don't check if the various array elements exist in $POST, you will get errors if this page is accessed without a form post, or with a malformed post request forgery.

RossBryan;10986472 wrote:

$user=$_POST['user']; 
$pass=$_POST['pass']; 
$login=$_POST['login']; 
$query = "SELECT id FROM login WHERE user='$user' AND pass='$pass' ";

You need to connect to your database before you can use mysql_real_escape_string, which is the function used to escape strings. Integers and floats can just be cast to their respective types with (int) and (float).
Moreover, you should not store passwords in plain text in the db or otherwise. At the veyr least, store the password as an md5 hash, and later on retrieve it by also creating the md5 hash of the given password. It would be even better if you also supplied a salt on a per user basis. A simple salt would just be time() from when the user account was created, mt_rand() or simialar. You might also want to include a few non alpha-num characters in the salt, such as $, # etc.

$con = ...;

$user = isset($_POST['user']) ? mysql_real_escape_string($_POST['user'], $con) : '';
$pass = isset($_POST['pass']) ? mysql_real_escape_string($_POST['pass'], $con) : '';
$query = "SELECT user, pass, salt, other_fields FROM login WHERE user='$user' AND
pass=MD5(CONCAT(salt, '$pass'))";

I'd just like to point out why this always let the user login. The reason is that $result will NEVER be 1. It will be either false or a mysql result set resource, as documented by [man]msyql_query[/man].

RossBryan;10986472 wrote:
if ($result!=1) 
{ 
	granted($user); 
} 

While this isn't incorrect in any way, it's not at all user friendly. If a user fails to fill out either username or password, it's a lot better to show them the same form again, and in my opinion their username should also be pre set with the same value as they entered the first time, rather than giving them a useless page without even a link back to the login form.

leatherback;10986492 wrote:

if($login=='login')
{
	if(empty($user) || empty($pass))
	{
		denied();
		# Don't die. Show the same form again, and fill in the previously given username
		die("<br>Please fill out user login fields carefully....<br>");
	}

This is not at all good for security reasons. You should never give out information to potential hackers. If someone sends a forged post request that doesn't work due to a SQL error, you will actually provide them with feedback about the error along with some information about table and field names.
Also, any normal user will have no help at all by being presented with the information from mysql_error().
And finally, you should never display errors in a production environment, only log them. You can however opt to display errors in a development environment, and since you can setup php.ini to do either or both, you should use trigger_error if you want this handled automatically for you. Or, if you don't want to ever display errors, use error_log() to log errors. What you should do is present the user with feedback helpful to the user.

leatherback;10986492 wrote:

else
{
	$result = mysql_query($query) or die ("ERROR IN SQL STATEMENT: " . mysql_error());

This part should also be corrected. First off, you always have to check if you get a result set resource (success) or boolean false (failure) before using the returned value from mysql_query. This of course goes for any other function returning similar set of values.
And finally, since it is critical that logging in is handled properly, you should make sure not just that you get a result with one row back, but that it actually makes sense as well. This would not matter if you sanitized user input, but in the case of logging in, I see no reason not to err on the safe side.

leatherback;10986492 wrote:

	$row = mysql_fetch_assoc($result);
	if(mysql_num_rows($result) == 1)
	{
		granted($user);
	}				
}

# top of page
$errors = array();
$feedback = array();

# ...

if (isset($_POST['login'])
{
	# ...
}
else
{
	# Here you first check if your query was successful or not
	if (!($result = mysql_query($query))
	{
		# either...
		trigger_error(mysql_error() . ' ('. mysql_errno.() .'): ' . $query);

	# or
	error_log(mysql_error() . ' ('. mysql_errno.() .'): ' . $query);

	# But always do provide the user with information useful to them.
	$errors[] = 'Unable to login. Please try again later. Contact <a href="mailto:support@example.com">support@example.com"</a> if the problem persists.';
}
# And then you check if the given data matches a user
elseif (!($row = mysql_fetch_assoc($result) ||
	$row['user'] != $user || $row['pass'] != md5($row['salt'].$pass))
{
	# bad user credentials
	$feedback[] = 'Bad username and/or password. Please try again';
}
else
{
	# login successful
}
}

echo '<html>....<body>';

# CSS classes and appropriate CSS styling to display feedback and error messages
# differently, e.g. red color for errors.
foreach ($errors as $e)
{
	printf('<div class="error">%s</div>', $e);
}
foreach ($feedback as $f)
{
	printf('<div class="feedback">%s</div>', $f);
}
echo '<form ...>
	<div>';
# Provide the username prefilled with the same value as given in a failed login attempt
printf('			<input type="text" name="user" value="%s" />',
	isset($_POST['user']) ? htmlentities($_POST['user'], ENT_QUOTES, 'utf-8') :
	''
# But don't do the same for password
echo '			<input type="text" name="pass" value="" />';
);