mysql credentials leak in drush sqlc

Do folks think this is worth "fixing"?

I'd like to tidy this to use one call to http://php.net/manual/en/function.file-put-contents.php.

Does this have implications for remote sql calls? I don't think so, but just asking.

Yes, this patch breaks sql sync for remote sites. Sql sync calls mysqldump via ssh, but this patch separates the credentials from the command, so the cred file would be on the wrong machine whenever you were pulling an sql dump from a remote machine.

I don't leak creds when using postgres because I set up cat $HOME/.pgpass on my local and remote system. Postgres checks for this credentials file even when there is no reference to it on the commandline. I didn't check if mysql has a similar feature.

The credential leak is worth checking when using a web hosting system where you have shell access on a single shared machine, where other users might be able to observe your process list. It doesn't matter on your local machines, or on virtual private servers.

Fixing for sql sync would be annoying. You'd have to rsync the creds file over, and then delete it again afterwards. Perhaps for the short term this fix could just be parameterized so that it only happens for people who need it. You could even set it in $options and clear it in $options['command-specific']['sql sync'] if you wanted.

Upon reflection, I think that the right thing to do is have people who want to avoid the credentials leak set up a $HOME/.mysqlcreds, and then point to their credential information by setting up command line options or $options in drushrc.php, or override same with options in a remote site alias. These command line options would be processed per #518184: Allow Users To Set myqldump Arguments In drushrc.php (for drush sql dump). Just give me some time to review that issue; I don't have time this morning. I suspect the patch in that issue will have to be expanded slightly to allow options to be set in _drush_sql_get_credentials as well.

The always current DB credentials are stored in settings.php. Are you suggesting that users copy those to home dir and keep them up to date for all their sites? Seems onerous to me. Perhaps we only solve this for local sites to avoid that rsync hassle for remote sites.

Okay, I can agree with #5 and #7. I haven't tested #7.

sql sync calls sql dump which calls _drush_sql_get_credentials in "don't leak" mode, which will break if you are syncing from a remote system. This much should be fixed.

It also seems like sql sync could avoid leaking if you required drush to be installed on the remote machine. There are multiple ways this could be done, and it would still need to be controlled by a switch on the local machine to stipulate whether you wanted a leaky non-drush call or a non-leaky drush-requiring call. I suppose it would also be possible to make it non-leaky without drush with an appropriate grep + sed line on settings.php. The sql sync stuff can be a separate issue, though.

