Posted by Xano on May 16, 2007 at 4:12pm
31 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | user system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | CSRF, Security improvements, WSCCI |
Issue Summary
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.
Comments
#1
Just curious: has this already been fixed?
#2
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.
#3
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.
#4
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.
#5
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.)
#6
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.
#7
indeed a good place to implement #374646: Popbox (Popups Lite): Adding Modal Dialogs to Confirmations
#8
The filter system should prevent this.
#9
How? There's nothing wrong with an element pointing to /logout, as long as the page isn't requested without the user wanting it.
#10
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!!! )#11
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 inuser_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>';
?>
#12
Let's try that. Dead simple.
#13
The last submitted patch failed testing.
#14
Dear DamZ, at least run the code you wrote before throwing it at my face next time.
- The test bot
#15
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_pathshould use%tokenas well?#16
Nope, paths are stored in their simple forms (% instead of %token) in the database. The loader functions themselves are in
menu_router.load_functions.#17
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.
#18
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).
#19
The last submitted patch failed testing.
#20
Erm. This looks like a bug in the menu module:
<?phpfunction 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.
#21
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();
}
}
?>
#22
This requires #204077: Allow menu links pointing to dynamic paths.
#23
subscribing
#24
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
#25
#26
Reroll.
#27
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
#28
The last submitted patch, 144538-harden-logout-link.patch, failed testing.
#29
We now need to store the state of the browser (session ID, cookies, isLoggedIn) when switching back from one session to the other. Fixed.
#30
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()
#31
I was neither working nor tested before.
Not measurable. And this only concerns the session tests.
D'oh. Rerolled for that.
#32
A patch is good to go, also tests are fixed.
this change should be documented at http://drupal.org/node/719612
#33
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).
#34
@grendzy good catch
Somebody suggested a logout function to help users creating the link. Shouldn't we create it?
#35
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.
#36
According to http://api.drupal.org/api/function/menu_block_info/7 , menu blocks are not cached.
#37
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).
#38
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?
#39
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.
#40
Setting to needs review so others can see the test failure.
#41
The last submitted patch, 144538.39-harden-logout-link.patch, failed testing.
#42
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.
#43
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.
#44
Maybe better to postpone this as pointed in #22 for #204077: Allow menu links pointing to dynamic paths
#45
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.
#46
I believe this has been fixed in Drupal 7 by adding a random hash path component to the logout URL.
#47
Nope. /user/logout still logs you out.
#48
#39: 144538.39-harden-logout-link.patch queued for re-testing.
#49
The last submitted patch, 144538.39-harden-logout-link.patch, failed testing.
#50
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)) {
?>
#51
Of course there can be many other ways to exploit this but -- this is more a nuisance than anyhing else.
#52
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.
#53
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.
#54
Tagging, I recall seeing auto CSRF protection discussions in/around router patch.
#55
I think the discussion with CSRF and WSCCI was "this will need a re-roll post the router patch" so wait for that.
#56
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. :-)