Page 1 of 2

Login script

Posted: Tue May 15, 2007 8:46 pm
by Mirux
I wrote this login script and I get $num = 0 everytime I try to log in. Is this because I'm using md5 encryption for pass when I register? Please let me know of you have any idea what's wrong with my script.
I've tryed copying and paste the md5 encrypted password for each user and it doesn't work either. Any suggestions? Please? I should get a $num = 1 because the username and pass I try to log in with exist in the DB.

Edit: Another question, shall I use session_register(); before the $_SESSION keys?

Thank you very much.

Code: Select all

<?php
include ("dbconnect.php");
function Login($myusername, $mypassword){
	global $connect, $table;
	$query= "SELECT * FROM $table WHERE name='$myusername' AND pw='$mypassword'";
	$result= mysql_query($query) or die (mysql_error());
	$num= mysql_num_rows($result);
	if ($num != 1){
		echo $num;
	}
	else
	{
		$row= mysql_fetch_array($result);
		$id= $row['m_id'];
		session_start();
		$_SESSION['username']= $row['name'];
		$_SESSION['password']= $row['pw'];
		header("location:index.php?user=$id");
}
}
if (isset($_POST['submit'])){
Login($_POST['myusername'], $_POST['mypassword']);
}
?>
<head>
<link rel='stylesheet' type='text/css' href='style.css'>
</head>
<body>
<font>
<table width='245' border='0' align='center' cellpadding='0' cellspacing='1' bgcolor='#CCCCCC'>
<tr>
<form name='form1' method='POST'>
<td>
<table width='100%' border='0' cellpadding='3' cellspacing='1' bgcolor='#FFFFFF'>
<tr>
<td colspan='3'><b>Member Login </b></td>
</tr>
<tr>
<td width='78'>Username</td>
<td width='6'>:</td>
<td width='294'><input name='myusername' type='text' id='myusername'></td>
</tr>
<tr>
<td>Password</td>
<td>:</td>
<td><input name='mypassword' type='password' id='mypassword'></td>
</tr>
<td><input type="submit" name="submit" value="Login"></td>
</tr>
</table>
</td>
</form>
</tr>
</table>
</font>

Re: Login script

Posted: Tue May 15, 2007 9:16 pm
by volka
Mirux wrote:Edit: Another question, shall I use session_register(); before the $_SESSION keys?
No, forget session_register. There's only session_start() and _SESSION. And treat _SESSION just like you would any other array.

Mirux wrote:global $connect, $table;
Exactly what is $connect?
Mirux wrote:$query= "SELECT * FROM $table WHERE name='$myusername' AND pw='$mypassword'";
You need to prepare/filter/encode the parameters $myusername and $mypassword or your query will be prone to sql injections. see http://de2.php.net/security.database.sql-injection
Mirux wrote:return header("location:index.php?user=$id");
header() doesn't return anything. What are you trying to achieve?

Re: Login script

Posted: Wed May 16, 2007 9:19 am
by Mirux
volka wrote:
Mirux wrote:global $connect, $table;
Exactly what is $connect?
$connect is where the connection to the db command are stored. This $connect is in dbconnect.php which I include outside of the function.
Mirux wrote:$query= "SELECT * FROM $table WHERE name='$myusername' AND pw='$mypassword'";
You need to prepare/filter/encode the parameters $myusername and $mypassword or your query will be prone to sql injections. see http://de2.php.net/security.database.sql-injection
[/quote]

Man, thanks for that, I really want to understand how to secure it. If you could explain me more about it, I'd appreciate it.
Mirux wrote:return header("location:index.php?user=$id");
header() doesn't return anything. What are you trying to achieve?[/quote]

Forget about the return, that was a code bug.

Why $num = 0 still?

Posted: Wed May 16, 2007 9:27 am
by guitarlvr
I'm not sure if this would stop the query from running but there is a dollar sign in front of your table name

Code: Select all

$query= "SELECT * FROM $table WHERE name='$myusername' AND pw='$mypassword'";
If you take that out the query should run and should return $num = 1.

Wayne

Posted: Wed May 16, 2007 9:40 am
by Mirux
Not really fellah, the $ sign is okay there because if you check "global $connect, $table", I take the $table variable from outside of the function. It is included in 'dbconnect.php', that's why the dollar sign is there.

Posted: Wed May 16, 2007 9:55 am
by guitarlvr
do you need an action="#" so the script uses itself as the action in your open form tag?

Wayne

Posted: Wed May 16, 2007 10:00 am
by Mirux
Nope, because if I got what you mean, the action is in the script. :S I mean, this is a login script, the html code is in it too, and the action i between <? and ?>.

Re: Login script

Posted: Wed May 16, 2007 10:03 am
by volka
Mirux wrote:Is this because I'm using md5 encryption for pass when I register?
meaning: Not the password but only the md5 hash is stored in the database? There's nothing in your login script reflecting that.

Posted: Wed May 16, 2007 11:11 am
by Mirux
Okay, no worries. I fixed it now. It was set to varchar(30) wich is not enough for the md5 encrypted pw's.

