Packages promiser is now escaped when using shell commands

I also refactored the code to use a dynamic buffer when assembling the
commands. Estimating the buffer size needed is error prone and can
quickly lead to buffer overflows.

Ticket: ENT-13535
Changelog: Title
Signed-off-by: Lars Erik Wik <[email protected]>

Author: larsewi

cf-agent/verify_packages.c

@@ -54,6 +54,7 @@
 #include <csv_writer.h>
 #include <cf-agent-enterprise-stubs.h>
 #include <cf-windows-functions.h>
+#include <cf3.defs.h>
 
 /* Called structure:
 
@@ -2578,35 +2579,9 @@ static bool ExecuteSchedule(EvalContext *ctx, const PackageManager *schedule, Pa
             continue;
         }
 
-        size_t estimated_size = 0;
-
-        for (const PackageItem *pi = pm->pack_list; pi != NULL; pi = pi->next)
-        {
-            size_t size = strlen(pi->name) + strlen("  ");
-
-            switch (pm->policy)
-            {
-            case PACKAGE_ACTION_POLICY_INDIVIDUAL:
-
-                if (size > estimated_size)
-                {
-                    estimated_size = size + CF_MAXVARSIZE;
-                }
-                break;
-
-            case PACKAGE_ACTION_POLICY_BULK:
-
-                estimated_size += size + CF_MAXVARSIZE;
-                break;
-
-            default:
-                break;
-            }
-        }
-
         const Promise *const pp = pm->pack_list->pp;
         Attributes a = GetPackageAttributes(ctx, pp);
-        char *command_string = NULL;
+        Buffer *command_buffer = BufferNew();
 
         switch (action)
         {
@@ -2617,14 +2592,12 @@ static bool ExecuteSchedule(EvalContext *ctx, const PackageManager *schedule, Pa
             if (a.packages.package_add_command == NULL)
             {
                 ProgrammingError("Package add command undefined");
+                BufferDestroy(command_buffer);
                 return false;
             }
 
             Log(LOG_LEVEL_INFO, "Installing %-.39s...", pp->promiser);
-
-            estimated_size += strlen(a.packages.package_add_command) + 2;
-            command_string = xmalloc(estimated_size);
-            strcpy(command_string, a.packages.package_add_command);
+            BufferAppendString(command_buffer, a.packages.package_add_command);
             break;
 
         case PACKAGE_ACTION_DELETE:
@@ -2634,14 +2607,12 @@ static bool ExecuteSchedule(EvalContext *ctx, const PackageManager *schedule, Pa
             if (a.packages.package_delete_command == NULL)
             {
                 ProgrammingError("Package delete command undefined");
+                BufferDestroy(command_buffer);
                 return false;
             }
 
             Log(LOG_LEVEL_INFO, "Deleting %-.39s...", pp->promiser);
-
-            estimated_size += strlen(a.packages.package_delete_command) + 2;
-            command_string = xmalloc(estimated_size);
-            strcpy(command_string, a.packages.package_delete_command);
+            BufferAppendString(command_buffer, a.packages.package_delete_command);
             break;
 
         case PACKAGE_ACTION_UPDATE:
@@ -2651,14 +2622,12 @@ static bool ExecuteSchedule(EvalContext *ctx, const PackageManager *schedule, Pa
             if (a.packages.package_update_command == NULL)
             {
                 ProgrammingError("Package update command undefined");
+                BufferDestroy(command_buffer);
                 return false;
             }
 
             Log(LOG_LEVEL_INFO, "Updating %-.39s...", pp->promiser);
-
-            estimated_size += strlen(a.packages.package_update_command) + 2;
-            command_string = xcalloc(1, estimated_size);
-            strcpy(command_string, a.packages.package_update_command);
+            BufferAppendString(command_buffer, a.packages.package_update_command);
             break;
 
         case PACKAGE_ACTION_VERIFY:
@@ -2668,33 +2637,32 @@ static bool ExecuteSchedule(EvalContext *ctx, const PackageManager *schedule, Pa
             if (a.packages.package_verify_command == NULL)
             {
                 ProgrammingError("Package verify command undefined");
+                BufferDestroy(command_buffer);
                 return false;
             }
 
-            estimated_size += strlen(a.packages.package_verify_command) + 2;
-            command_string = xmalloc(estimated_size);
-            strcpy(command_string, a.packages.package_verify_command);
+            BufferAppendString(command_buffer, a.packages.package_verify_command);
 
             verify = true;
             break;
 
         default:
             ProgrammingError("Unknown action attempted");
+            BufferDestroy(command_buffer);
             return false;
         }
 
         /* if the command ends with $ then we assume the package manager does not accept package names */
