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.
Comment | File | Size | Author |
---|---|---|---|
#17 | admin_menu.csrf-fixes.17.patch | 2.5 KB | sun |
#12 | 1189672-adminmenu-csrf.patch | 2.94 KB | Dave Reid |
#8 | 1189672-adminmenu-csrf.patch | 1.88 KB | Dave Reid |
#1 | 1189672-1.patch | 1.74 KB | pwolanin |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedComment #2
pwolanin CreditAttribution: pwolanin commentedfixed
Comment #3
sunComment #6
sunSorry for the noise; now for real.
Comment #7
iancu35 CreditAttribution: iancu35 commented#1: 1189672-1.patch queued for re-testing.
Comment #8
Dave ReidPatch attached for 7.x-3.x.
Comment #9
sunFor 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:
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.
Comment #10
Dave ReidWell - I tested this out though and the cache is per-user so this should actually be fine.
Comment #11
sunDid 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.
Comment #12
Dave ReidYeah, caching by session ID probably is better. Revised patch.
Comment #13
Dave ReidAssigning to sun for review since this changes admin_menu to be cached by session ID rather than user ID.
Comment #14
sunThanks, looks good.
Merely this:
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.
Comment #15
Dave ReidI 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?
Comment #16
sunAh, 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.
Comment #17
sunCommitted the attached follow-up fixes to 6.x-3.x.
Comment #19
sunThe 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