From eed10ea359d0fe144da90a8425cd14dc3c6c8f18 Mon Sep 17 00:00:00 2001
From: Craig Gumbley <craiggumbley@gmail.com>
Date: Thu, 11 Aug 2022 20:13:11 +0000
Subject: [PATCH 1/2] Harden apt-mark defined type

Prior to this commit the title parameter of this defined
type was not properly validated. This means that it could have been
possible to use a resource title outside of the normal bounds of
a package name.

Additionally the `onlyif` and `command` parameter values were
interpolated strings meaning that it may have been possible to
execute unsafe code on the remote system.

This commit fixes the above issues by adding a regex to check that the
resource title is a valid apt package name and also breaks out the
`onlyif` and `command` parameter values in to arrays of args ensuring
that the commands executed in a safe manor on the remote system.

The exception in this commit is the `unless_cmd`. This has not been
broken out in to an array of args due to the requirement of the command.
This is a reasonable trade of however due to the fact that action is
created from known enum values and title would be pre-validated.
This is also explained in mark.pp:20.
---
 examples/mark.pp  |  3 +++
 manifests/mark.pp | 35 +++++++++++++++++++++++++----------
 2 files changed, 28 insertions(+), 10 deletions(-)
 create mode 100644 examples/mark.pp

diff --git a/examples/mark.pp b/examples/mark.pp
new file mode 100644
index 0000000000..52bae6eeb3
--- /dev/null
+++ b/examples/mark.pp
@@ -0,0 +1,3 @@
+apt::mark { 'vim':
+  setting => 'auto',
+}
diff --git a/manifests/mark.pp b/manifests/mark.pp
index f4b7de0a37..08e396c02f 100644
--- a/manifests/mark.pp
+++ b/manifests/mark.pp
@@ -8,16 +8,31 @@
 define apt::mark (
   Enum['auto','manual','hold','unhold'] $setting,
 ) {
-  case $setting {
-    'unhold': {
-      $unless_cmd = undef
-    }
-    default: {
-      $unless_cmd = "/usr/bin/apt-mark show${setting} ${title} | /bin/fgrep -qs ${title}"
-    }
+  if $title !~ /^[a-zA-Z0-9\-_]+$/ {
+    fail("Invalid package name: ${title}")
   }
-  exec { "/usr/bin/apt-mark ${setting} ${title}":
-    onlyif => "/usr/bin/dpkg -l ${title}",
-    unless => $unless_cmd,
+
+  if $setting == 'unhold' {
+    $unless_cmd = undef
+  } else {
+    $action = "show${setting}"
+
+    # It would be ideal if we could break out this command in to an array of args, similar
+    # to $onlyif_cmd and $command. However, in this case it wouldn't work as expected due
+    # to the inclusion of a pipe character.
+    # When passed to the exec function, the posix provider will strip everything to the right of the pipe,
+    # causing the command to return a full list of packages for the given action.
+    # The trade off is to use an interpolated string knowing that action is built from an enum value and
+    # title is pre-validated.
+    $unless_cmd = ["/usr/bin/apt-mark ${action} ${title} | grep ${title} -q"]
+  }
+
+  $onlyif_cmd = [['/usr/bin/dpkg', '-l', $title]]
+  $command = ['/usr/bin/apt-mark', $setting, $title]
+
+  exec { "apt-mark ${setting} ${title}":
+    command => $command,
+    onlyif  => $onlyif_cmd,
+    unless  => $unless_cmd,
   }
 }

From 79bec3d3efdb897038cbab0b4a24572b94be507f Mon Sep 17 00:00:00 2001
From: Craig Gumbley <craiggumbley@gmail.com>
Date: Fri, 12 Aug 2022 09:41:23 +0000
Subject: [PATCH 2/2] Add spec tests for apt-mark

This commit adds additional spec tests for mark.pp.

The tests validate the new resource name requirements
introduced in the previous commit.
---
 spec/defines/mark_spec.rb | 48 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/spec/defines/mark_spec.rb b/spec/defines/mark_spec.rb
index b451bedd30..6c673c41c3 100644
--- a/spec/defines/mark_spec.rb
+++ b/spec/defines/mark_spec.rb
@@ -32,7 +32,7 @@
     end
 
     it {
-      is_expected.to contain_exec('/usr/bin/apt-mark manual my_source')
+      is_expected.to contain_exec('apt-mark manual my_source')
     }
   end
 
@@ -47,4 +47,50 @@
       is_expected.to raise_error(Puppet::PreformattedError, %r{expects a match for Enum\['auto', 'hold', 'manual', 'unhold'\], got 'foobar'})
     end
   end
+
+  [
+    'package',
+    'package1',
+    'package_name',
+    'package-name',
+  ].each do |value|
+    describe 'with a valid resource title' do
+      let :title do
+        value
+      end
+
+      let :params do
+        {
+          'setting' => 'manual',
+        }
+      end
+
+      it do
+        is_expected.to contain_exec("apt-mark manual #{title}")
+      end
+    end
+  end
+
+  [
+    '|| ls -la ||',
+    'packakge with space',
+    'package<>|',
+    '|| touch /tmp/foo.txt ||',
+  ].each do |value|
+    describe 'with an invalid resource title' do
+      let :title do
+        value
+      end
+
+      let :params do
+        {
+          'setting' => 'manual',
+        }
+      end
+
+      it do
+        is_expected.to raise_error(Puppet::PreformattedError, %r{Invalid package name: #{title}})
+      end
+    end
+  end
 end