14
\$\begingroup\$

I'm writing my first CMS (for personal use) and I'm trying to make it the most secure I can. This is my project structure:

.
+-- css
|   +-- images
|   +-- style.css
+-- includes
|   +-- config.php
|   +-- connection.php
|   +-- footer.php
|   +-- header.php
|   +-- session.php
|   +-- user.php
+-- dashboard.php
+-- login.php
+-- logout.php

I'm gonna write here what I have in each file:

config.php

<?php
    define('DB_HOST', 'localhost');
    define('DB_USER', 'root');
    define('DB_PASS', '');
    define('DB_NAME', 'cms');
?>

connection.php

<?php

include 'config.php';

class Connection{
    public function openConnection(){
        try{
            $db = new PDO('mysql:host='.DB_HOST.';dbname='.DB_NAME, DB_USER, DB_PASS);
            return $db;
        }catch (PDOException $e){
            return 'Database Error.';
        }
    }
}

?>

user.php

<?php

class User{

    private $userData;

    public function createUser($userId){
        $connection = new Connection;
        $db = $connection->openConnection();

        $stmt = $db->query("SELECT * FROM users WHERE user_id = '$userId'");

        $results = $stmt->fetch();
        $this->userData = $results;
    }

    public function getId(){
        return $this->userData['user_id'];
    }

    public function getUsername(){
        return $this->userData['user_username'];
    }
}

?>

session.php

<?php

include "includes/user.php";

class Session{
    public function createSession(){
        session_start();
    }

    public function logUser($userId){
        $_SESSION['userId'] = $userId;

        $selector = base64_encode(random_bytes(8));
        $token = bin2hex(random_bytes(32));

        $cookieValue = $selector.':'.base64_encode($token);
        $hashedToken = hash('sha256', $token);

        $timestamp = time() + (86400 * 14);

        setcookie('authToken', $cookieValue, $timestamp, NULL, NULL, NULL, true);

        $connection = new Connection;
        $db = $connection->openConnection();

        $stmt = $db->query("INSERT INTO logins (login_selector, login_token, login_userId, login_expires) VALUES ('$selector', '$hashedToken', '$userId', '$timestamp')");
    }

    public function relogUser($userId){
        $_SESSION['userId'] = $userId;
    }

    public function isLogged(){
        if(isset($_SESSION['userId'])){
            return true;
        }else{
            if(isset($_COOKIE['authToken'])){
                $connection = new Connection;
                $db = $connection->openConnection();

                list($selector, $token) = explode(':', $_COOKIE['authToken']);

                $stmt = $db->prepare('SELECT * FROM logins WHERE login_selector = :login_selector');
                $stmt->bindValue(':login_selector', $selector);

                $stmt->execute();
                $results = $stmt->fetch();

                if($results){
                    if(hash_equals($results['login_token'], hash('sha256', base64_decode($token)))){
                        $this->relogUser($results['login_userId']);
                    }else{
                        $this->logOut();
                        return false;
                    }
                }else{
                    return false;
                }
            }else{
                return false;
            }
        }
    }

    public function logOut(){
        $connection = new Connection;
        $db = $connection->openConnection();

        list($selector, $token) = explode(':', $_COOKIE['authToken']);

        $stmt = $db->prepare('DELETE FROM logins WHERE login_selector = :login_selector');
        $stmt->bindValue(':login_selector', $selector);

        $stmt->execute();

        $stmt = $db->prepare('DELETE FROM logins WHERE login_userId = :login_userId');
        $stmt->bindValue(':login_userId', $_SESSION['userId']);

        $stmt->execute();

        unset($_SESSION['userId']);
        setcookie('authToken', '', 1);
        unset($_COOKIE['authToken']);
    }

    public function getId(){
        return $_SESSION['userId'];
    }
}

?>

login.php

