One of the users on my site posted the following code yesterday: <img src="/logout">. This causes every user to log out when visiting the page that code is on. He suggested making a special page at that address that uses a form to log out.

While we're at that, a system similar to that at Tweakers.net might be made. There you can select the session you want to log out. In that way you can log out the session you started on work bur forgot to end and you can do it from your home computer.

Files: 
CommentFileSizeAuthor
#78 interdiff-144538-78.txt3.35 KBdamiankloip
#78 144538-78.patch4.52 KBdamiankloip
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,783 pass(es), 3,952 fail(s), and 33 exception(s).
[ View ]
#18 144538-harden-logout-link.patch6.79 KBDamien Tournoud
Failed: 11525 passes, 4 fails, 3 exceptions
[ View ]
#14 144538-harden-logout-link.patch5.34 KBDamien Tournoud
Passed: 11510 passes, 0 fails, 0 exceptions
[ View ]
#12 144538-harden-logout-link.patch5.34 KBDamien Tournoud
Failed: 11497 passes, 1 fail, 42470 exceptions
[ View ]

Comments

Xano’s picture

Just curious: has this already been fixed?

drumm’s picture

Title:security: Image with logout url as source» Image with logout url as source
Version:5.x-dev» 7.x-dev
Category:feature» task
Issue tags:+Security improvements

The security team has decided to re-open this as a public issue. It is well-known and a good solution requires community input.

Please do report any other security issues to security@drupal.org and not the public issue queue.

drumm’s picture

We can't add a token, many themes that expect the path as-is. Detecting if the request was a forgery is impossible. Specifically filtering for that tag with that attribute might be doable.

I guess this explains how img tags can be used maliciously. Users posting images can be useful, I would like to see a solution implemented if something is possible.

Xano’s picture

How do you suggest we filter this? HTML is themable and users may create exactly the same links in their posts as the official logout link.

alexanderpas’s picture

if we implement tweakers.net solution, we certainly need usability testing for it!

however, a simpler might be a form on /logout, just like we handle delete actions.

also we can catch the logout link on other pages with JS to add a (session-specific) token so it can be handled automatically (can be combined with previous option.)

wretched sinner - saved by grace’s picture

Would this be a good use of [#3374646] for D7, if we put the confirmation page in place - it would then degrade to the current delete style confirmation page for non-JS browsers and D6 (and D5 I guess?)

It will need to have a graceful degredation, otherwise the issue will still be there for non-JS browsers.

alexanderpas’s picture

sun’s picture

Title:Image with logout url as source» Image with /logout URL as source
Issue tags:+FilterSystemRevamp

The filter system should prevent this.

Xano’s picture

How? There's nothing wrong with an element pointing to /logout, as long as the page isn't requested without the user wanting it.

alexanderpas’s picture

filter is not a solution, as this does not protect against malicious external websites

for example <img src="http://drupal.org/logout" /> on http://buytaert.net/ ( fictional!!! )

sun’s picture

Category:task» bug
Issue tags:-FilterSystemRevamp

Good point. I agree that the filter system can't handle this.

So, the usual thing to do in cases like this is to ask: How do others do it?

Looking at some other system, I found an approach that really is bad-ass simple:

Just append a query string to the logout link, a md5 hash of $user->login (timestamp of last login). So the menu callback of user/logout simply validates that query string before proceeding.

If we really want or have to aid in theming of custom logout links, we can simply use and add a helper function user_logout_query_string(), which only builds the query string bit that can be used both in user_translated_menu_link_alter() as well as in custom themes:

<?php
// page.tpl.php

echo '<div class="logout-link">';
echo
l('Logout', 'user/logout', array('query' => user_logout_query_string()));
echo
'</div>';
?>
Damien Tournoud’s picture

Status:Active» Needs review
StatusFileSize
new5.34 KB
Failed: 11497 passes, 1 fail, 42470 exceptions
[ View ]

Let's try that. Dead simple.

Status:Needs review» Needs work

The last submitted patch failed testing.

Damien Tournoud’s picture

Status:Needs work» Needs review
StatusFileSize
new5.34 KB
Passed: 11510 passes, 0 fails, 0 exceptions
[ View ]

Dear DamZ, at least run the code you wrote before throwing it at my face next time.

- The test bot

sun’s picture

Awesome! Even simpler than I envisioned :)

