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
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. |
Comments
Comment #1
tstoecklerIt'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.Comment #2
tstoecklerI think security improvements are generally major.
Comment #3
willzyx CreditAttribution: willzyx commentedwhy not simply add _csrf_token: 'TRUE' to the route definition?
Comment #4
BerdirThe 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?
Comment #5
David_Rothstein CreditAttribution: David_Rothstein commentedYeah, 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.
Comment #9
willzyx CreditAttribution: willzyx commentedComment #10
willzyx CreditAttribution: willzyx commentedAdded beta phase evaluation
Comment #11
Fabianx CreditAttribution: Fabianx commentedRTBC!
Comment #12
Dave ReidHave we confirmed there is in fact another test somewhere that tests this link's functionality?
Comment #13
alexpott@Dave Reid - you're right this is now untested.
Comment #14
willzyx CreditAttribution: willzyx commentedAdded manual cron run test as for #12-#13
Comment #15
BerdirCan we extend the tests and try to go the the URL manually and assert that we get a 403?
Comment #16
willzyx CreditAttribution: willzyx commentedtaken into account #15
Comment #17
BerdirLooks good, can you also upload a patch-only patch, so that we can see that it fails without the change?
Comment #18
willzyx CreditAttribution: willzyx commentedNot sure.. a patch-only or a test-only?
Comment #19
BerdirHaha, sorry, a test-only patch I meant.
Comment #20
willzyx CreditAttribution: willzyx commented:)
Comment #22
BerdirFailed as expected, looks good, thanks!
Comment #23
alexpottThis 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!
Comment #25
willzyx CreditAttribution: willzyx commented@Berdir thanks for your patience :)
Let's try the patch for drupal 7
Comment #27
Fabianx CreditAttribution: Fabianx commentedAlmost 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.
Comment #28
David_Rothstein CreditAttribution: David_Rothstein commentedWe 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.
Comment #29
willzyx CreditAttribution: willzyx commentedehmm.. thanks for noticing this incredible oversight!
Comment #30
willzyx CreditAttribution: willzyx commented@David_Rothstein
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).
Comment #31
Fabianx CreditAttribution: Fabianx commented#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. )
Comment #32
willzyx CreditAttribution: willzyx commentedRe-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.
Comment #38
salvis#32 for D7 needs a re-roll.
Comment #39
salvisHere's a re-roll of willzyx's patch in #32.
Comment #41
salvisDevel and Administration Menu have both done their parts, so this can go live now.
Comment #42
thallesPatch for d7 applied with success!
Comment #43
truls1502The patch works :)
Comment #44
joseph.olstadComment #45
joseph.olstadComment #46
joseph.olstadComment #47
MustangGB CreditAttribution: MustangGB commentedComment #48
MustangGB CreditAttribution: MustangGB commentedComment #49
joseph.olstadComment #50
mcdruid#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.
Comment #51
joseph.olstadThanks again @mcdruid for all your excellent work on core.
Comment #52
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedThis needs a change record.
RTBC + 1, approved for Merge! Thanks all!
Comment #54
mcdruidAdded the CR.
Thank you everyone!