Page 1 of 1

Can you tear my code apart?

Posted: Thu Apr 03, 2008 11:06 pm
by flying_circus
I'm working on creating a session handler. It's obviously not complete with plenty left to do, but this basic functionality seems to work well.

My questions are more in line with: Is this the preferred "industry standard" way of doing it? Am I missing key components?

Looking to be steered on the right track or confirmed that I am on the right track and this is a reasonable start.

A couple of notes.

- Code requires cookies. cookies disabled = no access (they are to be leveraged for logon security).

- Sessions in the DB are checked against the user agent and user IP. I would like to explore the option of doing a WHOIS to verify the returning user is from the same provider.

- Session ID's are regenerated within 10 minutes of creation.

Code: Select all

<?php
  define("SESSION_NAME","PHPSESSID");
  define("SESSION_TIMEOUT","600");   # 10 Minutes
  define("SESSION_LIFESPAN","3600"); # 1 Hour
 
  class Session{
    private $dbServer;
    private $php_cookie_id;
    private $php_session_id;
    private $php_session_index;
    private $logged_in;
    private $user_id;
    private $user_agent;
    private $user_address;
    private $session_regen = false;
    private $session_timeout = SESSION_TIMEOUT;
    private $session_lifespan = SESSION_LIFESPAN;
    
    private function _session_open_method($save_path, $session_name) {
      return true;
    }
    
    private function _session_close_method() {
      return true;
    }
    
    private function _session_read_method($id) {
    # Determine if this is a returning / traversing user  
      if(isset($_COOKIE[SESSION_NAME])) {
      # Security and age check
        $this->php_cookie_id = $_COOKIE[SESSION_NAME];
        
      # Check for Session Existence and Expiration
        $result = $this->dbServer->mysqlQuery("SELECT session_index, session_id, last_impression, created, logged_in, user_id, user_address, user_secret FROM user_session WHERE session_id = '$this->php_cookie_id' AND user_agent='$this->user_agent' AND ((UNIX_TIMESTAMP(now()) - UNIX_TIMESTAMP(last_impression)) < '$this->session_lifespan')");
        if ($this->dbServer->mysqlNumRows($result) == 1) {
        # Session Exists, get session details
          $arResult = $this->dbServer->mysqlFetchArray($result);
          
        # Check Users IP Address against IP Address of Stored Session
          if($this->_validateUserAddress($arResult['user_address'])) {
            $this->php_session_id = $arResult['session_id'];
            $this->php_session_index = $arResult['session_index'];
            
            if((strtotime(now) - strtotime($arResult['last_impression'])) > $this->session_timeout) {
            # Session OK \ Login has timed out (Log out User)
              if($arResult['logged_in']) {
                $this->dbServer->mysqlQuery("UPDATE user_session SET logged_in='0', user_id='0' WHERE session_index = '$this->php_session_index' AND user_agent='$this->user_agent'");
                $this->logged_in = false;
              }
            } else {
            # Session OK \ Login OK
              if($arResult['logged_in']) {
                if($this->_validateSecret($arResults['user_secret'])) {
                  $this->logged_in = true;
                  $this->user_id = $arResult['user_id'];
                } else {
                  $this->dbServer->mysqlQuery("UPDATE user_session SET logged_in='0', user_id='0' WHERE session_index = '$this->php_session_index' AND user_agent='$this->user_agent");
                  $this->logged_in = false;
                }
              }
            }
            
          # If it is later than the session timeout but sooner than the session lifetime, regenerate a new session ID.
            if((strtotime(now) - strtotime($arResult['created'])) > $this->session_timeout) $this->session_regen = true;
            
          # Update Session Impress Timestamp to now()
            $this->Impress();
          }
        } else {
        # Delete from database - Do garbage cleanup at the same time
          $this->_session_gc_method($this->php_cookie_id);
          
        # Get rid of this old cookie
          unset($_COOKIE[SESSION_NAME]);
        
        # We need to create a new entry in the database
          if($this->dbServer->mysqlQuery("INSERT INTO user_session (session_id, logged_in, user_id, created, user_agent, user_address) VALUES ('$id',0,0,now(),'$this->user_agent','$this->user_address')")) {
            $this->php_session_index = $this->dbServer->mysqlInsertId();
            $this->php_session_id = $id;
          }
        }
      } else {
      # This is a Brand New Session!
        if($this->dbServer->mysqlQuery("INSERT INTO user_session (session_id, logged_in, user_id, created, user_agent, user_address) VALUES ('$id',0,0,now(),'$this->user_agent','$this->user_address')")) {
          $this->php_session_index = $this->dbServer->mysqlInsertId();
          $this->php_session_id = $id;
        }
      }
      
    # Just return empty string
      return("");
    }
    
    private function _session_write_method($id, $sess_data) {
      return(true);
    }
    
    private function _session_destroy_method($id) {
      $result = $this->dbServer->mysqlQuery("DELETE FROM user_session WHERE session_id = '$id'");
      return($result);
    }
    
    private function _session_gc_method($session_id) {
      # Garbage collection for expired sessions
        if(isset($session_id)) {
          $this->dbServer->mysqlQuery("DELETE FROM user_session WHERE (session_id = '$session_id') OR (UNIX_TIMESTAMP(now()) - UNIX_TIMESTAMP(last_impression)) > '$this->session_lifespan'");
        } else {
          $this->dbServer->mysqlQuery("DELETE FROM user_session WHERE (UNIX_TIMESTAMP(now()) - UNIX_TIMESTAMP(last_impression)) > '$this->session_lifespan");
        }
      # Clean up expired session variables
        $this->dbServer->mysqlQuery("DELETE FROM session_variable WHERE session_index NOT IN (SELECT session_index FROM user_session)");
      return(true);
    }
    
    private function _sessionRegenId($regen_id) {
    # Create a local copy of old session index
      $old_php_session_index = $this->php_session_index;
      
    # Convert the old session to the newly regenerated one.
      if($this->dbServer->mysqlQuery("INSERT INTO user_session (session_id, logged_in, user_id, created, user_agent, user_address) VALUES ('$regen_id',0,0,now(),'$this->user_agent','$this->user_address')")) {
        $this->php_session_index = $this->dbServer->mysqlInsertId();
        $this->php_session_id = $regen_id;
        
        $this->dbServer->mysqlQuery("UPDATE session_variable SET session_index = '$this->php_session_index' WHERE session_index = '$old_php_session_index'");
      }
      
    # Update Session Cookie
      setcookie(SESSION_NAME,$this->php_session_id,0,'/',HTTP_ROOT,0,1);
    }
    
    private function _validateSecret($secret) {
      // To Do: Implement User Secret Checking
      // This is stored in a 2nd cookie issued upon sucessful login
      return true;
    }
    
    private function _validateUserAddress($user_address) {
      // To Do: Implement User IP Checking - Reference ARIN, RIPE, LACNIC, APNIC, AfriNIC
      // It is likely that this will not change by more than it's last 2 octets
      
      if($this->user_address == $user_address) {
      # Address Match!
        return true;
      } else if(substr($this->user_address,0,7) == substr($user_address,0,7)) {
      # Still within Class B Address
        return true;
      } else {
        // Redirect to an error screen
        // IP Address Wildly Different
        return false;
      }
    }
    
    private function _check_cookies_enabled() {
      if(!setcookie('TestCookie','Test',0,'/',HTTP_ROOT,0,1)) {
        # User has disabled cookies on their browser.
        # Redirect them to an error page, do not proceed.
        header('location:' . HTTP_ROOT . 'error_cookies.php');
      }
      return true;
    }
    
    private function _begin_session($dbServer) {
    # Reference Database Server
      $this->dbServer = &$dbServer;
    
    # Check that cookies are enabled on the client
      if($this->_check_cookies_enabled()) {
      
      # Get User Agent
        $this->user_agent = $_SERVER["HTTP_USER_AGENT"];
      
      # Get User IP Address
        $this->user_address = $_SERVER["REMOTE_ADDR"];
        
      # Setup Session Handler
        session_set_save_handler(
          array(&$this, '_session_open_method'), 
          array(&$this, '_session_close_method'), 
          array(&$this, '_session_read_method'), 
          array(&$this, '_session_write_method'), 
          array(&$this, '_session_destroy_method'), 
          array(&$this, '_session_gc_method')
        );
          
      # Begin Session
        session_start();
      
      # Regen Session ID if timeout from creation exceeded
        if($this->session_regen) {
          if(session_regenerate_id(true)) {
            $this->_sessionRegenId(session_id());
          }
          session_start();
        }
      
      # Set Session Cookie
        setcookie(SESSION_NAME,$this->php_session_id,0,'/',HTTP_ROOT,0,1);
      }
    }
    
    public function __construct($dbServer) {
    # Begin the Session
      $this->_begin_session($dbServer);
    }
    
    public function Impress() {
      if($this->php_session_index) {
        $this->dbServer->mysqlQuery("UPDATE user_session SET last_impression = now() WHERE session_index = '$this->php_session_index'");
      }
    }
    
    public function IsLoggedIn() {
      return($this->logged_in);
    }
    
    public function GetUserID() {
      if ($this->logged_in) {
        return($this->user_id);
      } else {
        return(false);
      }
    }
    
    public function GetSessionIdentifier() {
      return($this->php_session_id);
    }
    
    public function GetSessionIndex() {
      return($this->php_session_index);
    }
    
    public function __get($nm) {
      $result = $this->dbServer->mysqlQuery("SELECT variable_value FROM session_variable WHERE session_index = '$this->php_session_index' AND variable_name = '$nm'");
      if ($this->dbServer->mysqlNumRows($result) == 1) {
        $row = $this->dbServer->mysqlFetchArray($result);
        return(unserialize($row["variable_value"]));
      } else {
        return(false);
      }
    }
    
 
    public function __set($nm, $val) {
      if(!$this->__get($nm)) {  # Session Variable doesn't exist: Create it
        $strSer = serialize($val);
        $this->dbServer->mysqlQuery("INSERT INTO session_variable (session_inddex, variable_name, variable_value) VALUES ('$this->php_session_index', '$nm', '$strSer')");
      } else {  # Session Variable exists: Update it
        $strSer = serialize($val);
        $this->dbServer->mysqlQuery("UPDATE session_variable SET variable_value = '$strSer' WHERE session_index = '$this->php_session_index' AND variable_name = '$nm'");
      }
    }
  }
