PHP Session Wrapper Class

Question

I recently worked on a system for the company I work for and would love some feedback on the following class for managing sessions. I don't claim to be a PHP guru but I like to think I gave it a try!

<?php
/*
* CLASS: Session
*
* @author Nathan Fitzgerald
*/

class Session {

    var $user;
    private $auth_key = SECURE_AUTH_KEY;
    const COOKIE_DOMAIN = '.mydomain.com';

    /*
     * INIT
    */  
    public function __construct() {

        if(!session_id()) session_start();
        $this->checkRememberCookie();

    }

    /*
     * Set Session Notice
     * @param $message (string)
     * @param $context (string)
    */      
    public function setSessionNotice($message, $context) {
        $_SESSION['bcqa_session_message'] = $message;
        $_SESSION['bcqa_session_message_context'] = $context;
    }

    /*
     * Get Session Notice
     * @return html 
    */          
    public function getSessionNotice() {

        global $core;
        if( !isset($_SESSION['bcqa_session_message_context']) || !isset($_SESSION['bcqa_session_message']) ) return;

        switch($_SESSION['bcqa_session_message_context']):
            case 'success':
                $icon = 'fa fa-check-circle';
            break;
            case 'danger':
                $icon = 'fa-exclamation-circle';
            break;
            case 'info':
                $icon = 'fa-info-circle';
            break;
            case 'warning':
                $icon = 'fa-exclamation-triangle';
            break;
        endswitch;

?>
    <div class="container<?php if($core->isAdmin()): ?>-fluid<?php endif; ?>">
        <div class="alert alert-dismissible alert-<?php echo $_SESSION['bcqa_session_message_context']; ?>" role="alert">
            <button type="button" class="close" data-dismiss="alert"><span aria-hidden="true">&times;</span><span class="sr-only">Close</span></button>
            <i class="fa <?php echo $icon; ?>"></i> <?php echo $_SESSION['bcqa_session_message']; ?>
        </div>
    </div>
<?php
        $this->destroySessionNotice();

    }

    /*
     * Destroy Session Notice
    */      
    public function destroySessionNotice() {
        unset($_SESSION['bcqa_session_message']);
        unset($_SESSION['bcqa_session_message_context']);
    }