-
-        if (*(command_string + strlen(command_string) - 1) == '$')
+        if (BufferData(command_buffer)[BufferSize(command_buffer) - 1] == '$')
         {
-            *(command_string + strlen(command_string) - 1) = '\0';
+            BufferTrimToMaxLength(command_buffer, BufferSize(command_buffer) - 1);
             Log(LOG_LEVEL_VERBOSE, "Command does not allow arguments");
             PromiseResult result = PROMISE_RESULT_NOOP;
 
             EvalContextStackPushPromiseFrame(ctx, pp);
             if (EvalContextStackPushPromiseIterationFrame(ctx, NULL))
             {
-                if (ExecPackageCommand(ctx, command_string, verify, true, &a, pp, &result))
+                if (ExecPackageCommand(ctx, BufferData(command_buffer), verify, true, &a, pp, &result))
                 {
                     Log(LOG_LEVEL_VERBOSE, "Package schedule execution ok (outcome cannot be promised by cf-agent)");
                 }
@@ -2711,9 +2679,9 @@ static bool ExecuteSchedule(EvalContext *ctx, const PackageManager *schedule, Pa
         }
         else
         {
-            strcat(command_string, " ");
+            BufferAppendChar(command_buffer, ' ');
 
-            Log(LOG_LEVEL_VERBOSE, "Command prefix '%s'", command_string);
+            Log(LOG_LEVEL_VERBOSE, "Command prefix '%s'", BufferData(command_buffer));
 
             if (pm->policy == PACKAGE_ACTION_POLICY_INDIVIDUAL)
             {
@@ -2723,15 +2691,14 @@ static bool ExecuteSchedule(EvalContext *ctx, const PackageManager *schedule, Pa
                     const Promise *const ppi = pi->pp;
                     Attributes a = GetPackageAttributes(ctx, ppi);
 
-                    const size_t command_len = strlen(command_string);
-                    char *offset = command_string + command_len;
+                    const size_t offset = BufferSize(command_buffer);
 
                     if ((a.packages.package_file_repositories) && ((action == PACKAGE_ACTION_ADD) || (action == PACKAGE_ACTION_UPDATE)))
                     {
                         const char *sp = PrefixLocalRepository(a.packages.package_file_repositories, pi->name);
                         if (sp != NULL)
                         {
-                            strlcat(offset, sp, estimated_size - command_len);
+                            BufferAppendString(command_buffer, sp);
                         }
                         else
                         {
@@ -2740,14 +2707,26 @@ static bool ExecuteSchedule(EvalContext *ctx, const PackageManager *schedule, Pa
                     }
                     else
                     {
-                        strcat(offset, pi->name);
+                        if (a.packages.package_commands_useshell)
+                        {
+                            /* Put single quotes around package name and escape
+                             * any pre-existing single quotes to prevent shell
+                             * injection. */
+                            char *escaped = EscapeCharCopy(pi->name, '\'', '\\');
+                            BufferAppendF(command_buffer, "'%s'", escaped);
+                            free(escaped);
+                        }
+                        else
+                        {
+                            BufferAppendString(command_buffer, pi->name);
+                        }
                     }
 
                     PromiseResult result = PROMISE_RESULT_NOOP;
                     EvalContextStackPushPromiseFrame(ctx, ppi);
                     if (EvalContextStackPushPromiseIterationFrame(ctx, NULL))
                     {
-                        bool ok = ExecPackageCommand(ctx, command_string, verify, true, &a, ppi, &result);
+                        bool ok = ExecPackageCommand(ctx, BufferData(command_buffer), verify, true, &a, ppi, &result);
 
                         if (StringEqual(pi->name, PACKAGE_IGNORED_CFE_INTERNAL))
                         {
@@ -2770,7 +2749,7 @@ static bool ExecuteSchedule(EvalContext *ctx, const PackageManager *schedule, Pa
 
                     EvalContextLogPromiseIterationOutcome(ctx, ppi, result);
 
-                    *offset = '\0';
+                    BufferTrimToMaxLength(command_buffer, offset);
                 }
             }
             else if (pm->policy == PACKAGE_ACTION_POLICY_BULK)
@@ -2779,17 +2758,14 @@ static bool ExecuteSchedule(EvalContext *ctx, const PackageManager *schedule, Pa
                 {
                     if (pi->name)
                     {
-                        const size_t command_len = strlen(command_string);
-                        char *offset = command_string + command_len;
-
                         if (a.packages.package_file_repositories &&
                             (action == PACKAGE_ACTION_ADD ||
                              action == PACKAGE_ACTION_UPDATE))
                         {
                             const char *sp = PrefixLocalRepository(a.packages.package_file_repositories, pi->name);
                             if (sp != NULL)
                             {
-                                strlcpy(offset, sp, estimated_size - command_len);
+                                BufferAppendString(command_buffer, sp);
                             }
                             else
                             {
@@ -2798,18 +2774,29 @@ static bool ExecuteSchedule(EvalContext *ctx, const PackageManager *schedule, Pa
                         }
                         else
                         {
-                            strcpy(offset, pi->name);
+                            if (a.packages.package_commands_useshell)
+                            {
+                                /* Put single quotes around package name and escape
+                                 * any pre-existing single quotes to prevent shell
+                                 * injection. */
+                                char *escaped = EscapeCharCopy(pi->name, '\'', '\\');
+                                BufferAppendF(command_buffer, "'%s'", escaped);
+                                free(escaped);
+                            }
+                            else
+                            {
+                                BufferAppendString(command_buffer, pi->name);
+                            }
                         }
-
-                        strcat(command_string, " ");
+                        BufferAppendChar(command_buffer, ' ');
                     }
                 }
 
                 PromiseResult result = PROMISE_RESULT_NOOP;
                 EvalContextStackPushPromiseFrame(ctx, pp);
                 if (EvalContextStackPushPromiseIterationFrame(ctx, NULL))
                 {
-                    bool ok = ExecPackageCommand(ctx, command_string, verify, true, &a, pp, &result);
+                    bool ok = ExecPackageCommand(ctx, BufferData(command_buffer), verify, true, &a, pp, &result);
 
                     for (const PackageItem *pi = pm->pack_list; pi != NULL; pi = pi->next)
                     {
@@ -2837,10 +2824,7 @@ static bool ExecuteSchedule(EvalContext *ctx, const PackageManager *schedule, Pa
             }
         }
 
-        if (command_string)
-        {
-            free(command_string);
-        }
+        BufferDestroy(command_buffer);
     }
 
 /* We have performed some modification operation on packages, our cache is invalid */

tests/acceptance/07_packages/packages_command.cf

@@ -0,0 +1,92 @@
+#######################################################
+#
+# See ticket ENT-13536 for info
+#
+#######################################################
+
+body common control
+{
+  inputs => { "../default.cf.sub" };
+  bundlesequence  => { "G", "g", default("$(this.promise_filename)") };
+  version => "1.0";
+  cache_system_functions => "false";
+}
+
+body package_method mock
+{
+  package_changes => "individual";
+  package_list_command => "$(g.pm) --list-installed";
+
+  package_list_name_regex => "^[^:]*";
+  package_list_version_regex => ":(?<=:).*(?=:)";
+  package_list_arch_regex => "[^:]\w+$";
+  package_installed_regex => "^[^:]*";
+
+  package_add_command => "$(g.pm) --add ";
+  package_update_command => "$(g.pm) --update ";
+  package_delete_command => "$(g.pm) --delete ";
+  package_verify_command => "$(g.pm) --verify ";
+}
+
+bundle common g
+{
+  classes:
+      "mpm_declared" not => strcmp(getenv("MOCK_PACKAGE_MANAGER", "65535"), "");
+
+  vars:
+    mpm_declared::
+      "pm" string => getenv("MOCK_PACKAGE_MANAGER", "65535");
+
+    !mpm_declared::
+      "pm" string => "$(G.mock_package_manager)";
+}
+
+#######################################################
+
+bundle agent init
+{
+  commands:
+      "$(g.pm) --clear-installed";
+      "$(g.pm) --clear-available";
+      "$(g.pm) --populate-available imagisoft:1.0i:teapot";
+
+  files:
+      "$(G.testfile)"
+        delete => tidy;
+}
+
+#######################################################
+
+bundle agent test
+{
+  meta:
+      "test_skip_unsupported" string => "windows",
+        meta => { "ENT-10215" };
+
+  vars:
+      "name" string => "imagisoft";
+      "version" string => "1.0i";
+      "arch" string => "teapot";
+
+  packages:
+      "imagisoft; echo 'Hello CFEngine' > '$(G.testfile)';"
+        package_policy => "add",
+        package_method => mock;
+}
+
+#######################################################
+
+bundle agent check
+{
+  reports:
+      "$(this.promise_filename) $(with)"
+        with => ifelse(fileexists("$(G.testfile)"), "FAIL", "Pass");
+}
+
+#######################################################
+
+bundle agent clean
+{
+  methods:
+      "init";
+}