session help, check if exist

[adammc]

Hi folks,

I was wondering if someone could ensure that I am doing this the right way?

I have a login page with a form asking for username, password and state. The username and pass are validated against my database to ensure they exist, if not they are sent back to the login form page. I then want to check on my inside pages wether the user is logged in or not, if not redirect them back to the login form.

login form

[php]
<form action = "authenticate.php" method = "post">

<table>
<TR><TD><font class="maintext">Username:</font> </TD><TD><INPUT TYPE="text" NAME="username" SIZE=20></TD></TR>

<TR><TD><font class="maintext">Password:</font> </TD><TD><INPUT TYPE="text" NAME="password" SIZE=20></TD></TR>

<tr>
<td valign="center"><font class="maintext">State:</font></td>
<td valign="center">
<SELECT name="state" class="textbox">
<OPTION value="">please select</OPTION>
<OPTION value="ACT">ACT</OPTION>
<OPTION value="New South Wales">New South Wales</OPTION>
<OPTION value="Northern Territory">Northern Territory</OPTION>
<OPTION value="Queensland">Queensland</OPTION>
<OPTION value="South Australia">South Australia</OPTION>
<OPTION value="Tasmania">Tasmania</OPTION>
<OPTION value="Victoria">Victoria</OPTION>
<OPTION value="Western Australia">Western Australia</OPTION>

</SELECT>
</tr>

<tr><td> <input type="Submit" value="Submit"></td></tr>
</table>

</form>
[/php]

authenticate.php

[php]
<?php
session_start();

session_register("userinfo");
if((!$_POST['username']) or (!$_POST['password']))
{ header("Location:$HTTP_REFERER"); exit(); }

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

session_register("state");
session_register("username");
session_register("password");

$conn = @mysql_connect("localhost","user","pass") or die("Err:Conn");
$rs = @mysql_select_db("database",$conn) or die("Err:Db");

$sql = "select * from members where username='$username' and password = '$password' ";
$rs = mysql_query($sql, $conn) or die("Err:Query");

$match = mysql_numrows($rs);

if ($match!= 0) {
$userinfo=mysql_fetch_array($rs);
mysql_close($conn);
header("Location:test-login-success.php");

exit();
}

else {
header("Location:login.html");
exit();
}

?>
[/php]

code to put on my member pages

[php]
<?php

session_start();

if ($_SESSION["username"]=="") {
header('Location: login.html');

if ($_SESSION["password"]=="") {
header('Location: login.html');

if ($_SESSION["state"]=="") {
header('Location: login.html');

}else{

?>

Welcome back <? echo $_SESSION['username'];?>, login was successful!
<br> you are from <? echo $_SESSION['state'];?>

<br><br>sdfdsfdsfsdfsdfsdfsdfs<br><br>

<A HREF="test-login-destroy.php">logout</A>


<?
}

?>
[/php]

Is this code going to do the job, is it secure enough?
Any help would be GREATLY appreciated.

Regards
Adam

[TomAnthony]

Ok, a few points:

- session_register() is deprecated, you should really use $_SESSION

- You need to clean your input, this line you have:

$sql = "select * from members where username='$username' and password = '$password' ";

is dangerous and easily abused. Imagine if I typed "admin" for the user and for password I typed "' OR 'foo'='foo". Your SQL would now read:

select * from members where username='admin' and password = '' OR 'foo'='foo'

Now I have logged in as admin. :(

For more details on cleaning input and protecting against this sort of thing, check out Chris Shiflett's great book 'Essential PHP Security'. This book also covers encrypting the passwords in your database, which is another step you should consider.

Then you do your query, you check to see if you have not 0 results; I'd be tempted to fetch the user then compare the passwords in PHP, but I guess that is a either/or thing.

Finally, your code for your header for members pages isn't secure. It doesn't check that a user has been found, it just checks you actually tried to log in. If I try to log in with user 'admin' but incorrect password, then I still pass this check later (as neither field is empty).

I would suggest you make a functions.inc file that you require() at the top of these pages. In it I would make a function confirm_login() which actually does the check.

This way you can improve or change your check on all pages later on.

As for the actual check, you need to improve that, there is a variety of ways you can do it, but you must check a particular value, not that they aren't just empty.

I hope this gives you a nudge in the right direction. :)

[eelixduppy]

You have many flaws with this code:

1)You register the session variables before you check to see if the username and password are valid. You must set this after you check, and only if their username and password are correct. With your current way, no matter what they type in, they will still be able to see the pages that require login.

2)You are using the username and password variables exactly as they are submitted. This method is prone to many security issues, primarily SQL Injections [webmasterworld.com]. You must first use mysql_real_escape_string() [us2.php.net] to make sure the information you are using in the query won't do any harm.

3)This line: $match = mysql_numrows($rs); has an error. It should be $match = mysql_num_rows($rs);

4)Although most of your function calls aren't going to show errors, it is more secure if you set error_reporting(0); at the top of your validation script, this way NO errors are printed to the screen.

5)I also don't know why you have this line of code: $userinfo=mysql_fetch_array($rs); It doesn't seem to serve any purpose.

