row count function doesn't behave properly

MrSkeed

im trying to build a login system with php and sql. but it doesnt work, this is my code:

<?php
SESSION_START();
if($_SESSION['login'] == 1) {
	echo "<meta http-equiv='refresh' content='2;url=http://webdesign.about.com/'>";
} else {
	$host = "sql208.byethost13.com";
	$username = "*******";
	$password = "*******";
	$db_name = "*******_user";
	$tbl_name = "user";

mysql_connect("$host", "$username", "$password") or die("cannot connect");
mysql_select_db("$db_name") or die("cannot select database");

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

$input = "SELECT * FROM $tbl_name WHERE user='$user' AND password='$pass'";
$result = mysql_query($input) or die(mysql_error());
print("$result");
//good till here
	$rows = mysql_num_rows($result) or die(mysql_error());
    print("$rows");
	if($rows == "1") {
		$_SESSION['user'] = $user;
		$_SESSION['pass'] = $pass;
		$_SESSION['login'] = 1;
        print("HURRDURR");
	} else {
		print("Wrong username or Password");
	}
}
?>

the mistake is located by the first $rows variable, but i cant figure out what is wrong. please help.

MrSkeed

the code is a little messy, but i fixed that in my own file

NogDog

$rows = mysql_num_rows($result) or die(mysql_error());

Note that this means any time the query does not find a match, your program will die() at this point, since a return value of zero will be cast to false by the "or". So the query may be fine but something else may be causing it to always return 0 rows (for which you probably don't want to die()).

MrSkeed

NogDog;11025151 wrote:
$rows = mysql_num_rows($result) or die(mysql_error());
Note that this means any time the query does not find a match, your program will die() at this point, since a return value of zero will be cast to false by the "or". So the query may be fine but something else may be causing it to always return 0 rows (for which you probably don't want to die()).

Thanks! i got rid of the or die() function and now it is working good!

laserlight

By the way, your authentication process looks flawed: typically, the password as-is is not stored in the database. Rather, some crytographic hash function (or a more complex function derived from one) is used to hash the password with some salt value. Both the hashed password and this salt value is then stored in the database. To authenticate the user, you would select the stored hashed password and salt value given the username, then hash the password supplied with the salt and see if the result matches the hashed password stored.

Bonesnap

+1 for use of HURRDURR 🙂

MrSkeed

Bonesnap;11025251 wrote:
+1 for use of HURRDURR 🙂

hehehe 😃

MrSkeed

laserlight;11025237 wrote:
By the way, your authentication process looks flawed: typically, the password as-is is not stored in the database. Rather, some crytographic hash function (or a more complex function derived from one) is used to hash the password with some salt value. Both the hashed password and this salt value is then stored in the database. To authenticate the user, you would select the stored hashed password and salt value given the username, then hash the password supplied with the salt and see if the result matches the hashed password stored.

do you mean i have to do something like this:

 $pass = $_post['pass']; $realpass = md5($pass);

and then use realpass in the query and validate with the encrypted pass in the database?

laserlight

That would be slightly better, but not enough. Notice what I wrote concerning a salt value.

johanafm

Well, more or less, except for not using a salt. For example md5($salt.$password). The salt should be individual for each user.

The reason is that if someone gets access to the db and the stored passwords, without a salt they could use a precomputed hash table called a rainbow table, containing hasshes for common passwords, dictionary words, letter combinations. This would, without a salt, give an immediate reverse lookup for bad passwords, which is what users tend to use.

If you store each individual salt in the db along with the password (i.e: user(id, name, hashed password, salt)) and someone breaks into your database and get this information, at least they will now have to generate their rainbow table using your salt and password. Besides, they'd have to guess is you use md5(salt + password)), md5(password + salt), md5(md5(password) + salt)... Where guess would mean try them one after the other => takes a lot longer time.

Should you also add a pepper, which is used as a salt but is the same for all users and stored in your code instead, the attacker would need access to both db and code, since you'd now use something like md5($salt . $pasword . $pepper).

But the use of a pepper should only matter if someone can gain access to the database information through sql injection, which they shouldn't if you use prepared statements. Since you don't, this is the very first thing you should look into, which means stop using the deprecated and soon to be dead mysql extension and start using mysqli which was meant to replace mysql years ago. Or use PDO.

MrSkeed

i don't really know what a "salt" value or something is..

MrSkeed

oh nvm, it is explained above :p, still, this is just a site to test my skills in php/html/css/sql and stuff. it isn't supposed to grow out to a big site or something. so, thanks for your idea for putting a "salt" value into it but it isn't necessary yet xD

bradgrafelman

MrSkeed;11025273 wrote:
this is just a site to test my skills in php/html/css/sql and stuff.

What good is testing your skills if you're just going to oversimplify things and/or undermine security practices that you should otherwise be mindful of? 😉

EDIT: Speaking of PHP/MySQL skills: Stop Using the MySQL Extension!

MrSkeed

ok, you do have a point there. i will try to use it in my code. but, the mysql extension i have to use for school. i also have another question: i want to make a registration form, but something in my code gives an error so (again) the row count function misbehaves. this is my registration.php code:

<?php
SESSION_START();

$username = $_POST['username'];
$password = $_POST['password'];
$email = $_POST['email'];

$host = "localhost";
$username = "******_db";
$password = "*****";
$db_name = "******_db";
$tbl_name = "user";

mysql_connect("$host", "$username", "$password") or die("cannot connect");
mysql_select_db("$db_name") or die("cannot select database");

$input = "SELECT * FROM $tbl_name WHERE user = '$username'";
$result = mysql_query($input);


$rows = mysql_num_rows($result);
print("$rows");

if($rows == 0)
{
print("This username is available!");
}
else
{
print("This username is already chosen, thus unavailable. please choose another");
}
?>

the result of $rows keeps staying 0 even when i insert an already inserted username. i really have no idea how this comes.

bradgrafelman

MrSkeed;11025281 wrote:
the mysql extension i have to use for school.

Yikes. You should consider passing that link along to your instructor, then, and asking him/her to comment as to why the extension is still being taught. That is, of course, unless he/she is also teaching that the world is flat and is at the center of the universe... in which case you might just have to bite your tongue and take their teachings with a grain of salt. 😉

As for your new issue, here are some comments I have:

You're overwriting the variable $username when defining your SQL login credentials.
User-supplied data should never be placed directly into a SQL query string, else your code will be vulnerable to SQL injection attacks and/or just plain SQL errors. See [man]security.database.sql-injection[/man] for more info.
You should always be checking to see if your query was executed successfully and, if not, logging/outputting relevant information to aid in debugging (such as the error message returned by MySQL, the query as it was sent to the SQL server, etc.).

EDIT: Some additions to the above list:

You shouldn't be able to "insert an already inserted username" regardless of whether you even execute the above code, because you should be properly defining all relevant constraints in the database. In this case, it sounds like you should have defined a UNIQUE or PRIMARY KEY index on the user column.
Assuming you correct the above issue, note that there's probably no reason to even do a separate SELECT query before the INSERT query. Instead, simply execute the INSERT query and check to see if it fails due to error 1066 (duplicate data for key).

MrSkeed

You're overwriting the variable $username when
defining your SQL login credentials.

now i see, how stupid of me not seeing this. thanks! and indeed i didn't assign a primary key in my database yet, i am going to do that, but i also just wanted to check if this way worked 😉 and for the checking issue, the way i do it is insert print tags to see where the code ends (as might be seen in my very first post, not sure though). 🙂