Note: this issue has been reviewed by the Drupal security team and it was decided that this can be handled as public security improvement.

Problem/Motivation

In Drupal 7 and 8 cron is vulnerable to CSRF attacks. Cron doesn't have csrf protection so sites can be affected via GET requests and no tokens are checked.

Mitigation

The vulnerability may be mitigated because running cron is task generally considered safe. Protecting cron runs was considered more of a hardening issue than an actual security fix.
However, depending on the type of the CSRF attack (and the site) this could lead to a Denial of Service attack.

Proposed resolution

Use csrf token protection or add a confirmation form to cron.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because it exposes cron to CSRF vulnerability.
Issue priority Major because, although it is a security improvement, protecting cron runs was considered more of a hardening issue than an actual security fix.
Prioritized changes The main goal of this issue is security improvement, and therefore a prioritized change.
Disruption None.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

It's already possible to run cron from the outside with a key at /cron/{key}, so I think we should just remove the separate /admin/reports/status/run-cron route and just use the former even when logged in.

tstoeckler’s picture

Priority: Normal » Major
Issue tags: +Security improvements

I think security improvements are generally major.

willzyx’s picture

why not simply add _csrf_token: 'TRUE' to the route definition?

Berdir’s picture

The difference between those links is that one redirects back to the status page and the other returns a 204 no content. We'd have to make that optionally redirect as well I guess?

David_Rothstein’s picture

Status: Active » Needs review
Issue tags: +Needs backport to D7
FileSize
484 bytes

Yeah, cron.php won't display any information to the site admin, whereas for the admin-specific path there's currently a success message displayed after cron runs. I don't think we should mix the two.

Here's a patch that just adds the _csrf_token as suggested in #3. Seems to work.

Status: Needs review » Needs work

The last submitted patch, 5: cron-csrf-2431283-5.patch, failed testing.

Status: Needs work » Needs review

willzyx queued 5: cron-csrf-2431283-5.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 5: cron-csrf-2431283-5.patch, failed testing.

willzyx’s picture

Status: Needs work » Needs review
FileSize
1.13 KB
willzyx’s picture

Issue summary: View changes

Added beta phase evaluation

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC!

Dave Reid’s picture

Have we confirmed there is in fact another test somewhere that tests this link's functionality?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

@Dave Reid - you're right this is now untested.

willzyx’s picture

Status: Needs work » Needs review
FileSize
1.92 KB

Added manual cron run test as for #12-#13

Berdir’s picture

Can we extend the tests and try to go the the URL manually and assert that we get a 403?

willzyx’s picture

taken into account #15

Berdir’s picture

Issue tags: -Needs tests

Looks good, can you also upload a patch-only patch, so that we can see that it fails without the change?

willzyx’s picture

can you also upload a patch-only patch

Not sure.. a patch-only or a test-only?

Berdir’s picture

Haha, sorry, a test-only patch I meant.

willzyx’s picture

Status: Needs review » Needs work

The last submitted patch, 20: d8-cron-csrf-vulerability-2431283-18-test-only.patch, failed testing.

Berdir’s picture

Status: Needs work » Reviewed & tested by the community

Failed as expected, looks good, thanks!

alexpott’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed a9c2c2d and pushed to 8.0.x. Thanks!

  • alexpott committed a9c2c2d on 8.0.x
    Issue #2431283 by willzyx, David_Rothstein: Cron CSRF vulnerability
    
willzyx’s picture

Status: Patch (to be ported) » Needs review
FileSize
918 bytes
3.87 KB

@Berdir thanks for your patience :)

Let's try the patch for drupal 7

The last submitted patch, 25: d7-cron-csrf-vulerability-2431283-25-test-only.patch, failed testing.

Fabianx’s picture

