2
\$\begingroup\$

I'm trying to implement a system of restricted access. Right now I'm focusing on the "session fixation". I am concerned about the following aspects:

  1. Control of a "fingerprint" of the user created by mixing UserAgent, IPAddress and a salt key. If the fingerprint were to change, destroy the user's session.

  2. Using the same session id for a certain number of times, after which regeneration session id.

  3. Regenerate the session id every time change the level of user authentication

What I want to know is whether my approach is right and if it is safe enough. I apologize if the code is long, I tried to remove all the unnecessary parts.

I do not want a code review, but the opinions about it ... particularly if the mine is a safe approach.


<?php class oLogonUser { const SESSION_LIMIT = 10; const SESSION_SALT = 'AS86F(,sa)8as7d+/234N&&"$£%'; function __construct() { // init session $this->InitSession(); // check if user logged if( $this->isUserLogged() ) { // Check if finger print is valid if( !$this->isValidFingerPrint() ) $this->LogOutUser(); // log out } } private function InitSession() { // check if session already start if( session_id() == '' ) session_start(); // Set user's fingerprint $this->setUserFingerPrint(); // Session counter if( isset($_SESSION['UserSessionCounter']) ) $_SESSION['UserSessionCounter'] = (int)$_SESSION['UserSessionCounter'] + 1; else $_SESSION['UserSessionCounter'] = 0; // if session counter exeed limit, regenerate session id if( $_SESSION['UserSessionCounter'] > self::SESSION_LIMIT ) { $_SESSION['UserSessionCounter'] = 0; session_regenerate_id(); } } private function isValidFingerPrint() { // checking if the user fingerprint is the same as that stored in session if( isset($_SESSION['UserFingerPrint']) ) return ( $_SESSION['UserFingerPrint'] === $this->getUserFingerPrint() ); return false; } private function getUserFingerPrint() { // return user fingerprint return md5($_SERVER['REMOTE_ADDR'] . $_SERVER['HTTP_USER_AGENT'] . self::SESSION_SALT ); } private function setUserFingerPrint() { // store in session user fingerprint if( !isset($_SESSION['UserFingerPrint']) ) $_SESSION['UserFingerPrint'] = $this->getUserFingerPrint(); } public function LogOnUser($UserName, $Password) { //NB: in fact control the credentials in the database if( $UserName == 'test' && $Password == 'test' ) $this->setUserLogged(); } public function LogOutUser($UserName, $Password) { // destroy user session session_destroy(); session_regenerate_id(); } private function setUserLogged() { session_regenerate_id(); // set user logged $_SESSION['UserLogged'] = TRUE; } public function isUserLogged() { // check if user is logged return (isset($_SESSION['UserLogged']) && $_SESSION['UserLogged'] == TRUE ); } } // Test it $objLogonUser = new oLogonUser(); if( !$objLogonUser->isUserLogged() ) $objLogonUser->LogOnUser('test', 'test'); echo session_id() . '<BR />'; print_r($_SESSION); 
\$\endgroup\$
1
  • 1
    \$\begingroup\$Welcome! On this site, reviewers are allowed to comment on any aspect of the code. While it is still good to have your primary request addressed, code reviews can still be done.\$\endgroup\$
    – Jamal
    CommentedFeb 15, 2014 at 21:17

1 Answer 1

2
\$\begingroup\$

Preferably, you would generate a random salt. Add a function to generate random salt, then be sure to call it in your constructor. If a user discovers you salt, that is a major security hole right now.

In my opinion, it would be best to separate this class into a separate file for easier use in multiple situations. Also, a separate file with useful functions like generating salt could be useful.

Your code is relatively secure, but you need a random salt generator. Otherwise, your code looks good.

\$\endgroup\$
0

    Start asking to get answers

    Find the answer to your question by asking.

    Ask question

    Explore related questions

    See similar questions with these tags.