Login Script Secure?

millercj · April 20, 2008

I was wondering if someone could test out this login script for me and tell me if it's secure and or what I can do to make it more so. It's just blank pages, and php echos. Below is my php files and here is a link to the login page. A working username/password are UnI9371/Ce4447611528.

http://www.recorded-live.com/portal/info.php

info.php (the login page)

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<title>Portal Test</title>
<script type="text/javascript" src="char.js"></script>
</head>
<body>
<form action="powerscripts/action_login.php" method="post" class="formation">
  <input type="text" id="UN" name="UN" value="Username" onkeydown="valid(this,'special');" onblur="valid(this,'special');" />
  <input type="password" id="PW" name="PW" onkeydown="valid(this,'special');" onblur="valid(this,'special');" />
  <button class="custombutton" type="submit" name="submit" value="submit"><img src="../images/utility/logbutton.png" alt="Login" /></button>
</form>
</body>
</html>

char.js (Illegal Character Removal)

var r=
{
  'special':/[^\w]/g,
  'allowspace':/[^\w|\s]/g,
  'email':/[^\w|@|\.]/g
}


//The Function
function valid(o,w)
{
o.value=o.value.replace(r[w],'');
}

action_login.php (login Script)

<?php
require_once "action_connect.php";
session_start();

if (isset ($_POST['submit']))
{
if (preg_match('/[!@#$%^&*()-+=`~<>,.?}{|]/', $_POST['UN']))
{
	echo "Illegal Characters In Username";
}
else
{
	if (preg_match('/[!@#$%^&*()-+=`~<>,.?}{|]/', $_POST['PW']))
	{
		echo "Illegal Characters In Password";
	}
	else
	{
		$username = $_POST['UN'];
		$password = md5 ($_POST['PW']);
		$sql = "SELECT * FROM users WHERE username='$username' AND password='$password'";
		if ($r = mysql_query ($sql)) 
		{
			$row = mysql_fetch_array ($r);
			$num = mysql_num_rows ($r);
			if ($num > 0)
			{
				$_SESSION['users_id'] = $row['users_id'];
				$_SESSION['username'] = $row['username'];
				$_SESSION['fname'] = $row['first_name'];
				$_SESSION['lname'] = $row['last_name'];
				$_SESSION['email'] = $row['email'];
				$_SESSION['loggedin'] = TRUE;
				$cookiename = 'stjucc_portal';
				$cookievalue=rand(100000,999999);
				$_SESSION['cookieverify'] = $cookievalue;
				setcookie($cookiename,$cookievalue,time()+3600,"/");
				$today=date(r);
				mysql_query("UPDATE users SET login = '$today' WHERE username = '$username'") or die (mysql_error());
				header("Location:../index.php");
				exit;			
			}
			else{echo 'Username or Password are Incorrect';}
		}
		else{echo 'Server Error';}
	}
}
}
else{echo 'Form Not Submitted';}
?>

action_connect.php

<?php
/*Database Connection File*/
$host   = 'xxx';
$dbuser = 'xxx';
$dbpass = 'xxx';
$dbname = 'xxx';
$connect = @mysql_connect ($host, $dbuser, $dbpass) or die ('We could not connect you to the Database System');
$select_db = @mysql_select_db ($dbname) or die ('We could not find the proper database');
?>

Head Code (php at the top of every secure page)

<?php
session_start();
if (isset($_COOKIE["stjucc_portal"]))
{ 
if($_COOKIE['stjucc_portal']==$_SESSION['cookieverify'])
{
	if($_SESSION['loggedin'] == TRUE)
	{
		print '';		
	}
	else{header("Location:http://www.recorded-live.com/portal/validation.php");}//not logged in

}
else{header("Location:http://www.recorded-live.com/portal/validation.php");}//validationfailed
  
}
else{header("Location:http://www.recorded-live.com/portal/validation.php");} //nocookies 
?>

action_logout.php(logout script)

<?php
session_start();
setcookie("stjucc_portal",$_SESSION['cookieverify'],time()-3601,"/");
unset ($_SESSION);
session_destroy();
header("Location:http://www.recorded-live.com/portal/logout.php");
?>

DeanWhitehouse · April 20, 2008

it seems ok

discomatt · April 20, 2008

Couple things. Javascript is easy to disable... only use it to prevent the user from having to reload the page before errors are detected.

What I'm saying is your regex in PHP should verify the same things your javascript does (spaces, ect)

Also, you didnt escape special characters in your regex character class (-, +, ect). You also didnt restrict quotes, which makes you vulnerable to SQL injection. You should also real_escape_string your username data.

Finally, you should never restrict characters in passwords. Many people like to use a broad range of characters to make bruteforcing more difficult

millercj · April 20, 2008

Ok, I changed my preg match to this

if (preg_match("/[^0-9a-z\_]/i", $_POST['UN']))

but are you saying i should use real_escape_string() instead? I've never used that before, the documentation on php.net didn't seem to provide any benefit over preg_match

woobarb · April 20, 2008

If it's going in a database then real_escape_string() should definitively be used, also that regex expression allows for characters before and after the bit which will return true... so a stray ' could get through!

millercj · April 20, 2008

Alright, reasoning makes sense...how do i implement this. I tried this as a test but it's not working it allows all characters through:

$username = $_POST['UN'];
$password = md5 ($_POST['PW']);
mysql_real_escape_string($username);		
echo $username;

GingerRobot · April 20, 2008

I think you misunderstood - preg_match does an different thing to mysql_real_escape_string.

By using the preg_match(), you're restricting which characters can be part of the string.

By using mysql_real_escape_string(), you're escaping all dangerous characters before they go near the database.

You should look to use mysql_real_escape_string() on all strings being stored in the database.

For a login system, i would use preg_match() to check the username, but not the password. Why should you prevent someone using special characters in their password? You want to be using it on the username, because you dont want people having stupid things quote html tags in their names.

As for the comment regarding your regex, you should make sure to include the ^ and $ to signify the start of the string. You want your pattern to be the entire string, not just part of it.

To explain, say you had the pattern:

"/[a-zA-Z]+/"

This would match and string with at least one letter in it. That means that someone could place dangerous characters in their username, so long as it still had a letter in. However, the pattern:

"/^[a-zA-Z]+$/"

Would only allow a string which only contained letters.

discomatt · April 20, 2008

With your regex, you don't need to real_escape_string.. and with the MD5 on the password, you don't have to escape that either (an injection attempt would become garbage )

You only need to real_escape_string data that you want quotes and other potentially dangerous characters in. This is common in blog/news/forum posts, ect. As long as you have strict rules that prevent those dangerous characters (^[A-z\d_]++$ or md5() for example) you have no need to call real_escape_string.

As a side note, anything you will output to the browser that a user has inputted should be stripped of html (easier -> htmlentities() ) or filtered for dangerous tags (harder) unless it's from a trusted source. Dangerous javascript or css could mask the page into a fake login that captures user's information.

millercj · April 20, 2008

Thanks everyone for your input

Sign In

Login Script Secure?

Recommended Posts

millercj

Link to comment

Share on other sites

DeanWhitehouse

Link to comment

Share on other sites

discomatt

Link to comment

Share on other sites

millercj

Link to comment

Share on other sites

woobarb

Link to comment

Share on other sites

millercj

Link to comment

Share on other sites

GingerRobot

Link to comment

Share on other sites

discomatt

Link to comment

Share on other sites

millercj

Link to comment

Share on other sites

Join the conversation

Browse

Activity

Important Information