Skip to content

Commit ab7a695

Browse files
david22swandavid22swan
committed
(maint) Codebase Hardening
Changes made to ensure that no malformed commands are passed through to the system. Certain commands were left undivided as the commands did not get correctly interpreted and so a shell_escape was used instead.
1 parent c2806e9 commit ab7a695

File tree

7 files changed

+46
-25
lines changed

7 files changed

+46
-25
lines changed

‎manifests/server/config.pp

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -142,8 +142,11 @@
142142

143143
ensure_packages([$package_name])
144144

145+
$exec_command = ['/usr/sbin/semanage', 'port', '-a', '-t', 'postgresql_port_t', '-p', 'tcp', $port]
146+
$exec_unless = "/usr/sbin/semanage port -l | grep -qw ${port}"
145147
exec { "/usr/sbin/semanage port -a -t postgresql_port_t -p tcp ${port}":
146-
unless => "/usr/sbin/semanage port -l | grep -qw ${port}",
148+
command => $exec_command,
149+
unless => $exec_unless,
147150
before => Postgresql::Server::Config_entry['port'],
148151
require => Package[$package_name],
149152
}
@@ -218,8 +221,9 @@
218221
#
219222
# This can be removed when Puppet < 6.1 support is dropped *and* the file
220223
# old-systemd-override is removed.
224+
$systemd_command = ['systemctl', 'daemon-reload']
221225
exec { 'restart-systemd':
222-
command => 'systemctl daemon-reload',
226+
command => $systemd_command,
223227
refreshonly => true,
224228
path => '/bin:/usr/bin:/usr/local/bin',
225229
before => Class['postgresql::server::service'],

‎manifests/server/config_entry.pp

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -98,10 +98,13 @@
9898
# is managed for us in postgresql::server::config.
9999
if $facts['os']['name'] == 'Debian' or $facts['os']['name'] == 'Ubuntu' {
100100
if $name == 'data_directory' {
101+
$stop_command = ['service', $postgresql::server::service_name, 'stop']
102+
$stop_onlyif = ['service', $postgresql::server::service_name, 'status']
103+
$stop_unless = [['grep', "data_directory = '${value}'", $postgresql::server::postgresql_conf_path]]
101104
exec { "postgresql_stop_${name}":
102-
command => "service ${postgresql::server::service_name} stop",
103-
onlyif => "service ${postgresql::server::service_name} status",
104-
unless => "grep \"data_directory = '${value}'\" ${postgresql::server::postgresql_conf_path}",
105+
command => $stop_command,
106+
onlyif => $stop_onlyif,
107+
unless => $stop_unless,
105108
path => '/usr/sbin:/sbin:/bin:/usr/bin:/usr/local/bin',
106109
before => Postgresql_conf[$name],
107110
}
@@ -111,10 +114,13 @@
111114
# We need to force postgresql to stop before updating the port
112115
# because puppet becomes confused and is unable to manage the
113116
# service appropriately.
117+
$stop_command = ['service', $postgresql::server::service_name, 'stop']
118+
$stop_onlyif = ['service', $postgresql::server::service_name, 'status']
119+
$stop_unless = "grep 'PGPORT=${shell_escape($value)}' /etc/sysconfig/pgsql/postgresql"
114120
exec { "postgresql_stop_${name}":
115-
command => "service ${postgresql::server::service_name} stop",
116-
onlyif => "service ${postgresql::server::service_name} status",
117-
unless => "grep 'PGPORT=${value}' /etc/sysconfig/pgsql/postgresql",
121+
command => $stop_command,
122+
onlyif => $stop_onlyif,
123+
unless => $stop_unless,
118124
path => '/sbin:/bin:/usr/bin:/usr/local/bin',
119125
require => File['/etc/sysconfig/pgsql/postgresql'],
120126
}
@@ -130,10 +136,13 @@
130136
} elsif $name == 'data_directory' {
131137
# We need to force postgresql to stop before updating the data directory
132138
# otherwise init script breaks
139+
$stop_command = ['service', $postgresql::server::service_name, 'stop']
140+
$stop_onlyif = ['service', $postgresql::server::service_name, 'status']
141+
$stop_unless = [['grep', "PGDATA=${value}", '/etc/sysconfig/pgsql/postgresql']]
133142
exec { "postgresql_${name}":
134-
command => "service ${postgresql::server::service_name} stop",
135-
onlyif => "service ${postgresql::server::service_name} status",
136-
unless => "grep 'PGDATA=${value}' /etc/sysconfig/pgsql/postgresql",
143+
command => $stop_command,
144+
onlyif => $stop_onlyif,
145+
unless => $stop_unless,
137146
path => '/sbin:/bin:/usr/bin:/usr/local/bin',
138147
require => File['/etc/sysconfig/pgsql/postgresql'],
139148
}

‎manifests/server/passwd.pp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
# psql will default to connecting as $user if you don't specify name
1717
$_datbase_user_same = $database == $user
1818
$_dboption = $_datbase_user_same ? {
19-
false => " --dbname ${database}",
19+
false => " --dbname ${shell_escape($database)}",
2020
default => ''
2121
}
2222

@@ -26,10 +26,11 @@
2626
# without specifying a password ('ident' or 'trust' security). This is
2727
# the default for pg_hba.conf.
2828
$escaped = postgresql::postgresql_escape($postgres_password)
29+
$exec_command = "${shell_escape($psql_path)}${_dboption} -c \"ALTER ROLE \\\"${shell_escape($user)}\\\" PASSWORD \${NEWPASSWD_ESCAPED}\""
2930
exec { 'set_postgres_postgrespw':
3031
# This command works w/no password because we run it as postgres system
3132
# user
32-
command => "${psql_path}${_dboption} -c \"ALTER ROLE \\\"${user}\\\" PASSWORD \${NEWPASSWD_ESCAPED}\"",
33+
command => $exec_command,
3334
user => $user,
3435
group => $group,
3536
logoutput => true,

‎manifests/validate_db_connection.pp

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -44,29 +44,29 @@
4444
$module_workdir = $postgresql::params::module_workdir
4545
$validcon_script_path = $postgresql::client::validcon_script_path
4646

47-
$cmd_init = "${psql_path} --tuples-only --quiet "
47+
$cmd_init = "${psql_path} --tuples-only --quiet"
4848
$cmd_host = $database_host ? {
4949
undef => '',
50-
default => "-h ${database_host} ",
50+
default => "-h ${database_host}",
5151
}
5252
$cmd_user = $database_username ? {
5353
undef => '',
54-
default => "-U ${database_username} ",
54+
default => "-U ${database_username}",
5555
}
5656
$cmd_port = $database_port ? {
5757
undef => '',
58-
default => "-p ${database_port} ",
58+
default => "-p ${database_port}",
5959
}
6060
$cmd_dbname = $database_name ? {
61-
undef => "--dbname ${postgresql::params::default_database} ",
62-
default => "--dbname ${database_name} ",
61+
undef => "--dbname ${postgresql::params::default_database}",
62+
default => "--dbname ${database_name}",
6363
}
6464
$pass_env = $database_password_unsensitive ? {
6565
undef => undef,
6666
default => "PGPASSWORD=${database_password_unsensitive}",
6767
}
6868
$cmd = join([$cmd_init, $cmd_host, $cmd_user, $cmd_port, $cmd_dbname], ' ')
69-
$validate_cmd = "${validcon_script_path} ${sleep} ${tries} '${cmd}'"
69+
$validate_cmd = [$validcon_script_path, $sleep, $tries, $cmd]
7070

7171
# This is more of a safety valve, we add a little extra to compensate for the
7272
# time it takes to run each psql command.
@@ -86,9 +86,10 @@
8686
}
8787

8888
$exec_name = "validate postgres connection for ${database_username}@${database_host}:${database_port}/${database_name}"
89+
$exec_command = "echo 'Unable to connect to defined database using: ${shell_escape($cmd)}' && false"
8990

9091
exec { $exec_name:
91-
command => "echo 'Unable to connect to defined database using: ${cmd}' && false",
92+
command => $exec_command,
9293
unless => $validate_cmd,
9394
cwd => $module_workdir,
9495
environment => $env,

‎spec/classes/server/config_spec.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@
4141

4242
it 'has systemctl restart command' do
4343
is_expected.to contain_exec('restart-systemd').with(
44-
command: 'systemctl daemon-reload',
44+
command: ['systemctl', 'daemon-reload'],
4545
refreshonly: true,
4646
path: '/bin:/usr/bin:/usr/local/bin',
4747
)

‎spec/classes/server_spec.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ class { 'postgresql::globals':
8383
is_expected.to contain_postgresql_conn_validator('validate_service_is_running')
8484
end
8585
it 'sets postgres password' do
86-
is_expected.to contain_exec('set_postgres_postgrespw').with('command' => '/usr/bin/psql -c "ALTER ROLE \"postgres\" PASSWORD ${NEWPASSWD_ESCAPED}"',
86+
is_expected.to contain_exec('set_postgres_postgrespw').with('command' => ['/usr/bin/psql -c "ALTER ROLE \"postgres\" PASSWORD ${NEWPASSWD_ESCAPED}"'],
8787
'user' => 'postgres',
8888
'environment' => ['PGPASSWORD=new-p@s$word-to-set', 'PGPORT=5432', 'NEWPASSWD_ESCAPED=$$new-p@s$word-to-set$$'],
8989
'unless' => "/usr/bin/psql -h localhost -p 5432 -c 'select 1' > /dev/null")

‎spec/defines/validate_db_connection_spec.rb

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,10 @@
3030
it { is_expected.to contain_postgresql__validate_db_connection('test') }
3131

3232
it 'has proper path for validate command' do
33-
is_expected.to contain_exec('validate postgres connection for test@test:5432/test').with(unless: %r{^/usr/local/bin/validate_postgresql_connection.sh\s+\d+})
33+
is_expected.to contain_exec('validate postgres connection for test@test:5432/test').with(unless: ['/usr/local/bin/validate_postgresql_connection.sh',
34+
4,
35+
30,
36+
'/usr/bin/psql --tuples-only --quiet -h test -U test -p 5432 --dbname test'])
3437
end
3538
end
3639

@@ -55,7 +58,10 @@ class { 'postgresql::client': validcon_script_path => '/opt/something/validate.s
5558
end
5659

5760
it 'has proper path for validate command and correct cwd' do
58-
is_expected.to contain_exec('validate postgres connection for test@test:5432/test').with(unless: %r{^/opt/something/validate.sh\s+\d+},
61+
is_expected.to contain_exec('validate postgres connection for test@test:5432/test').with(unless: ['/opt/something/validate.sh',
62+
2,
63+
10,
64+
'/usr/bin/psql --tuples-only --quiet -h test -U test -p 5432 --dbname test'],
5965
cwd: '/var/tmp')
6066
end
6167
end

0 commit comments

Comments
 (0)