Session security... have i done this right?

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

Post Reply
Leandro-AL
Forum Newbie
Posts: 23
Joined: Thu Sep 04, 2008 10:05 am
Location: Albania

Session security... have i done this right?

Post 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
sushil
Forum Newbie
Posts: 8
Joined: Sun Sep 14, 2008 12:11 am
Location: Surat
Contact:

Re: Session security... have i done this right?

Post by sushil »

You can also create a session or pass hidden fields.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: Session security... have i done this right?

Post 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.
User avatar
The_Anomaly
Forum Contributor
Posts: 196
Joined: Fri Aug 08, 2008 4:56 pm
Location: Tirana, Albania

Re: Session security... have i done this right?

Post 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.
Leandro-AL
Forum Newbie
Posts: 23
Joined: Thu Sep 04, 2008 10:05 am
Location: Albania

Re: Session security... have i done this right?

Post 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!
Leandro-AL
Forum Newbie
Posts: 23
Joined: Thu Sep 04, 2008 10:05 am
Location: Albania

Re: Session security... have i done this right?

Post 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.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: Session security... have i done this right?

Post 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));
Leandro-AL
Forum Newbie
Posts: 23
Joined: Thu Sep 04, 2008 10:05 am
Location: Albania

Re: Session security... have i done this right?

Post 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.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: Session security... have i done this right?

Post 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.
Leandro-AL
Forum Newbie
Posts: 23
Joined: Thu Sep 04, 2008 10:05 am
Location: Albania

Re: Session security... have i done this right?

Post by Leandro-AL »

Ok so my header is now:

Code: Select all

session_start();
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.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: Session security... have i done this right?

Post 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.
Leandro-AL
Forum Newbie
Posts: 23
Joined: Thu Sep 04, 2008 10:05 am
Location: Albania

Re: Session security... have i done this right?

Post 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!
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: Session security... have i done this right?

Post by Mordred »

On the other hand i'm wondering if checking a server variable with another server variable is of any real use.
Bingo.
Leandro-AL
Forum Newbie
Posts: 23
Joined: Thu Sep 04, 2008 10:05 am
Location: Albania

Re: Session security... have i done this right?

Post 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?
Post Reply