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 Feb 15 '14 at 21:17
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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.