5
\$\begingroup\$

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

  1. Change the session storage engine from files on the servers to a database.
  2. Create a random session id and not relay on PHP to generate the session id for me.
  3. Change the length of the session id to the max allowed (ie. 40 Characters)
  4. 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.
  5. Set an idle time out so that the system will automatically destroy the user session after 1800 seconds of idle time.
  6. 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

  1. A valid session ID in the user's Browser.
  2. 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.
  3. 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

  1. How can I regenerate the session id often? do I really need to do this?
  2. Is this class secured? if not how can I improve it?
<?php
ini_set('session.hash_function', 1);
ini_set('session.hash_bits_per_character', 4);
class sessionManager {
 private $db;
 private $user_id;
 private $user_ip;
 private $user_agent;
 private $autherizedUser = false;
 private $cookie_name;
 private $current_session_id;
 private $max_session_idle_time = 1800;
 private $current_time;
 public function __construct($name, $limit = 0, $path = '/', $domain = null, $secure = null){
 // Set the cookie name
 session_name($name);
 //assign the cookie name that will be used for the session
 $this->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"),
 array($this, "delete"),
 array($this, "garbageCollector")
 );
 //Set session cookie options
 session_set_cookie_params($limit, $path, $domain, $https, true);
 //if there is no IP detected - make it invalid
 if( empty($this->user_ip) || empty($this->user_agent) ){
 echo 'Invalid Request!!!';
 exit();
 }
 }
 //This function resume existing session
 public function resumeSession(){
 //if this it not an attempt to log in then resume session
 if(session_id() == '' )
 session_start();
 //grab the current session_id 
 $this->current_session_id = session_id();
 // Make sure the session hasn't expired, and destroy it if it has 
 if( $this->isValidSession() ){
 if($this->isHijacking()){
 //destroy the session
 $this->destroy();
 } else {
 //reset the idle time out
 $_SESSION['MA_IDLE_TIMEOUT'] = $this->current_time + $this->max_session_idle_time;
 $this->autherizedUser = true;
 }
 } 
 }
 //This function starts a new session - on the login
 //@userid is the logged in user id 
 public function startNewSession($userid){
 //Set the user id
 $this->user_id = $userid;
 $new_session_id = $this->generateSessionID();
 session_id($new_session_id);
 //grab the current session_id 
 $this->current_session_id = $new_session_id;
 //echo 'New';
 //exit();
 session_start();
 $this->setSessionValues();
 $this->autherizedUser = true;
 }
 //This function destroy existing session
 public function destroy(){
 if(session_id() == '' )
 session_start();
 $this->autherizedUser = false;
 session_unset();
 session_destroy();
 unset($_COOKIE[$this->cookie_name]);
 }
 //This function set a new values to the session
 private function setSessionValues(){
 $_SESSION = array();
 //set the IP address info
 $_SESSION['MA_IP_ADDRESS'] = $this->user_ip;
 // save the agent information
 $_SESSION['MA_USER_AGENT'] = $this->user_agent;
 //set the idle timeout
 $_SESSION['MA_IDLE_TIMEOUT'] = $this->current_time + $this->max_session_idle_time;
 }
 //This function check if the current session is valid or not
 private function isValidSession(){
 if( empty($this->current_session_id) )
 return false;
 if( !isset($_SESSION['MA_IP_ADDRESS']) || !isset($_SESSION['MA_USER_AGENT']) || !isset($_SESSION['MA_IDLE_TIMEOUT']) )
 return false;
 if( empty($_SESSION['MA_IP_ADDRESS']) || empty($_SESSION['MA_USER_AGENT']) || empty($_SESSION['MA_IDLE_TIMEOUT']) )
 return false;
 //if the session expired - make it invalid
 if( $_SESSION['MA_IDLE_TIMEOUT'] < $this->current_time )
 return false;
 //the session is valid
 return true;
 }
 //This function check if this is a session Hijacking attempt or nor
 private function isHijacking(){
 //if the set IP address no not match the current user's IP address value - make it invalid
 if( $_SESSION['MA_IP_ADDRESS'] != $this->user_ip )
 return true;
 //if the set user agent value do not match the current user agent value - make it invalid
 if( $_SESSION['MA_USER_AGENT'] != $this->user_agent )
 return true;
 //the session is valid
 return false;
 }
 public function isAutherized(){
 return $this->autherizedUser;
 }
 public function currentSessionID(){
 return $this->current_session_id;
 }
 //This function generate new random string
 private function generateSessionID($len = 40) {
 $characters = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ,-';
 $newStr = '';
 $maxLen = strlen($characters)-1;
 for ($i = 0; $i < $len; ++$i) 
 $newStr .= $characters[rand(0, $maxLen)];
 return $newStr;
 }
 //open the database connection for the session storage engine
 public function open(){
 $this->db = new connection();
 if($this->db)
 return true;
 // Return False
 return false;
 }
 //close the database connection for the session storage engine
 public function close(){
 if($this->db->endConnection())
 return true;
 // Return False
 return false;
 }
 //read current session variables from the session database
 public function read($id){
 // Set query
 $data = $this->db->getDataSet('SELECT data FROM sessions WHERE session_id = ?', array($id));
 if(count($data) == 1)
 return $data[0]['data'];
 return '';
 }
 //replace the existing data using the current session id
 public function write($id, $data){
 // Set query 
 $replace = $this->db->processQuery('INSERT INTO sessions(session_id, access, data, user_id) VALUES (?, ?, ?, ?)
 ON DUPLICATE KEY UPDATE
 session_id = ?,
 access = ?,
 data = ?', array($id, $this->current_time, $data, $this->user_id, $id, $this->current_time, $data));
 if($replace)
 return true;
 // Return False
 return false;
 }
 //delete a session record from the storage engine
 public function delete($id){
 // Set query
 $delete = $this->db->processQuery('DELETE FROM sessions WHERE session_id = ? OR user_id IS NULL', array($id));
 if($delete)
 return true;
 // Return False
 return false;
 } 
 //deletes all expired session - if the access time is less that current time
 public function garbageCollector($max){
 // Calculate what is to be deemed old
 $old = $this->current_time - $max;
 // Set query
 $delete = $this->db->processQuery('DELETE FROM sessions WHERE access < ?', array($old));
 if($delete)
 return true;
 // Return False
 return false;
 } 
}
?>