Aegir had to deal with similar problems. The way we handled this by passing credentials through file descriptors, as such:

      $cmd = sprintf("mysql --defaults-file=/dev/fd/3 %s", escapeshellcmd($db_name));
      drush_log(sprintf("Importing database using command: %s", $cmd));
      # pipe handling code, this is inspired by drush_provision_mysql_pre_provision_backup()
      # we go through all this trouble to hide the password from the commandline, it's the most secure way (apart from writing a temporary file, which would create conflicts in parallel runs)
     $mycnf = sprintf('[client]
host=%s
user=%s
password=%s
', $db_host, $db_user, $db_passwd);
      $descriptorspec = array(
        0 => array("file", $dump_file, "r"),
        1 => array("pipe", "w"),  // stdout is a pipe that the child will write to
        2 => array("pipe", "w"),  // stderr is a file to write to
        3 => array("pipe", "r"),  // fd3 is our special file descriptor where we pass credentials
      );
      $process = proc_open($cmd, $descriptorspec, $pipes);
      $output = "";
      if (is_resource($process)) {
        fwrite($pipes[3], $mycnf);
        fclose($pipes[3]);
    
        $output = stream_get_contents($pipes[1]) . stream_get_contents($pipes[2]);
        // "It is important that you close any pipes before calling
        // proc_close in order to avoid a deadlock"
        fclose($pipes[1]);
        fclose($pipes[2]);
        $return_value = proc_close($process);
      } else {
        // XXX: failed to execute? unsure when this happens
        $return_value = -1;
      }

That was lots of fun, but it's fairly bullet-proof, from a security POV. I'm not sure how portable it is: I know it works on Debian (and probably on all Linux) and FreeBSD, but that's all. Furthermore, I'm not sure SSH would pass along all file descriptors (I sure hope so).

I urge you to consider this approach instead of messing around with temporary files.

Note that this idea was originally inspired by a comment in the MySQL manual, although it's not credited in those comments. http://dev.mysql.com/doc/refman/5.0/en/password-security-user.html

we need something more portable, unfortunately.

@moshe - I think this is portable: it works for aegir, and that means it works on Solaris, CentOS and Debian, and I assume it also works on BSD. I hardly see how it could be made more portable if we disregard Windows support. Regarding that, I think that a special exception should be made for that poor retard rather than cripple our implementation with an insecure information leak.

I have no idea have we can securely pass credentials between processes under windows, but I consider it's the responsability of the poor sods using that evil platform to figure this out. I can't really install Windows here right now (I don't have access to the software) and I haven't for 8 years so I don't see why I should change that now. :P

@hexmode - that is unfortunate... I don't see why it shouldn't be possible for SSH to pass along all file descriptors, but then again, I think that creds should be stored on the server in the first place, they shouldn't need to go through the wire...

In #980782: backups shouldn't depend on udev or /dev/fd/3, I detail a simple workaround to the portability issue: just check if /dev/fd exists before using it to pass around credentials, and rollback to the insecure (or the temp file) behaviour if it's not available..

for the temporary file creation and removal we have drush_save_data_to_temp_file() or drush_tempnam()

Is someone actively working on this issue? I think it's quite important and yet it doesn't get the attention it deserves. This puts a lot of people in danger without them even knowing about it.

No one is working on this right now; patches welcome.

I do not understand why the status was changed here. I am putting this back to critical, since this *is* a critical security issue, which still affect 4.x and 5.x. In fact it should probably have been posted using the regular security process of d.o instead of here.

Also note that this happens in a few places, not only in sql* commands, but also in archive-dump:

11589 pts/10   SN+    0:00  |           \_ php /usr/bin/drush @www.example.com archive-dump --destination=/var/aegir/backups/example-archive-backup2.tar.gz
11673 pts/10   SN+    0:00  |               \_ sh -c /usr/share/drush/drush.php  --db_type='mysqli' --db_host='mysql.aegir.koumbit.net' --db_port='3306' --db_passwd='CENSORED' --db_name='examplecom_0' --db_user='examplecom_0' --installed --root='/var/aegir/platforms/drupal-6-build-2012.04.26-production' --uri='www.example.com' --context_type='site' --platform='@platform_drupal6build20120426production' --server='@server_master' --db_server='@server_mysqlaegirkoumbitnet' --site_path='/var/aegir/platforms/drupal-6-build-2012.04.26-production/sites/www.example.com' --site_enabled --language='en' --client_name='cl-example' --profile='default' --ssl_enabled='1' --ssl_key='www.example.com' --loaded-config core-status --backend  2>&1
11674 pts/10   RN+    1:19  |                   \_ php /usr/share/drush/drush.php --db_type=mysqli --db_host=mysql.aegir.koumbit.net --db_port=3306 --db_passwd=CENSORED --db_name=examplecom_0 --db_user=examplecom_0 --installed --root=/var/aegir/platforms/drupal-6-build-2012.04.26-production --uri=www.example.com --context_type=site --platform=@platform_drupal6build20120426production --server=@server_master --db_server=@server_mysqlaegirkoumbitnet --site_path=/var/aegir/platforms/drupal-6-build-2012.04.26-production/sites/www.example.com --site_enabled --language=en --client_name=cl-example --profile=default --ssl_enabled=1 --ssl_key=www.example.com --loaded-config core-status --backend

CENSORED is *not* actually censored by drush, and appears in the process list. Anyone having PHP execution in any Drupal hosted on this server can root this example.com site.

It seems that drush sqlc doesn't always leak credentials directly through the 'sh -c' call, which is a bit confusing. Running Drush 4.5 on two different servers, yields different results.

This is on a squeeze server:

anarcat@ceres:/var/aegir/clients/cl-anarcat/anarcat.koumbit.org$ ps fww
  PID TTY      STAT   TIME COMMAND
29381 pts/7    S      0:00 su anarcat
29382 pts/7    S      0:00  \_ bash
24447 pts/7    T      0:00      \_ php /usr/bin/drush sqlc
24459 pts/7    T      0:00      |   \_ mysql --database=anarcatkoumbitor --host=mysql.aegir.koumbit.net --port=3306 --user=anarcatkoumbitor --password=x xxxxxxxx
[..]

Meanwhile on my home server running wheezy:

anarcat@marcos:test.orangeseeds.org$ ps fww
  PID TTY      STAT   TIME COMMAND
 2914 pts/13   Ss     0:00 /bin/bash
 7063 pts/13   T      0:00  \_ php /usr/bin/drush sqlc
 7075 pts/13   T      0:00  |   \_ sh -c mysql --database=testorangeseedso --host=localhost --port=3306 --user=testorangeseedso --password=CENSORED
 7076 pts/13   T      0:00  |       \_ mysql --database=testorangeseedso --host=localhost --port=3306 --user=testorangeseedso --password=x xxxxxxxx

Notice the password showing up in the process list, because of the sh -c call... Not sure why this difference, but it sure makes finding the password harder on the former environment.

It is still possible, however.

I wrote a simple perl script to demonstrate:

#!/usr/bin/perl -w

use Proc::ProcessTable;

$| = 1;

my $i = 0;
my $start = time();
while (1) {
  $i++;
  my $delta = time() - $start;
  if ($delta > 0) {
    printf("\r%d loops per second", ($i/$delta));
    $start = time();
    $i = 0;
  }
  my $t = new Proc::ProcessTable;
  foreach my $p ( @{$t->table} ){
    if ($p->cmndline =~ m/mysql.*--password=([^s]+)/
        && not ($1 =~ m/^[x ]*$/)) {
      printf("\npass: $1, cmd: %s\n", $p->cmndline);
    }
  }
}

(Note that there is some extra code in there to show how many process listings we're capable of generating per second, this is probably not necessary, but is a good indication. On my environment I can generate around 40 process lists per seconds.)

In order to be able to sniff the password, I had to actually run 4 of those processes in parallel. (I didn't bother writing that threading code directly in the script, just start 4 background processes and you should be fine.) That way there are more chances of hitting the process list at the right time *and* this makes the whole machine slower because the CPU is now busy answering that script's silly requests, which in turn makes the mysql command load slower and gives us time to find its commandline arguments before it wipes them, which is a fairly short code path.

I was therefore able to sniff the password of my database, on both servers, with this.

Here's a patch:

http://drupalcode.org/project/drush.git/commitdiff/09f2bef313cdda4465b96...

It doesn't address the archive-dump issue. I pushed the patch on the dev-secure-creds branch, which can be used to fix the archive-dump issue later.

This does not work with sql-sync when the source machine is remote. The --defaults-file will be written to the local machine, but the mysqldump will be executed on the remote machine, and will not be able to find the file.

Another issue is that the credentials are not deleted if the sqlc is interrupted or if running drush 5 (#1589108: Drush doesn't clean up temporary files if interrupted).

How do you suggest this issue be fixed?

Well, I suppose as a simple interim solution, #23 would be okay if it only prevented credential leaks on local dumps, and continued to leak credentials for remote sql-sync calls. You could examine $db_spec before using --defaults-file. There should be a 'remote-host' component in it (just for reference) if called via sql-sync for a remote database.

A "real" fix for remote sql-sync commands would be kind of difficult, as you'd have to rsync your credentials over first, and then get rid of them again after the rsync finished. Awkward.

When using postgres, I just write a .pgpass file into the $HOME directory of the remote user that Drush uses to do the pgdump. I then do not need to include the postgres password in the pgdump commandline parameters. Perhaps something similar might be done with mysql. If a special Drush option is passed to sql-dump (perhaps indirectly, by being provided to sql-sync), then Drush could eliminate the --password= from the mysql parameters, and instead put in a --defaults-extra-file= argument. It would be assumed that the user had prepared a file (perhaps $HOME/.mysqlpass, or something similar) readable only by the file owner, that contains the --password option. The path to your mysqlpass file could be specified as the value of the Drush option that enables this mode.

The weakness of this scheme is that anyone who can run code as the designated user can also write to your mysql databases. I don't think this is terrible, though; while it does not conform to the principal of minimum permissions, presumably code execution on your designated user is restricted, so the security of the system should still be okay.

BTW: In MySQL the standard file is $HOME/.my.cnf and has format:

[client]
user=user
password="password"

Some work has gone into recognizing my.cnf but none of it landed yet. See #1234696: Read db creds from ~/.my.cnf

I do not think that writing to standard locations like .my.cnf or .pgpass is a good idea: the problem is that those files may already exist and be used by the user, overwriting them may destroy valid data. Therefore i think that using a tmpfile is the proper way of doing things.

I'll try to reroll a patch that respects sql-sync, which I am really not familiar with, but you are giving great advice.

As for the issue of using the credentials from an existing .my.cnf file, I think it's a different issue - parsing existing creds is a good idea, but this is about sending creds *to* mysql, not from it...

Here's an almost trivial rewrite to support sql-sync.

http://drupalcode.org/project/drush.git/commitdiff/e0483b7caacd3996adf49...

I'll test if we can pass file descriptors through SSH, otherwise I guess the only secure way would be to rsync a tmpfile and feed that to mysql...

I am okay with #31, which supports sql-sync acceptably, but still leaks credentials for remote syncs.

I think Moshe previously said that the file descriptor solution was not portable enough, but perhaps it would be acceptable if it was turned on via an option. The default could go either way, provided that the "default to secure" would, in failure scenarios, print out an error message with the option to switch to an insecure sync.

Forgot to change status.

#31 looks good to me too. i think this is OK to add to the 4.x and 5.x branches but perhaps someone can think of a reason why we should do this for 6.x only.

So I have pushed this to the master and 4.x branches, and my own use case is mostly fixed here.

Since there are still other places where the credentials are leaked, I have opened specific issues about this:

* #1600778: sql-sync leaks credentials
* #1600782: drush archive-dump leaks sql credentials
* #1589108: Drush doesn't clean up temporary files if interrupted (to a lesser extent)

sigh... I just realized this breaks the sql-connect, which points to a non-existing file...

@greg.1.anderson - i am having a hard time finding where in sql-sync the _drush_sql_get_credentials() function is being called. are you just assuming this breaks or have you tested it? :)

from what I can tell, it *does* call the _drush_sql_connect() function, which does call the _drush_sql_get_credentials(), so I have rerolled the patch to have a "insecure" flag to those functions so that they return the right values.

in the process I broke the tests and also pushed a fix for that. everything should now be in order...

i'll wait for feedback before porting this to 4.x.

I have merged this back into 4.x because otherwise sql-connect is just broken.

Tests pass, moving along. :)

The assumptions in #37 are all correct.

I'm showing that 4.x tests are broken after 06fb429. Haven't dug into it much yet, but it looks like it's archive-dump related.

I'd prefer that backports happen through the normal process in the queue, but if we are committing directly lets at least make sure tests pass beforehand.

Sorry about this, I am not very familiar with the test suite. We ran tests on 5.x but somehow I figured tests wouldn't fail on 4.x because the test file that was failing in 5.x wasn't present in 4.x...

I can take a look and fix this monday.

Alright, a bunch of commands in drush 4 are breaking in tests because they also depend on temporary files. This is now bug #1603744: can't create two temporary files, which this depends on for proper fixing in Drush 4.x.

I think I *may* have broken sql-sync after all, as it seems I have forgotten to add proper exceptions for sql-sync calls to _drush_sql_connect() so that it uses the insecure method. Not sure how I missed that. :(

Alright... the attached patch makes sql-sync work again on Drush 5.x, assuming it is broken now - I have no way of testing it now and tests do not find the issue mentionned by greg (which is another bug I guess).

Since I consider I have done enough damage as things stand (finding about 4 bugs along the way), I'll just leave the patch here instead of pushing it, and people more familiar with sql-sync can test this.

I have reverted the 3 commits to 5.x related to this patch, and reverted the 2 commits related to this patch on 4.x branch. All tests are passing now.

If someone wants to restart the effort on this issue, be sure to fix #1605998: Can't drush sql-sync from Mac OS X to Linux due to /tmp path difference and #1613418: Segmentation fault when drush site-install tries to install over existing database and maybe #1606798: Drush sql-sync: access denied as well. I'd also like to see expanded test coverage to cover the stuff we broke here along the way. This issue will be fixed on 6.x branch first and then possibly backported to 5.x if we are happy with it in the wild for a bit. It is no longer eligible for 4.x branch as I want to be conservative there.

Regarding #1605998, by inspection, it looked like the reverted change that was causing breakage there should have worked okay, but in practice it did not. I did not have time to investigate the cause. I think this is "the issue mentionned by [me]" referred to in #44. Note that it might be the case that 'remote-host' is not always set in $db_spec, even when the command is remote.

This is really disappointing. I do not understand why a serious security vulnerability would be continually disregarded like this.

What happened here is that I have forgotten one commit on the 5.x branch, it's a simple two line fix and I was planning on fixing it tomorrow (thursday). I had mentionned the issue to Mark on IRC, but this seems to have been lost in the noise, as tests were passing. Also, the 4.x branch was fine: no regression were reported against it so I don't see why it was reverted there also. This is especially frustrating to me since the release is shipped so the harm is already done.

I do not feel I was given sufficient time to fix my blunder before this was reverted and I hope this position can be reconsidered. Tests were passing on 5.x when I pushed my code, because, as mentioned earlier, tests seem to be lacking for sql-sync. And while it would be nice for those tests to exist, I do not feel responsible for creating those tests, although I do understand it is my duty to make sure I do not break existing functionality while fixing security issues.

I will work on providing a more complete patch tomorrow that will also fix the issues reported against sql-sync. It would be rather unfortunate to have to wait for a complete rearchitecturing of sql.drush.inc before fixing a security issue that allows anyone to root a drupal install just by looking at the process table. Especially since it doesn't seem like anyone is stepping up to actually do that rearchitecturing right now...

Sorry for the rant. I have attached the actual fix for the sql-sync problem, if someone who uses that command could test it, i would be grateful. See #1605998: Can't drush sql-sync from Mac OS X to Linux due to /tmp path difference. I assume #1606798: Drush sql-sync: access denied is a dupe.

sql-dump is now broken for me in 5.3

mysqldump: Got error: 2002: Can't connect to local MySQL server through socket '/tmp/mysql.sock' (2) when trying to connect
Database dump failed [error]

Reverting to 5.2, sql-dump works perfectly fine again. Having reviewed the diff between both versions, the only reason I can see for the failure in 5.3 is the new code referenced in this issue. Reading through the above thread, I'm a bit surprised by the process for reviewing and releasing such fundamental changes.

Is this on a remote alias?

It's a drush sql-dump command running locally, with the site settings holding the database details (the database runs on a separate server).

This is very strange. Can you show me:

1. the alias
2. the way you are calling sql-dump
3. a full --debug trace?

Thanks

@greg.1.anderson - I am trying to fix this in a clean way, including for sql-dump, but it doesn't look like the remote-host parameter gets propagated down to the sql-sync db_spec, i get this when trying to use it:

Undefined index: remote-host sync.sql.inc:400 [5.67 sec, 9.92 MB]                                                                                                                                                                      [notice]

Here's the dump of the target_url db_spec in there:

array (
  'driver' => 'mysql',
  'username' => 'test2orangeseeds',
  'password' => 'XXXXXX',
  'port' => '3306',
  'host' => 'localhost',
  'database' => 'test2orangeseeds',
)

This makes it very difficult to fix this for the general sql-dump case.

Note that I couldn't reproduce the issue mentionned in #49. With or without the patch, sql-dump works for me.

Yeah, 'remote-host' is stuffed into the $db_spec by sql-sync for its own nefarious purposes, but this is not standard behavior. I agree that this issue is difficult to fix. Drush assumes that it is running on a dedicated server, and has never tried to shield sensitive information from the process list. Perhaps the best short-term solution would be to only protect info when the user specifies an option on the command line. Then it would be up the user to not use this option when invoking commands on remote systems. More robust fixes would take considerably more effort.

Well, one way to fix this is to reverse the logic of the patch and make the insecure behavior the default and add the flag to secure critical paths like sqlc.

#55 sounds okay to me.

I should add, though, that ideally, sql-sync would check for and fail fast when secure mode is used with a remote site alias.

I understand that this is a tough nut to crack and I certainly appreciate all the effort here. My nickel on this would be that making the insecure method the default is almost as bad as doing nothing at all. It means that folks are relying on tired, overworked devs to remember the flag and that those who aren't as familiar with drush have to dig around to find out that the flag is there.

I guess a workaround would be to add an alias to everyone's .bashrc to force the flag all the time ( will that work? ). It still relies on someone in the organization to a) be smart enough to realize they can do that b) remember to add it by default to everyone's bash.

I do think this is a serious security concern. Many drupal sites are running on shared hosts and whether they're dedicated machines or not, you still run the risk of many users on one system. The threat threshold seems fairly high.

Thanks for your efforts, I'll climb off my soapbox now. :)

If this issue were fixed, it would probably also take care of #1103416: Drush does not correctly shell-escape certain special characters (e.g. in mysql db passwords).

This issue comes up when I'm using Drush as part of my deploy scripts. Essentially I have to pass Drush my MySQL credentials in the open and like the original poster mentioned, they become visible via the ps command and in the shell history.

Because I'm most concerned with deploys I think the solution could be limited to situations where there aren't already db credentials in a site, like durring install. During these install operations maybe Drush should give you the option to use a flag which would cause the command to read the db credentials from some standard locations like ~/.my.cnf and other (http://dev.mysql.com/doc/refman/5.1/en/option-files.html). I'm just spelling this out here since I hadn't seen any activity on this issue in a while and also wasn't sure if the above patches tackle the site-install scenario.

My remote sites are hosted on virtual private servers; in this environment, credential leaks are not an issue, because there are no other users on the system who might examine the process list, etc.

To make progress, we need someone to make a patch. I'll help with testing & will answer questions, but I'm not likely to have time to fix the whole thing by myself.

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.

I've copied this issue to the github project:
https://github.com/drush-ops/drush/issues/365

FYI, this is now fixed in Drush (master branch). Will be included in Drush 7. See https://github.com/drush-ops/drush/issues/365

@moshe weitzman: Thanks for the notice. I'll try it out soon.