Peer Review Of Logon Script Please

[Pigmaster]

I am trying to create a secure logon script for normal web site usage and would like peer review of the following code.

I have put everything into a single file called index.php so it will be easy for people to drop into their testing server etc.

Think that the initial registration process are done and the user has an account. The username is Sean and the password used to setup the account is qwerty. Then during the process the SALT is generated and just the username and salt is stored in the db.

Can I make this script more secure in a normal shared hosting enviroment.
I do run the inputs through a simple SQLsafe function to stripslashes etc.
I have a scriptauth variable which also checks to make sure that the data beein presented is from an actual POST

I hope you can help as I have been trying to find suitable code that most people agree about.

Thanks

<?php
/* This is a test script for Authentcation of users
  The initial vars are what would be held in the database and are only hard coded here for test this script
   The salt would be unquiely generated at registation and the salt posted to the DB along with the Hash of (Users Initial Password and Salt)  but not the password.
 */
$Users_Initital_Password = "qwerty";
 // Inital Vars 
$Username_From_DB = "Sean";
$Salt_From_DB = "fjhsfjksah"; 
$Hashed_Password_From_DB = hash('sha256',$Users_Initital_Password . $Salt_From_DB);


function LogonBox () {
	if (isset($_SESSION['session_account'])) {
		Global $User_Name;
		$output= "<form method='post' name='theForm' action='index.php?page=Logout'>";
		$output.= "<input type='hidden' name='scriptauth' value='SCAMS' />";
		$output.= "Welcome back ".$User_Name."&nbsp;<a href='javascript:document.theForm.submit();'>Logout</a>";
		$output.= "</form>";
		return $output;
	} else {
		$output= "<form method='post' name='theForm' action='index.php?page=Login'>";
		$output.= "Username";
		$output.= "<input type='text' name='username' maxlength='10' size='10' />";
		$output.= "Password";
		$output.= "<input type='password' name='password' maxlength='10' size='10' />&nbsp;";
		$output.= "<input type='hidden' name='scriptauth' value='SCAMS' />";
		$output.= "<a href='javascript:document.theForm.submit();'>Login</a>";
		$output.= "</form>";
		return $output;
	}
}

function login() {
	Global $User_Name;
	Global $Username_From_DB;
	Global $Users_Initital_Password;
	Global $Hashed_Password_From_DB;
	Global $Salt_From_DB;
	If (!isset($_POST['scriptauth'])) {
		return "ScriptError";
	}
	// Check if POST username is a value, if not return error
	if (!isval($_POST['username'])){
		$User_Name = "Guest";
		return "LogonError";
	// Check if POST password is a value, if not return error
	} elseif (!isval($_POST['password'])){ 
		$User_Name = "Guest";
		return "LogonError";
	} else {
		$user = $_POST['username'];
		$pass = ($_POST['password']);
		echo "Initial Inputs From Login Form<br>";
		echo "U:".$user."<br>P:".$pass."<br>-------------------------------<br>";
		// Do processing here on inputs		
		$user = SQLSafe($user);
		$pass = SQLSafe($pass);
		echo "Processing Inputs From Login Form Through SQLSafe Function<br>";
		echo "U:".$user."<br>P:".$pass."<br>-------------------------------<br>";
		$pass = hash('sha256',$pass . $Salt_From_DB);
		echo "Processing Password Now Through sha1<br>";
		echo "U:".$user."<br>P:".$pass."<br>-------------------------------<br>";

	// Get data from DB goes here but hard coded for now
	echo "DB Outputs<br>";
	echo "U:".$Username_From_DB."<br>P:".$Hashed_Password_From_DB."<br>-------------------------------<br>";
	if (($Username_From_DB == $user) && ($Hashed_Password_From_DB == $pass)) {
		$User_Name = $Username_From_DB;
		// Start the session
		session_start();
		// Register the session account variables
		$_SESSION['session_account'][0]= $User_Name;
		return "LogonOK";
	} else {
		// Query returned no results, so logon unsuccessful
		$User_Name = "Guest";
		return "LogonError";
	}
}
}

function logout() {
	Global $User_Name;
	Global $_SESSION;
	If (!isset($_POST['scriptauth'])) {
		return "ScriptError";
	}
	session_start(); //start the session
	// Check to see if any Session account has been registered
	if (isset($_SESSION['session_account'])) {
		// Session is registered so logout the user
		session_unset();
		session_destroy();
		$User_Name = "Guest";
		return "LogOutOK";
	} else {
		return "NotLoggedIn";
	}
}
// Function to check inputs
function isval($inp){
    if(isset($inp)){
        $len = strlen($inp);
        if($len > 0){
            return true;
        }
        else{
            return false;
        }
    }
    else{
        return false;
    }
}

//Clean SQL Variables (Security Function)
function SQLSafe($value) { 
	// Stripslashes 
	if (get_magic_quotes_gpc()) { 
		$value = stripslashes($value); 
	} 
	// Quote value
	if(version_compare(phpversion(),"4.3.0")=="-1") {
		$value = mysql_escape_string($value);
	} else {
		$value = mysql_real_escape_string($value);
	}
	return trim($value);
}
// Start of normal page consiting of php and HTML etc

