have a whitelist of modules to bootstrap commandfiles and drupal code

A first attempt at hitting the #3 vector.

There's another attack vector: .install files are loaded by drupal_get_schema(), and that function is called all over the place.

The hook_init() gets called only if we're not in MAINTENANCE_MODE

   if (!defined('MAINTENANCE_MODE') || MAINTENANCE_MODE != 'update') {
     module_invoke_all('init');
   }

... so maybe it's something we could use to remove that attack vector... However, some module *do* need that to work properly, so a lot of stuff would break, and it doesn't resolve the hook_update() stuff.

Still digging.

You are confusing two attack vectors: hook_cron() is not what I'm trying to resolve with the above patch. I'm only trying to resolve the vector #3, which is the .drush.inc files that get loaded when drush itself bootstraps. Right now, I assume I will *not* be able to resolve the hook_cron() problem and I have a workaround: don't run cron through aegir, but run it through the web interface.

Which leaves us with hook_init(), hook_update(), hook_install(), hook_schema(), etc.. Those are real bitches...If we solve those, we also solve the hook_cron() problem, but it's so hard to figure out that i'm not sure how to actually do it. Maybe it's something that needs to be done outside of drush, in aegir, that would disable all non-sites/all modules before bootstrapping...

There is a way to force Drupal to a limited set of modules by calling module_list() before the full bootstrap is done. The way to do this would be to do something like this:

$list_of_modules = ...; # a whitelist of modules to load
include("includes/module.inc");
module_list(true, false, $list_of_modules);
drupal_bootstrap(...);

The trick here is that module_list() can be passed a fixed_list of modules to load that persists through a static cache... Not sure it would work, but it's certainly something to investigate...

This would address attack vectors 1, 2 and 4 (init, cron and schema hooks).

Just regarding the .drush.inc vector of attack here..

So I like your approach: the load path is the first thing that crossed my mind, and I implemented it as such, but when I looked at the code, I realized there was already some kind of a search path there and I was basically over-complexifying this thing.

Therefore, I think having some "security" binary flag makes a lot of sense for .drush.inc. As much as I hate the notion of "safe mode" function, I think we could have some kind of "restricted shell" notion (a bit like rbash works) implemented. It's a far-ranging thing that potentially touches a lot of areas, but I think it would be good to start *somewhere*. I would prefer option 1 from #8 (do not load Drupal modules' config files): it would make the code much simpler, much cleaner. I am not sure how we should name the option then however, as --restricted seems a bit too wide (although it could be reused in other places).

Now regarding the Drupal bootstrap attack vector (we really have two attack vectors here: the drush and the drupal bootstrap)...

