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

&lt;?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-&gt;prepare("UPDATE auth_tokens SET token=:token, expires=:expires WHERE selector =:selector");
    $query-&gt;execute(array(
        ":token" =&gt; "",
        ":expires" =&gt; "",
        ":selector" =&gt; $selector
    ));

    $count = $query-&gt;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');
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-&gt;prepare("UPDATE auth_tokens SET token=:token, expires=:expires WHERE selector =:selector");
$query-&gt;execute(array(
    ":token" =&gt; "",
    ":expires" =&gt; "",
    ":selector" =&gt; $selector
));

$count = $query-&gt;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-&gt;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 = &lt;&lt;&lt;'EOT'

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

EOT
$statement = $dbh-&gt;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.