transferring this from security.drupal.org per team decision for a public issue.

reported by greggles:

Admin menu provides a "flush all caches" feature at example.com/admin_menu/flush-cache this doesn't use a token or confirmation form.

I thought of this because a new module - http://drupal.org/node/1097916 - is aiming to provide the same feature.

I feel like flushing all caches can be disastrous for sites that are utilizing most of their hardware capacity - do others think we should look to get a token or form on this page?

The same issue affects the link that may enable/disable modules.

Moshe notes that devel has a similar flush cache link itself.

Files: 
CommentFileSizeAuthor
#17 admin_menu.csrf-fixes.17.patch2.5 KBsun
#12 1189672-adminmenu-csrf.patch2.94 KBDave Reid
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1189672-adminmenu-csrf.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#8 1189672-adminmenu-csrf.patch1.88 KBDave Reid
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1189672-adminmenu-csrf.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 1189672-1.patch1.74 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1189672-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new1.74 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1189672-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs review» Fixed

fixed

Version:6.x-1.x-dev» 7.x-3.x-dev
Priority:Major» Normal
Status:Fixed» Patch (to be ported)

Sorry for the noise; now for real.

Status:Patch (to be ported)» Needs review

#1: 1189672-1.patch queued for re-testing.

StatusFileSize
new1.88 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1189672-adminmenu-csrf.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Patch attached for 7.x-3.x.

For 3.x, it's a bit more complicated, as the links are cached on the client-side.

I think the following is all that can be done:

  1. Generate a single token
  2. Supply that token via Drupal.settings
  3. Make the JS append the token to selected links
  4. Validate the token in the callbacks (as in the patch)

Hm... This also means that one is no longer able to recover from disaster situations by entering and hitting /admin_menu/flush-caches manually. Saved my day a couple of times already, especially in D7... but well, need to find a different solution in those cases then.

Well - I tested this out though and the cache is per-user so this should actually be fine.

Did you log out and back in? The session ID is part of the token, whereas the cached menu stays identical across sessions (until menu cache is cleared).

Of course, there'd be two ways out... we could as well add the session ID to admin_menu's cache hash. But I'm not sure how often Drupal actually regenerates a session though.

StatusFileSize
new2.94 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1189672-adminmenu-csrf.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Yeah, caching by session ID probably is better. Revised patch.

Assigned:Unassigned» sun

Assigning to sun for review since this changes admin_menu to be cached by session ID rather than user ID.

Thanks, looks good.

Merely this:

This also means that one is no longer able to recover from disaster situations by entering and hitting /admin_menu/flush-caches manually.

still concerns me. At least I need to recover from totally bogus data very very frequently on local dev sites.

I wonder whether we could add a 'admin_menu_security' variable, defaulting to TRUE, which would allow developers to disable the token validation by setting it to FALSE in settings.php?

Other ideas welcome.

I would think that we'd recommend update.php for that now that it flushes caches regardless of if there are updates to run or not?

Status:Needs review» Fixed

Ah, yeah, forgot about that issue.

Well, that applies to D7+. As I'm personally not really developing on D6 anymore, I'd be totally fine with the "regression" in D6.

So let's see how this goes.

Thanks for reporting, reviewing, and testing! Committed to all 3.x branches.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

StatusFileSize
new2.5 KB

Committed the attached follow-up fixes to 6.x-3.x.

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

The change form $uid to session_id() seems to have caused a critical bug when switching users:
#1468482: Menu is cached by session ID, but session ID does not change when switching users via Devel