Check login script letting in guests?

Question

For some reason my check login script is letting in guests.

I have not made the site live yet so its all good.

I check the database for the username and the password the user puts in the html form but for some reason if it don't even get a result it still sets the username to nil if it gets the result it sets the username to the username but if it don't get any results it sets the username to nothing.

I have a if statement but still setting it.

$myusername = mysql_real_escape_string($_POST['myusername']);
$mypassword = mysql_real_escape_string($_POST['mypassword']);


$sql        = "SELECT * FROM users WHERE username='$myusername'";
$result     = mysql_query($sql) or die(mysql_error());
$battle_get = mysql_fetch_array($result);    

if ($battle_get['password'] == $mypassword)
{    
    $_SESSION['username'] = $myusername ; // store session data    
    header('Location: http://mydomainname.net/new_rpg/dashboard.php');      
} else {
    echo "wrong password" ;
}

**1.)** Why don't you save the password as a hash? **2.)** Then you won't have to escape the password. **3.)** Why don't you check the password MySQL-side? **4.)** Have you cleared your cache for testing? — ComFreek, Oct 12 '11 at 15:22
Yes ive cleared the cache and was in a rush and i hate md5 making variables md5 is hard i find . And i don't mind escaping. I find it part of making the script safe — Nickk Davies, Oct 12 '11 at 15:24
You don't need to escape the password anymore after you hashed it. It will be hexadecimal string. Furthermore there are other (safer) hash algorithms available (see [`hash()`](http://php.net/manual/function.hash.php)) — ComFreek, Oct 12 '11 at 15:29
@comfreek: escaping everything is a good habit to get into, even if you know some piece of data (like an md5 hash) couldn't possibly have a quote in it. Better to waste a few microseconds of cpu/memory on a pointless escape than forgot to actually do it somewhere else where it IS required. — Marc B, Oct 12 '11 at 15:31

score 2 · Answer 1 · answered Oct 12 '11 at 15:28

2

You don't check if the user account actually exists. You just blindly fetch a row from the result set, even if that result set has NO records in it. That means $battle_get will be an empty array (or a boolean false if the query failed). You then do a string comparison against the submitted password. If that password is also empty, you're doing if (empty == empty) and boom... the user's in.

What you SHOULD be doing is:

$username = mysql_real_escape_string($_POST['username']);
$password = mysql_real_escape_string($_POST['password']);

$sql = "SELECT ... FROM users WHERE (username = '$username') AND (password = '$password')";
$result = mysql_query($sql) or die(mysql_error());

if (mysql_num_rows($sql) != 1) {
    die("Invalid username and/or password"); // don't tell the user which failed.
}

Checking how many rows were returned is critical - if no rows are returned, then the user doesn't exist or the password is wrong. If 1 row is returned, then it's a valid login. If more than 1 row is returned, you've got duplicate username/password pairs in the database and need to fix that right away.

And, having just seen your "md5 is hard" comment above: You're dead wrong. MD5 is trivially EASY.

When you create the user record, you can hash the password easily:

INSERT INTO users (password) VALUES (MD5('$password'));

and for the login check:

SELECT ... WHERE (password = MD5('$password'));

Nothing to it at all.

answered Oct 12 '11 at 15:28

Marc B

356,200
43
426
500

mysql_real_escape_string is handy, but parametrized queries are the way to go. Check out this post http://spotthevuln.com/2011/08/boundaries-sql-injection/ Also Sha1 is a more secure hash than md5, and its just as easy to use :) sha1($Password) – CountMurphy Oct 12 '11 at 15:42
parametrized queries are all fine and dandy for where they apply, but there's plenty of queries where they simply cannot work. – Marc B Oct 12 '11 at 15:44
How so? I have never heard of any issues, and I would definitely use it for a log in page – CountMurphy Oct 12 '11 at 15:47
Im getting Invalid username and/or password even tho the detials are correct which i enter – Nickk Davies Oct 12 '11 at 15:53
From what I'm reading, (and I could be wrong) I don't think where in(?) is a problem. According to http://php.net/manual/en/pdo.prepare.php, You must "pick one or the other parameter style" (? or :parameter). – CountMurphy Oct 12 '11 at 16:03
I may have misread your comment Marc. If this is what you where talking about (http://stackoverflow.com/questions/337704/parameterizing-a-sql-in-clause), there are work-a-rounds – CountMurphy Oct 12 '11 at 16:14

ahmetunal · Accepted Answer · 2011-10-12T18:30:29.407

0

Yur mistake:

Say I am not a user.

So $battle_get['password'] = false;

and $mypassword is also false,

so $battle_get['password'] equals $mypassword

Two way you can resolve this.

First, chek the password with sql:

$sql = "SELECT * FROM users WHERE username='$myusername' AND password = '$mypassword'";

or

if(!$battle_get) {

echo "wrong password" ;

}

edited Oct 12 '11 at 18:30

answered Oct 12 '11 at 15:29

ahmetunal

3,930
1
23
26

Check login script letting in guests?

2 Answers2