One Page Login System

[evil_stevo]

This is my one page log in system. Using this on the header so guests can log in on ANY page. Let me know what you think needs improving for security. I'm also wondering if putting the include "disconnect.php"; where I have is correct. Thanks!

 

<?php
session_start();

$message = ""; //error message needs to be blank
$loginstatus = ""; //error message needs to be blank

//if $_POST "username" and "password" exist, check for consistency.
if (isset($_POST['username'])&&($_POST['password']))
{
include 'connect.php'; //connect
$username = mysql_real_escape_string($_POST['username']); //set variables from session
$password = mysql_real_escape_string($_POST['password']); //set variables from session

//remove slashes and HTML
$username = stripslashes($username);
$password = stripslashes($password);
$username = strip_tags($username);
$password = strip_tags($password);

$password = md5($password); //md5 encryption

$query = mysql_query("SELECT * FROM users WHERE username='$username' AND password='$password'"); //checking if row exists that has $username and $password together.
$num = mysql_num_rows($query); //number of rows. if not equal to one login will fail.

if($num==1)
{
$_SESSION['username'] = $username; //store session data
$message = "$username, you are logged in!";
include "disconnect.php";
}
else
{
$message = "<font color='red'>Wrong Username or Password. Please try again.</font>";
}
}

//if $_SESSION "username" and "password" exist, check for consistency.
if (isset($_SESSION['username']))
{
$username = $_SESSION['username'];
$loginstatus = "
<table cellspacing='0' cellpadding='0'>
<tr>
<td align='right'><b>$message</b> <a href='logout.php'>[logout]</a></td>
</tr>
</table>
";
}
else
{
$loginstatus = "
<b>$message</b>
<table cellspacing='0' cellpadding='0'>
<form action='index.php' method='post'>
<tr>
<td><b>Username: </td>
<td><input type='text' name='username' class='inputbox'></td>
<td>   <b>Password: </td>
<td><input type='password' name='password' class='inputbox'></td>
<td>   <input type='submit' value='Log In' class='submitbutton'></td>
</tr>
</table>
</form>
";
}

echo $loginstatus;
?>

[Rifts]

I always do this on inputs

 

$myusername = stripslashes($myusername);

$mypassword = stripslashes($mypassword);

$myusername = mysql_real_escape_string($myusername);

$mypassword = mysql_real_escape_string($mypassword);

[evil_stevo]

Alright, thanks! That's already in there though. So, what else should be done? Does anyone know of anything else that can be done to ensure a more secure system without making things really complicated (unnecessarily complicated). Thanks.

[Pikachu2000]

You should not use stripslashes() unconditionally. You should only use stripslashes() if magic_quotes_gpc() is on. Moreover, using it after mysql_real_escape_string() actually undoes what mysql_real_escape_string() does, rendering it useless.

 

There is no need to do any sanitizing of a value that will be run through a hashing algorithm before being inserted in the database, such as the password.

 

strip_tags() is unnecessary for inserting data into a database; it should be used immediately prior to displaying the data.

 

So with that in mind, here's what that part should look like:

session_start();

$message = ""; //error message needs to be blank
$loginstatus = ""; //error message needs to be blank

//if $_POST "username" and "password" exist, check for consistency.
if( isset($_POST['username'])&&($_POST['password']) ) {
require_once 'connect.php'; //connect

if( get_magic_quotes_gpc() ) {
	$username = mysql_real_escape_string(stripslashes($_POST['username']));
} else {
	$username = mysql_real_escape_string($_POST['username']); //set variables from session
}

$password = md5($password); //md5 encryption

$query = mysql_query("SELECT * FROM users WHERE username='$username' AND password='$password'"); //checking if row exists that has $username and $password together.
$num = mysql_num_rows($query); //number of rows. if not equal to one login will fail.

if( $num == 1 ) {
	$_SESSION['username'] = $username; //store session data
	$message = "$username, you are logged in!";
	include "disconnect.php";
} else {
	$message = "<font color='red'>Wrong Username or Password. Please try again.</font>";
}
}

[evil_stevo]

Very nice. Googling everything you said makes sense and is smart. Ok, so should I 'require', 'require_once' or 'include' the disconnect.php? That's my next good question. Thanks! 

[Pikachu2000]

I'd say require_once() it. Since the script is essentially useless without the database query, it needs to be in the script, but using _once will allow it to be ignored if some other script has already successfully included it.

[evil_stevo]

Oh, does password need anything done to it other than md5?

 

Maybe something like this??

<?php
session_start(); //start session

//required
$message = "";
$loginstatus = "";

//if $_POST "username" and "password" exist, check for consistency.
if (isset($_POST['username'])&&($_POST['password']))
{
require_once 'connect.php'; //connect

if(get_magic_quotes_gpc())
{
$username = mysql_real_escape_string(stripslashes($_POST['username'])); //set variables from session
$password = mysql_real_escape_string(stripslashes($_POST['password'])); //set variables from session
}
else
{
$username = mysql_real_escape_string($_POST['username']); //set variables from session
$password = mysql_real_escape_string(($_POST['password'])); //set variables from session
}

$password = md5($password); //md5 encryption

$query = mysql_query("SELECT * FROM users WHERE username='$username' AND password='$password'"); //checking if row exists that has $username and $password together.
$num = mysql_num_rows($query); //number of rows. if not equal to one login will fail.

if($num==1)
{
$_SESSION['username'] = $username; //store session data
$message = "$username, you are logged in!";
include "disconnect.php";
}
else
{
$message = "<font color='red'>Wrong Username or Password. Please try again.</font>";
}
}

 

Also, let's say above is my header.php and below is my index.php...

 

<?php include "head.php"; ?>

<div style="margin:5px">
Here is my page!

<?php
require "connect.php";
CODE CODE CODE
require "disconnect.php";
?>
</div>

<?php include "foot.php" ?>

 

Will this conflict or be conflicted by the the require_once code in the header.php? Is it bad to have multiple connections to the database as long as there disconnected?

[Pikachu2000]

From a sql injection standpoint the password field is fine with just md5(), since all you can get back from it is a hexadecimal number.

 

I tend to always use require_once() in situations when it's possible I'll have another script that may try to include/require a file that has been previously included/required.

[evil_stevo]

So, I'm thinking it's fine if I have another piece of code in there after wards that needs reconnecting to the database. All I'll have to do is add include "connect.php" and "disconnect.php" again. Shouldn't be any problems right as long as I don't over do it?

[Pikachu2000]

I never include an explicit call to mysql_close(). From the PHP manual: 'Using mysql_close() isn't usually necessary, as non-persistent open links are automatically closed at the end of the script's execution.'.

[evil_stevo]

So, your saying I don't even really need to do the disconnect with either require or include? Or, should I be writing it out fully in the code itself instead of including or requiring?