mariadb_ctrl job has problems with quotation marks in the password

[ericpromislow]

Feature Request Info

Problem you are trying to solve

Help us understand the problem you are facing. Please provide as much context as possible.

I did this:

$ credhub set --name '/bosh-lite/cf/cc_database_password' -t password -w $'abc-q:\'qq:"-def'
id: b5fed08e-2471-42a1-a8aa-85cf228a32ad
name: /bosh-lite/cf/cc_database_password
type: password
value: abc-q:'qq:"-def

and then deployed to a bosh-lite, and got this error message for the mysql job:

mariadb_ctrl.combined.log:{
"timestamp":"1530319643.075540066",
"source":"/var/vcap/packages/mariadb_ctrl/bin/mariadb_ctrl",
"message":"/var/vcap/packages/mariadb_ctrl/bin/mariadb_ctrl.Error 1064: 
     You have an error in your SQL syntax; 
    check the manual that corresponds to your MariaDB server version 
    for the right syntax to use near 'qq:\"-def')' at line 1","log_level":1,"data":{}}

In /var/vcap/jobs/mysql/config, there are two key lines of code:

mariadb_ctl_config.yml:    Password: abc-q:'qq:"-def

The value is fine in an unquoted yaml string, as shown in this irb session:

○ → irb
irb(main):001:0> require 'yaml'
=> true
irb(main):002:0> s = <<EOT
irb(main):003:0" Db:
irb(main):004:0"   DaemonPath: /var/vcap/packages/mariadb_ctrl/bin/mysql_daemon.sh
irb(main):005:0"   UpgradePath: /var/vcap/packages/mariadb/bin/mysql_upgrade
irb(main):006:0"   User: root
irb(main):007:0"   Password: eEmsPEGT2RwazUi0Cj05cKT53pdIS5
irb(main):008:0"   PreseededDatabases:
irb(main):009:0" 
irb(main):010:0"   - DBName: cloud_controller
irb(main):011:0"     User: cloud_controller
irb(main):012:0"     Password: abc-q:'qq:"-def
irb(main):013:0" EOT
=> "Db:\n  DaemonPath: /var/vcap/packages/mariadb_ctrl/bin/mysql_daemon.sh\n  UpgradePath: /var/vcap/packages/mariadb/bin/mysql_upgrade\n  User: root\n  Password: eEmsPEGT2RwazUi0Cj05cKT53pdIS5\n  PreseededDatabases:\n\n  - DBName: cloud_controller\n    User: cloud_controller\n    Password: abc-q:'qq:\"-def\n"
irb(main):014:0> x = YAML.load(s)
=> {"Db"=>{"DaemonPath"=>"/var/vcap/packages/mariadb_ctrl/bin/mysql_daemon.sh", "UpgradePath"=>"/var/vcap/packages/mariadb/bin/mysql_upgrade", "User"=>"root", "Password"=>"eEmsPEGT2RwazUi0Cj05cKT53pdIS5", "PreseededDatabases"=>[{"DBName"=>"cloud_controller", "User"=>"cloud_controller", "Password"=>"abc-q:'qq:\"-def"}]}}
irb(main):019:0> print x['Db']['PreseededDatabases'][0]['Password']
abc-q:'qq:"-def

The error message suggests there's some unsafe SQL injection taking place somewhere -- not in this repo, as the only hit on PreseededDatabases is in the template with the above yaml.

Also, I would quote a string like this with double-quotes, backslash-escape any double-quotes and hex-encode a few other characters to support non-alnum characters in password fields -- see https://github.com/cloudfoundry/capi-release/commit/f76291eeb82a8f04629b9a74281c7b96dc6a2703 for details

Proposed solution/feature

What is the proposed solution and why do you think it is the best approach to the problem above.

Outlined above

Would you want to open an PR for this feature?

Sure, I'll do this next week. Easy to quote the yaml; it would be helpful if someone could point to which mysql client is failing to inject the password correctly, or how characters in the password field like quotation marks should be encoded.

Bug Report Info

Currently:

What is the current broken behavior?

• mysql fails to start

Expected:

What the correct behavior should be?

• mysql should start and run as expected

Steps to Reproduce:

see above

Deployment Context:

This is on a bosh-lite

Reference:

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/158741786

The labels on this github issue will be updated when the story is started.

[jpalermo]

Hey @ericpromislow, thanks for reporting the issue and including all the details.

We took a look at the problem today. It seems like YAML quoting is part of the problem, and the other part that you found is coming from seeding the DBs, Users, and Passwords in here: https://github.com/cloudfoundry/mariadb_ctrl/blob/master/mariadb_helper/seeder/seeder.go

We were able to solve this problem, but in doing so, we found several other spots that also take DB user passwords that need to be escaped. Some of those are directed to SQL, others end up in mysql defaults files. The escaping between the two is totally different.

While this is a solvable problem, at this point we feel like the code complexity introduced by it is not worth the value provided by escaping the strings.

The release assumes a level of trust with the Operator deploying it. So while it is possible for the Operator to inject sql statements via the password, they are already in possession of the admin credentials.

The current recommended way of generating passwords is using the bosh cli with a --vars-store or using credhub from the bosh director itself. The passwords generated by these two methods do not contain characters that need escaping.

[ericpromislow]

... and I actually agree that you can achieve the needed entropy in a password by selecting N1 characters from a small set of candidates over N2 characters (where N2 < N1) from the full set of printable characters, but wanted to raise the issue. These passwords all live within the CF universe, and aren't subject to issues where end-users enter their own passwords and get disallowed character error messages when trying to create harder-to-guess passwords.

[jpalermo]

Agreed. Thanks for raising the issue. I think it was something we hadn't really considered before and it gave us a chance to dig around and see exactly what the risks were.