Skip to content

Commit

Permalink
Harden root_password class
Browse files Browse the repository at this point in the history 
Prior to this commit there was a possibility that malformed
strings could be passed in to the resource. This could lead
to unsafe executions on a remote system.

The parameters that are susceptible are `install_secret_file`.

This commit fixes the above by adding validation to ensure the given
values confirm to expectation.

`secret_file` is validated with a regular expression that ensures the
given value is a valid path.
  • Loading branch information
@chelnak
chelnak committed Aug 23, 2022
1 parent f83792b commit 90168d9
Show file tree
Hide file tree
Showing 4 changed files with 4 additions and 11 deletions.
1 change: 0 additions & 1 deletion manifests/params.pp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
$purge_conf_dir = false
$restart = false
$root_password = 'UNSET'
$install_secret_file = '/.mysql_secret'
$server_package_ensure = 'present'
$server_package_manage = true
$server_service_manage = true
Expand Down
3 changes: 0 additions & 3 deletions manifests/server.pp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@
# The location, as a path, of !includedir for custom configuration overrides.
# @param install_options
# Passes [install_options](https://docs.puppetlabs.com/references/latest/type.html#package-attribute-install_options) array to managed package resources. You must pass the appropriate options for the specified package manager
# @param install_secret_file
# Path to secret file containing temporary root password.
# @param manage_config_file
# Whether the MySQL configuration file should be managed. Valid values are `true`, `false`. Defaults to `true`.
# @param options
Expand Down Expand Up @@ -81,7 +79,6 @@
$config_file_mode = $mysql::params::config_file_mode,
$includedir = $mysql::params::includedir,
$install_options = undef,
$install_secret_file = $mysql::params::install_secret_file,
$manage_config_file = $mysql::params::manage_config_file,
Mysql::Options $options = {},
$override_options = {},
Expand Down
9 changes: 2 additions & 7 deletions manifests/server/root_password.pp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,15 @@
}

$options = $mysql::server::_options
$secret_file = $mysql::server::install_secret_file
$login_file = $mysql::server::login_file

# New installations of MySQL will configure a default random password for the root user
# with an expiration. No actions can be performed until this password is changed. The
# below exec will remove this default password. If the user has supplied a root
# password it will be set further down with the mysql_user resource.
$rm_pass_cmd = join([
"mysqladmin -u root --password=\$(grep -o '[^ ]\\+\$' ${secret_file}) password ''",
"rm -f ${secret_file}",
], ' && ')
exec { 'remove install pass':
command => $rm_pass_cmd,
onlyif => "test -f ${secret_file}",
command => "mysqladmin -u root --password=\$(grep -o '[^ ]\\+\$' /.mysql_secret) password && (rm -f /.mysql_secret; exit 0) || (rm -f /.mysql_secret; exit 1)",
onlyif => [['test', '-f' ,'/.mysql_secret']],
path => '/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/bin:/usr/local/sbin',
}

Expand Down
2 changes: 2 additions & 0 deletions pdk.yaml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
---
ignore: []

6 comments on commit 90168d9

@anarcat
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

er. what's going on here? you take a command that was split up in multiple elements of an array and just merge it all together in one list? and then the hardened part is basically the commadn test -f /.mysql_secret?

seems like a missed opportunity here...

@chelnak
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anarcat PRs are welcome! Alternatively, open an issue and we can get this refactored.

@anarcat
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right, of course... i'm a little busy trying to mitigate #1513 in puppet 5 right now, which involves basically rolling back this patch and a few others... ;) so this is a little far down my list of priorities, unfortunately...

@kjetilho
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A safer technique than interpolating Puppet variables into shell commands is to let the shell do the interpolation itself.

exec { 'remove install pass':
  environment => [ "SECRET_FILE=${secret_file}" ],
  onlyif  => 'test -f "$SECRET_FILE"',
  ...
}

The above code is safe for all possible characters in $secret_file.

@kjetilho
Copy link
Contributor

@kjetilho kjetilho commented on 90168d9 Dec 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's worse is that the patch removes the empty password which is set, so it will prompt interactively for a new password! Also it doesn't solve the potential problem with whitespace in the install password. The below version fixes the issues. It keeps the extraction of only last word of secret, but it could be replaced by a cat if desired.

$rm_pass_cmd = join([
      'mysqladmin -u root --password="$(egrep -o "[^ ]+$" "${SECRET_FILE}")" password ""',
      'rm -f "${SECRET_FILE}"',
  ], ' && ')

@chelnak
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good morning!

Some good suggestions here @kjetilho.

Would you mind putting them in to a new issue (if it’s not already)? It will be much easier to track also it’s much more visible to others.

Thank you!

Please sign in to comment.