From c26ad2a54f318b4d6fbe55f837b00cd6afd9f1eb Mon Sep 17 00:00:00 2001 From: Craig Gumbley Date: Thu, 11 Aug 2022 15:20:36 +0000 Subject: [PATCH] Harden PPA defined type Prior to this commit there was a possibility that malformed strings could be passed as the resources name. This could lead to unsafe executions on a remote system. This was also a possibility for the options parameter as it was constrained to a string. 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 the resource name ensuring that the given ppa name conforms to expectation. Also, commands are now broken down in to arrays of arguments appropriately. This ensures safer execution on the remote system. Given that the options parameter, passed as a raw string, could lead to unsafe code execution it was reasonable to change the accepted type to an `Optional[Array[String]]. This means that an array of options can now be passed to the exec resource inside the original command. --- examples/ppa.pp | 2 +- lib/facter/apt_sources.rb | 13 +++++ manifests/init.pp | 75 +++++++++++++++-------------- manifests/params.pp | 2 +- manifests/ppa.pp | 60 +++++++++++++++-------- templates/add-apt-repository.sh.epp | 8 +++ 6 files changed, 102 insertions(+), 58 deletions(-) create mode 100644 lib/facter/apt_sources.rb create mode 100644 templates/add-apt-repository.sh.epp diff --git a/examples/ppa.pp b/examples/ppa.pp index 1dea2b4..4f4432a 100644 --- a/examples/ppa.pp +++ b/examples/ppa.pp @@ -1,4 +1,4 @@ class { 'apt': } # Example declaration of an Apt PPA -apt::ppa { 'ppa:openstack-ppa/bleeding-edge': } +apt::ppa { 'ppa:ubuntuhandbook1/apps': } diff --git a/lib/facter/apt_sources.rb b/lib/facter/apt_sources.rb new file mode 100644 index 0000000..c985802 --- /dev/null +++ b/lib/facter/apt_sources.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +# This fact lists the .list filenames that are used by apt. +Facter.add(:apt_sources) do + confine osfamily: 'Debian' + setcode do + sources = ['sources.list'] + Dir.glob('/etc/apt/sources.list.d/*.list').each do |file| + sources.push(File.basename(file)) + end + sources + end +end diff --git a/manifests/init.pp b/manifests/init.pp index 7043110..2de585c 100644 --- a/manifests/init.pp +++ b/manifests/init.pp @@ -37,10 +37,14 @@ # Configures various update settings. Valid options: a hash made up from the following keys: # # @option update [String] :frequency -# Specifies how often to run `apt-get update`. If the exec resource `apt_update` is notified, `apt-get update` runs regardless of this value. -# Valid options: 'always' (at every Puppet run); 'daily' (if the value of `apt_update_last_success` is less than current epoch time minus 86400); -# 'weekly' (if the value of `apt_update_last_success` is less than current epoch time minus 604800); and 'reluctantly' (only if the exec resource -# `apt_update` is notified). Default: 'reluctantly'. +# Specifies how often to run `apt-get update`. If the exec resource `apt_update` is notified, +# `apt-get update` runs regardless of this value. +# Valid options: +# 'always' (at every Puppet run); +# daily' (if the value of `apt_update_last_success` is less than current epoch time minus 86400); +# 'weekly' (if the value of `apt_update_last_success` is less than current epoch time minus 604800); +# 'reluctantly' (only if the exec resource `apt_update` is notified). +# Default: 'reluctantly'. # # @option update [Integer] :loglevel # Specifies the log level of logs outputted to the console. Default: undef. @@ -122,38 +126,37 @@ # Specifies whether to perform force purge or delete. Default false. # class apt ( - Hash $update_defaults = $apt::params::update_defaults, - Hash $purge_defaults = $apt::params::purge_defaults, - Hash $proxy_defaults = $apt::params::proxy_defaults, - Hash $include_defaults = $apt::params::include_defaults, - String $provider = $apt::params::provider, - String $keyserver = $apt::params::keyserver, - Optional[String] $key_options = $apt::params::key_options, - Optional[String] $ppa_options = $apt::params::ppa_options, - Optional[String] $ppa_package = $apt::params::ppa_package, - Optional[Hash] $backports = $apt::params::backports, - Hash $confs = $apt::params::confs, - Hash $update = $apt::params::update, - Hash $purge = $apt::params::purge, - Apt::Proxy $proxy = $apt::params::proxy, - Hash $sources = $apt::params::sources, - Hash $keys = $apt::params::keys, - Hash $ppas = $apt::params::ppas, - Hash $pins = $apt::params::pins, - Hash $settings = $apt::params::settings, - Boolean $manage_auth_conf = $apt::params::manage_auth_conf, - Array[Apt::Auth_conf_entry] - $auth_conf_entries = $apt::params::auth_conf_entries, - String $auth_conf_owner = $apt::params::auth_conf_owner, - String $root = $apt::params::root, - String $sources_list = $apt::params::sources_list, - String $sources_list_d = $apt::params::sources_list_d, - String $conf_d = $apt::params::conf_d, - String $preferences = $apt::params::preferences, - String $preferences_d = $apt::params::preferences_d, - String $apt_conf_d = $apt::params::apt_conf_d, - Hash $config_files = $apt::params::config_files, - Boolean $sources_list_force = $apt::params::sources_list_force, + Hash $update_defaults = $apt::params::update_defaults, + Hash $purge_defaults = $apt::params::purge_defaults, + Hash $proxy_defaults = $apt::params::proxy_defaults, + Hash $include_defaults = $apt::params::include_defaults, + String $provider = $apt::params::provider, + String $keyserver = $apt::params::keyserver, + Optional[String] $key_options = $apt::params::key_options, + Optional[Array[String]] $ppa_options = $apt::params::ppa_options, + Optional[String] $ppa_package = $apt::params::ppa_package, + Optional[Hash] $backports = $apt::params::backports, + Hash $confs = $apt::params::confs, + Hash $update = $apt::params::update, + Hash $purge = $apt::params::purge, + Apt::Proxy $proxy = $apt::params::proxy, + Hash $sources = $apt::params::sources, + Hash $keys = $apt::params::keys, + Hash $ppas = $apt::params::ppas, + Hash $pins = $apt::params::pins, + Hash $settings = $apt::params::settings, + Boolean $manage_auth_conf = $apt::params::manage_auth_conf, + Array[Apt::Auth_conf_entry] $auth_conf_entries = $apt::params::auth_conf_entries, + String $auth_conf_owner = $apt::params::auth_conf_owner, + String $root = $apt::params::root, + String $sources_list = $apt::params::sources_list, + String $sources_list_d = $apt::params::sources_list_d, + String $conf_d = $apt::params::conf_d, + String $preferences = $apt::params::preferences, + String $preferences_d = $apt::params::preferences_d, + String $apt_conf_d = $apt::params::apt_conf_d, + Hash $config_files = $apt::params::config_files, + Boolean $sources_list_force = $apt::params::sources_list_force, Hash $source_key_defaults = { 'server' => $keyserver, diff --git a/manifests/params.pp b/manifests/params.pp index 5ef175b..f045491 100644 --- a/manifests/params.pp +++ b/manifests/params.pp @@ -91,7 +91,7 @@ class apt::params { 'key' => '630239CC130E1A7FD81A27B140976EAF437D05B5', 'repos' => 'main universe multiverse restricted', } - $ppa_options = '-y' + $ppa_options = ['-y'] $ppa_package = 'software-properties-common' $auth_conf_owner = '_apt' } diff --git a/manifests/ppa.pp b/manifests/ppa.pp index bab7b33..39852d1 100644 --- a/manifests/ppa.pp +++ b/manifests/ppa.pp @@ -24,12 +24,12 @@ # Specifies whether Puppet should manage the package that provides `apt-add-repository`. # define apt::ppa ( - String $ensure = 'present', - Optional[String] $options = $::apt::ppa_options, - Optional[String] $release = fact('os.distro.codename'), - Optional[String] $dist = $facts['os']['name'], - Optional[String] $package_name = $::apt::ppa_package, - Boolean $package_manage = false, + String $ensure = 'present', + Optional[Array[String]] $options = $::apt::ppa_options, + Optional[String] $release = fact('os.distro.codename'), + Optional[String] $dist = $facts['os']['name'], + Optional[String] $package_name = $::apt::ppa_package, + Boolean $package_manage = false, ) { unless $release { fail('os.distro.codename fact not available: release parameter required') @@ -39,6 +39,11 @@ define apt::ppa ( fail('apt::ppa is not currently supported on Debian.') } + # Validate the resource name + if $name !~ /^ppa:([a-zA-Z0-9\-_]+)\/([a-zA-z0-9\-_]+)$/ { + fail("Invalid PPA name: ${name}") + } + if versioncmp($facts['os']['release']['full'], '14.10') >= 0 { $distid = downcase($dist) $dash_filename = regsubst($name, '^ppa:([^/]+)/(.+)$', "\\1-${distid}-\\2") @@ -62,6 +67,9 @@ define apt::ppa ( $trusted_gpg_d_filename = "${dash_filename_no_specialchars}.gpg" } + # This is the location of our main exec script + $script_path = "/opt/puppetlabs/puppet/cache/add-apt-repository-${dash_filename_no_specialchars}-${release}.sh" + if $ensure == 'present' { if $package_manage { ensure_packages($package_name) @@ -81,24 +89,36 @@ define apt::ppa ( $_proxy_env = [] } - exec { "add-apt-repository-${name}": - environment => $_proxy_env, - command => "/usr/bin/add-apt-repository ${options} ${name} || (rm ${::apt::sources_list_d}/${sources_list_d_filename} && false)", - unless => "/usr/bin/test -f ${::apt::sources_list_d}/${sources_list_d_filename} && /usr/bin/test -f ${::apt::trusted_gpg_d}/${trusted_gpg_d_filename}", - user => 'root', - logoutput => 'on_failure', - notify => Class['apt::update'], - require => $_require, - } + unless $sources_list_d_filename in $facts['apt_sources'] { + $script_content = epp('apt/add-apt-repository.sh.epp', { + command => ['/usr/bin/add-apt-repository', shell_join($options), $name], + sources_list_d_path => $::apt::sources_list_d, + sources_list_d_filename => $sources_list_d_filename, + }) - file { "${::apt::sources_list_d}/${sources_list_d_filename}": - ensure => file, - require => Exec["add-apt-repository-${name}"], + file { "add-apt-repository-script-${name}": + ensure => 'file', + path => $script_path, + content => $script_content, + mode => '0755', + } + + exec { "add-apt-repository-${name}": + environment => $_proxy_env, + command => $script_path, + logoutput => 'on_failure', + notify => Class['apt::update'], + require => $_require, + } } } else { - file { "${::apt::sources_list_d}/${sources_list_d_filename}": - ensure => 'absent', + tidy { "remove-apt-repository-script-${name}": + path => $script_path, + } + + tidy { "remove-apt-repository-${name}": + path => "${::apt::sources_list_d}/${sources_list_d_filename}", notify => Class['apt::update'], } } diff --git a/templates/add-apt-repository.sh.epp b/templates/add-apt-repository.sh.epp new file mode 100644 index 0000000..e23c485 --- /dev/null +++ b/templates/add-apt-repository.sh.epp @@ -0,0 +1,8 @@ +<%- | Array $command, String $sources_list_d_path, String $sources_list_d_filename | -%> + +<%= $command.join(' ') %> + +if [ $? -gt 0 ]; then + rm <%= $sources_list_d_path %>/<%= $sources_list_d_filename %> + exit 1 +fi -- 2.32.3