From 547483f3816e7d8b0975992073e003feea8833ef Mon Sep 17 00:00:00 2001 From: Craig Gumbley Date: Tue, 16 Aug 2022 14:00:07 +0000 Subject: [PATCH 1/4] Harden db defined type 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 `dbname`, `sql` and `import_cat_cmd`. In addition, commands were not properly broken out in to arrays of arguments when passed to the exec resource. This commit fixes the above by adding validation to parameters (where possible) to ensure that the given values conform to expectation. `dbname` is validated with a regular expression that ensures that it matches expectations set by mysql. `sql` now only accepts values as an `Array[String]`. This can be one or more value. Values are also validated with a regular expression that ensures that the given paths are valid. `import_cat_cmd` has been removed in favour of a boolean parameter called `use_zcat`. Setting this parameter to `true` will set the value of the private `import_cat_cmd` to `zcat`. This commit also contains some minor refactoring. --- manifests/db.pp | 64 +++++++++++++++++++++++++++++++++---------------- 1 file changed, 43 insertions(+), 21 deletions(-) diff --git a/manifests/db.pp b/manifests/db.pp index e3c6a9899..4ff2b288e 100644 --- a/manifests/db.pp +++ b/manifests/db.pp @@ -9,6 +9,12 @@ # grant => ['SELECT', 'UPDATE'], # } # +# @param name +# The name of the database to create. Database names must: +# * be longer than 64 characters. +# * not contain / \ or . characters. +# * not contain characters that are not permitted in file names. +# * not end with space characters. # @param user # The user for the database you're creating. # @param password @@ -28,7 +34,7 @@ # @param grant_options # The grant_options for the grant for user@host on the database. # @param sql -# The path to the sqlfile you want to execute. This can be single file specified as string, or it can be an array of strings. +# The path to the sqlfile you want to execute. This can be a an array containing one or more file paths. # @param enforce_sql # Specifies whether executing the sqlfiles should happen on every run. If set to false, sqlfiles only run once. # @param ensure @@ -41,25 +47,41 @@ define mysql::db ( $user, Variant[String, Sensitive[String]] $password, - $tls_options = undef, - $dbname = $name, - $charset = 'utf8', - $collate = 'utf8_general_ci', - $host = 'localhost', - $grant = 'ALL', - $grant_options = undef, - Optional[Variant[Array, Hash, String]] $sql = undef, - $enforce_sql = false, - Enum['absent', 'present'] $ensure = 'present', - $import_timeout = 300, - $import_cat_cmd = 'cat', - $mysql_exec_path = undef, + $tls_options = undef, + String $dbname = $name, + $charset = 'utf8', + $collate = 'utf8_general_ci', + $host = 'localhost', + $grant = 'ALL', + $grant_options = undef, + Optional[Array] $sql = undef, + $enforce_sql = false, + Enum['absent', 'present'] $ensure = 'present', + $import_timeout = 300, + Enum['cat', 'zcat'] $import_cat_cmd = 'cat', + $mysql_exec_path = undef, ) { - $table = "${dbname}.*" + include 'mysql::client' - $sql_inputs = join([$sql], ' ') + # Ensure that the database name is valid. + if $dbname !~ /^[^\/?%*:|\""<>.\s;]{1,64}$/ { + $message = "The database name '${dbname}' is invalid. Values must: + * be longer than 64 characters. + * not contain // \\ or . characters. + * not contain characters that are not permitted in file names. + * not end with space characters." + fail($message) + } - include 'mysql::client' + # Ensure that the sql files passed are valid file paths. + if $sql { + $sql.each | $sqlfile | { + if $sqlfile !~ /^\/(?:[A-Za-z0-9_-]+\/?+)+(?:.[A-Za-z0-9]+)$/ { + $message = "The file '${sqlfile}' is invalid. A a valid file path is expected." + fail($message) + } + } + } if ($mysql_exec_path) { $_mysql_exec_path = $mysql_exec_path @@ -84,6 +106,8 @@ ensure_resource('mysql_user', "${user}@${host}", $user_resource) if $ensure == 'present' { + $table = "${dbname}.*" + mysql_grant { "${user}@${host}/${table}": privileges => $grant, provider => 'mysql', @@ -96,14 +120,12 @@ ], } - $refresh = ! $enforce_sql - if $sql { exec { "${dbname}-import": - command => "${import_cat_cmd} ${sql_inputs} | mysql ${dbname}", + command => "${import_cat_cmd} ${shell_join($sql)} | mysql ${dbname}", logoutput => true, environment => "HOME=${::root_home}", - refreshonly => $refresh, + refreshonly => ! $enforce_sql, path => "/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/bin:${_mysql_exec_path}", require => Mysql_grant["${user}@${host}/${table}"], subscribe => Mysql_database[$dbname], From 211de4fcb87ba4002f06909e29285c3f4c0fcc8a Mon Sep 17 00:00:00 2001 From: Craig Gumbley Date: Tue, 16 Aug 2022 14:08:15 +0000 Subject: [PATCH 2/4] Add spec tests This commit adds spec tests for the the changes made in the previous commit. --- spec/defines/mysql_db_spec.rb | 75 ++++++++++++++++++++++++++++++----- 1 file changed, 66 insertions(+), 9 deletions(-) diff --git a/spec/defines/mysql_db_spec.rb b/spec/defines/mysql_db_spec.rb index 688e7b701..edbd94151 100644 --- a/spec/defines/mysql_db_spec.rb +++ b/spec/defines/mysql_db_spec.rb @@ -17,40 +17,42 @@ 'mysql_exec_path' => '' } end + let(:sql) { [ '/tmp/test.sql' ] } + it 'does not notify the import sql exec if no sql script was provided' do is_expected.to contain_mysql_database('test_db').without_notify end it 'subscribes to database if sql script is given' do - params['sql'] = 'test_sql' + params['sql'] = sql is_expected.to contain_mysql_database('test_db') is_expected.to contain_exec('test_db-import').with_subscribe('Mysql_database[test_db]') end it 'onlies import sql script on creation if not enforcing' do - params.merge!('sql' => 'test_sql', 'enforce_sql' => false) + params.merge!('sql' => sql, 'enforce_sql' => false) is_expected.to contain_exec('test_db-import').with_refreshonly(true) end it 'imports sql script on creation' do - params.merge!('sql' => 'test_sql', 'enforce_sql' => true) + params.merge!('sql' => sql, 'enforce_sql' => true) # ' if enforcing #refreshonly' is_expected.to contain_exec('test_db-import').with_refreshonly(false) # 'if enforcing #command' - is_expected.to contain_exec('test_db-import').with_command('cat test_sql | mysql test_db') + is_expected.to contain_exec('test_db-import').with_command('cat /tmp/test.sql | mysql test_db') end - it 'imports sql script with custom command on creation ' do - params.merge!('sql' => 'test_sql', 'enforce_sql' => true, 'import_cat_cmd' => 'zcat') + it 'imports sql script with custom command on creation' do + params.merge!('sql' => sql, 'enforce_sql' => true, 'import_cat_cmd' => 'zcat') # if enforcing #refreshonly is_expected.to contain_exec('test_db-import').with_refreshonly(false) # if enforcing #command - is_expected.to contain_exec('test_db-import').with_command('zcat test_sql | mysql test_db') + is_expected.to contain_exec('test_db-import').with_command('zcat /tmp/test.sql | mysql test_db') end it 'imports sql scripts when more than one is specified' do - params['sql'] = ['test_sql', 'test_2_sql'] - is_expected.to contain_exec('test_db-import').with_command('cat test_sql test_2_sql | mysql test_db') + params['sql'] = ['/tmp/test.sql', '/tmp/test_2.sql'] + is_expected.to contain_exec('test_db-import').with_command('cat /tmp/test.sql /tmp/test_2.sql | mysql test_db') end it 'does not create database' do @@ -79,6 +81,61 @@ params['grant_options'] = ['GRANT'] is_expected.to contain_mysql_grant('testuser@localhost/test_db.*').with_options(['GRANT']) end + + # Invalid file paths + [ + '|| ls -la ||', + '|| touch /tmp/foo.txt ||', + '/tmp/foo.txt;echo', + 'myPath;', + '\\myPath\\', + '//myPath has spaces//', + '/', + ].each do |path| + it "fails when provided '#{path}' as a value to the 'sql' parameter" do + params['sql'] = [path] + is_expected.to raise_error(Puppet::PreformattedError, %r{The file '#{Regexp.escape(path)}' is invalid. A a valid file path is expected.}) + end + end + + # Valid file paths + [ + '/tmp/test.txt', + '/tmp/.test', + '/foo.test', + ].each do |path| + it "succeeds when provided '#{path}' as a value to the 'sql' parameter" do + params['sql'] = [path] + is_expected.to contain_exec('test_db-import').with_command("cat #{path} | mysql test_db") + end + end + + # Invalid database names + [ + 'test db', + 'test_db;', + 'test/db', + '|| ls -la ||', + '|| touch /tmp/foo.txt ||', + ].each do |name| + it "fails when provided '#{name}' as a value to the 'name' parameter" do + params['name'] = name + is_expected.to raise_error(Puppet::PreformattedError, %r{The database name '#{name}' is invalid.}) + end + end + + # Valid database names + [ + 'test_db', + 'testdb', + 'test-db', + 'TESTDB', + ].each do |name| + it "succeeds when the provided '#{name}' as a value to the 'dbname' parameter" do + params['dbname'] = name + is_expected.to contain_mysql_database(name) + end + end end end end From 603287f78a485826e04da30e4cd2c01f8e187e97 Mon Sep 17 00:00:00 2001 From: Craig Gumbley Date: Wed, 17 Aug 2022 15:43:50 +0000 Subject: [PATCH 3/4] Fixes for acceptance tests This commit fixes some small issues where tests were failing because of a type mismatch. --- spec/acceptance/01_mysql_db_spec.rb | 2 +- spec/acceptance/types/mysql_grant_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/acceptance/01_mysql_db_spec.rb b/spec/acceptance/01_mysql_db_spec.rb index b00b68277..c16dd61d6 100644 --- a/spec/acceptance/01_mysql_db_spec.rb +++ b/spec/acceptance/01_mysql_db_spec.rb @@ -43,9 +43,9 @@ class { 'mysql::server': override_options => { 'root_password' => 'password' } } mysql::db { 'spec2': user => 'root1', password => 'password', - sql => '/tmp/spec.sql', charset => '#{fetch_charset}', collate => '#{fetch_charset}_general_ci', + sql => ['/tmp/spec.sql'], } MANIFEST end diff --git a/spec/acceptance/types/mysql_grant_spec.rb b/spec/acceptance/types/mysql_grant_spec.rb index e39eace53..942eb938d 100644 --- a/spec/acceptance/types/mysql_grant_spec.rb +++ b/spec/acceptance/types/mysql_grant_spec.rb @@ -686,8 +686,8 @@ class { 'mysql::server': override_options => { 'root_password' => 'password' } } mysql::db { 'grant_spec_db': user => 'root1', password => 'password', - sql => '/tmp/grant_spec_table.sql', charset => '#{fetch_charset}', + sql => ['/tmp/grant_spec_table.sql'], } MANIFEST it 'creates table' do From cd2e4851505d5a7d75f89d1e6097013e64dddf89 Mon Sep 17 00:00:00 2001 From: Craig Gumbley Date: Tue, 23 Aug 2022 07:11:02 +0100 Subject: [PATCH 4/4] Update manifests/db.pp Co-authored-by: Ben Ford --- manifests/db.pp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/manifests/db.pp b/manifests/db.pp index 4ff2b288e..baf681273 100644 --- a/manifests/db.pp +++ b/manifests/db.pp @@ -58,7 +58,7 @@ $enforce_sql = false, Enum['absent', 'present'] $ensure = 'present', $import_timeout = 300, - Enum['cat', 'zcat'] $import_cat_cmd = 'cat', + Enum['cat', 'zcat', 'bzcat'] $import_cat_cmd = 'cat', $mysql_exec_path = undef, ) { include 'mysql::client'