HiveBrain v1.2.0
Get Started
← Back to all entries
snippetphpMinor

How to regenerate my session ID often when using this class?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thishowoftenregenerateusingsessionwhenclass

Problem

I have created a class that will manage my session. I am hoping to acomplish a class that will secure my site from all known attacks (ie. fixation, precedent, and capture.)

The idea is to

  • Change the session storage engine from files on the servers to a database.



  • Create a random session id and not relay on PHP to generate the session id for me.



  • Change the length of the session id to the max allowed (ie. 40 Characters)



  • Store the new session id into a table along with a user_id (application user identifier,) along with the user's IP address and user agent info.



  • Set an idle time out so that the system will automatically destroy the user session after 1800 seconds of idle time.



  • In addition, I will only use SSL connection to ensure the session_ids are not sniffed



After implementing the above, for every requested page I check for the following to be true

  • A valid session ID in the user's Browser.



  • Make sure I can match the current IP address and the current user agent info to those info that were captured at the time of login.



  • If the session is valid and not expired then use it otherwise create a new one if the user_is is available (aka a log in attempt.)



Can you please review my code below and tell me

  • How can I regenerate the session id often? do I really need to do this?



  • Is this class secured? if not how can I improve it?



```
cookie_name = $name;

//get the current time
$this->current_time = time();

if(isset($_SERVER['REMOTE_ADDR']))
$this->user_ip = $_SERVER['REMOTE_ADDR'];

if(isset($_SERVER['HTTP_USER_AGENT']))
$this->user_agent = $_SERVER['HTTP_USER_AGENT'];

// Set SSL level
$https = isset($secure) ? $secure : isset($_SERVER['HTTPS']);

//set the session storage to point custom method
session_set_save_handler(
array($this, "open"),
array($this, "close"),
array($this, "read"),
array($this, "write")

Solution

I like this question because I think it has a lot of answering potential. I'm going to touch on a few things, but my answer shouldn't be considered complete!
Security of custom sessions

There's one thing that's most important here: don't roll out a security system that's been built with only one pair of hands. It's asking for a disaster! I won't do the research for you, but I suggest you look at other more established session customization libraries.

Head over to Github and start searching. You're looking for a library that meets your requirements, plus has some sort of accreditation. The more users using the code, the safer it will be.

Posting code here on Code Review is a start, but it's not enough. you'll want to see a security expert before implementing your own security guards, especially if you have data worth protecting. (That includes users' passwords!)

So are there any flaws that the naked eye can see in your code? Most definitely! Let's point out a few:

-
You're relying a lot on the user here: $_SERVER['REMOTE_ADDR'], $_SERVER['HTTP_USER_AGENT'], and $_SESSION.

These come from the user, therefore I'd advise not trusting these solely 100%, as it's unlikely they'll be consistent/reliable/safe.

-
I don't like the way generateSessionID() looks. rand() is hardly random, and therefore predictable.

-
Perhaps you've read this already, but here's a good session hijacking article. It's a couple years old, but I think it has some useful links.Don't pour all of this in your code, as I'm not sure how much is outdated.

Coding style

There's a lot to say here, but I'll try and make a quick summary:

Follow a coding style convention (Zend, PHP-FIG, etc.), be consistent, and DRY out your code.

Examples? Sure:

-
Comments are very important. Stuff like //This function resume existing session doesn't help the reader. Consider using PHPDocs, and keep the comments to only necessary remarks that tell why, not how.

Classes are normally CapitalCase, so sessionManager wouldn't fit convention.

-
Is it $user_agent or $autherizedUser? What style will you choose? :)

-
This one is very apparent in your code! It really sticks out in places like:

if($this->db)
    return true;

// Return False
return false;


Why not:

return isset($this->db);


Notice I left out the comment too. It was useless to have a comment like that. The last thing comments should do is reiterate what the code does!

There was no reason to have two return statements.

And I'll wrap it up there! Hopefully we get the gist of what needs to be done!

Code Snippets

if($this->db)
    return true;

// Return False
return false;
return isset($this->db);

Context

StackExchange Code Review Q#54564, answer score: 4

Revisions (0)

No revisions yet.