6)Also, the code you use to determine if a user has logged in or not should be like this:

<?php

session_start();

if (isset($_SESSION["username"])&& isset($_SESSION["password"]) && isset($_SESSION["state"])) {
?>
Welcome back <? echo $_SESSION['username'];?>, login was successful!
<br> you are from <? echo $_SESSION['state'];?>

<br><br>sdfdsfdsfsdfsdfsdfsdfs<br><br>

<A HREF="test-login-destroy.php">logout</A>
<?
}
}else{
header('Location: login.html');
}

?>

There are also many other resources available in our library [webmasterworld.com]

I would suggest you make a functions.inc file

I would NOT advise you to make the extension of this file .inc considering it will be shown on the browser as regular text and information that you don't want someone to viewcan be seen. I would suggest just giving this a regular .php extension.

[adammc]

Thank you so much for the replies everyone, it has been a GREAT help and lesson :)

OK, I have made a few changes going on your recommendation and came up with this:

[php]

<?php
session_start();

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

$conn = @mysql_connect("localhost","user","pass") or die("Err:Conn");
$rs = @mysql_select_db("database",$conn) or die("Err:Db");

$sql = "select * from members where username='$username' and password = '$password' ";
$rs = mysql_query($sql, $conn) or die("Err:Query");

$match = mysql_num_rows($rs);

if ($match!= 0) {
mysql_close($conn);

if((!$_POST['username']) or (!$_POST['password']))
{ header("Location:$HTTP_REFERER"); exit(); }

$_SESSION['username'] = $_POST['username'];
$_SESSION['password'] = $_POST['password'];
$_SESSION['state'] = $_POST['state'];

header("Location:members2.php");

exit();
}

else {
header("Location:login.html");
exit();
}

?>

[/php]

I will have to read up on how to protect it from sql injections and how to encrypt the password.

Code that appears on members page:
[php]
<?php

session_start();
if(empty($_SESSION) ¦¦ empty($_SESSION['username']) ¦¦ empty($_SESSION['password']) ¦¦ empty($_SESSION['state']))
{
header("Location: login.html");
exit;
}

?>
[/php]

I am trying to find a way to check the session username and pass a 2nd time against the user and pass in the DB.

[eelixduppy]

Just remember to use mysql_real_escape_string or this login page is potentially useless. To implement this into your code, use the following in place of your current declarations:

$username=mysql_real_escape_string($_POST['username']);
$password=mysql_real_escape_string($_POST['password']);
$state=$_POST['state'];//not required to escape this because it's not going into the query, but be careful if you decide to use it later on.

Also, make sure you should use mysql_real_escape_string whenever you use user-defined text in a query. This is crucial to the security of your site!

>>>and how to encrypt the password.

Password encryption with MySQL is easy, unless you don't want to use MySQL's functions to handle this. When you add a user password to the table, add this in place of where you would type the password string: password("their_password") Make sure, however, that the field is large enough to hold the data. Now when you want to check to see if the passwords match during login time, you could change your query to this:
$sql = "select * from members where username='$username' and password = password('$password' )";

Good luck!

[eelixduppy]

Just reading over what i have, i noticed that you are probably going to get a notice--if not an error--when you use mysql_real_escape_string the way i said to add it. It should really be added after mysql_connect() in order to properly function.

[adammc]

Hi again, i didnt receieve any errors or notices.
What I have ended up with is:

[php]
<?php

session_start();

if (!isset($_POST['username']) ¦¦!isset($_POST['password'])) {
header("Location: login2.html?error=" . urlencode("Please fill in all fields"));
exit;

} else {
$conn = @mysql_connect("localhost","aaa","aaa") or die("Err:Conn");
$rs = @mysql_select_db("aaa",$conn) or die("Err:Db");

$sql = "select * from members where username='" . mysql_real_escape_string($username) . "' and password = '" . mysql_real_escape_string($password) . "'";
$sql_result = mysql_query($sql, $conn) or die(mysql_error());
if (mysql_num_rows($sql_result) > 0) {

$_SESSION['loggedIn'] = 1;
$_SESSION['username'] = $_POST['username'];
$_SESSION['state'] = $_POST['state'];

mysql_close($conn);
header("Location: members2.php");
exit();

} else {

header("Location: login2.html");
exit();
}
}

?>
[/php]

members page

[php]
<?php

session_start();
if (!isset($_SESSION['loggedIn'])) {
header("Location: login2.html");
exit;
}

?>

Welcome back <? echo $_SESSION['username'];?>, login was successful!
<br> you are from <? echo $_SESSION['state'];?>

<br><br>sdfdsfdsfsdfsdfsdfsdfs<br><br>

<A HREF="test-login-destroy.php">logout</A>
[/php]

One problem i have founfd is that the code below doesnt seem to do anything:

[php]
header("Location: login2.html?error=" . urlencode("Please fill in all fields"));

[/php]

[eelixduppy]

>> or die(mysql_error());

I would only keep this here for debugging purposes, but then i would change that to not reveal valuable information on an error.