<?php
    include "includes/header.php";
    include "includes/connection.php";
    include "includes/session.php"; 

    $session = new Session;
    $session->createSession();

    if($session->isLogged()){
        header('Location: dashboard.php');
        exit();
    }

    $connection = new Connection;
    $db = $connection->openConnection();

    if(isset($_POST['username']) && isset($_POST['password'])){
        $username = $_POST['username'];
        $password = $_POST['password'];

        if(strlen($username) > 0 && strlen($password) > 0){
            $stmt = $db->prepare('SELECT * FROM users WHERE user_username = :username');
            $stmt->bindValue(':username', $username);

            $stmt->execute();
            $results = $stmt->fetch();

            if($results){
                $verify = password_verify($password, $results['user_password']);
                if($verify){
                    $session->logUser($results['user_id']);
                    header('Location: dashboard.php');
                }else{
                    //handle wrong password
                }
            }else{
                //handle no user found
            }
        }
    }
?>

<div class="wrapper">
    <div class="login">
        <form method="post" action="">
            <input type="text" name="username" placeholder="Username">
            <input type="password" name="password" placeholder="Password">

            <button>Log In</button>
        </form>
    </div>
</div>




<?php
    include "includes/footer.php";
?>

dashboard.php

<?php
    include "includes/header.php";
    include "includes/connection.php";
    include "includes/session.php";

    $session = new Session;
    $session->createSession();

    if(!$session->isLogged()){
        header('Location: login.php');
        exit();
    }

    $user = new User;
    $user->createUser($_SESSION['userId']);
?>

<div class="wrapper">
    <p>Secret page, testing!</p>
    <a href="logout.php">Log out</a>
</div>


<?php
    include "includes/footer.php";
?>

Ok, so a few points and things to clear:

  • The header.php and footer.php is just the HTML body, head tags, etc.
  • The users will be admins always, no roles whatsoever.
  • It currently has nothing, just the login system so that's the focus point.

Having said that I want to point out that I have two major concerns about this.


1) The first one is the architecture of the code, the classes, etc.

I feel like there's a lot of unnecessary bits and inconsistencies in the code but I can't figure out a way to tidy it up.

When I first started to make it, I didn't use classes and didn't separate session and user files from the connection code so it was even worse than now but at least I'm progressing.

I want to use the MVC pattern to make it flexible, tidy, and easy to mantain.

What changes can I make to this?


2) The second one is the security of the code.

I'm striving to make this as secure as I can so I'm taking care of all the problems it could have but I'm not sure that I'm doing it right.

Right now I'm trying to prevent:

  • SQL Injection, for that I'm using PDO with prepared statements.
  • XSS I don't have that problem yet because the "MANAGEMENT" part isn't done yet but I have an idea of how to prevent it.

Also, I'm storing the password as hashes, and I'm using a persistent login technique I read from a blog, which uses a validator or token and a selector to prevent from:

  • An attacker hijacking the cookie.
  • Timing leaks (using hash_equals).
  • User impersonation (if somehow an attacker manages to get the databases).

Is there anything I should change to make this more secure?

\$\endgroup\$
  • 1
    \$\begingroup\$ Completely aside from looking at your code, logging into your DB server as root is a bad idea. You should have an account that only has access to the database it needs. \$\endgroup\$ – Gerrit0 Sep 11 '17 at 4:30
  • \$\begingroup\$ Yeah I know, this is just for testing in my PC (localhost) \$\endgroup\$ – nick Sep 11 '17 at 5:17
10
\$\begingroup\$

Ok, moving down the list...

  • I would separate the content of the includes folder into 2 semantically named folders: includes and classes - or if you want to eventually turn this into an MVC, maybe views and classes would be more appropriate.
  • It's not uncommon to put database credentials in constants, but truth is you should never have more than one connection to the database at a time, so if you're writing proper code, constants are not the way to go as PHP won't garbage collect them. I would suggest using an ini file and parsing it into an array with parse_ini_file.
  • You Connection class is abusing OOP.. Your class is not constructing anything. It's a single static method that you didn't even bother to mark as static and which you're constructing with new. Use it as a static method.. public static function openConnection() then instead of using new to construct a class that has no constructor you can just do $db = Connection::openConnection()
  • Don't return a string if your connection fails. It's just going to throw an error later attempting to call prepare method on a non-object or something like that. Just let PDO throw an error in your openConnection method. Don't bother catching the error unless you're going to try and fix it. By the way, don't you have to set the error mode with PDO::setAttribute to squeeze error messages out of PDO anyway. As is, I don't think your catch will ever be called.
  • Your User class should be constructed in a constructor. Rename createUser to __construct, then instead of doing $user = new User(); $user->createUser($id); you can knock it out in one line: $user = new User($id); I feel like you should read this.
  • Why are you storing all User properties in a single associative array? This is another example of how not to use OOP. If you were married to OOP you'd be arrested for spousal abuse. Each index in that array should be its own property.

