0
\$\begingroup\$

I have the following PHP logout script and I would like to know the loopholes present in it. How can I improve it?

<?php

require_once 'config.php';

if(session_id() != "") {
    session_destroy();
}

if(isset($_COOKIE[session_name()])) {
    // Delete the session cookie
    setcookie(session_name(), "", time() - 1);
}

if(isset($_COOKIE['authUser'])) {
    $cookie_contents = $_COOKIE['authUser'];
    list($selector, $plain_token) = explode(':', $cookie_contents);

    //Empty the values
    $query = $dbh->prepare("UPDATE auth_tokens SET token=:token, expires=:expires WHERE selector =:selector");
    $query->execute(array(
        ":token" => "",
        ":expires" => "",
        ":selector" => $selector
    ));

    $count = $query->rowCount();
    if($count == 0) {
        die('Something went wrong..Could not log you out.' );
    }

    // Delete the authentication cookie
    setcookie('authUser', "", time() - 1);
}


header('Location: login.php');
\$\endgroup\$
2
\$\begingroup\$

I would suggest that, as you are looking to implement your code around session management, you would think about it more globally. Having several lines of procedural code to manage session logout, rather than a logout() method on a class that provides methods to manage sessions, limits re-use of this code. Why should you need to route a user to a specific URL to log them out as opposed to just having ability to do something like Session::destroy() from anywhere in your application, with that method basically doing what is in your code now?

