I made a small patch to disable public access to cron.php. This patch will add the following new settings under administer => settings => cron jobs:

  • Enable cron security
  • Security type (allow from secific ip addresses / only from commandline)
  • Ip list of ip addresses that are allowed to access cron.php
  • Log unauthorized requests to cron.php

patch file attached.

- Tero

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jaza’s picture

Title: PATCH: cron security » Restrict access to cron
Version: x.y.z » 6.x-dev
Status: Needs review » Needs work

This would be a very handy feature. However, we don't need an option to restrict cron to command-line use only. In fact, we are currently actively discouraging users from executing cron via the command-line (the /scripts/cron-*.sh scripts in core use CLI browsers, e.g. lynx and wget). We should continue to discourage this, as Drupal is intended to be invoked only from a web browser, and cron.php is no exception. We cannot currently guarantee that a CLI execution willl work 100% (although work is being done to change this).

So the patch looks good, but we only need to restrict cron access by IP, IMO. Moving to 6.x-dev.

Pancho’s picture

Version: 6.x-dev » 7.x-dev

Moving feature requests to D7.

kbahey’s picture

I like this feature. It will prevent denial of service and resource waste.

Since in 99% of the cases, cron would be running from the same machine that Drupal is running on.

So, Drupal can automatically check the IP of the server, vs. the IP of the client and if they are the same, then all is well (allowed). No need for options or anything like that.

We need an option that says : allow cron from a different IP address, then a list of IPs. By default this is OFF, and the IP list is empty, and Drupal does the above check (server IP and client IP are the same).

Alternative approach: use a cron key

An alternate method would be to define an md5sum at install time (e.g. of the timestamp of the install), and instruct the installer to specify that in the cron command line.

$cron_key = md5sum(time());

variable_set('cron_key', $cron_key);

print "Use this key for cron: $cron_key";

Then we can display a line like this for them to put in cron:

00 * * * * wget -O - -q -t 1 http://example.com/cron.php?cron_key=1248661beefdead

This key is checked via a variable_get() from cron.php and it refuses to run if the key is not correct.

This is more robust than IP addresses, and requires no configuration or options.

Susurrus’s picture

+1 to the cron_key method, as it is more versatile then the other methods and doesn't require more options.

jvlagsma’s picture

May I suggest that user/1 is still always allowed to run cron.php. Cron problems happen a lot and fault finding in loads of contrib modules is often difficult for the average admin, I know it is for me anyway. user/1 running cron.php from the browser is most probably the admin testing this type of cron problems, why else would this busy person bother to start cron manually ;-)
It would be great to even take it a step further and provide user/1 with diagnostic info instead of the white screen. Modules could provide diagnostic messages and cron could chose to display them to user/1 or suppress them for any other user (incl anonymous)

RobLoach’s picture

jvlagsma: The idea is that you run cron.php using a cron job on the server, and then read through the watchdog for entries that were made during the cron process.

I like the idea of a cron key, as well as the strict list of IP addresses. This might need more discussion.

kbahey’s picture

Assigned: Unassigned » kbahey

@jvlagsma

I think exempting user 1 from the key is a good idea. This will avoid the need to enter a key when running cron from somewhere in /admin. I will include that in the patch.

As for diagnostics, see my patch here which provides times and such http://drupal.org/node/131536. It uses watchdog, since you want to be able to see what happened in the past, not just what happens when you run it now (not all crons would run, or the database is not in a state where a problem happened in a past run).

@Rob Loach

I object to using the IP address. It is simply too cumbersome and unreliable. The cron key will be trouble free since it is predictable.

I will try rolling a patch for this.

jvlagsma’s picture

@kbahey
Thanks, that was a very useful tip.
About IP address, I'm running the site in a load balanced environment, the machine cron runs on is a different one from any of the webservers. Using IP would make it impossible for even admin to run cron manually, right? I do think using a key will need a bit of explaining in the doc's especially for error situations, or a meaningful error message in the log, to avoid support questions. I mean saying 'Unauthorized request..' might not trigger admin's to look at an incorrect key. Under what circumstances would the key need to be changed, with every new drupal release?

kbahey’s picture

@jvlagsma

I agree that the IP address is problematic for the reasons you stated, and for other reasons too (some users may not know what the IP address of their host is, sites behind proxies as you said, ...etc.).

The cron_key on the other hand is within Drupal so we can control it any way we want.

My current thinking is this:
- On install, generate a cron_key and tell the user to put it in the cron job (with appropriate instructions).
- Have a place under admin where the key is listed (admin/reports/status)
- Document it on Drupal.org and in INSTALL.txt as well.

