session security

[PartialReptile]

Hey,

I'm currently working on something at the moment that requires sessions, and to be honest before about two weeks ago, I've never used them.

The following is what I came up with to check if sessions are set and / or valid, but before I go ahead and use this at the top of a ton of pages I though I'd check if much better people than me think its sufficient.

Are there any gaping secutiry holes? Have I missed anything obvious that you think I should be checking? Is this compleatly wrong? 😃 And, of course, if anyone can suggest ways this can be done better (if needed), i'd apreciate any thoughs you have.

Many thanks.

session_start();
include('/home/userName/configure/smallConfigure.php');

if( (!isset($_SESSION['sesUSER'])) or (!isset($_SESSION['sesPW'])) or (!isset($_SESSION['sesSellerID'])) )
{
	header("Location: " . $thisSiteToRoot . "guest/logIn/");
	exit();
}

include('/home/' . $adminUserName . '/configure/configure.php');
@ $con = mysql_connect($masterHost,$masterUN,$masterPW);
if(!$con)
{
	header("Location: " . $thisSiteToRoot . "error/service/");
	exit();
}
@ $selectDB = mysql_select_db( $sellerDB, $con );
if(!$selectDB)
{
	mysql_close( $con );
	header("Location: " . $thisSiteToRoot . "error/service/");
	exit();
}
$user = mysql_real_escape_string($_SESSION['sesUSER']);
$password = mysql_real_escape_string($_SESSION['sesPW']);
$sellerID = mysql_real_escape_string($_SESSION['sesSellerID']);

$sql ="SELECT sellerID FROM seller WHERE sEmail='$user' and password='$password'";
@ $rs = mysql_query( $sql, $con );
if(!$rs)
{
	mysql_close( $con );
	header("Location: " . $thisSiteToRoot . "error/service/");
	exit();
}
$num = mysql_num_rows( $rs );
if( ($num == 0) or ($num > 1) )
{
	mysql_close( $con );
	header("Location: " . $thisSiteToRoot . "guest/logIn/");
	exit();
}
@ $row = mysql_fetch_assoc( $rs );
if(!$row)
{
	mysql_close( $con );
	header("Location: " . $thisSiteToRoot . "error/service/");
	exit();
}
$sellerIDCalled = $row['sellerID'];
$indexCalled = $row['index'];
mysql_close($con);

if( $sellerIDCalled != $_SESSION['sesSellerID'] )
{
	header("Location: " . $thisSiteToRoot . "guest/logIn/");
	exit();
}

[TeraTask]

You haven't shown us how you populate the $_SESSION array which could be a security risk, but your code above seems OK. Here are some suggestions:

Any include statements with variables require you to check to make sure that the entered value for the variable is acceptable or your server will be hacked.

Do not store passwords. Hash them and include a seed. This means users will have to reset their passwords, but it's far more secure. For example, you are storing sessions in the default location (instead of in the database), so it's likely that someone could read your stored session data on your server and fetch people's username and password.

Check out database-based sessions. They're more secure.

Good first try! Keep up the diligence to doing it right.

[PartialReptile]

hi TeraTask, Thanks for the reply. I will indeed look into database-based sessions (suggestions like that are why I thought it better to ask now and not later). The script I'm using to process the log in is below.

I've one or two questions if you don't mind:

1. The password that someone enters in a form (acb1234 for example) isn't the value that is stored in the sessions var for password, what's in the session (and in the D😎 is md5($secret_add_on . $password) assuming the first var is a long string of random characters and the second is $_POST['password']. Is this a secure enough way of doing this?

2. The var in the include satements is hard-coded (it's in the initial include) and all files are outside of the public directory. I'm simply doing this because the server is going to change before it goes live, so I wanted to avoid a search and replace on all the includes, are you saying not to do this?

anyway, I'm off to check out the db-based sessions.Cheers again.

session_start();
include('/home/userName/configure/smallConfigure.php');

$user = $_POST['user'];
$password = $_POST['password'];

function valid_email($user) 
{
  	return preg_match('#^[a-z0-9.!\#$%&\'*+-/=?^_`{|}~]+@([0-9.]+|([^\s]+\.+[a-z]{2,6}))$#si', $user);
}

if( ($_SERVER['REQUEST_METHOD'] != "POST") or (!isset($user)) or (!isset($password)) )
{
	header("Location:" . $thisSiteToRoot . "guest/logIn/");
	exit();
}
elseif( (empty($user)) or (empty($password)) )
{
	header("Location:" . $thisSiteToRoot . "error/missingFields/");
	exit();
}
elseif( (preg_match("/(%0A|%0D|\\n+|\\r+)/i", $user) != 0) or (preg_match("/(%0A|%0D|\\n+|\\r+)/i", $password) != 0) )
{
	header("Location:" . $thisSiteToRoot . "error/badAction/");
	exit();
}
elseif (!valid_email($user))
{
	header("Location:" . $thisSiteToRoot . "error/invalidEmail/");
	exit();
}
else
{
	include('/home/' . $adminUserName . '/configure/configure.php');
	@ $con = mysql_connect( $masterHost,$masterUN,$masterPW );

if(!$con)
{
	header("Location:" . $thisSiteToRoot . "error/service/");
	exit();
}

$user = mysql_real_escape_string( $user );
$password = mysql_real_escape_string( $password );

include('/home/' . $adminUserName . '/configure/secret.php');
$password = md5($secret_add_on . $password);

@ $selectDB = mysql_select_db( $sellerDB, $con );
$sql ="SELECT sellerID FROM seller WHERE sEmail='$user' and password='$password' LIMIT 1";
@ $rs = mysql_query( $sql, $con );

if(!$selectDB)
{
	mysql_close( $con );
	header("Location:" . $thisSiteToRoot . "error/service/");
	exit();
}
if(!$rs)
{
	mysql_close( $con );
	header("Location:" . $thisSiteToRoot . "error/service/");
	exit();
}

$num = mysql_num_rows( $rs );

if( ($num == 0) or ($num > 1) )
{
	mysql_close( $con );
	header("Location:" . $thisSiteToRoot . "error/noMatch/");
	exit();
}
else
{
	$row = mysql_fetch_assoc( $rs );
	$sellerID = $row['sellerID'];

	$_SESSION['sesUSER'] = "$user";
	$_SESSION['sesPW'] = "$password";
	$_SESSION['sesSellerID'] = "$sellerID";
	mysql_close( $con );

	header("Location:" . $thisSiteToRoot . "goto/userAdmin/");
	exit();
}
}

[TeraTask]

PartialReptile;10879070 wrote:

1. The password that someone enters in a form (acb1234 for example) isn't the value that is stored in the sessions var for password, what's in the session (and in the D😎 is md5($secret_add_on . $password) assuming the first var is a long string of random characters and the second is $_POST['password']. Is this a secure enough way of doing this?

That's exactly what I was talking about. MD5 is ok to use, but SHA512 is much better. If you're on PHP 5, then check out http://www.php.net/hash ; otherwise, you could use SHA1, but if this is a live system then I wouldn't go around changing anything b/c that will have the effect of resetting everyone's password and you were smart enough to include a seed ($secret_add_on).

PartialReptile;10879070 wrote:

2. The var in the include satements is hard-coded (it's in the initial include) and all files are outside of the public directory. I'm simply doing this because the server is going to change before it goes live, so I wanted to avoid a search and replace on all the includes, are you saying not to do this?

So long as the variable is hard-code and you don't have register globals on, then you're fine. But, if you have register globals on, then it is possible that someone could find a way around this. Register globals should be off anyway -- your code doesn't need it.

Your code posted in your last reply looks fine to me.

Best wishes!

[PartialReptile]

Hey TeraTask, thanks (again) for the reply and the clarity (it's good to know you're doing something half right every so often). Oh yes! Register Globals OFF and always have been.

again, I really appreciate the replies. Have a good evening/morning/other time of day.

[laserlight]

I suggest a slight change, namely to check that the incoming variables exist before assigning them:

$user = '';
$password = '';
if( ($_SERVER['REQUEST_METHOD'] == "POST") && isset($user, $password) )
{
    $user = $_POST['user'];
    $password = $_POST['password'];
}
else
{
    header("Location:" . $thisSiteToRoot . "guest/logIn/");
    exit();
}

// Now perform the validation.
if( empty($user) or empty($password) )
{
    header("Location:" . $thisSiteToRoot . "error/missingFields/");
    exit();
}
elseif // ...

Incidentally, $secret_add_on looks like it is fixed across the board. It would be better for the salt to be specific to each user, but of course having a salt is better than having none at all.

[PartialReptile]

Hi Laserlight, thanks for the reply. Both suggestions make sense.

How do you suggest I generate a user specific salt?The only values in the log in are user (an email address) and a password. Should I be using the email address or portion of the email address for the salt (As I nderstand it, the point of a salt is for it to be a unknowen value, so I assume this isn't the way to do it).

or, should I be creating a user specific salt (a random string or such) on sign up, and int login / session checks do a call to the DB to garb it before doing the main call (as in; select the salt value from the db where the emill address matches, then do the validation of the user / password in another sql statement?

cheers again.

[laserlight]

How do you suggest I generate a user specific salt?The only values in the log in are user (an email address) and a password. Should I be using the email address or portion of the email address for the salt (As I nderstand it, the point of a salt is for it to be a unknowen value, so I assume this isn't the way to do it).

A salt need not be secret. You could just say, take the hash of a random number when the user registers. To authenticate a user, select the password hash and salt based on the username. Compute the hash of the salt and the given password, and compare the result to the hashed password retrieved.

[PartialReptile]

Excellent.

Again, thank you both for your time.