5
\$\begingroup\$

This is a user login (some session wrapper I managed to put together after a lot of web searching).

It's for a simple CMS I'm trying to build. It only needs one user and there is no need for multiple user log in at the same time. It works, the problem being I think it has safety issues.

It works like this:

  1. The user accesses CMS index page, and inserts the user and password.
  2. If one of them does not coincide with the data in the database it does not set the session and triggers a "die" or a error message.
  3. If the info is ok then it sets the session and redirects the user from the index page to an admin.php page as you can see in the code below.

I commented it quite well. If you see any safety issues please point them out as I can't seem to notice them.

The form is a simple user, password and submit form.

The class:

      <?php
      class Session{

    private static $_user;
    private static $_password;

    private static $_sessionStart = false;

    //here we check if the data inserted by the user in the form coincides with the rows in the db
    //if it does it sets the session for the user
    public static function CheckLog($received_user,$received_password){
        self::$_user            = $received_user;
        self::$_password        = $received_password;

        $db = Database::getInstance();
        $mysqli = $db->getConnection();

        $sql    = 'SELECT * FROM utilizatori '; 
        $sql   .= 'WHERE user = "'.self::$_user.'" AND password = "'.self::$_password.'" ';
        $result = $mysqli->query($sql) or die (mysqli_error());
        if($row = $result->fetch_assoc()){
            if(($row['user'] === self::$_user)&&($row['password'] === self::$_password)){
                self::set('user',self::$_user);
                self::set('key',session_id());
            }
        }



      }
    //this method starts the session
    public static function start(){
       if(self::$_sessionStart == false){   
         session_start();
         self::$_sessionStart = true;
       }
    }
    //this function sets the session values
    public static function set($key,$value){
        $_SESSION[$key] = $value;
    }
    public static function get($key){
        if(isset($_SESSION[$key])){
            return $_SESSION[$key];
        }
        else{
            return false;
        }    
    }
    }


//from here is the handeling of the data in the "index" page:


        include("class_def/database.inc");
        include("class_def/user.inc");

        Session::start(); //here we start the session
        $up = strip_tags($_POST['user']); //post the user name
        $pp = strip_tags($_POST['password']);     //post the password
        //check if the variables are set and access the CheckLog method
       if((isset($up))&&(isset($pp))){
           Session::CheckLog($up,$pp);
       }
       //get the name of the current file
       $current_file = basename($_SERVER["SCRIPT_FILENAME"], '.php');

       //if the current page is index and the key is set redirect to admin.php
       if(isset($_SESSION['key'])){
           if($current_file == 'index')
      { echo '
        <script type="text/javascript">
            <!--
              window.location = "admin.php"
            //-->
            </script>';
      }
       }
       else{ //if the key is not set and the page is any other than index die and display message
           if($current_file != 'index'){
          die('You must log in first');
      }
          else{ // if the page is index echo out an error message
              $error_msj = 'Wrong user or pass';
          }
       }
    ?>
\$\endgroup\$

1 Answer 1

Reset to default
6
\$\begingroup\$

MVC

If you're trying to make a CMS, then there's a possibility that this CMS will become bigger than you first though. More pages, more modules, more code. You might want to consider doing an MVP approach to splitting code (rather than MVC).

A good reference framework for this is CodeIgniter. Presenter (CodeIgninter calls it the Controller) receives a request, then operates the data on the model (the model layer + database), then grabs a view (A template, your HTML) and renders it ("echo" the template).

SQL Injection

Then I notice this part:

$sql    = 'SELECT * FROM utilizatori '; 
$sql   .= 'WHERE user = "'.self::$_user.'" AND password = "'.self::$_password.'" ';

You should read more about SQL injection. In a gist, it allows users to run arbitrary SQL, even be able to fake logins, see through existing usernames and passwords, as well as drop entire databases.

Sessions

Then there's sessions. For simple situations, that's fine. But for security, there's a lot to consider. If I can shoot malicious code through the forms, then I just might be able to fake my session. Read on this post for things to consider with sessions.

Validation

I see you use strip_tags() but it only does what it does, strip tags. It does not strip the other stuff, like backticks and quotes which can still be used to break in. A good example is the story of Little Bobby Tables. No tags, still broke through the database.

Redirect

window.location = "admin.php"

This operates on the browser. This means that index.php is actually served before redirecting, possibly rendering the page before this runs. I suggest you do a header redirect. It does not serve the page to the browser. It just tells the browser directly to load another page instead.

\$\endgroup\$
0

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.