The following are the steps that I follow to use the class.

On the login page and after the user name and password match the database. I start a new session:

$session = new sessionManager('test_session', 0, '/uat/', 'uat.domain.com', true); 
$session->startNewSession($info['user_id']);
if( $session->isAutherized() === true ){
 header('Location: index.php');
 exit();
}

Then on every page I do the following to check if the info are valid:

$session = new sessionManager(SESSION_NAME, SESSION_MAX_AVAILABLE, SESSION_SAVE_PATH, SESSION_SAVE_URL, SESSION_HTTPS_ONLY);
$session->resumeSession();
$session_id = $session->currentSessionID();
if( $session->isAutherized() === false ){
 $session->destroy();
 header('Location: '.ABSOLUTE_PATH.'login.php');
 exit();
}

Following to this code I do a database lookup using the $session_id value:

SELECT u.user_id, u.name, u.access_level
FROM sessions AS s
INNER JOIN users AS u ON u.user_id = s.user_id
WHERE s.session_id = $session_id
Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Jun 17, 2014 at 22:11
\$\endgroup\$
5
  • \$\begingroup\$ IP verification can give false positives for mobile users. When moving between different service areas a cellular IP device may receive a new IP. You need to consider if you want to force a re-login for your mobile users that move about a lot. \$\endgroup\$ Commented Jun 18, 2014 at 0:17
  • \$\begingroup\$ Thanks Emily, how would I do that? \$\endgroup\$ Commented Jun 18, 2014 at 0:30
  • \$\begingroup\$ That depends on your implementation. \$\endgroup\$ Commented Jun 18, 2014 at 9:13
  • \$\begingroup\$ So if the device used is mobile then I probably would need to skip the ip check. \$\endgroup\$ Commented Jun 18, 2014 at 14:07
  • \$\begingroup\$ AOL use (at least did years ago) floating IPs, mixed in with proxies can cause a different IP address every page hit (persistent connections such as websockets will maintain their IP). So do NOT rely on the IP at all else you may completely block some users from logging in at all. You may get away with a partial match on the users browser, but you're not really left with much. \$\endgroup\$ Commented Jun 19, 2014 at 14:02

1 Answer 1

5
\$\begingroup\$

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:

  1. 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.

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

  3. 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!

answered Aug 1, 2014 at 1:23
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.