Perhaps take a look at existing libraries to see if they meet you needs (for example something like https://github.com/auraphp/Aura.Session).


For better depth of knowledge around securely handling sessions, I would recommend you read and understand the PHP sessions "basics" documentation at http://php.net/manual/en/features.session.security.management.php

In particular, I would point you to this section:

Session Data Deletion

Obsolete session data must be inaccessible and deleted. Current session module does not handle this well.

Obsolete session data is better to be removed as soon as possible. However, active sessions MUST NOT be removed immediately. To satisfy these requirements, you MUST implement time-stamp based session data management by yourself.

Set and manage expiration time-stamp in $_SESSION. Prohibit access to obsolete session data. When obsolete session data access is detected, it is advised to remove all authenticated status from the user's sessions and force them to re-authenticated. Obsolete session data access could be an attack. To do this, you must keep track active sessions per user.

Note: Access to obsolete session could happen by unstable network and/or concurrent access to web site also. Server tried to set new session ID via cookie, but Set-Cookie packet may not be reached to client due to lost connection. One connection may issue new session ID by session_regenerate_id(), but another conncurrent connection may not get the new session ID yet. Therefore, you must prohibit access to obsolete session a while later. i.e. Time-stamp based session management is mandatory. In short, do not destroy session data by session_regenerate_id() nor session_destroy(), but use time-stamp to control access to session data. Let session_gc() to remove obsolete data from session data storage. In short, do not destroy session data by session_regenerate_id() nor session_destroy(), but use time-stamp to control access to session data. Let session_gc() to remove obsolete data from session data storage.

Note particularly here the bolded text.

What you should take away from this is that you need to begin to think of sessions and authentication (login) separately. Really the sole purpose of the session is to persist data on the server between client calls. PHP Sessions are not authentication or reliable data access control mechanisms.

You do not need to "destroy" the session at all upon a logout event in your application. You can simply regenerate_session_id() upon this change in authentication state (just as you would need to do when user logs in). But before that action you would need to programmatically set the data to expire on the current session id. This is the only reliable way to "lock out" this session data. This of course means that you would also always need to validate that the session id passed to the page does not contain expired data directly after calling session_start() near the beginning request processing and treating this as an invalid session if expired data is encountered.

Also, think of it from an application functionality perspective. Just because a user is logged out, doesn't mean your application might not need to still store some data in session. For example, maybe you might need to store something in session before performing a redirect to let the destination page know some piece of information (like the fact that the user just logged out). That target page might need to change the display on page the user is redirected to (like perhaps a successful logout message in your case).


if(session_id() != "") {
    session_destroy();
}

Why the conditional here? It doesn't seem necessary. Would you ever expect to be in a condition when session_id would NOT be set anyway? (Again I question use of session_destroy() at all based on earlier comment)


if(isset($_COOKIE[session_name()])) {
    // Delete the session cookie
    setcookie(session_name(), "", time() - 1);
}

Again, the conditional doesn't seem necessary. Why would you ever get into the state where you have an active session and don't have this cookie set?

If you are running your sessions in strict mode (you should be). Then this code is really not necessary, as the cookie would in essence be ignored going forward. If you are trying to be specific about overall cookie size then this code still makes sense.


$cookie_contents = $_COOKIE['authUser'];
list($selector, $plain_token) = explode(':', $cookie_contents);

//Empty the values
$query = $dbh->prepare("UPDATE auth_tokens SET token=:token, expires=:expires WHERE selector =:selector");
$query->execute(array(
    ":token" => "",
    ":expires" => "",
    ":selector" => $selector
));

$count = $query->rowCount();
if($count == 0) {
    die('Something went wrong..Could not log you out.' );
}

I would consider taking this functionality and building a class (or function) around it (perhaps userAuthPersistence or similar). You could get this to something like:

if(isset($_COOKIE['authUser'])) {
    try {
        userAuthPersistence::persist($_COOKIE['authUser']);
    } catch (Exception $e) {
        // do something here to handle error
    }

    // Delete the authentication cookie
    setcookie('authUser', "", time() - 1);
}

list($selector, $plain_token) = explode(':', $cookie_contents);

I would consider moving away from using your own serialization methodology (in this case simply concatenating with :) in favor of using PHP's serialization methods, JSON, or some other well-structured, well-supported approach to serializing structured data into a string. I think over time you will find them more powerful, more flexible, and less fragile.


$query = $dbh->prepare("UPDATE auth_tokens SET token=:token, expires=:expires WHERE selector =:selector");

This line of code is to long and harder to read. Consider getting in the habit of making sure the SQL within your PHP is visually differentiated from surrounding code to make it easier to read.

$sql = <<<'EOT'

UPDATE auth_tokens
SET
    token = :token,
    expires = :expires
WHERE selector = :selector

EOT
$statement = $dbh->prepare($sql);

I like using heredoc or nowdoc syntax (especially on longer queries), but even breaking up the SQL string across lines in a standard string assignment works.

Note also the change in variable name from $query to $statement which is my guess as to what you are actually representing in this variable.


die('Something went wrong..Could not log you out.' );

It is generally a bad idea to just echo errors to standard out. Consider at least logging an error here. You also might want to have portion of code closer to display layer handle the actual end user messaging.

\$\endgroup\$
6
  • \$\begingroup\$ "Why would you ever get into the state where you have an active session and don't have this cookie set?"-if someone without logging in hits the logout url intentonally. I just use sessions, didn't set it to strict mode as I am basically relying on cookies. \$\endgroup\$
    – Ayan
    Nov 11 '16 at 19:14
  • \$\begingroup\$ Can you give me an example of using PHP's serialization methods? \$\endgroup\$
    – Ayan
    Nov 11 '16 at 19:18
  • \$\begingroup\$ could you please explain me "You also might want to have portion of code closer to display layer handle the actual end user messaging."? \$\endgroup\$
    – Ayan
    Nov 11 '16 at 19:21
  • \$\begingroup\$ @Ayan Yes. Take my suggestion from my example of moving the authUser data persistence into a class. That class is where you logic on whethere an update is successful will now live. Rather than have that class do something like die('message'); You should either throw an Exception or return a value that indicates failure and not have the class worry about messaging the end user. Even right now your die() would end up just giving the user that brief message on an otherwise blank white page. You need to be thinking about how to provide user-facing error messages in a user friendly way. \$\endgroup\$
    – Mike Brant
    Nov 11 '16 at 22:49
  • \$\begingroup\$ @Ayan And now that I thin about it, I question whether the application should actually die if the DB update is not successful. You still can log the user out even in this situation, it just means you have lost this data in the process. What is the user going to think if there is a "failed to log out" message and to there appearances, they are logged out. Is this an error condition you can handle more gracefully in your app? \$\endgroup\$
    – Mike Brant
    Nov 11 '16 at 22:51

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.