+  $ret[] = update_sql("UPDATE {menu_links} SET link_path = 'user/logout/%' WHERE link_path = 'logout'");
+  $ret[] = update_sql("UPDATE {menu_links} SET router_path = 'user/logout/%' WHERE router_path = 'logout'");
...
+  $items['user/logout/%token'] = array(

I guess that router_path should use %token as well?

Damien Tournoud’s picture

Nope, paths are stored in their simple forms (% instead of %token) in the database. The loader functions themselves are in menu_router.load_functions.

sun’s picture

I think this is ready to go...

Only remaining idea: Would a test to ensure that user/logout returns a 403 make sense? We would be testing the menu system actually... Not sure.

Damien Tournoud’s picture

StatusFileSize
new6.79 KB
Failed: 11525 passes, 4 fails, 3 exceptions
[ View ]

Well, it wasn't working, because the load function was missing from the previous patch. Added a very simple login / logout test (this is very well tested already by DrupalWebTestCase).

Status:Needs review» Needs work

The last submitted patch failed testing.

Damien Tournoud’s picture

Erm. This looks like a bug in the menu module:

<?php
function menu_reset_item($item) {
 
$new_item = menu_link_load(menu_get_item($item['router_path']));
  foreach (array(
'mlid', 'has_children') as $key) {
   
$new_item[$key] = $item[$key];
  }
 
menu_link_save($new_item);
  return
$new_item;
}
?>

menu_reset_item() is calling menu_get_item() with a $path like 'node/%', where menu_get_item() is expected a non-replaced path, like 'node/123'.

I'm not sure how to fix that.

andypost’s picture

I use following in my own module:

<?php
function andy_menu_alter(&$items) {
 
$items['logout']['options']['alter'] = TRUE;
 
$items['logout']['page callback'] = 'andy_logout';
}

function
andy_translated_menu_link_alter(&$item, $map) {
  if (
user_is_logged_in()) {
   
$item['href'] = 'logout/'.drupal_get_token();
  }
}

function
andy_logout($token = NULL) {
  if (
drupal_valid_token($token)) {
     
user_logout();
  }
  else {
      print
'?';
      exit();
  }
}
?>
Damien Tournoud’s picture

Status:Needs work» Postponed
grendzy’s picture

Issue tags:+CSRF

subscribing

hunmonk’s picture

posting this here for reference -- perhaps somebody could turn Damien's patch into a small module as a workaround for now?

#620280-1: Protect the logout link against CSRF attacks

moshe weitzman’s picture

Priority:Critical» Normal
Damien Tournoud’s picture

Status:Postponed» Needs review
StatusFileSize
new7.17 KB
FAILED: [[SimpleTest]]: [MySQL] 18,981 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

Reroll.

sun’s picture

Interesting. This patch contains at least 50% of what would be required for #755584: Built-in support for csrf tokens in links and menu router

Status:Needs review» Needs work

The last submitted patch, 144538-harden-logout-link.patch, failed testing.

Damien Tournoud’s picture

Status:Needs work» Needs review
StatusFileSize
new9.11 KB
PASSED: [[SimpleTest]]: [MySQL] 18,979 pass(es).
[ View ]

We now need to store the state of the browser (session ID, cookies, isLoggedIn) when switching back from one session to the other. Fixed.

andypost’s picture

Great fix!

+++ includes/path.inc
@@ -589,9 +589,9 @@ function drupal_valid_path($path, $dynamic_allowed = FALSE) {
-      $item['link_path']  = $form_item['link_path'];
-      $item['link_title'] = $form_item['link_title'];
-      $item['external']   = FALSE;
+      $item['link_path']  = $path;
+      $item['link_title'] = '';

Interesting, how does it works before without notices ($form_item is undefined)

+++ modules/simpletest/tests/session.test
@@ -186,14 +186,35 @@ class SessionTestCase extends DrupalWebTestCase {
+      $state = array(
+        'loggedInUser' => $this->loggedInUser,
+        'cookies' => $this->cookies,
+        'session_id' => $this->session_id,
+      );
+      file_put_contents($this->cookieFile . '.state', serialize($state));

Another file that makes simpletest getting slower

+++ modules/user/user.test
@@ -929,6 +929,39 @@ class UserPictureTestCase extends DrupalWebTestCase {
+    // No assertion needed here: assertions are already verified in drupalLogin().
+    $this->drupalLogout();

Suppose drupalLogin() should be drupalLogout()

Damien Tournoud’s picture

StatusFileSize
new9.11 KB
PASSED: [[SimpleTest]]: [MySQL] 18,980 pass(es).
[ View ]

Interesting, how does it works before without notices ($form_item is undefined)

I was neither working nor tested before.

Another file that makes simpletest getting slower

Not measurable. And this only concerns the session tests.

Suppose drupalLogin() should be drupalLogout()

D'oh. Rerolled for that.

andypost’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:+Needs Documentation

A patch is good to go, also tests are fixed.

this change should be documented at http://drupal.org/node/719612

grendzy’s picture

Status:Reviewed & tested by the community» Needs work

shouldn't the calls to drupal_get_token() pass a value (a fixed string like 'user/logout' would be fine), to prevent a sort of replay attack? (e.g. if a hypothetical contrib module used drupal_get_token() with no value to authenticate a different action, a user might be able to forge the token by copying the argument from the logout link).

meba’s picture

@grendzy good catch

Somebody suggested a logout function to help users creating the link. Shouldn't we create it?

kwinters’s picture

Is this going to have an impact on menu block caching? It seems like blocks that once cached per role (anon vs not) now must cache per uid AND the cache has to be cleared every time their last login time changes.

grendzy’s picture

According to http://api.drupal.org/api/function/menu_block_info/7 , menu blocks are not cached.

kwinters’s picture

Well, it also says that "menu.inc manages its own caching." So, there is still the question of whether the menu caching system will handle it appropriately.

Custom blocks that don't use the menu module directly, but still want to link to to the logout function still seem like they're going to be affected. Or for that matter, it will no longer be possible to embed a logout link into a node body (like a page explaining to users how to manage their accounts or security or something along those lines).

If the logout function worked automatically with the token and displayed a delete-form-like confirm if the token was missing or incorrect, that would be acceptable. It may have some caching effects but they'd be greatly mitigated (the cost of an incorrectly cached link is very low then).

jhodgdon’s picture

Issue tags:-Needs Documentation

Removing the Needs Documentation tag, as I think this patch didn't go in and it's clogging the Needs Doc issue queue. Does it need to be moved to D8 at this point?

deviantintegral’s picture

Version:7.x-dev» 8.x-dev
StatusFileSize
new7.52 KB
FAILED: [[SimpleTest]]: [MySQL] 32,108 pass(es), 3,542 fail(s), and 113 exception(s).
[ View ]

Here's a rerolled patch against 8.x. The patch seems to work fine with manual testing, but I'm running into an issue with the test. For some reason, the "Log out" link doesn't render in the simpletest environment. The token load function doesn't even get called in in the test. Any ideas?

This patch also adds 'user/logout' as a token value as suggested in #33.

deviantintegral’s picture

Status:Needs work» Needs review

Setting to needs review so others can see the test failure.

Status:Needs review» Needs work

The last submitted patch, 144538.39-harden-logout-link.patch, failed testing.

sun’s picture

This issue is kind of a duplicate of #1344078: Local image input filter in core now, which attacks the original cause instead (malicious user posting HTML containing arbitrary references). It's the same code that runs on drupal.org today.

I'm not sure whether adding and requiring a token for user/logout, as proposed here, is a sensible answer and solution, and whether this issue shouldn't be marked as duplicate.

kwinters’s picture

No such luck, Sun. This issue is for a CSRF exploit, and the other issue can't prevent posting of said image tag on random forums, etc.

andypost’s picture

Maybe better to postpone this as pointed in #22 for #204077: Allow menu links pointing to dynamic paths

greggles’s picture

Title:Image with /logout URL as source» User logout is vulnerable to CSRF

The title described one potential way to exploit the issue rather than the fundamental nature (which probably caused the confusion in 42/43).

Better title.

Xano’s picture

I believe this has been fixed in Drupal 7 by adding a random hash path component to the logout URL.

webchick’s picture

Nope. /user/logout still logs you out.

Abhishek Verma’s picture

Status:Needs work» Needs review
Issue tags:-Security improvements, -CSRF

#39: 144538.39-harden-logout-link.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Security improvements, +CSRF

The last submitted patch, 144538.39-harden-logout-link.patch, failed testing.

chx’s picture

Actually, while the CSRF is valid, the local image filter does protect because it checks whether the image source is a valid image :

<?php
      $local_image_path
= $local_dir . drupal_substr($src, $base_path_length);
      if (@
getimagesize($local_image_path)) {
?>
chx’s picture

Of course there can be many other ways to exploit this but -- this is more a nuisance than anyhing else.

Fabianx’s picture

Needs work:

To fallback it should be:

user/logout => Page which says: Do you really want to logout? Click here to logout, such the image is harmless
user/logout/hash => Logout directly

That way it is backwards compatible and links to /user/logout still work and then themes can update.

The extra step does not hurt.

webchick’s picture

That could be a security hole in the other direction, though... someone clicks "log out" before leaving a public computer, doesn't notice the confirm page, since you typically don't get one elsewhere on the web. H4X0R3D.

larowlan’s picture

Issue tags:+WSCCI

Tagging, I recall seeing auto CSRF protection discussions in/around router patch.

greggles’s picture

I think the discussion with CSRF and WSCCI was "this will need a re-roll post the router patch" so wait for that.

Crell’s picture

WSCCI is looking at a CSRF token integrated into routes: #1798296: Integrate CSRF link token directly into routing system

Otherwise this issue is entirely new to me. :-)

b8x’s picture

so what?
problem still exist, you can put in code of any site :
<img src="https://drupal.org/logout" border="0" alt="" />
and it will log out you from drupal.org. all i want to know is how i can disable or modify reactions on "user/logout" path? i can simply use token but i don't know where to put this code to preprocess logout. or i can create my own logout page, but how to disable core "user/logout" path?

yannickoo’s picture

You could download the Menu token protect (CSRF protection) module :/

b8x’s picture

Now I comment out the code in user.module:

<?php
  $items
['user/logout'] = array(
   
'title' => 'Log out',
   
'access callback' => 'user_is_logged_in',
   
'page callback' => 'user_logout',
   
'weight' => 10,
   
'menu_name' => 'user-menu',
   
'file' => 'user.pages.inc',
  );
?>

and created my own log out page. my project already has 100 contrib. modules, and increasing its number is last i want.

linclark’s picture

Beyond the potential for CSRF attacks, we should still consider whether we want to switch to using a POST request for logout instead of GET.

http://stackoverflow.com/questions/3521290/logout-get-or-post

(Not sure if this is the best issue for addressing this, but wanted to make a quick note).

damiankloip’s picture

Issue summary:View changes
StatusFileSize
new2.73 KB
FAILED: [[SimpleTest]]: [MySQL] 55,415 pass(es), 3,554 fail(s), and 50 exception(s).
[ View ]

So, now we have csrf integrated into the routing system. We can do something just like this. I guess alot of tests will break with this. Should work in a normal site env though, with a token being added to the logout link (in the toolbar user section in this patch, as a demo).

damiankloip’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 61: 144538-61.patch, failed testing.

Xano’s picture

Status:Needs work» Needs review
StatusFileSize
new3.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal_144538_64.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new2.89 KB

Status:Needs review» Needs work

The last submitted patch, 64: drupal_144538_64.patch, failed testing.

damiankloip’s picture

This interdiff confuses me.. :)

Xano’s picture

64: drupal_144538_64.patch queued for re-testing.

The last submitted patch, 64: drupal_144538_64.patch, failed testing.

dawehner’s picture

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -719,7 +719,7 @@ protected function drupalLogout() {
-    $this->drupalGet('user/logout', array('query' => array('destination' => 'user')));
+    $this->drupalGet(\Drupal::url('user.logout'), array('query' => array('destination' => 'user')));
     $this->assertResponse(200, 'User was logged out.');

So I guess this will not longer work?

dibyadel’s picture

I corrected all files as above and it worked well for my D 7.22 site. Thanks a lot.

d dt

regilero’s picture

A good way to fix that is to require a POST usage. On the client side you can capture the click on logout buttons (maybe with a central "a.drupal-logout" javascript behavior) and transform it in POST requests via jQuery.

For user not having js activated, so still using a GET, or modules not implementing this class on the logout link, a confirmation form should be presented, with a POST confirmation.

This would prevent nasty images and will be transparent for most users.

greggles’s picture

I don't think POST alone completely solves the problem unless the post includes a csrf-token. 3rd party sites could still have javascript that executes the POST and logs someone out.

Crell’s picture

Yes, the POST would need a token. The same logic applies to any link-action we have in Drupal: We should set a CSRF token in a meta or header or JS setting or something; then the link itself goes to a confirmation form. Then JS can, if enabled, mutate that link to just send a POST request itself, using the provided token, sans confirmation form.

That is probably worth a standard operation in Drupal that we can then leverage for logout and various other places (eg, flag module's various flag links, etc.)

Xano queued 64: drupal_144538_64.patch for re-testing.

The last submitted patch, 64: drupal_144538_64.patch, failed testing.

Xano’s picture

Status:Needs work» Needs review
StatusFileSize
new2.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,198 pass(es), 4,333 fail(s), and 101 exception(s).
[ View ]

Re-roll.

Status:Needs review» Needs work

The last submitted patch, 76: drupal_144538_76.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new4.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,783 pass(es), 3,952 fail(s), and 33 exception(s).
[ View ]
new3.35 KB

So I think we may need to add a drupalGetRoute() or something that we can use to generate the user logout link in this case. If we use the path we will not get outbound url processing.

A couple of other fixes too, like the user edit route name.

So I think now we might just have a problem with the token seeds or something?

Status:Needs review» Needs work

The last submitted patch, 78: 144538-78.patch, failed testing.