login script, please comment all ideas are well welcomed

wilson382 · February 17, 2014, 12:22am

login.php
[php]if (($_POST[‘username’] == $row[‘username’]) && (pwhash($_POST[‘password’]) == $row[‘password’]))
{
if ($row[‘account_status’] == ‘1’)
{
$_SESSION[‘auth’] = “7469a286259799e5b37e5db9296f00b3”;
$_SESSION[‘LoggedUser’] = $row[‘username’];
$_SESSION[‘expire_time’] = time()+10*60;

			date_default_timezone_set('US/Eastern');
			$last_login=  date('m/d/Y');
			mysql_query("UPDATE `debtor_register_users` SET `last_login`='$last_login' WHERE `username`='{$row['username']}' LIMIT 1") or die(mysql_error()); 			
			
			if (isset($_SESSION['previous_page']))
			{
				$redirect = $_SESSION['previous_page'];
				unset($_SESSION['previous_page']);
				header("location: $redirect");
			}else
			{
				header("location: index.php");
			}
		}else
		{
			$msg = "<font face=\"Arial\" size=\"2\" color=\"red\">Account is not activated. Check your Email.</font>";
		}
	}else
	{
		$msg = "<font face=\"Arial\" size=\"2\" color=\"red\">Wrong Information Provided.</font>";

}
[/php]

header.php
[php]if ((@$_SESSION[‘auth’] !== “7469a286259799e5b37e5db9296f00b3”) && (!isset($_SESSION[‘auth’])))
{
//another layer of security goes here
// check if password match from database

//user is not logged in, redirect to login page
$_SESSION['previous_page'] = $_SERVER['SCRIPT_NAME'];
header("Location: login.php");
exit();

}else
{
if ($_SESSION[‘expire_time’] < time())
{
//user is logged in but has been inactive for 10 mins or more
$_SESSION[‘previous_page’] = $_SERVER[‘SCRIPT_NAME’];
header(“Location: login.php”);
} else
{
//update active time
$_SESSION[‘expire_time’] = time()+10*60;
}
}[/php]

Is this really secure for protecting my online website where I have really secret stuff?

any idea are welcome, please comment and stay your opinion im pretty sure i’m missing security measure for this login.

benanamen · February 17, 2014, 1:16am

Your #1 problem is you are using old Mysql calls. Update your code to use PDO or at the least mysqli with prepared statements.

wilson382 · February 17, 2014, 3:24pm

I’m aware of that I will start implementing PDO in my whole website when Iget the time.

any more comments

benanamen · February 17, 2014, 4:11pm

Yes, If your database is setup correctly you dont need the LIMIT 1 on line 11 since you should be updating one specific record anyways. You are updating based on the username rather than the id of that username. You need to make sure your database is setup for NO DUPLICATES on the username column. Database operations are faster on a unique id rather than a varchar or char column. You should create an index on username if thats how you are going to do it to speed it up.

wilson382 · February 17, 2014, 4:15pm

true that, yeah username is unique in my database, I copied that query from another update query and just edited the username field, forgot to remove the limit

wilson382 · February 17, 2014, 9:18pm

Understood, I have unique index for username.
back to my question, please comment on about the security I fee like is vulrable for someone to break it

benanamen · February 17, 2014, 9:51pm

I modified my post. Yes, its insecure. Change it to PDO and we can go from there. No point trying to lock down something you shouldn’t be using anyways.

PDO is very simple to learn and there are many tutorials available.

wilson382 · February 18, 2014, 6:14am

all is missing is PDO?

JimL · February 18, 2014, 6:37am

What level of security are you aiming for?

What is $_SESSION[‘auth’] for?

What does the pwhash function do?

Do you enforce SSL / HTTPS?

If so, how is this configured?

[hr]

General coding style:

Shouldn’t output html like this, instead add the error messages to an errors array, then print them all in your views.
[php]else
{
$msg = “<font face=“Arial” size=“2” color=“red”>Account is not activated. Check your Email.”;
}
}else
{
$msg = “<font face=“Arial” size=“2” color=“red”>Wrong Information Provided.”;[/php]

wilson382 · February 18, 2014, 6:52am

in login.php i set $_SESSION[‘auth’] = 7469a286259799e5b37e5db9296f00b3 and i just check to see if is logged in.

function pwhash is my encrypting function
[php]function pwhash($password,$iterations=13)
{
$hash =md5($password);
for ($i = 0; $i < $iterations; ++$i)
{
$hash = md5($hash . $password);
}
return $hash;
}[/php]

no I don enforce SSL/HTTPS and I have not paid for SSL

JimL · February 18, 2014, 6:59am

Not neccessary, you have $_SESSION[‘LoggedUser’], just check that one

Throw it away, MD5 is not secure, use Bcrypt or pbkdf2

Then it’s not secure. These free SSL certificates work for most uses
http://www.startssl.com/

benanamen · February 18, 2014, 12:12pm

No, but you don’t start building a house until you have laid a solid foundation. You have built a house of straw on sand. You are obviously concerned about security as we all should be. Lets start from the ground up and build something others reading the post can benefit from.

We are doing no good propagating bad code. I posted complete working procedural PDO to start from so there is no excuse to not do it now or not having time. JimL has also posted a PDO tutorial that you can learn from. The link is in his signature. You can find mine here:
http://www.phphelp.com/forum/the-occasional-tutorial/beginners-pdo-bumpstart-code-use-it-now!/

It’s time we all get serious about enforcing good/better coding practices.

wilson382 · February 18, 2014, 3:58pm

Yes I totally agree Im going to replace my mysql_query function with pdo on my entire website and POSt the codes back here.

indeed i heard something like that in another forum that is why i came up with that loop func over and over the same md5 hash, that makes it a lil bit stronger.

now I will fix/update everything you have helped me with so far and post back the updated codes. and then we continue from there

JimL · February 18, 2014, 4:00pm

Yeah but it’s still not good enough. An attacker may be able to calculate 950.000.000 MD5 hashes/s, while with a correct setup he may only calculate 1-4 Bcrypt hashes/s

wilson382 · February 18, 2014, 4:06pm

WoW, I guess i can use the same pwhash funtion but this time using bcrypt instead of md5, but all my users will need to reset their passwords but that’s nothing for the sake of security

JimL · February 18, 2014, 4:16pm

You don’t really need a function, bcrypt also takes care of the salt for you, so no need to store a salt separate any more.

hash a password:
[php]$hash = password_hash($_POST[‘password’], PASSWORD_BCRYPT, array(‘cost’ => 14));[/php]

verify a password:
[php]$hash = ‘$2y$14$a99FNvSsnfT.mx//9bgpSenBGe6gwZ7UCRYWeONf1daNECPNKdGgC’; // get from DB

if (password_verify($_POST[‘password’], $hash)) {
echo ‘Correct!’;
} else {
echo ‘Wroooong…’;
}[/php]

wilson382 · February 20, 2014, 6:56am

I have Bcrypt encryptiong working Im stuck at PDO Now

I need help translating this Mysql code to PDO code.

eg:
[php]
$SQL = “SELECT column1,column2,column3 FROM debtor_information WHERE id=‘78454’”; //only 1 record will match

$result  = mysql_query($SQL) or die(mysql_error());

$row = mysql_fetch_assoc($result) or die(mysql_error());

echo $row['column1'];
echo $row['column2'];
echo $row['column3'];

[/php]

This is what I got so far, what’s Next?

[php]
$SQL = “SELECT column1,column2,column3 FROM debtor_information WHERE id=?”; //only 1 record will match

$result = $conn->prepare($SQL);
$result->bindParam(1, 78454],PDO::PARAM_INT);
$result->execute();

[/php]

JimL · February 20, 2014, 8:46am

[php]$result->bindParam(1, 78454],PDO::PARAM_INT); // <---- invalid syntax. ][/php]
then
[php]$result->setFetchMode(PDO::FETCH_OBJ);
$info = $result->fetch();
print_r($info);
[/php]

wilson382 · February 20, 2014, 5:09pm

Sorry i could not spot any wrong syntax, please point me out.

I also read this http://www.php.net/manual/en/pdostatement.bindparam.php

wilson382 · February 20, 2014, 6:31pm

ok i made changes to my login script now I using bcrypt and PDO

First time that I start using PDO so I had to read the php.net manual couple times

[php] //login Form has been submitted

	$hashed_password = password_hash($_POST['password'], PASSWORD_BCRYPT, array('cost' => 14)); //from form submission
	
	$SQL = "SELECT `creditor_id`,`debtor_id`,`username`,`password`,`account_status`,`rank` FROM `debtor_register_users` 
	WHERE (`username`=? OR `email`=?)";
	$result = $conn->prepare($SQL);
	$result->bindParam(1, $_POST['username'],PDO::PARAM_STR);  //`username` PlaceHolder
	$result->bindParam(2, $_POST['username'],PDO::PARAM_STR); // `email` PlaceHolder
	$result->execute();
	$result -> setFetchMode(PDO::FETCH_ASSOC);

	$row = $result->fetch();
	
	if (password_verify($_POST['password'], $row['password'])) 
	{			
		if ($row['account_status'] == 1)
		{
	
			$_SESSION['LoggedUser'] = $row['username'];
			$_SESSION['expire_time'] = time()+10*60;
		
			date_default_timezone_set('US/Eastern');
			$last_login=  date('m/d/Y');
			
			$result = $conn->prepare("UPDATE `debtor_register_users` SET `last_login`='$last_login' WHERE `username`=?");
			$result->bindParam(1, $row['username'],PDO::PARAM_STR);
			$result->execute();
			
			if (isset($_SESSION['previous_page']))
			{
				$redirect = $_SESSION['previous_page'];
				unset($_SESSION['previous_page']);
				header("location: $redirect");
			}else
			{
				header("location: index.php");
			}
		}else
		{
			$msg = "<font face=\"Arial\" size=\"2\" color=\"red\">Account is not activated. Check your Email.</font>";
		}
	}else
	{
		$msg = "<font face=\"Arial\" size=\"2\" color=\"red\">Wrong Information Provided.</font>";
	}[/php]

i still need to store errors on an array.