?>

Re: Can you tear my code apart?

Posted: Fri Apr 04, 2008 2:21 am
by arjan.top
Think about putting database session handling into another class

EDIT: and the validation

Re: Can you tear my code apart?

Posted: Mon Apr 07, 2008 4:54 am
by Mordred
SQL injection all over.

Re: Can you tear my code apart?

Posted: Mon Apr 07, 2008 3:12 pm
by flying_circus
Mordred wrote:SQL injection all over.

Mordred, I am all ears.

I was of the opinion that since the queries are in private functions using private variables, SQL injection was not a realistic threat. Some of the private variables are assigned from $_SERVER variables, can you not trust these? I'd appreciate your input and/or guidance.

To be completely honest, I have not modified the __get and __set functions, I took them from an example script in a book I purchased.

Re: Can you tear my code apart?

Posted: Mon Apr 07, 2008 3:36 pm
by Mordred
Many things in $_SERVER come from the user/attacker. HTTP_* are the request HTTP headers which can be malicious.
You should escape everything that goes in SQL queries for two reasons:
1. Today you know the assumption that this data comes from a "trusted" source, but tomorrow this may change and you'll forget the assumption and you'll forget to escape it
2. (truer/better reason) Escaping is not done to prevent SQL injection, it is done to keep the data entering the query intact (SQL injection is just the most notorious and tragic consequence of failing to do so)