There are a few other things that I could point out but they wouldn't really benefit you much until you get a slightly better grasp on OOP. You're doing great so far, don't give up, but read a little bit about OOP in PHP because you're missing some of the base concepts here.

\$\endgroup\$
  • \$\begingroup\$ Hi, first of all thanks for your response, very informative and I had a good laugh reading it! I've seen a lot of people using a Views, Model and Controller class. Is this preferable? I agree with the __construct, I know how to so I don't really know why I did the createUser shit but oh well. What do you mean storing all properties in a single associate array? "Each index in that array should be it's own property." Like $user->user_id, $user->user_username etc? Thanks again! \$\endgroup\$ – nick Sep 11 '17 at 4:00
  • \$\begingroup\$ @nick - most mvc systems will have a base model and controller class and then have each of their models, controllers extend those classes. views don't really need a class, they're usually just html pages or snippet that are dynamically inserted. you should fuck around with codeigniter a bit, you'll get a good grasp on mvc and once you do implementing your own system is more or less trivial. you gotta master oop first though. your last sentence: yes, that's exactly what i mean. :) \$\endgroup\$ – I wrestled a bear once. Sep 11 '17 at 5:00
  • 2
    \$\begingroup\$ I'm currently binge watching MVC OOP PHP videos on youtube and it's like a whole new world to me! Thanks for your tips dude, I'll look into codeigniter next! \$\endgroup\$ – nick Sep 11 '17 at 5:20
4
\$\begingroup\$

As you are already using PDO, you should use prepare/execute logic to apply values to SQL statements. Currently they can be subject to an SQL injection.

Example:

Instead of

$stmt = $db->query("SELECT * FROM users WHERE user_id = '$userId'");

You should use this:

$stmt = $db->prepare("SELECT * FROM users WHERE user_id = ?");
$result = $stmt->execute([$UserId]);

Alternatively, use object-oriented query builders (Zend, Illuminate, etc), where it would look like this:

$result = DB::Select('*')->from('users')->where('user_id', $UserId);
\$\endgroup\$
  • \$\begingroup\$ Thanks, just one question in your last example, is it preparing the statement like when using PDO or not? \$\endgroup\$ – nick Sep 11 '17 at 16:39
  • \$\begingroup\$ The last example is an illustration of the principle, you cannot use it directly. Just have a look at Zend Framework or Laravel (and many more others) and choose one you prefer. And yes, most query builders are based on PDO, so they do prepare before execute. \$\endgroup\$ – TimSparrow Sep 11 '17 at 16:44
  • \$\begingroup\$ Ok that's good to know, thanks for the tips! \$\endgroup\$ – nick Sep 11 '17 at 18:03
4
\$\begingroup\$

You must protect against SQL injections. You said you know about that topic, so why are you using interpolated SQL statements at all? An SQL statement in the source code must never contain a ${var}, only :var and bound parameters.

Having some random bytes as the auth token is enough. There's no point in hashing random bytes.

The isLogged function must return something in every possible branch. One branch is missing a return true. You should not use nested if-else in that function, but rather just a sequence of if-return blocks.

\$\endgroup\$
  • \$\begingroup\$ Ok thank for your tips, can you elaborate on why random bytes as the auth token is enough and there's no point in hashing? Stack overflow guide says: DO NOT STORE THE PERSISTENT LOGIN COOKIE (TOKEN) IN YOUR DATABASE, ONLY A HASH OF IT! The login token is Password Equivalent, so if an attacker got their hands on your database, they could use the tokens to log in to any account, just as if they were cleartext login-password combinations. Therefore, use hashing (a weak hash will do just fine for this purpose) when storing persistent login tokens. \$\endgroup\$ – nick Sep 11 '17 at 16:38
  • \$\begingroup\$ @nick Thanks for the comment, I was wrong. I didn't read the code as carefully as I should have done. \$\endgroup\$ – Roland Illig Sep 11 '17 at 16:48

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.