First, let me say that I assume the user doesn't have write access to settings.php and can only modify its own modules, files and themes. I know it's a bad idea to allow that to people, but "my boss wants it" (I don't really have a boss, but clients do want this functionality ;).

You make good points in #5, which I considered: running the tasks as the right user makes perfect sense, and in fact it's probably the best solution all around. However, it requires a *major* rearchitecturing of the aegir backend (which is already being re-written) in order to have that kind of privilege separation. Basically, it means splitting a lot of provision commands in two and call the second command through "sudo -u anyuser" or with the first command running as root, both of which mean giving the aegir user root, something we want to avoid. So I have put that solution aside for the moment. Suid scripts will have the same problem.

Again in #7, you seem to assume the problem only stands with hook_cron(): it doesn't. Any drush command that's going to hit DRUPAL_BOOTSTRAP_FULL (provision-verify does that, for example) will fire hook_init(). Anything that calls drupal_write_record() will call hook_schema() hooks, and the list goes on. So the problem is not only with cron, it's with any command.

I admit the solution of calling module_list() early is hackish and ugly as hell, but it *should* work if modules are not loaded at all as a result of us hacking the list. We could implement a solution similar to what you're suggesting for the drush bootstrap problem: just a flag that would tell drupal/drush to bootstrap only modules in sites/all/modules or modules/ and ignore the site-specific modules. The whitelist could be constructed out of the output of modules_list and we could refeed it a filtered list...

Besides, short of patching core, modules_list() is the only thing we got to restrict that bootstrapping process... We could try to get that patch (see this for an example) in core, but then it would hit only Drupal 8, *may* be backported to 7, and never hit 5 or 6... it's not what we want. The idea here is to restrict the loaded modules, and have a more fine-grained control over the drupal bootstrap process. I think that would only be a good thing over all.

I agree it's not a complete solution to the security problem. It's only fixing one bit. But it's the only bit I got right now, short of rewriting the Aegir dispatch, which will happen, but not in the next 6 months (by my wild guess), while a Drush patch would be much simpler to implement.

Anyways, all this is moot if hacking at modules_list() doesn't work in the first place... I have to dig into this deeper, but my bet is that the list is reset in a few places anyways, so initializing it would fail in the first place... :(

Forget that last part: even if module_list() is called with $refresh = TRUE the $fixed_list still applies:

function module_list($refresh = FALSE, $bootstrap = FALSE, $sort = FALSE, $fixed_list = NULL) {
  static $list = array(), $sorted_list;

  if (empty($list) || $refresh || $fixed_list) {
    $list = array();
    $sorted_list = NULL;
    if ($fixed_list) {
      foreach ($fixed_list as $name => $module) {
        drupal_get_filename('module', $name, $module['filename']);
        $list[$name] = $name;
      }
    }
    else {
      // As this is the $refresh case, make sure that system_list() returns
      // fresh data.
      drupal_static_reset('system_list');
      if ($bootstrap) {
        $list = system_list('bootstrap');
      }
      else {
        $list = system_list('module_enabled');
      }
    }
  }
  [...]

Also... Just defining myevilmodule_cron() is not sufficient for the hook to be called: module_invoke_all() explicitely requests the module_list() and will only call hooks on those modules. So unless some retarded module explicitely calls module_invoke("myevilmodule", "cron"), just changing the module_list() should be sufficient.

I am therefore considering writing that module_list() hack. However, I'll probably start by writing an attack scenario so that we actually verify it works. :)

Oh, and thanks for the generous feedback and the patch review. I know this is insane stuff and I'm insane to try and fix this, so I really appreciate your openness here.

Alright, a new patch here. This creates a "--restricted" mode (also -R) that keeps drush from loading .drush.inc files from site-specific directories. The code is slightly cleaner, but it may need improvements as it's 1AM here and I mostly need to brain dump this stuff so I don't forget and we can forward the discussion. :)

Note that I think that -R can do more stuff than just that: the next step is to look at the module_list() hijack, in a similar way.

One thing that may be missing is the passing of -R around backend_invoke() calls... not sure that would get propagated now, but it would probably need to.

I also include an "evil" module to demonstrate how to test for this attack. With that module, I can confirm that the attached patch fixes the drush bootstrap attack vector successfully.

The problem with the module_list() approach is that it's a all-or-nothing approach: we can't make it work for hook_cron() and not for other uses... So yes, that will have the unfortunate implication of breaking drush cron for regular user. However, cron.php will still work, with all installed modules! So that problem is really not that bad...

Thanks for the review again.

So I misread module_list(): the $fixed_list parameter doesn't survive $refresh = TRUE; which gets called during bootstrap_all() and module_load_all().

So in short: without patching Drupal or fiddling with the system table to disable those modules, it's not possible to only bootstrap some whitelisted modules. It's all or nothing.

I have investigated the bootstrap code and it seems like DRUSH_BOOTSTRAP_DRUPAL_DATABASE is safe, while DRUSH_BOOTSTRAP_DRUPAL_FULL and DRUSH_BOOTSTRAP_DRUPAL_LOGIN are not (ie. they load all modules). So in short, to be on the safe side, we could make the --restricted mode simply abort when trying to bootstrap to full.. Other ideas?

Right. Basically, I consider the Drupal attack vector to be "by design". Note that the current patch doesn't try to address that issue at all, so let's please focus on getting that in (see #12 for the patch), which still covers some part of the vulnerability..

I may have gotten lost in my git mirror.. I'll reroll the patch based on the CVS code later tomorrow.

I think it's still valuable if commands are careful not to bootstrap drupal completely or bootstrap it only in controlled (ie. suid/sudo) environments. Without the patch, *any* drush command that runs is vulnerable to user-supplied code because of the too enthusiastic drush bootstrap.

The other thing I was saying was that the patch could also be improved to keep drush from bootstrapping drupal's module altogether...

I would like to appeal this, if possible. I'm at the BOF to talk about this, but basically I think that the current patch is just the first step. As I mentionned in #15:

I have investigated the bootstrap code and it seems like DRUSH_BOOTSTRAP_DRUPAL_DATABASE is safe, while DRUSH_BOOTSTRAP_DRUPAL_FULL and DRUSH_BOOTSTRAP_DRUPAL_LOGIN are not (ie. they load all modules). So in short, to be on the safe side, we could make the --restricted mode simply abort when trying to bootstrap to full.. Other ideas?

I think that the patch should be improved to abort the bootstrap with an error if a command attempts to go beyond BOOTSTRAP_DRUPAL_DATABASE. With that, the solution would be quite complete and drush would be effectively isolated from the users's modules.

Would that make it a sufficiently complete solution to be included in drush? Thanks for your consideration, and I'm available to discuss this at the BoF.. although there's a lot of people here. :)

I failed to communicate at the BoF (sorry!)... Can we meetup at the codesprint tomorrow? :)

I don't think we'd need to change the load path order: if this override sits in aegir (~/.drush or $sharepath), it loads before the the drupal-specific modules.

Greg, I think I like your idea! It keeps drush clean and allows us to hook it. I think we can stick with that, but let me think about it a bit more and get the code working if you will. :)

Thanks,

I'm digging again in this marvelous issue here. :)

By the way, this is a key issue in shared hosting security in Aegir, so I will not let go of this. I will, however, take it to third party modules if Drush thinks it's too invasive. ;) See #762138: Design security issue with developer access to sites' modules and themes for the Aegir side of things.

I think the hook_drush_load() hack is probably the best option so far. It could allow dropping modules from the DRUSH_COMMAND_FILES context externally, as the context can be edited by third party modules (now that's sick, I know).

It's the approach I'm looking at right now - of course, this doesn't fix the issue for Drupal core itself or site files, for which nothing less of "running as the right user" will solve the problem. Maybe a mix of the two approaches could work.

Thanks very much for the feedback.

assigning the right component and owner.

I think I agree. Let's close this.

By the way, the "evil module" I uploaded here is now a fully fledged Drupal project :P

https://drupal.org/project/evil