Susurrus’s picture

Status: Needs work » Needs review
FileSize
2.72 KB

I think the cron_key is really the way to go. We should always allow access for user/1 however, though all users who can visit admin/reports/status can run cron now, we might want to allow them to run it without the cron_key, though I'm not so sure I like that.

I've whipped up a patch that created cron_key on installation and shows the correct cron link on admin/reports/status, though I haven't added any documentation or help text anywhere else as I want opinions on this first.

Susurrus’s picture

FileSize
2.13 KB

Updated to use key generation suggestion from #3 which removes a lot of duplicate code (was using user_password() to generate random cron_key).

breyten’s picture

I suppose cron.php should display an error message when cron.php is not called as user id 1 or when no key is specified.

Other than that, seems like a sensible patch. I like the simplicity of the approach.

dww’s picture

+1 on restricting cron.

However, I really don't like uid 1 being hard-coded in more places. Other users beyond uid 1 click around in /admin and might try to hit the "run cron manually" links, etc. Any solution that assumes either a cron job with a key or uid 1 is going to be wrong in the general case.

catch’s picture

Status: Needs review » Needs work

I agree with dww on this. I don't see admins abusing this somehow, user 1 should be reserved for update.php (and even that has the access flag in settings.php now).

Could we put this in 'administer site configuration'? Or alternatively whatever permissions currently give access to the 'run cron manually link' even if it's a couple of different ones?

Susurrus’s picture

As of right now, cron is handled differently when run through that administrative link and the cron.php file. This patch only addresses access to cron.php, the access to that other "manual cron" URL has not been changed.

IMHO the cron.php file should be secured, while the administrative link can keep it's current access. I think we should remove the $user->uid == 1 and just use the cron_key, because it's much more flexible than anything else.

catch’s picture

Ah ok. In that casejust removing uid 1 seems like the only thing left to do then.

Susurrus’s picture

Status: Needs work » Needs review
FileSize
3.58 KB

Alright, I've removed the reliance on user 1, but is there enough documentation? I've added explanation text to INSTALL.txt, but I'm not sure what to do about the cron scripts provided in the /scripts directory, as those don't have the cron key. I've added to the documentation that the cron key will need to be manually added, but should we add ?cron_key=_____ to the URL in those scripts to suggest to the user that they're missing something?

Dries’s picture

Status: Needs review » Needs work

In the text, ""Cron maintenance Tasks" should be "Cron maintenance tasks".

Some additional documentation or pointers might be useful. For example, it might be useful to log a watchdog entry when the cron_key is not set or when it doesn't match. That would give administrators a bit of a clue when things aren't configured properly.

keith.smith’s picture

Status: Needs work » Needs review
FileSize
3.09 KB

How about the attached patch, which more significantly reworks the new help text? While the earlier version was ok, I thought "This page requires the addition of a cron key that protects cron against unauthorized access." might be a bit confusing, since it is not the page, but the URL of the page that requires the cron key.

keith.smith’s picture

Status: Needs review » Needs work

Oops. Hold on. All that patch isn't there for some reason.

keith.smith’s picture

Status: Needs work » Needs review
FileSize
4.7 KB

Weirdness. Anyway, the attached patch should now have the system.install hunk missing from the one in #19.

jgoldberg’s picture

+1 to the idea for a cron key. It would be nice if this was accessible in admin in case a sysadmin loses it.

keith.smith’s picture

@jgoldberg: From the proposed new text in INSTALL.txt:

"+   your site. The full URL of the page, with cron key, is available in the "Cron
+   maintenance tasks" section of the "Status report page" at:
+
+             Administer > Reports > Status report
Susurrus’s picture

@jgoldberg: Maybe you can test out this patch and then offer any suggestions? We need more testers for this as the status indicates.

x-lette’s picture

Good idea! I was wondering, why there is just no way of securing the access to cron.php.

Some suggestions:

  • add an option for renewal of the key in case the key might be published in any way
  • add restrictions to .htaccess like these ones:
    <FilesMatch "cron.php">
      Order deny,allow
      Allow from localhost, 127.0.0.1, server-ip/servername
      Deny from all
    </FilesMatch>
    
  • probably add functionality to automagically adding the admins' IP to the list of allowed IPs, this should work even when admin doesn't know about his actual IP
Susurrus’s picture

x-lette: I think that all those suggestions you made are a little over the top. While cron should be secure, I would think having an access key would be more than sufficient, especially considering anyone who is that worried about it can make that simple .htaccess change and limit access that way.