    /*
     * Check if User has Checked Remember Me and Login Automatically
    */      
    public function checkRememberCookie() {

        global $mysql, $core;   

        if( !$this->isUserLoggedIn() ):

            if( isset($_COOKIE['bc_qandq_auth']) && $_COOKIE['bc_qandq_auth'] == md5($this->auth_key) && isset($_COOKIE['bc_qanda_user_id'])) {

                $user_q = $mysql->query('
                    SELECT * FROM bc_users 
                    WHERE user_id = "'.$_COOKIE['bc_qanda_user_id'].'"
                ');

                if(!$user_q->is_empty()) {

                    $user = $user_q->fetchObject();

                    $this->authenticate($user->username, $user->password);

                } else {
                    $this->logout();
                }

            }

        endif;

    }

    /*
     * Login
    */      
    public function authenticate($username, $password, $redirect = false, $remember = false) {

        global $mysql, $core;

        if( !BraveHelpers::isValidMD5($password) ) {
            $password = md5($password);
        }

        $user_q = $mysql->query('
            SELECT * FROM bc_users 
            WHERE username = "'.$username.'" 
            AND password = "'.$password.'"
        ');

        if(!$user_q->is_empty()) {
            $this->user = $user_q->fetchObject();
            unset($this->user->password);
            unset($this->user->key);

            // Do actual login...       
            $_SESSION['bc_qandq_auth'] = md5($this->auth_key);
            $_SESSION['user_id'] = $this->user->user_id;        
            $_SESSION['user_info'] = (array) $this->user;   

            if($remember) {

                // Set Cookies (7 days)
                setcookie('bc_qandq_auth', md5($this->auth_key), time()+3600*168, '/', self::COOKIE_DOMAIN);
                setcookie('bc_qanda_user_id', $this->user->user_id, time()+3600*168, '/', self::COOKIE_DOMAIN);

            }

            // Update Last Login & Login Count
            $mysql->update('bc_users', 
               array(
                  'date_lastlogin'  =>  BraveHelpers::date('Y-m-d H:i:s'),
                  'login_count'     =>  $this->user->login_count+1
               ),
               array(
                  'user_id' =>  $this->user->user_id
               )
            );      

            $this->setSessionNotice('You have been logged in successfully.', 'success');    

        } else {
            $this->setSessionNotice('<strong>ERROR:</strong> Invalid username or password combination.', 'danger');
        }   

        $return_url = $core->getSystemBaseUrl();

        if( $redirect ) {
            $return_url = $redirect;
        }

        if( $this->user->role == 'admin' && !$redirect) {
            $return_url = $core->getAdminUrl();
        }

        $this->_redirect($return_url);

    }

    /*
     * Logout
    */      
    public function logout() {

        // Destroy Session Variables
        unset($_SESSION['bc_qandq_auth']);
        unset($_SESSION['user_id']);
        unset($_SESSION['user_info']);

        // Destroy Cookies
        if( isset($_COOKIE['bc_qandq_auth']) && isset($_COOKIE['bc_qanda_user_id'])) {
            setcookie('bc_qandq_auth', 0, time()-3600*24, '/', self::COOKIE_DOMAIN);
            setcookie('bc_qanda_user_id', 0, time()-3600*24, '/', self::COOKIE_DOMAIN);
        }       

    }

    /*
     * Is the user logged in?
    */      
    public function isUserLoggedIn() {  
        if( isset($_SESSION['bc_qandq_auth']) && $_SESSION['bc_qandq_auth'] == md5($this->auth_key)  && isset($_SESSION['user_id']) ) {
            return true;
        }       
        return false;           
    }

    public function doAuthChecks() {
        global $core;

        if( !$this->isUserLoggedIn() ) {
            $this->setSessionNotice('You need to be logged in to view that page.', 'warning');
            $this->_redirect($core->getSystemBaseUrl().'?redirect_to='.$core->getCurrentUrl());
        }

    }

    /*
     * Redirect
    */  
    public function _redirect($url) {
        header('Location: '.$url);
        exit;
    }

    public function getLogoutUrl() {
        global $core;
        return $core->getSystemBaseUrl('logout');
    }

    public function getUserInfo($key) {
        return $_SESSION['user_info'][$key];
    }

    public function checkPermissions() {

        global $core;

        if( $this->getUserInfo('role') == 'admin' ) {
            return true;
        }

        $core->_dead('You do not have permission to view that page.', 'error');

    }

}

?>

Answer 1

Security

This code doesn't look secure at all.

SQL injection

SELECT * FROM bc_users 
 WHERE user_id = "'.$_COOKIE['bc_qanda_user_id'].'"

As anyone can set the cookie value to what they want, this is open to SQL injection (from valid users only, but still).

    $user_q = $mysql->query('
        SELECT * FROM bc_users 
        WHERE username = "'.$username.'" 
        AND password = "'.$password.'"
    ');

Here, again, you are open to SQL injection.

You can (and should) use prepared statements to fix this (don't just use input escaping via mysql_real_escape_string).

Hashing

md5 is not secure. use bcrypt or equivalent. Also, use salts (random for each user, not just site salts; although those would be better than none at all).

$auth_key and logging in as another user

What's $auth_key? Is this just a hardcoded value that is the same for all users?

In that case it seems to me that any valid user can login as any user they desire (as long as they know the corresponding bc_qanda_user_id). This is not good.

Single Responsibility

A class should have one responsibility. Yours has a lot more. It doesn't just manage sessions as the name suggest, it also handles cookies, logging in and out, outputting HTML, redirecting, and role management.

This is way too much. Each of those functionalities should have its own class.

The same is true for your functions. authenticate for example doesn't just authenticate either, it also manages cookies and sessions, updates the database, and redirects.

Other

Just a quick list of things that jumped out at me (I think the main concern right now should be the security issues and the general structure of the code):

• you use global quite a lot. This seems like a code smell
• you mix HTML and PHP, which I would try to avoid
• you use newlines a bit too much
• if (boolean) return true; else return false; can be simplified to return boolean

Answer 2

I think it is imperative that the security of your code be addressed in detail, as I can see quite a few things that need desperately to be changed. I'll see what I can do about going through each one, and for now they are just in an order of which I've noticed them!

1. The first security issue I see is the size and scope of your class. This may sound irrelevant, but the complexity of your code will indirectly affect how secure your code is. This isn't the case each time, but as a beginner, I suggest the SOLID principles.

   Keeping your classes small, modular, and encompassing only a single responsibility will help keep your mind focused and you will find it much easier to write secure code. If you have ten functions, only somewhat related, and each has a different security protocol, it is almost guaranteed you will get confused. Allow yourself a simple, efficient, system to effectively and properly lock down your application. I see a lot of repetition of md5() and lots of session setting, un-setting, and validating. KISS!

2. Again, I see md5 used in several instances. The first instance I see is md5($this->auth_key). What's $auth_key though? Well it looks like it's pointing the the global constant SECURE_AUTH_KEY. So, based on this information, I can assume you don't want thing variable being compromised. Yet, it alreayd is. It's in a cookie, which does not make any sense at all, but we'll get back to that.

   So, that key is really not a key at all. A key is for encryption, and it determines the ciphertext. What you have is a password. It's hard to tell why you have used md5, becuase having a hash here did not add any security to your application. The problem is that every user has the same hashed session value, which can easily be duplicated, and then bam! your log in is comprimised! Sorry :/

   What can you do? Unfortunately, what you have is poorly structured, and so anything I suggest will just confuse everyone! Even trying to implement either bcrypt, scrypt, or PBKDF2 would simply make things over the top complicated and I think it would be unwise to dive into that at your level of expertise. I will suggest you don't do this, and instead, take a look at this, this, and this. I highly recommend all three.

3. What you store in cookies and sessions should be assumed to be compromised. Having an SSL connection could protect you from man-in-the-middle attacks, but even then you are not guaranteed perfect security. Here is a recommended read about session security.
4. You currently are susceptible to SQL injections. I recommend combining Defense Option 1 and 2 from OWASP to protect you. I wrote a good summary of SQL injection protection with a focus on PHP here, which I also suggest you read.

I'm sure there's more that could be determined on a security standpoint, but that should get you started in a sucess bound direction! I wouldn't provide a link to you if I didn't think it would benefit, so I highly suggest you understand the contents behind each one of the links I've supplied.