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] 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 0000000..52bae6e
--- /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 f4b7de0..08e396c 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,
   }
 }
-- 
2.45.2