//start the session
session_start();
// Check if user is logged in and assign variables
if (isset($_SESSION['session_account'])) {
	$User_Name = $_SESSION['session_account'][0];
} else {
	$User_Name = "Guest";
}

if (isset($_GET['page'])) {
	if ($_GET['page'] == "Logout") {
		$Returned_Result = logout();
		if ($Returned_Result == "ScriptError") {
			$Result = "Error: Unauthorised Script Access";
		} elseif  ($Returned_Result == "LogOutOK") {
			$Result = "Sucess: You have logged out";
		} elseif   ($Returned_Result == "NotLoggedIn") {
			$Result = "Error: You were not logged in";
		}
	}
	if ($_GET['page'] == "Login") {
		$Returned_Result = login();
		if ($Returned_Result == "ScriptError") {
			$Result = "Error: Unauthorised Script Access";
		} elseif  ($Returned_Result == "LogonOK") {
			$Result = "Sucess: You have logged in";
		} elseif   ($Returned_Result == "LogonError") {
			$Result = "Error: Username / Password Incorrect";
		}
	}
}

echo $Result."<p>";
echo LogonBox();

?>

[laserlight]

Moved to Code Critique.

[Weedpacket]

    Global $User_Name;
    Global $Username_From_DB;
    Global $Users_Initital_Password;
    Global $Hashed_Password_From_DB;
    Global $Salt_From_DB;

This was pretty much the first thing my eyeballs focused on. A bit of a turn-off, that. Lots of globals make me twitchy.

function isval($inp){
	return isset($inp) && $inp!='';
}

[Pigmaster]

Weedpacket;10900245 wrote:

    Global $User_Name;
    Global $Username_From_DB;
    Global $Users_Initital_Password;
    Global $Hashed_Password_From_DB;
    Global $Salt_From_DB;

This was pretty much the first thing my eyeballs focused on. A bit of a turn-off, that. Lots of globals make me twitchy.

They are only there to get the variables into the function, in normal operation they would come from the DB.

May I ask why they make you twitchy 🙂

function isval($inp){
	return isset($inp) && $inp!='';
}

[/QUOTE]

Thanks for the shortening of the code, I have added it my current code. Anything else?

I have also put the following into Inital Vars

$Pattern = '[A-Za-z0-9!#$%&*:_.\ ]';

