2
\$\begingroup\$

Just wanted to run through my PHP session handling and get some feedback and tips with regard to what is good (if anything!), what could be better and what is either plain wrong, or using out of date methods.

Just as a bit of background, I have only ever dabbled with PHP (and many years ago that was) and want to try and embrace the OOP side of it.

Every page that is required to be part of a session has the following at the top:

<?php
include_once 'Session.php';

$session = new Session();
$session->authenticate();
?>

Session.php is as follows: 

class Session {
    const password = "5baa61e4c9b93f3f0682250b6cf8331b7ee68fd8";
    private $sessionInfo;

    public function __construct() {
        session_start();
        if (!isset($_SESSION['sessionInfo'])) {
            $sessionInfo = new SessionInfo();
            $_SESSION['sessionInfo'] = $sessionInfo;
        } else {
            $sessionInfo = $_SESSION['sessionInfo'];
        }
    }

    public function authenticate() {
        if (isset($_SESSION['loggedin'])) {
            if ($_SESSION['loggedin'] === true) {
                return;
            }
        }
        header('Location: login.php');
    }

    public function logIn($password) {
        if (sha1($password) === self::password) {
            $_SESSION['loggedin'] = true;
            header('Location: index.php');
        }
    }

    public function logOut() {
        unset($_SESSION['loggedin']);
        session_destroy();
        header('Location: login.php');
    }

    public function getSessionInfo() {
        return $this->sessionInfo;
    }
}

?>

SessionInfo is currently just a blank class, it's intended purpose is to act as an information store for the current session.

login.php:

<?php
include_once 'Session.php';
    if(isset($_POST['submit'])) {
        if(isset($_POST['password'])) {
            $session = new Session();
            $session->logIn($_POST['password']);
        }

    }
?>
<html>
    <body>
        <form method="post" action="<?php echo $_SERVER['PHP_SELF'];?>">
            <input type="password" name="password" />
            <input type="submit" name="submit" />
        </form>
    </body>
</html>

Do I need to call die() (or exit() seeing as they are identical) after a failed authenticate to terminate any page which may attempt to load? Or do I need to return a boolean and then terminate from the calling page?

Please disregard the use of SHA1 and the hashed password in the file, I will be extending this to use a MySQL background, and hope to incorporate the use of a salted hash. I will be using prepared statements, will this be sufficient to protect from SQL injection and allow me to not bother filtering $_POST[...]?

Also, I realise that the location headers may better be stored as a class constant in case I wish to change the locations.

\$\endgroup\$
4
  • \$\begingroup\$ Question: is 5baa61e4c9b93f3f0682250b6cf8331b7ee68fd8 the hash of any important password? If it is, CHANGE IT NOW! \$\endgroup\$
    – Ismael Miguel
    Commented Aug 3, 2015 at 21:15
  • \$\begingroup\$ I'm downvotting this due to it's high insecurity and low reliability. I'm sorry, but it is really insecure. I would never recommend someone using your code in it's current form. I recommend that you read security.stackexchange.com/questions/211/… before using this code in a production environment. \$\endgroup\$
    – Ismael Miguel
    Commented Aug 3, 2015 at 21:25
  • 2
    \$\begingroup\$ Sorry to scare you @IsmaelMiguel, it is simply "password" hashed. The whole point of my post here was to receive advice on the security and usability of my method of working with sessions, rather than the storing of my passwords. \$\endgroup\$
    – BadAtMaths
    Commented Aug 3, 2015 at 21:28
  • \$\begingroup\$ To you, it is "simply". To me, it's a hole bigger than the blackhole in the middle of our galaxy. I sound like a mad guy, but I'm not. Your hash now is vulnerable to offline attacks (an attacker copies your hash and using offline methods like a GPU farm may crack your password). Security is no joke. Since you are asking for a code review (one of the points in the help section, which I assume you read on codereview.stackexchange.com/help/on-topic the point "Reviewers may comment on any part of the code"), I gave my view. And, honestly, your code is insecure. I'll write this in an answer. \$\endgroup\$
    – Ismael Miguel
    Commented Aug 3, 2015 at 21:35

2 Answers 2

Reset to default
6
\$\begingroup\$

Let's see the code:

  1. Your Session class does too many things:

    • Session management
    • Login
    • Data validation
    • Logout
    • Authentication
    • Inclusion management

  2. Your class has no clean way to destroy a session.

  3. You don't verify if a session is active.
  4. Your constructor seems a little off. Sessions are a singleton. Use a single static final class.
  5. You don't need to repeat public on every method;
  6. You are storing a class on a $_SESSION. I would avoid that like plague. You should start and end the session. And you should have a method to save the session (that runs session_write_close() on shutdown or at any given time).
  7. There's no way to check if a session was opened before or not, or if it was saved already or not.