While security is always a concern, so is usability and making Drupal as easy to manage as possible. By adding restrictions about what IP address you can access cron from you can prevent people whose IP addresses are DHCPed to sometimes not have access to cron, which would be most frustrating for admins.

As far as key regeneration is concerned, don't let anyone get it. And even if they do, the site's no worse off than it would be right now, with cron.php not secured at all. I think regenerating that key is more than is necessary right now as it's only viewable by people who can 'administer site configuration', which should be restricted to trust-worthy people as it is.

kbahey’s picture

-1 for all IP based solutions.
-1 for solutions relying only .htaccess too.

+1 for regeneration of the key, in case it is compromised.

x-lette’s picture

@susurrus:
Over the top is relative. It would be easy to implement and most of the users wouldn't ever notice the existence of these variables/ entries / restrictions.

If the IP of a logged-in admin would change, the system would only have to update its entries. Thats all.

And for your last point: do I understand right, that you are talking about the manual firing of the cronjob via admin interface and not about direct access to cron.php? If so, then you misunderstood my intention. Of course the admin interface is protected against unallowed hackings and everyone I give permission to access the interface I need to trust.
But I'm talking about direct access to cron.php which normally is accessed via drupal.site/cron.php

kbahey’s picture

@x-lette

Please read the concerns voiced earlier in the thread.

There are people (entire countries and service providers) who are behind proxies. We don't want the admins to muck with something that is outside the control of Drupal (IP addresses).

A key on the other hand is under the control of Drupal, and hence we can generate it, document it, use it, trouble shoot it far better than a factor outside of our control with so many permutations possible.

x-lette’s picture

OK, don't want to go too deep in trouble here, but one more word on this ;-)

What I meant was a method, where drupal recognises the IP of a user who is admin (or has similar rights) and stores this IP to the database or wherever it might be stored. The IP comes with every request the user sends, so it could be updated every time the user calls a URL. You even could skip this step in case drupal is storing statistical infroamtion to the database. The IP might be stored with it as well.

So, as far as I can see, there is nothing that any user has to worry about. He might be behind a firewall or a proxy or what else, it doesn't matter because drupal always sees the actual IP.

kbahey’s picture

@x-lette

No trouble at all. We are just discussing things.

Now, that is a better explanation of your scheme, and a better way to use IPs.

It has an advantage over the key method in that it does not require copy/pasting of some value to the crontab.

But, it requires us to track more than one IP. One for the server where cron.php runs from, another for each user with privileges to click the "run cron" links under /admin.

How are we initially going to set these up? First time cron runs (e.g. cron.php) we take that IP? Makes sense.