and into the login function

} elseif (preg_match($Pattern,$_POST['username'])) {
	return "LogonError1";

This is to limit the type of characters that can be used.
When this is peer reviewed and shall I say acceptable to most I will then put up the full edited code for others to use as I have spent many hours trying to find a good code to start with, but on most blogging sites etc when someone posts some auth code many people pick holes in the code but never post the corrections etc.

[NogDog]

Global variables are generally frowned upon due the close-coupling they cause between the function and the application. The result is that the function cannot easily be re-used in another application (or even another circumstance within the same application), plus they can cause a number of maintenance and debugging nightmares when you start changing variable names or changing the values of those variables, etc.

If you google on something like "why are globals bad" you'll probably find lots of results with more technical, in-depth explanations.

PS: The work-around is to pass them as parameters into the function, passing them by reference if the function needs to be able to directly modify them.

[Pigmaster]

NogDog;10900287 wrote:

PS: The work-around is to pass them as parameters into the function, passing them by reference if the function needs to be able to directly modify them.

Thanks, I'll remember that for the future.

Please disregard the previous posting of $Pattern and replace with this.

This allows alphanumeric input with underscores and spaces, check there are two alphanumerics, start with alphas, may include space/numerics/underscores/alpha and end with alphanumerics.
Taken from http://www.webkami.com/programming/php/php-secure-class-to-avoid-xss/php-secure-class-to-avoid-xss-1-0-1.php

$Pattern = "/^[A-Za-z]([A-Za-z0-9\s_]*[A-Za-z0-9])*$/";

[Weedpacket]

The pattern needs correction: it allows 'A' even though that doesn't contain two alphanumerics. The first thought i had was:

/[a-z](?![\s_]*$)[a-z0-9\s_]+$/i

Where I also shortened by using case-insensitive matching.

[Pigmaster]

Thanks for that, I can not get my head around Regex even though I have read several tutorials so for, so I appreciate the help.

Can anyone else see any faults with the security aspect of the code, have what I done enough to stop XSS, SQL Injection attacks etc.

Thanks

PS, I wish there was a place either on PHPbuilder or even PHP.net where there would be a code library that was filled with Peer reviewed scripts etc that were 100% secure, thus allowing us poor programmers etc to use in out projects and also the main point understand whay it's coded that way so WE CAN LEARN.

If there is such a place, could someone shine a light for me to follow. Thanks

[NogDog]

I'm not sure any non-trivial PHP web application code can ever be guaranteed 100% secure. :rolleyes:

Anyway, one suggestion is to get your hands on Chris Shiflett's Essential PHP Security. It's a fairly short, concise book that will explain most of the security issues you need to be concerned with and how to address them. His web site has quite a few articles on the subject, too.

[Pigmaster]

NogDog;10900443 wrote:

I'm not sure any non-trivial PHP web application code can ever be guaranteed 100% secure. :rolleyes:

Anyway, one suggestion is to get your hands on Chris Shiflett's Essential PHP Security. It's a fairly short, concise book that will explain most of the security issues you need to be concerned with and how to address them. His web site has quite a few articles on the subject, too.

Thanks NogDog, I have read some of his site articles and also going to Amazon to buy the book. Thanks

[aaarrrggh]

Hi there,

I thought I'd add a few thoughts. Please don't take these comments personally, as I'm only trying to help 🙂

You're making a mistake in hardcoding the outputted html inside your class. This is a really bad idea for a few reasons. For one, the obvious problem is that now your class cannot really be used for another project, as the look of the site is hardcoded into the class itself. It's also going to be difficult for you to change the look of the site at any other point in time for the same reason. If you've got lots of classes in your eventual system that are doing this kind of thing, it'll be difficult to find out where to make changes to what on the surface is just simple html.

A far better approach would be to seperate your design from your content. In practise, what this might mean is having your functions inside your class return things like data arrays and boolean values.

For example, in your 'Login' method, you might have a sql query that checks the db to see if that user exists, and if that password is correct for that user. Instead of outputting any html at this point, you'd maybe simply return TRUE if the details are correct. You may then have a setSession() method or something like that, that would then be called to save the user details inside the session.

If you needed to pull user data out of your db, instead of doing it by having methods that pump all the details out (say like username, user access level and, I dunno, user's favourite hobbies or something), you'd instead return an array that contains all that information, maybe from a method called something like getUserDetails($id) (Where $id is the id of the user in the database).

What you'd then do in your presentation document is have pure html for the visual output itself, and then when you need to pull the data out, you'd create a user object and do a foreach loop over it (actually in this case, because you're only returning one user's details that might not be necessary, but if you were say listing 10 users or something, then you'd do a foreach loop).

So for one user, you might do something like this:

<div id="content">
<?PHP
$user = new User();
$user->getUserDetails(2); //get the details for user number 2
foreach($user as $value)
{
echo($value['username']);
}
</div>

Something like that anyway. This might not be the best example in practise, but you can see what I'm getting at.

[Pigmaster]

aaarrrggh;10900668 wrote:

Hi there,

I thought I'd add a few thoughts. Please don't take these comments personally, as I'm only trying to help 🙂

Of course not, that why I posted here asking for the feedback.

aaarrrggh;10900668 wrote:

You're making a mistake in hardcoding the outputted html inside your class. This is a really bad idea for a few reasons.

I said in the first post that I had put everything here into a single file to make things easy for people to test. I admit I am not a fantastic programmer but I can get by but will take your comments onboard when I build the code.

aaarrrggh;10900668 wrote:

For one, the obvious problem is that now your class cannot really be used for another project, as the look of the site is hardcoded into the class itself. It's also going to be difficult for you to change the look of the site at any other point in time for the same reason. If you've got lots of classes in your eventual system that are doing this kind of thing, it'll be difficult to find out where to make changes to what on the surface is just simple html.

A far better approach would be to seperate your design from your content. In practise, what this might mean is having your functions inside your class return things like data arrays and boolean values.

For example, in your 'Login' method, you might have a sql query that checks the db to see if that user exists, and if that password is correct for that user. Instead of outputting any html at this point, you'd maybe simply return TRUE if the details are correct. You may then have a setSession() method or something like that, that would then be called to save the user details inside the session.

If you needed to pull user data out of your db, instead of doing it by having methods that pump all the details out (say like username, user access level and, I dunno, user's favourite hobbies or something), you'd instead return an array that contains all that information, maybe from a method called something like getUserDetails($id) (Where $id is the id of the user in the database).

What you'd then do in your presentation document is have pure html for the visual output itself, and then when you need to pull the data out, you'd create a user object and do a foreach loop over it (actually in this case, because you're only returning one user's details that might not be necessary, but if you were say listing 10 users or something, then you'd do a foreach loop).

So for one user, you might do something like this:

<div id="content">
<?PHP
$user = new User();
$user->getUserDetails(2); //get the details for user number 2
foreach($user as $value)
{
echo($value['username']);
}
</div>

Something like that anyway. This might not be the best example in practise, but you can see what I'm getting at.

Kind of,but not really, would you like to post working example code to help?

[Pigmaster]

From my earlier posting:

Pigmaster wrote:

I have spent many hours trying to find a good code to start with, but on most blogging sites etc when someone posts some auth code many people pick holes in the code but never post the corrections etc.

From my reply to aaarrrggh:

Pigmaster;10900847 wrote:

Kind of,but not really, would you like to post working example code to help?

See it's happening again. So aaarrrggh could you provide snippets to back up your statements as it will help.