Page 1 of 1
Session security... have i done this right?
Posted: Thu Sep 11, 2008 10:47 am
by Leandro-AL
Hi all this is my first post here. I'm relatively noob at php and especially in security stuff. So i've made a login system and after doing some research this is the code that i've come up with. I just would like to know the more educated opinion of people who probably know this stuff better than me.
The pages in my site are made of the actual page, and header.php and footer.php, which are included.
In header.php, i have:
Code: Select all
session_start();
session_regenerate_id(true);
When a user successfully activates their account through a link in their email, this script automatically also logs them in:
Code: Select all
$clean['query'] = mysql_query("SELECT first_name FROM users WHERE activation_key = '{$clean['activation_string']}'");
if (mysql_num_rows($clean['query']) == 1){
$clean['row'] = mysql_fetch_array($clean['query']);
mysql_query("UPDATE users SET activation_key = NULL, user_level = '2' WHERE activation_key = '{$clean['activation_string']}' LIMIT 1");
if (mysql_affected_rows($dbc) == 1){
session_regenerate_id(true);
$_SESSION['user_level'] = 2;
$_SESSION['first_name'] = $clean['row']['first_name'];
//
// some memory freeing and irrelevant stuff here
//
}
In my login script, there is:
Code: Select all
$clean['query2'] = mysql_query("SELECT first_name, user_level FROM users WHERE username = '{$clean['user']}' AND password = '{$clean['password']}' AND activation_key IS NULL");
if (mysql_num_rows($clean['query2']) == 1){
$clean['row'] = mysql_fetch_array ($clean['query2']);
session_regenerate_id(true);
$_SESSION['user_level'] = $clean['row']['user_level'];
$_SESSION['first_name'] = $clean['row']['first_name'];
//
// some memory freeing and irrelevant stuff here
//
}
Also in the footer i use $_SESSION['user_level'] to determine if a user is logged in and what sort of access they have. In my system, 0 is not registered, 1 is pending email confirmation and 2 is registered and confirmed:
Code: Select all
if ($_SESSION['user_level'] >= 2){
echo 'Welcome, ' . $_SESSION['first_name'] . '!';
}else{
// display login form (a user can't login if they have not confirmed their email, so user level is either 0-unregistered or 2-registered)
}
Also some questions:
1) Will using "true" in session_regenerate_id(
true) cause any implication when someone clicks the "back" button in their browser (or any implication for that matter)?
2) Is using "user_level" a good way of authenticating if a user is logged in or do i have it wrong? At present, the identity of a user is of no importance, i just want to know if they are registered (and confirmed) or not.
Do i have it right or am i doing it wrong? My primary concern is around session functionality/security, i would like to know if i'm vulnerable to some sort of attack that i've missed, and if yes, some sort of directions on what can i do about it would be helpful. But any other comment is welcomed.
Thank you very much,
Leandro
Re: Session security... have i done this right?
Posted: Sun Sep 14, 2008 1:35 am
by sushil
You can also create a session or pass hidden fields.
Re: Session security... have i done this right?
Posted: Sun Sep 14, 2008 3:29 pm
by Mordred
No sense in regenerating the SID in "normal" cases. Do it only on login/logout. Giving it (true) is correct. If you are worried how this will work with the Back button, I suggest getting a browser that has one and trying it. We haven't seen how $clean[] is populated, and experience shows that this is one of the most common points of failure.
Re: Session security... have i done this right?
Posted: Sun Sep 14, 2008 3:45 pm
by The_Anomaly
sushil wrote:You can also create a session or pass hidden fields.
I'm not exactly sure how you're applying this, but be very cautious with hidden fields when it comes to security. Any type of form field (hidden or not) can be easily changed and manipulated in every manner.
I'm sure the majority (if not all) of members of this forum know this--but just throwing it out there in case the OP does not. Heck, I remember reading that some ecommerce sites actually put their price in a hidden field--which allows any amateur cracker to change the price of a product easier than ever.
Re: Session security... have i done this right?
Posted: Sun Sep 14, 2008 4:39 pm
by Leandro-AL
About the hidden fields, yea i realised i have omitted inserting a random value to a hidden input field and in a session variable and compare them in the recieving script. I read this in a php security blog as a way to protect from CSRF (
link) and it sounded a good idea, i don't know what you guys think, some feedback would be good.
Mordred, so is it ok to just remove it from the header since it doesn't help with anything? Is the way i have it in my login script ok?
The $clean array is initialised and stuff that are going in it are checked through regexp and escaped, or going direcly in if it's my own text. An example would be as follows:
Code: Select all
$clean = array();
$trimmed = array_map('trim', $_POST);
unset ($_POST);
$clean['match'] = '/^http://www\.somesite\.com/[A-Za-z/]+\.php$/';
if (preg_match($clean['match'], $trimmed['destination'])){
$clean['destination'] = mysql_real_escape_string($trimmed['destination']);
}else{
$clean['destination'] = 'http://www.somesite.com/index.php';
}
Is this ok?
Oh by the way this is my logout script:
Code: Select all
<?php
if ($_SESSION['user_level'] >= 2){
$_SESSION['user_level'] = 0;
$_SESSION['first_name'] = '';
$_SESSION = array();
session_destroy();
setcookie(session_name(), '', time() - 300);
header("Location: http://www.somesite.com/index.php?loggedout=yes");
ob_end_clean();
exit();
}else{
header("Location: http://www.somesite.com/index.php?notlogged=yes");
ob_end_clean();
exit();
}
?>
Thanks for your replies!
Re: Session security... have i done this right?
Posted: Sun Sep 14, 2008 6:03 pm
by Leandro-AL
About the hidden input/session variable comparison check, after following the steps on the site that i told you earlier, i have come up with this:
Registration form:
Code: Select all
$clean['registertoken'] = sha1(time());
$_SESSION['registertoken'] = $clean['registertoken'];
$_SESSION['registertokentime'] = time();
//
echo '<input type="hidden" name="registertoken" value="' . $clean['registertoken'] . '" />';
//
Form validation:
Code: Select all
$trimmed = array_map('trim', $_POST);
$clean['match'] = '/^[a-z0-9]{40}$/';
$clean['tokenage'] = time() - $_SESSION['registertokentime'];
if !(preg_match($clean['match'], $trimmed['registertoken']) && ($trimmed['registertoken'] == $_SESSION['registertoken']) && ($clean['tokenage'] <= 300)){
$errors[] = 'The system has detected a possible security threat.';
}
Each script has its own name for the tokens. For example the registration script has "registertoken", the login script "logintoken" etc. to avoid messing up the tokens for pages that contain multiple forms at the same time.
My primary concern at this point is if someone can somehow alter session data. I'm concerned if it's possible for someone to alter my $_SESSION['user_level']. I'm also thinking about the session_regenerate_id() on the header. Since there is session data (like the form token thing for example, or a registered but not confirmed user's first name) that exists without the user needing to log in, does regenerating the session help any to avoid this info being compromised somehow?
Also in my logout script, would it be a better idea if i don't check for user level and destroy all sessions?
Again thanks for your help.
Re: Session security... have i done this right?
Posted: Mon Sep 15, 2008 1:50 am
by Mordred
Mordred, so is it ok to just remove it from the header since it doesn't help with anything?
Remove
what? I don't understand the question, sorry.
Code: Select all
$clean['registertoken'] = sha1(time());
This it
not a random token, in fact it's trivially predictable. Use uniqid() (follow the sample, with mt_rand() instead of rand() )
Code: Select all
$better_token = md5(uniqid(mt_rand(), true));
Re: Session security... have i done this right?
Posted: Mon Sep 15, 2008 6:30 am
by Leandro-AL
Remove what? I don't understand the question, sorry.
I was talking about the
session_regenerate_id(true); in the header:
Code: Select all
session_start();
session_regenerate_id(true);
Thanks for the tip on the token.
Re: Session security... have i done this right?
Posted: Mon Sep 15, 2008 7:09 am
by Mordred
Ah. Yes, regenerate only on login/logout. If you're interested, this is a protection against something called session fixation, you can look it up.
Edit: waitwaitwait... I haven't read carefully!
No!
I was talking about the whole busyness of calling regenerate_id. You should call it only on login/logout with the "true" parameter. Remove from the header the whole function call.
Re: Session security... have i done this right?
Posted: Mon Sep 15, 2008 9:15 am
by Leandro-AL
Ok so my header is now:
The login script:
Code: Select all
if (authentication successful) {
session_regenerate_id(true);
$_SESSION['user_level'] = $clean['row']['user_level'];
$_SESSION['first_name'] = $clean['row']['first_name'];
}
Do i have these ok?
You should call it only on login/logout with the "true" parameter.
However i don't understand why one would call it on logout... since the session is being destroyed, what purpose would regenerating the id have?
Thanks for your help.
Re: Session security... have i done this right?
Posted: Mon Sep 15, 2008 9:39 am
by Mordred
Leandro-AL wrote:since the session is being destroyed, what purpose would regenerating the id have
You're right, if you destroy the session, then there's no need to regenerate.
Re: Session security... have i done this right?
Posted: Tue Sep 16, 2008 7:38 am
by Leandro-AL
Hi again! In
this article, the author discusses session hijackign and proposes the use of a random token that will be propagated from page to page with GET and will be checked against the session's token.
However, i find the use of GET not only not very secure but also a hassle to code. What would you say about the possibility of placing this token in a database and check it against the session's token? This way we could eliminate the risks of GET and also it's easier to code.
On the other hand i'm wondering if checking a server variable with another server variable is of any real use. Any ideas?
Thanks!
Re: Session security... have i done this right?
Posted: Tue Sep 16, 2008 8:10 am
by Mordred
On the other hand i'm wondering if checking a server variable with another server variable is of any real use.
Bingo.
Re: Session security... have i done this right?
Posted: Tue Sep 16, 2008 10:01 am
by Leandro-AL
I thought so. However it seems really painful to pass a variable to every page. Is there some easy way that i'm missing to do this?