Fixing these points and splitting your code into classes that do each thing in separate will make your code better in every conceivable way.
A while ago I've used JSON to store the session data. It generates files a bit longer, but it is faster to save. Consider making your own session management system, based out on PHP's way.
Since HDD is cheaper than CPU cycles, I would consider using such way to load/save your sessions.
Completely outside the scope of this review, you could use a RAMdisk with the /tmp partition to squeeze every tiny piece of performance you get. Use only 10% of your RAM for such purpose.

Before doing anything to store your password, I recommend a good read:
How to securely hash passwords?

One of the points is that you shouldn't use SHA1, but instead use bcrypt or password_verify (PHP5.6 and up) in the future.

Also, you should be extremely careful with SQL injection and other possible attacks, especially evolving different encodings.
I sound paranoid, and I know that. But I just want a safe code.

\$\endgroup\$
5
\$\begingroup\$

Some Notes:

  1. Your use of private variables to hide the inner workings of your class is a very good idea. This is one of the most founding principles of OOP, and you understand that part very well it seems. :)
  2. The Session class seems responsible for more than just a session, though with the small size of this programme it's hard to differentiate it without much overhead. I would consider moving some things out in the future, or renaming it.
  3. I'm not sure how far into best-practices this delves (I'm not a PHP guy, really. Just a novice) I would definitely recommend against directly changing URL destinations in methods like that.

I.e.:

public function authenticate() {
    if (isset($_SESSION['loggedin'])) {
        if ($_SESSION['loggedin'] === true) {
            return;
        }
    }
    header('Location: login.php');
}

Could be rewritten as:

public function isAuthenticated() {
    if (isset($_SESSION['loggedin'])) {
        if ($_SESSION['loggedin'] === true) {
            return true;
        }
    }
    return false;
}

This will allow you to do more with the function much more easily. It allows you to use the same function to, instead of redirecting the user, show a modal instead. It's a much more reusable class at that point. (Which is the idea here, is it not? :)) Part of the purpose of building something with an OOP mindset is to allow many sections of that programme to be reusable in many different ways. By strictly using header("Location: ?") you restrict the number of ways you can use it, to 1.

You could of course simplify this further (credit to Ismael for a chat message) into:

public function isAuthenticated() {
    return isset($_SESSION['loggedin']) && $_SESSION['loggedin'];
}

This keeps the LoC down, and increases maintainability. It uses short-circuiting, so if the left-hand side evaluates false, the right-hand side will not even be hit.

  1. Do not use include_once for something like this. The require_once handle is much more appropriate. Others may recommend using include_once or include, but I would personally recommend the require_once over it.

  2. Concerning:

Also, I realise that the location headers may better be stored as a class constant in case I wish to change the locations.

I would really not do that. This class should not be responsible for knowing that, this class is all about authentication, not redirection/authorization. One class, one responsibility. Instead, I would make those paths part of a Configuration class, or a SQL table that stores that type of data. This allows you to easily and concisely change them in one location for everything. (Who knows where else you might need to know that Login.php is the gateway?)

  1. You state:

Please disregard the use of SHA1 and the hashed password in the file, I will be extending this to use a MySQL background, and hope to incorporate the use of a salted hash. I will be using prepared statements, will this be sufficient to protect from SQL injection and allow me to not bother filtering $_POST[...]?

While this is technically true, prepared statements will not protect you against people entering blatantly invalid data. You should still do some filtering. (Though you need worry less about filtering against SQL injection, and more about filtering against data tampering.)

  1. You also state:

Do I need to call die() (or exit() seeing as they are identical) after a failed authenticate to terminate any page which may attempt to load? Or do I need to return a boolean and then terminate from the calling page?

Personally (and I use this concept in my ASP.NET work as well): if I encounter an error with authentication/authorization, instead of throwing an exception (or die/exit in your case), I have a custom error handler class that will send that data to a SQL database, the Event Viewer, and submit the user to a custom error page. Part of the security concerns are that throwing a default die/exit message

The following bit(s) deviate into ideas, rather than strictly commenting on the code:

  1. I would consider using a single session_id as the main key for your session, and instead of storing all of your $sessionInfo directly in the session, store them in a SQL (et. al.) table. A nice by-product of this is that it's very difficult for people to change session data. (Hijacking, on the other hand, is a different story.)

Those are all the notes I have now, I may come back later and add more. :)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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