Login script

PHP programming forum. Ask questions or help people concerning PHP code. Don't understand a function? Need help implementing a class? Don't understand a class? Here is where to ask. Remember to do your homework!

Moderator: General Moderators

Mirux
Forum Commoner
Posts: 29
Joined: Sun May 13, 2007 7:11 pm

Login script

Post 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>
Last edited by Mirux on Wed May 16, 2007 9:15 am, edited 1 time in total.
User avatar
volka
DevNet Evangelist
Posts: 8391
Joined: Tue May 07, 2002 9:48 am
Location: Berlin, ger

Re: Login script

Post 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?
Mirux
Forum Commoner
Posts: 29
Joined: Sun May 13, 2007 7:11 pm

Re: Login script

Post 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?
User avatar
guitarlvr
Forum Contributor
Posts: 245
Joined: Wed Mar 21, 2007 10:35 pm

Post 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
Mirux
Forum Commoner
Posts: 29
Joined: Sun May 13, 2007 7:11 pm

Post 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.
User avatar
guitarlvr
Forum Contributor
Posts: 245
Joined: Wed Mar 21, 2007 10:35 pm

Post by guitarlvr »

do you need an action="#" so the script uses itself as the action in your open form tag?

Wayne
Mirux
Forum Commoner
Posts: 29
Joined: Sun May 13, 2007 7:11 pm

Post 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 ?>.
User avatar
volka
DevNet Evangelist
Posts: 8391
Joined: Tue May 07, 2002 9:48 am
Location: Berlin, ger

Re: Login script

Post 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.
Mirux
Forum Commoner
Posts: 29
Joined: Sun May 13, 2007 7:11 pm

Post 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();
User avatar
jayshields
DevNet Resident
Posts: 1912
Joined: Mon Aug 22, 2005 12:11 pm
Location: Leeds/Manchester, England

Post 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.
Mirux
Forum Commoner
Posts: 29
Joined: Sun May 13, 2007 7:11 pm

Post 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.
User avatar
guitarlvr
Forum Contributor
Posts: 245
Joined: Wed Mar 21, 2007 10:35 pm

Post by guitarlvr »

Code: Select all

if ($num != 1){
return false;
}else{
return true;
}
Wayne
Mirux
Forum Commoner
Posts: 29
Joined: Sun May 13, 2007 7:11 pm

Post by Mirux »

Thought so.. Thanks pal.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post 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();
Mirux
Forum Commoner
Posts: 29
Joined: Sun May 13, 2007 7:11 pm

Post 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.
Post Reply