Note: this issue has been reviewed by the Drupal security team and it was decided that this can be handled as public security improvement.
Devel module has multiple CSRF vulnerabilities. Site can be affected via GET requests and no tokens are checked.
Code: (see also callbacks)
/**
* Implements hook_menu().
*/
function devel_menu() {
// Note: we can't dynamically append destination to querystring.
// Do so at theme layer. Fix in D7?
$items['devel/cache/clear'] = array(
'title' => 'Clear cache',
'page callback' => 'devel_cache_clear',
'description' => 'Clear the CSS cache and all database cache tables which store page, node, theme and variable caches.',
'access arguments' => array('access devel information'),
'file' => 'devel.pages.inc',
'menu_name' => 'devel',
);
...
$items['devel/run-cron'] = array(
'title' => 'Run cron',
'page callback' => 'system_run_cron',
'access arguments' => array('administer site configuration'),
'file' => 'system.admin.inc',
'file path' => drupal_get_path('module', 'system'),
'menu_name' => 'devel',
);
...
Mitigation
The vulnerability may be mitigated because the aforementioned callbacks would not compromise any data in most cases, as clearing the cache and running cron are tasks generally considered safe. However, depending on the type of the CSRF attack (and the site) this could lead to a Denial of Service attack.
Additionally, it is a "best practice" to not install Devel in production sites (although this is not documented in the project page and Devel module is marked as supported with recommended releases - so people may install it on production anyway)
Comments
Comment #1
klausioriginal discussion on security.drupal.org:
Comment #3 moshe weitzman commented December 15, 2014 at 5:25am
Very few people on a site get 'access devel information' perm. I dont see how you could get enough attackers to form a DOS.
Also, this perm is marked as 'restrict access' so can be a public bug.
Comment #4 Pere Orga commented December 15, 2014 at 5:52am
Hi Moshe
Very few people on a site get 'access devel information' perm. I dont see how you could get enough attackers to form a DOS.
I was thinking about multiple requests from a single host. An attacker could easily script them in a single attack. Wouldn't that - purging cache repetitively - affect most sites availability? Also, I also thought that cron implementations may have other implications or be resource expensive.
Also, this perm is marked as 'restrict access' so can be a public bug.
I agree this is issue is not a critical, but as most CSRF issues an attacker does not need any permission in order to exploit it, right?
Comment #5 Dave Reid commented December 15, 2014 at 8:20am
Agreed this can be fixed in public due to the permission.
Comment #6 Pere Orga commented December 15, 2014 at 8:26am
From What About Vulnerabilities Which Require Advanced Permissions?>/em>
Any user with one of the above permissions can already take over the site or bypass various access restrictions on the site, so there is no meaningful added risk if a vulnerability is only accessible to a user with one of these permissions.
This does not apply to CSRF, does it? Am I missing something?
Comment #7 Dave Reid commented December 15, 2014 at 8:43am
Given the nature of the module (best practice for not being enabled on public-visible site) and the fact that the user would have to have a restricted permission to be vulnerable to the CSRF, I'm saying I think it should be fixed in public as a security improvement, and not need the full SA process. I think that's a reasonable solution.
Comment #8 Pere Orga commented December 15, 2014 at 8:46am
Ok, sounds good
Comment #9 juampy commented December 15, 2014 at 12:38pm
Does adding flood support to these two resources sound like a good solution? That would block DOS attacks.
Comment #10 klausi commented December 15, 2014 at 1:09pm
Since an attacker can leverage CSRF vulnerabilities without having any permission at all on the site, I think we should fix privately. Of course they need to trick admins into invoking the request, but they can easily do so with image tags anywhere, sending direct links etc.
Comment #11 stella commented December 15, 2014 at 1:39pm
> Since an attacker can leverage CSRF vulnerabilities without having any permission at all on the site, I think we should fix privately. Of course they need to trick admins into invoking the request, but they can easily do so with image tags anywhere, sending direct links etc.
Yep, that was my reasoning to not immediately closing this issue.
Comment #12 moshe weitzman commented December 15, 2014 at 4:14pm
Cron has a semaphore so only one can run at same time. I really think it is a stretch to think that an admin gets tricked into clearing cache constantly on his site AND takes it down as a result. There are far easier ways to DOS a Drupal site.
I think it is a stretch to go through the SA ceremony for this. Yes, you can make a logical argument for it, but I think the opposite argument is more compelling.
Comment #13 Pere Orga commented December 15, 2014 at 6:43pm
I've just found what could be a precedent for the clear cache issue (fixed publicly): https://www.drupal.org/node/1189672
> Does adding flood support to these two resources sound like a good solution? That would block DOS attacks.
That would mitigate the issue, but still, IMO it is not expected that other people clear the cache or run cron for you, even if it's done only once.
Comment #2
willzyx CreditAttribution: willzyx commentedAdd related core issue
Comment #3
willzyx CreditAttribution: willzyx commentedAttached a patch to try to solve the reported vulnerabilities.
Note: cron protection will work effectively only after changes in #2431283: Cron CSRF vulnerability will be committed.
Comment #4
willzyx CreditAttribution: willzyx commentedComment #5
moshe weitzman CreditAttribution: moshe weitzman commentedLooks good to me. We already fixed D8?
Comment #6
willzyx CreditAttribution: willzyx commentedHey @moshe,
we still have not solved this issue for drupal 8. I know that we should focus on the latest major version first but some things make the solution of this issue difficult. The main problem is that the links are displayed in the devel menu block , which by default is cached. Introduce CRSF protection makes these features unusable (the token would be cached). The few solutions that I see feasible are: make the block uncachable or wait for #2351015: URL generation does not bubble cache contexts
I'm open to suggestions
Comment #7
moshe weitzman CreditAttribution: moshe weitzman commentedI think it is fine to make this block uncacheable in D8 but some of these dynamic/alter hooks are gone for menu links, I think.
Comment #8
willzyx CreditAttribution: willzyx commented@moshe to enable the CSRF protection in d8 we only need to insert
_csrf_token: 'TRUE'
to the routes definition (inrequirements
section)Comment #9
willzyx CreditAttribution: willzyx commentedThere have been many steps forward in the last five months with the cache system and I think now we can use the CSRF protection features offered by the core without problem.
Comment #12
willzyx CreditAttribution: willzyx commentedehmm added csrf protection to the wrong route..
Comment #14
moshe weitzman CreditAttribution: moshe weitzman at Acquia commentedLooks good.
Comment #17
willzyx CreditAttribution: willzyx commentedCommitted an pushed to 8.x
Now we should solve the issue for the 7.x branch
Comment #18
willzyx CreditAttribution: willzyx commentedre-uploading patch from #3
Note: cron protection will work effectively only after changes for d7 in #2431283: Cron CSRF vulnerability will be committed.
Comment #19
willzyx CreditAttribution: willzyx commentedand move to 7.x version..
Comment #20
salvisComment #21
salvisSeems like we're in a deadlock with
core (#2431283: Cron CSRF vulnerability) and
admin_menu (#2443855: Add CSRF protection to cron)...
Comment #22
salvisNeeds to be re-rolled.
Comment #23
salvis#18 re-rolled.
Comment #24
salvisI'm leaning towards checking this in. We can add the token with no immediate effect for cron, but it will help for securing the cache clearing.
We'll have three possible outcomes:
Comment #26
salvis