Now I'll post my code again because I added some stuff to secure the query. Please let me know if I need something else, thanks.

Code: Select all

<?php
include ("dbconnect.php");
function Login($myusername, $mypassword){
	global $connect, $table;
	$md5pw= md5($mypassword);
	mysql_escape_string($md5pw);
	$query= "SELECT * FROM $table WHERE name='".mysql_escape_string($myusername)."' AND pw='$md5pw'";
	$result= mysql_query($query) or die (mysql_error());
	$num= mysql_num_rows($result);
	if ($num != 1){
	echo "You've entered a wrong username or password.";
	}
	else
	{
		$row= mysql_fetch_array($result);
		session_start();
		$_SESSION['username']= $row['name'];
		$_SESSION['pw']= $row['pw'];
		header("location:index.php");
}
}
if (isset($_POST['submit'])){
Login($_POST['myusername'], $_POST['mypassword']);
}
?>
<head>
<link rel='stylesheet' type='text/css' href='style.css'>
</head>
<body>
<font>
<table width='245' border='0' align='center' cellpadding='0' cellspacing='1' bgcolor='#CCCCCC'>
<tr>
<form name='form1' method='POST'>
<td>
<table width='100%' border='0' cellpadding='3' cellspacing='1' bgcolor='#FFFFFF'>
<tr>
<td colspan='3'><b>Member Login </b></td>
</tr>
<tr>
<td width='78'>Username</td>
<td width='6'>:</td>
<td width='294'><input name='myusername' type='text' id='myusername'></td>
</tr>
<tr>
<td>Password</td>
<td>:</td>
<td><input name='mypassword' type='password' id='mypassword'></td>
</tr>
<td><input type="submit" name="submit" value="Login"></td>
</tr>
</table>
</td>
</form>
</tr>
</table>
</font>
check the query, I added some mysql_escape_string();

Posted: Wed May 16, 2007 11:52 am
by jayshields
After a brief scan over your code I can see 2 places for improvement.

Functions should always return something, so I would change your function so that it does; maybe remove the header call and return true instead (also remove your error message in the function and change that for reutrn false) and then run your function in an if block, if it returns true, trigger the header call, if it's false, print the error.

As others said, but you misinterpreted, your form tag is missing an action attribute. Add action="#" to your form tag.

Posted: Wed May 16, 2007 1:30 pm
by Mirux
jayshields wrote:Functions should always return something, so I would change your function so that it does; maybe remove the header call and return true instead (also remove your error message in the function and change that for reutrn false) and then run your function in an if block, if it returns true, trigger the header call, if it's false, print the error.
BOLD: I don't get what you mean there, like I want to redirect the client to the main site after log in, why return true? And btw, how do I return true there?

Code: Select all

return true;
This way?

ITALIC: You mean

Code: Select all

or die(mysql_error());
? Yeah, I can delete that but how change it to return false?

UNDERLINE: if block? What? if it returns true, I trigger the header call, yes I understand that. but I'd appreciate if you write an example how to return true or false?

I'm sorry but I'm newp. ^_^ And I'd love to learn more and more.

Posted: Wed May 16, 2007 1:33 pm
by guitarlvr

Code: Select all

if ($num != 1){
return false;
}else{
return true;
}
Wayne

Posted: Wed May 16, 2007 1:51 pm
by Mirux
Thought so.. Thanks pal.

Posted: Wed May 16, 2007 2:16 pm
by Mordred
I can see some problems with your code, here is a couple of minor ones:

Code: Select all

$md5pw= md5($mypassword); 
mysql_escape_string($md5pw); //<--this does nothing, but its okay, since the md5-ed password is SQL-safe
$query= "SELECT * FROM $table WHERE name='".mysql_escape_string($myusername)."' AND pw='$md5pw'";
Better:

Code: Select all

$username = mysql_real_escape_string($myusername); //mysql_real_escape_string is better than mysql_escape_string
$password = mysql_real_escape_string($mypassword);
$query= "SELECT * FROM `$table` WHERE `name`='$username' AND pw=MD5('$password')"; //using SQL's md5
Even better, select some ID for this user and keep it in the session as well, it is awkward to keep the username and password in the session just to be able to identify the user (although not a problem per se).

Make sure that the registration process will catch if someone tries to register an existing username.

The bigger problem is session fixation:

Code: Select all

session_start();
to become:

Code: Select all

session_start();
session_regenerate_id();

Posted: Wed May 16, 2007 6:46 pm
by Mirux
Okay, that looks better, thanks for the suggestion. And no worries, I've already covered that case. No one can register an existent username. Neither an e-mail address. ;)

Ohh, I didn't get the last two functions, what's the problem with session_start(); ?

Edit: You had some syntaxis error there:

Code: Select all

$query= "SELECT * FROM `$table` WHERE `name`='$username' AND pw=MD5('$password')"; //using SQL's md5
The correct way is:

Code: Select all

$query= "SELECT * FROM $table WHERE name='$username' AND pw='".MD5($password)."'";
greetings.