What about when an admin runs it from /admin/*? Do we blindly go and accept anyone who has access to /admin then?

It would cause issues with server moves (common for shared hosts) too. There has to be an admin link to reset the values.

What do others think?

Susurrus’s picture

I just don't think the code required along with the database space is necessary to secure cron, as this is a large step in security from merely a cron key.

Just to clear up a few things about the administrative access to cron, there are two ways to access cron.

One is through cron.php which can currently be done by anyone and isn't secured against unauthorized access right now.

The second way is through an admin link that goes to admin/logs/status/run-cron This second access point is secured using the user access system built into Drupal's menuing setup. The only direct access to this page can be by people with the "administer site configuration" permission I believe. The only direction I could see us going with that access point is to have it rely on a new permission "administer cron". This permission would also not show cron anywhere (the key, the links, only the status on the admin/reports/status page). This makes the key "secure" and doesn't need to be regenerated.

I think IP address tracking should just be considered out of the question. It's severly limited (which isn't always a good thing), setup becomes edge-case sensitive (proxy servers, DHCP, etc.), and relies on much more data storage (when would old records be purged? if this persists over years with administration happening for a user wherever they may be, it could be a huge amount of data to store).

As far as regenerating the cron key, it's like your password DON'T GIVE IT OUT. I mean, it's just that simple. You can't lose it (it's always in the admin page), only administrators have access to it (which could be further restricted if we add that new "administer cron" permission), and if someone does get access to your cron key you're no worse off than every other user of Drupal at this very moment (which is a lot, including many high profile clients). I think that the case of regenerating the cron key being necessary is easily fixed with a nice DB query (which is trivial with phpmyadmin) and just adds UI cruft as we have to figure out to fit this new "feature" in.

kbahey’s picture

@Sussurus

The good think about the auto-IP approach is that it needs no setup at all, while the cron_key requires the user to do things ahead of time. So, from the simplicity point of view, it is simpler and automatic. No documentation for install is needed.

The storage of IPs is not large at all. We only store A SINGLE IP ADDRESS: the first IP address that comes in from when cron.php is run the first time, and that is it. It lives in a variable_get(). No extra tables needed.

Users who use /admin are exempt from IP checks anyways, and don't need to check for a key or IP or anything.

The only part which needs intervention is when the IP address needs to be reset, in case of a server move or something. If we figure out an elegant solution to this, then I see the auto-IP as a viable contender.

Susurrus’s picture

Adding a cron key for users is not a problem, especially since its well documented, in the interface as well as the README. For new users, its just as easy as setting up a cron job right now, since they need to figure out how to do that anyways. The only downside I can see to the cron key idea is that it may be hard for existing users to realize that cron access rules have changed. While I think more restrictive is better, there becomes a point when it's not. I just think the extra work to solve these problems could be better spent elsewhere as we already have a "solution".

Steven Jones’s picture

If we went with a cron-key approach then a UI to change it (if you lost it or someone else worked it out) could go in contrib, since all it would need to do would be hash a random number and variable_set it.

I don't think many people would need to change their keys, and just put a line in the documentation about the contrib module.

Dries’s picture

The patch no longer applies, however, I think it is otherwise RTBC. I'd (a) re-roll the patch and (b) add an entry to CHANGELOG.txt.

keith.smith’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
4.7 KB

Here is a reroll from the patch in #21. (The INSTALL.txt hunk wouldn't apply any longer due to the removal of the "ping module" from a nearby paragraph.)

I'm setting RTBC based on Dries' comments in #36; I diffed it with the previous patch and the only changes are the INSTALL.txt portion.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

I've committed this patch to CS HEAD. Thanks.

Susurrus’s picture

Status: Fixed » Active

Should this patch have included an update so that the cron_key variable is created if it's on an upgrade? I'm not sure how Drupal upgrades in terms of the code executed, but I would think the full install function isn't called and a lot of people will be left without a cron_key set...

Not really sure what status to set this follow up to...

kbahey’s picture

Status: Active » Fixed

It is here http://drupal.org/node/235821

Please test it.

Anonymous’s picture

Status: Fixed » Closed (fixed)

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

Josh Waihi’s picture

Status: Closed (fixed) » Needs work
$cron_key = md5(time());

so all one needs to know if roughly when the site was created, then write a simple script that loops through EPOC time till it gets a different response, bingo! we've found the key! DOS attacks here we come. I agree that a key is making things more secure, however, I don't think this particular solution is quite right, maybe the cron key could be generated with sha1(rand(0, time())), that key would be less compromiseable. Also on the topic of 2 cron access points, why not just have 1? do we really need cron.php anymore? why not just use admin/logs/status/run-cron/MY-SPECIAL-KEY or just MY-SPECIAL-KEY

One other con about keys - its in the URL, whats to stop people looking at http request packets? To have the option to allow cron runs by IP is good because in most cases, the request will come from 127.0.0.1, a load balanced environment is a unique case, you should write your own rules on the load balance server to allocate which IP's can access the cron-run. IP auto-detect for cron is a bad idea, we shouldn't track IP's unless specifically told to.

Ideally, configurable cron is what we need, so people can secure cron as they need and how they need. We're a diverse community, so why are we trying to cut things down to a single specific way?

Another thought whilest on this topic, why not use wget and set the key in a cookie, 2 examples:

wget --no-cookies --header "Cookie: <name>=<value>"
wget --load-cookies /path/to/cookie/file

This keeps the url clean and cron more discret, if the cron_key resides in the cookie then cron runs. simple as.

to summarize wy thoughts:

  • use one cron url, not 2
  • store cron_key in a cookie
  • add the option to run cron from any of the given IPs (manually added) without keys

because this is ugly:

To run cron from outside the site, go to http://mydrupalsite.com/cron.php?cron_key=33ed1d58d474a3280f1c1fc6ec36f95e

grendzy’s picture

Status: Needs work » Closed (fixed)

@Josh Waihi - the security limitation of md5(time()); was resolved in #235821: Fix upgrade path for new cron key in 7.x

minesota’s picture

This wil be nice if backported to 6x also as there are plenty of 6x users
as well as many 5x users are upgarding to 6x

grendzy’s picture

Since this counts as a new feature, I think it will not be possible to commit a backport. Though one could make the argument that it's a security concern, and warrants an exception. I haven't really heard of folks having problems with it, though.

IMHO, if you're concerned about cron abuse in the current Drupal releases, you've got several options (none of which require patching core):

  • Copy cron.php to a non-guessable filename; then block access to the original via .htaccess
  • Restrict cron access to specific IP addresses, again via .htaccess
  • Create a contrib module for this feature: shut down cron.php via hook_boot, unless a key is provided in the URL