+++ b/modules/system/system.admin.inc
@@ -2352,6 +2352,10 @@ function system_status($check = FALSE) {
+  if (!isset($_GET['token']) || !drupal_valid_token($_GET['token'], 'run-cron')) {
+    return MENU_ACCESS_DENIED;
+  }

Almost great, but this should be in an access callback instead.

Else you can no longer run system_run_cron() from PHP manually.

One remark, the rest looks good.

David_Rothstein’s picture

We normally put token checks in the menu callback, actually (to prevent $item = menu_get_item($path) => $item['access'] from returning FALSE when it shouldn't). This is just a menu callback so putting it there won't prevent cron from being run normally via PHP, right?

The Devel module reuses this menu callback, though; there's already an issue to fix it at #2411615: CSRF vulnerabilities in menu callbacks but if we change it here before it gets fixed in Devel I guess we will break the Devel module's links (since they won't have the token attached to them). Not sure the best way to handle that. If we want to be extra backwards-compatible we could define a new menu callback that's a wrapper around the old one and only do the token check there, or we could just coordinate and assume people need to update core and Devel at the same time.

willzyx’s picture

ehmm.. thanks for noticing this incredible oversight!

willzyx’s picture

@David_Rothstein

Not sure the best way to handle that. If we want to be extra backwards-compatible we could define a new menu callback that's a wrapper around the old one and only do the token check there

Not sure the best way. As long as exists the old menu callback cron will still be vulnerable to CSRF attacks.

and we should not only take care of devel but also of the other modules that point directly to admin/reports/status/run-cron (eg admin_menu).

Fabianx’s picture

#29: I am sorry, David is right.

This would prevent the link from being displayed in the first place ...

What about using hook_link_alter() to add the CSRF token here?

That should take care of the normal links ...

( would need to port hook_link_alter() from D8 though, though that should be pretty simple if checking the original issue where it got introduced as there D8 was still without the Link Generator, etc. )

willzyx’s picture

Re-attached the rigth patch from #25

I created #2443855: Add CSRF protection to cron for Admin Menu and uploaded a patch in #2411615: CSRF vulnerabilities in menu callbacks for Devel.
We need to find a way to release this security improvement in the less disruptive way possible.

  • alexpott committed a9c2c2d on 8.1.x
    Issue #2431283 by willzyx, David_Rothstein: Cron CSRF vulnerability
    

  • alexpott committed a9c2c2d on 8.3.x
    Issue #2431283 by willzyx, David_Rothstein: Cron CSRF vulnerability
    

  • alexpott committed a9c2c2d on 8.3.x
    Issue #2431283 by willzyx, David_Rothstein: Cron CSRF vulnerability
    

  • alexpott committed a9c2c2d on 8.4.x
    Issue #2431283 by willzyx, David_Rothstein: Cron CSRF vulnerability
    

  • alexpott committed a9c2c2d on 8.4.x
    Issue #2431283 by willzyx, David_Rothstein: Cron CSRF vulnerability
    
salvis’s picture

Status: Needs review » Needs work

#32 for D7 needs a re-roll.

The last submitted patch, 39: d7-cron-csrf-vuln-2431283-39-test-only.patch, failed testing. View results

salvis’s picture

Devel and Administration Menu have both done their parts, so this can go live now.

thalles’s picture

FileSize
127.14 KB

Patch for d7 applied with success!

truls1502’s picture

Status: Needs review » Reviewed & tested by the community

The patch works :)

joseph.olstad’s picture

Issue tags: +Drupal 7.68 target
joseph.olstad’s picture

joseph.olstad’s picture

MustangGB’s picture

Issue tags: -Drupal 7.68 target +Drupal 7.69 target
MustangGB’s picture

Issue tags: -Drupal 7.69 target +Drupal 7.70 target
joseph.olstad’s picture

mcdruid’s picture

Issue tags: -Drupal 7.74 target +Pending Drupal 7 commit

#39 still applies.

The test looks good.

Verified that admin_menu added the token in #2443855: Add CSRF protection to cron. Devel was also mentioned, but it looks like that's a different callback so the token doesn't have to match (it does not).

If tests pass across the board, I think this is good to go.

joseph.olstad’s picture

Thanks again @mcdruid for all your excellent work on core.

Fabianx’s picture

Assigned: Unassigned » mcdruid
Issue tags: +Needs change record

This needs a change record.

RTBC + 1, approved for Merge! Thanks all!

  • mcdruid committed ff3cb66 on 7.x
    Issue #2431283 by willzyx, salvis, David_Rothstein, thalles, Berdir,...
mcdruid’s picture

Assigned: mcdruid » Unassigned
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs backport to D7, -Pending Drupal 7 commit, -Needs change record

Added the CR.

Thank you everyone!

Status: Fixed » Closed (fixed)

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