#2183 Fixed .my.cnf messing up mysql connection + minor credential-leak fix

[reinier-vegter]

Please merge this PR, it fixes the problem where .my.cnf is used to connect to mysql.
Furthermore, the filemode-option in drush_save_data_to_temp_file() provides a way to prevent credential leakage, since that seemed to be the whole point of using the --defaults-extra-file param.
Prior to this commit the sql credentials were exposed in the open-to-anyone /tmp folder.

Created this on the 8.x branch, but maybe it's just possible to merge this 1:1 to the master branch (didn't check).

[reinier-vegter]

It seems this code hasn't changed in the master branch, so it should be possible to pick this commit.

[weitzman]

Drush's temp files are cleaned up in a shutdown function. They are only exposed momentarily. I dont see this as a security risk.

Many people actually like that Drush respects my.cnf. We need to cater to those people as well.

[reinier-vegter]

Hi @weitzman ,

"They are only exposed momentarily"
Exposed means exposed. This being for a short moment is still exposure, so since the filemode feature isn't implying any changes I suggest it's being kept.

BTW, 'cleaned up in a shutdown function' assumes that drush will not crash, which is impossible to predict because as far as I know drush loads contrib and custom code as well.

As for using .my.cnf: looking at the commits it seems that the --defaults-extra-file param was put there accidentally while it was just meant to hide credentials.
The credentials are written to a temp file, and by coincidence Mysql prefers ~/.my.cnf .
Further, I think you should scope Drush to Drupal, where the drupal-scope / -environment stops at the settings.php credentials. Makes much more sense imho.

[dsnopek]

I can't speak to the change from 'defaults-extra-file' to 'defaults-file' - that's unrelated to the security issue, so maybe it should be put in a different PR?

However, setting the mode of the temporary file to 0700 seems like a good and simple way to improve the security of this file. Even though the file should only exist for a short time and be removed, in a shared environment it does present a potential security risk. Why not set the mode to be safe? :-)

[weitzman]

I'm fine with the mode change. The defaults-file change here is insufficient for reasons described above.

[reinier-vegter]

What's your proposal @weitzman ?
A switch like --no-creds ? Because this also involves consistency around sql-connect for example.
If --no-creds is supplied, also 'drush sql-connect' should provide a connection string without credentials.

It's still kind of messy then, because you can also provide a hostname (and other options) in .my.cnf, so there will be no consistency around this without custom parsing of .my.cnf (or any other file in the mysql include chain for that matter), which seems hacky.

I'll happily create a PR for that switch, but currently I don't see an obvious, consistent way of resolving this without skipping the whole .my.cnf thing.

[CashWilliams]

Having a command open such as drush sqlc can expose the credentials for a very long time.

Could this be written something private, like ~/.drush/tmp instead?

[weitzman]

To accomplish that, Just set the environment variable TEMP=$HOME/.drush/tmp (see drush_find_tmp()). This can fix other problems when multiple users on same server share caches.This could potentially become a default but then cleanup would become Drush's responsibility instead of re-using the OS temp dirs.

[CashWilliams]

I've looked into this a bit more, and at least in the versions I've tested, the file is owned by whatever process owner ran the drush command, with restrictive file permissions already in place.

-rw------- 1 vagrant vagrant 100 Oct 13 14:19 drush_GMhdA8

[CashWilliams]

wouldn't this be 0600?

[reinier-vegter]

@CashWilliams, you're right about that one. Don't know why I put 700
there...
I'll fix that.

Op 13 okt. 2016 16:24 schreef "Cash Williams" [email protected]:

@CashWilliams commented on this pull request.

In lib/Drush/Sql/Sqlmysql.php
#2387 (review):

@@ -20,8 +20,8 @@ public function creds($hide_password = TRUE) {
password="{$this->db_spec['password']}"
EOT;

•  $file = drush_save_data_to_temp_file($contents);
  

•  $parameters['defaults-extra-file'] = $file;
  

•  $file = drush_save_data_to_temp_file($contents, NULL, 0700);
  

wouldn't this be 0600?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#2387 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AEShykLoQMSN3cit-o8F6s1DDI4Qvu2-ks5qzj8lgaJpZM4KUzYl
.

[reinier-vegter]

@CashWilliams , you seem to be right about the default restrictive filemode.
Since I just added this quickly while fixing the mysql params, I might have misread the resulting filemodes. Questions are,

• is the default filemode for fopen/fwrite configurable in the PHP environment ? Seems not to be the case (maybe OS handles this transparently).
• how does this behave on windows ? The temp-files are probably world-readable on those systems anyway, which raises the question if this is any better than leaving credentials in the sql connection string. I don't have an answer on that one.
• Is it better to specifically set filemode on files containing sensitive information, despite that it's probably not necessary on most systems ? I think so.

[dylantack]

After wrestling this with problem, I discovered some strange behavior in MySQL:

If multiple instances of a given option are found, the last instance takes precedence, with one exception: For mysqld, the first instance of the --user option is used as a security precaution, to prevent a user specified in an option file from being overridden on the command line.
(from http://dev.mysql.com/doc/refman/5.7/en/option-files.html)

The manual says this is for mysqld, but it appears to apply for mysql client as well. The result is that since the first user, and last password is selected, even if both /tmp/drush_jBJkJX and ~/.my.cnf have valid username/password combos, Drush will fail (since it's using an invalid combo of the settings.php user, but the ~/.my.cnf password).

Given this, I don't see how using the --defaults-extra-file would ever be desirable, since it doesn't really work as advertised, and is totally inconsistent with drush sql-connect.

@weitzman, can you say more about what users like about pulling the password from ~/.my.cnf? Perhaps if we understood the use case we could suggest a solution (for example, using --defaults-extra-file only if $databases is undefined).

[reinier-vegter]

@dylantack , See commit above.
I replaced 'defaults-extra-file' with 'defaults-file', which makes mysql-client only use the temp credentials-file, which indeed is more consistent with sql-connect.

BTW, it doesn't seem entirely true what you say; if you specify user and password (so both, not only password) in ~/.my.cnf, that combo will be used and will work. In that case the whole drupal credentials-combo is disregarded.

This might provide a workaround for people who use drush 8.x .

[alexpott]

I have to agree with the users that are saying we should switch this to defaults-file if you've got multiple database servers set up via docker it's awkward that your local .my.cnf gets in the way. Also this means that the credentials provided in your settings.php are preferred helping people who are wondering why drush can connect but maybe their site can not.

[weitzman]

I fixed this to use defaults-file in master branch. Will make a new release of master very soon. It should be fully released in a couple months so I'm not so inclined to back port. Some people depend on current behavior so back porting is tricksy.

[reinier-vegter]

ok..

[rivimey]

Having encountered this problem and looked at the various options available, I agree that drush should be using --defaults-file and not defaults-extra-file.

Please can we make this change permanent?