The drush_verify_cli() function is not completely reliable as it returns false positives in some situations

You are so right.

Not only suPHP behave like this, it's the same for FastCGI/CGI.

PHP's php_sapi_name is enough to determine invocation path, and it's about time to close this (rather old) issue.

Changing status. Not sure really what the impact is of false positives or false negatives, really. Drush can not be effectively used unless argc > 1, so it seems that maybe the test should be converted to && rather than removing it, but I'm not really sure what the right test should be.

Converting to && will do the same, but then the function is no longer drush_verify_cli(), but drush_verify_cli_and_check_arguments_count().

When PHP is used as CGI, suPHP, or as FastCGI (all my production servers), accidentally exposing Drush to Apache will make a _potential_ security hole. I've tested, and luckily Drush only gives out the help text.

When you call http://example.com/script.php?param, under CGI-ish setups it's the same as calling "php script.php param" from CLI. I hope you see potential (not real at the moment) security breach.

PS. When using CGI, FastCGI or suPHP, php_sapi_name returns 'cgi', 'fcgi' and 'suphp', respectively.

Semantics aside, my question is, are there instances where php_sapi_name returns something other than 'cli', but drush is still expected to work? If so, then the patch in #1 would break those installations. I am just curious why the test for argc is done at all if it is completely unnecessary; we should proceed with caution.

are there instances where php_sapi_name returns something other than 'cli', but drush is still expected to work?

No.

why the test for argc is done at all

I guess somebody (needlessly) wanted to be extra sure that Drush is being started from CLI, but was not aware that there are web server setups where Drush gets arguments even though it is not called from CLI.

Bottom line is:

1) Drush is a CLI tool
2) Since it doesn't authenticate, it's dangerous to leave it open to public
3) php_sapi_name is *the* way to figure out how you're called
4) Drush, as is, has a potential security issue, not yet exploitable, but give it time

I don't know that I should be concerned about breaking drupal terminal (not yet fully working) or drush ui (marked obsolete), but I am. However, this is not why verify_cli was written the way it was; it seems that the argc check is a workaround to help people whose ISP does not set up the php cli environment correctly.

Here is what drush_verify_cli used to look like:

 function drush_verify_cli() {
-  if (php_sapi_name() == 'cgi') {
-    return (is_numeric($_SERVER['argc']) && $_SERVER['argc'] > 0);
-  }
-
-  return (php_sapi_name() == 'cli');
+  return (php_sapi_name() == 'cli' || (is_numeric($_SERVER['argc']) && $_SERVER['argc'] > 0));
 }

The most recent change was committed from issue #413502: cli test fails: "drush.php is designed to run via the command line". So, the question remains, which is more important, a security issue that is not yet a security issue, or supporting people whose ISPs can't get their php cli config right? This is a common problem with low-budget semi-shell-access ISPs.

I'm on the fence. Moshe?

Can we not come up with a hardening patch that doesn't screw the people we helped in #413502: cli test fails: "drush.php is designed to run via the command line"?

FYI, I also care about the drupal terminal and drush ui use cases. I'm not likely to work on those modules, but I want one of them to grow and prosper.

I'll try to figure out this issue and provide a "hardening patch".

But I'm pretty sure that this boils down to Greg's question: what's more important, potential security issue (Drush should not be able to do stuff remotely, via simple URI call) or obscure hosting issue.

If we have no better idea, we can just add some documentation and commented out code to help folks on these deficient web hosts.

I think that what we need is $options['cli-check'] = 'high | medium | no';. We could put this in the example drushrc.php; default it to 'high' if it is not set. Under 'high' mode, drush bails if php_sapi_name() does not return exactly 'cli'. Under 'medium' mode, it does the current check. Under 'low' / 'no' mode, it allows drush to run regardless of what php_sapi_name says. If it bails, it could also print out a message to say what the user must do to make it work.

There are two implications to this solution.

• This test must run later in the bootstrap, after the options are parsed
• We will need a new version of drush_get_option that skips contexts that the user can manipulate with environment variables and / or command line options, and rely only on contexts initialized from files

Is it worth the effort? Not sure, but I think that would be the way to do it.

Overkill, IMO.

To answer the question, the security issue is more important. I just want to guide folks to success in the misconfigured cli case.

After walking away from that, I came to the same conclusion. How about if we use function_exists('drush_security_policy'), and call it if it exsits, and require 'cli' if it does not. Comments can instruct people to put it in drushrc.php, and -not- define it in drush_ui & c. (lest we end up with two); we can even put a sample implementation in example.drushrc.php.

That would be pretty simple, and flexible enough. Commented-out code in a drush file would not be good, because folks who modified the function would have trouble every time they upgraded drush.

Here's one more option. What if we check environment variable, and act upon it?

For example:

export DRUSH_RELAXED && drush sql-sync

If getenv('DRUSH_RELAXED') -> don't be paranoid.

Folk from the Internets won't be able to set environment variable and thus we eliminate potential security threat, cli people who are doomed to use php cli as if cgi will still be able to go about their business, and the others - they won't notice a thing.

I'm okay with #13.

Good. I'll make a patch.

Setting right component.

This issue was marked closed (won't fix) because Drush has moved to Github.

If desired, you may copy this bug to our Github project and then post a link here to the new issue. Please also change the status of this issue to closed (duplicate).

Please ask support questions on Drupal Answers.