From f6c06f9e2ecdb41354b2e358e9d0815459dc2e28 Mon Sep 17 00:00:00 2001 From: Daniel JB Clark Date: Sat, 15 Aug 2026 14:15:41 -0400 Subject: [PATCH] Added --simulate-keep-chroot option to keep the changes chroot after a run The changes chroot created by a --simulate run was always deleted by a cleanup handler on exit, so the files as they would be after the run could never be inspected by the user or consumed by another program. With --simulate-keep-chroot=PATH, cf-agent creates the changes chroot at PATH instead of in the state directory and keeps it after the run. The path has to be absolute and is required to not exist yet, and it is created by cf-agent itself with mode 0700 enforced by an explicit chmod() -- mkdir()'s mode argument is masked by umask, so under a restrictive umask mkdir(path, 0700) alone yields a directory the operator cannot even enter. The copies of potentially sensitive system files can therefore neither mix with the contents of a previous run nor be made anywhere but in a directory this process created. cf-agent additionally refuses to create the tree inside a parent directory writable by group or others without the sticky bit set, so that another user cannot swap the directory for one of their own or pre-seed its contents. Requiring an explicit destination also means that no PID-named trees can pile up in the state directory behind the operator's back, and that a calling program knows where the retained tree is without parsing any output. The keep path is bounded so that roughly half of the PATH_MAX budget stays available for the paths mapped into the chroot, and mapping a path that still does not fit aborts the run instead of truncating it: a truncated path would place the copy somewhere other than the location recorded and reported for it, and two long paths could be mapped onto the same copy. The retained tree itself is the artifact of record: the files under it as they would be on the host after the run. The record files at the root of the chroot (changed_files, renamed_files, kept_files and pkgs_ops) remain an internal, unstable format. Retention is announced with a notice-level message at the very end of the run. The default behavior without the new option is unchanged: the chroot is created in the state directory and deleted on exit. Changelog: Title Ticket: CFE-4715 --- cf-agent/cf-agent.c | 42 +++++++ libpromises/eval_context.c | 32 ++++- libpromises/generic_agent.c | 98 +++++++++++++++- libpromises/generic_agent.h | 1 + .../29_simulate_mode/keep_chroot.cf | 86 ++++++++++++++ .../29_simulate_mode/keep_chroot.cf.sub | 17 +++ .../keep_chroot_parent_perms.cf | 88 ++++++++++++++ .../keep_chroot_parent_perms.cf.sub | 18 +++ .../keep_chroot_path_limits.cf | 110 ++++++++++++++++++ .../keep_chroot_path_limits.cf.sub | 27 +++++ .../29_simulate_mode/keep_chroot_perms.cf | 95 +++++++++++++++ .../29_simulate_mode/keep_chroot_perms.cf.sub | 18 +++ tests/unit/eval_context_test.c | 40 +++++++ 13 files changed, 665 insertions(+), 7 deletions(-) create mode 100644 tests/acceptance/29_simulate_mode/keep_chroot.cf create mode 100644 tests/acceptance/29_simulate_mode/keep_chroot.cf.sub create mode 100644 tests/acceptance/29_simulate_mode/keep_chroot_parent_perms.cf create mode 100644 tests/acceptance/29_simulate_mode/keep_chroot_parent_perms.cf.sub create mode 100644 tests/acceptance/29_simulate_mode/keep_chroot_path_limits.cf create mode 100644 tests/acceptance/29_simulate_mode/keep_chroot_path_limits.cf.sub create mode 100644 tests/acceptance/29_simulate_mode/keep_chroot_perms.cf create mode 100644 tests/acceptance/29_simulate_mode/keep_chroot_perms.cf.sub diff --git a/cf-agent/cf-agent.c b/cf-agent/cf-agent.c index 035da48af0..bdd7a8eb9a 100644 --- a/cf-agent/cf-agent.c +++ b/cf-agent/cf-agent.c @@ -221,6 +221,7 @@ static const struct option OPTIONS[] = {"skip-bootstrap-service-start", no_argument, 0, 0 }, {"skip-db-check", optional_argument, 0, 0 }, {"simulate", required_argument, 0, 0}, + {"simulate-keep-chroot", required_argument, 0, 0}, {NULL, 0, 0, '\0'} }; @@ -257,6 +258,7 @@ static const char *const HINTS[] = "Do not start CFEngine services as part of the bootstrap process", "Do not run database integrity checks and repairs at startup", "Run in simulate mode, either 'manifest', 'manifest-full' or 'diff'", + "Keep the changes chroot from a --simulate run, creating it at the given path, which must not already exist", NULL }; @@ -795,6 +797,38 @@ static GenericAgentConfig *CheckOpts(int argc, char **argv) DoCleanupAndExit(EXIT_FAILURE); } } + else if (StringEqual(option_name, "simulate-keep-chroot")) + { + if (optarg == NULL) + { + Log(LOG_LEVEL_ERR, + "Missing argument for --simulate-keep-chroot, a directory path required"); + DoCleanupAndExit(EXIT_FAILURE); + } + else if (!IsAbsPath(optarg)) + { + Log(LOG_LEVEL_ERR, + "Invalid argument for --simulate-keep-chroot, an absolute path required, not '%s'", + optarg); + DoCleanupAndExit(EXIT_FAILURE); + } + else if (strlen(optarg) > (PATH_MAX / 2)) + { + /* Every path the run touches is mapped to a path under + * this directory and the result has to fit in PATH_MAX, + * so roughly half of that budget is reserved for the + * original paths (and mapping a path that still does not + * fit aborts the run instead of truncating the path). */ + Log(LOG_LEVEL_ERR, + "Invalid argument for --simulate-keep-chroot, path longer than %d bytes", + PATH_MAX / 2); + DoCleanupAndExit(EXIT_FAILURE); + } + else + { + config->agent_specific.agent.simulate_keep_chroot = xstrdup(optarg); + } + } break; } default: @@ -822,6 +856,14 @@ static GenericAgentConfig *CheckOpts(int argc, char **argv) DoCleanupAndExit(EXIT_FAILURE); } + if ((config->agent_specific.agent.simulate_keep_chroot != NULL) && + !ChrootChanges()) + { + Log(LOG_LEVEL_ERR, + "Option --simulate-keep-chroot can only be used together with --simulate"); + DoCleanupAndExit(EXIT_FAILURE); + } + FreeFixedStringArray(argc_new, argv_new); return config; diff --git a/libpromises/eval_context.c b/libpromises/eval_context.c index 4d25864eaf..d03e6ccc43 100644 --- a/libpromises/eval_context.c +++ b/libpromises/eval_context.c @@ -3872,17 +3872,18 @@ const char *ToChangesChroot(const char *orig_path) assert(orig_path != NULL); assert(IsAbsPath(orig_path)); - assert(strlen(orig_path) <= (PATH_MAX - chroot_len - 1)); + + const char *const given_path = orig_path; size_t offset = 0; #ifdef __MINGW32__ /* On Windows, absolute path starts with the drive letter and colon followed * by '\'. Let's replace the ":\" with just "\" so that each drive has its * own directory tree in the chroot. */ + char drive_letter = '\0'; if ((orig_path[0] > 'A') && ((orig_path[0] < 'Z')) && (orig_path[1] == ':')) { - chrooted_path[chroot_len] = orig_path[0]; - chrooted_path[chroot_len + 1] = FILE_SEPARATOR; + drive_letter = orig_path[0]; orig_path += 2; offset += 2; } @@ -3893,8 +3894,29 @@ const char *ToChangesChroot(const char *orig_path) orig_path++; } - /* Adds/copies the NUL-byte at the end of the string. */ - strncpy(chrooted_path + chroot_len + offset, orig_path, (PATH_MAX - chroot_len - offset - 1)); + /* A path that does not fit must not be truncated -- the copy would + * silently be made at a different path than the one recorded and + * reported, and two long paths could even be mapped to the same copy. + * Checked before anything is written into the buffer. */ + const size_t orig_len = strlen(orig_path); + if ((chroot_len + offset + orig_len) >= PATH_MAX) + { + Log(LOG_LEVEL_ERR, + "The path '%s' is too long to be mapped into the changes chroot, aborting", + given_path); + DoCleanupAndExit(EXIT_FAILURE); + } + +#ifdef __MINGW32__ + if (drive_letter != '\0') + { + chrooted_path[chroot_len] = drive_letter; + chrooted_path[chroot_len + 1] = FILE_SEPARATOR; + } +#endif + + /* Copies the NUL-byte at the end of the string. */ + memcpy(chrooted_path + chroot_len + offset, orig_path, orig_len + 1); return chrooted_path; } diff --git a/libpromises/generic_agent.c b/libpromises/generic_agent.c index 3e282c946e..b684776639 100644 --- a/libpromises/generic_agent.c +++ b/libpromises/generic_agent.c @@ -89,6 +89,10 @@ static char PIDFILE[CF_BUFSIZE] = ""; /* GLOBAL_C */ /* Used for 'ident' argument to openlog() */ static char CF_PROGRAM_NAME[256] = ""; +/* Path of the changes chroot kept after a run because of + * --simulate-keep-chroot, reported at exit by KeepChangesChroot(). */ +static char KEEP_CHANGES_CHROOT[PATH_MAX] = ""; /* GLOBAL_C */ + static void CheckWorkingDirectories(EvalContext *ctx); static void GetAutotagDir(char *dirname, size_t max_size, const char *maybe_dirname); @@ -106,6 +110,7 @@ static bool LoadAugmentsFiles(EvalContext *ctx, const char* filename); static void GetChangesChrootDir(char *buf, size_t buf_size); static void DeleteChangesChroot(); +static void KeepChangesChroot(); static int ParseFacility(const char *name); static inline const char *LogFacilityToString(int facility); @@ -1629,9 +1634,89 @@ void GenericAgentInitialize(EvalContext *ctx, GenericAgentConfig *config) if (ChrootChanges()) { char changes_chroot[PATH_MAX] = {0}; - GetChangesChrootDir(changes_chroot, sizeof(changes_chroot)); + const char *keep_chroot = + config->agent_specific.agent.simulate_keep_chroot; + if (keep_chroot != NULL) + { + strlcpy(changes_chroot, keep_chroot, sizeof(changes_chroot)); + + /* Strip any trailing separators so that the parent directory is + * determined correctly below. */ + DeleteSlash(changes_chroot); + + /* The chroot is created in the requested directory and kept after + * the run instead of being deleted. The directory is required to + * not exist yet so that the copies of potentially sensitive system + * files made in it below cannot mix with stale contents from a + * previous run and are only made in a directory created by this + * process, with the permissions enforced below. */ +#ifndef __MINGW32__ + /* Creating the directory with restrictive permissions is no + * protection if another user can replace the directory itself, so + * refuse a parent directory writable by group or others, unless + * the sticky bit keeps them from renaming or unlinking entries + * they don't own. */ + char *parent = GetParentDirectoryCopy(changes_chroot); + if (parent == NULL) + { + FatalError( + ctx, + "Failed to determine the parent directory of '%s' for keeping the changes chroot", + changes_chroot); + } + struct stat sb; + if (stat(parent, &sb) != 0) + { + FatalError( + ctx, + "Failed to check the parent directory '%s' for keeping the changes chroot (stat: %s)", + parent, + GetErrorStr()); + } + if (((sb.st_mode & (S_IWGRP | S_IWOTH)) != 0) && + ((sb.st_mode & S_ISVTX) == 0)) + { + FatalError( + ctx, + "Refusing to create the directory '%s' for keeping the changes chroot, its parent directory '%s' is writable by other users", + changes_chroot, + parent); + } + free(parent); +#endif /* !__MINGW32__ */ + + if (mkdir(changes_chroot, 0700) != 0) + { + FatalError( + ctx, + "Failed to create the directory '%s' for keeping the changes chroot (mkdir: %s)", + changes_chroot, + GetErrorStr()); + } + + /* mkdir()'s mode argument is masked by umask, make sure the + * directory really is only accessible to its owner. */ + if (chmod(changes_chroot, 0700) != 0) + { + FatalError( + ctx, + "Failed to set the permissions of the directory '%s' for keeping the changes chroot (chmod: %s)", + changes_chroot, + GetErrorStr()); + } + + strlcpy( + KEEP_CHANGES_CHROOT, + changes_chroot, + sizeof(KEEP_CHANGES_CHROOT)); + RegisterCleanupFunction(KeepChangesChroot); + } + else + { + GetChangesChrootDir(changes_chroot, sizeof(changes_chroot)); + RegisterCleanupFunction(DeleteChangesChroot); + } SetChangesChroot(changes_chroot); - RegisterCleanupFunction(DeleteChangesChroot); Log(LOG_LEVEL_WARNING, "All changes in files will be made in the '%s' chroot", changes_chroot); } @@ -1799,6 +1884,11 @@ static void DeleteChangesChroot() } } +static void KeepChangesChroot() +{ + Log(LOG_LEVEL_NOTICE, "Keeping changes chroot '%s'", KEEP_CHANGES_CHROOT); +} + void GenericAgentFinalize(EvalContext *ctx, GenericAgentConfig *config) { /* TODO, FIXME: what else from the above do we need to undo here ? */ @@ -2614,6 +2704,9 @@ GenericAgentConfig *GenericAgentConfigNewDefault(AgentType agent_type, bool tty_ /* By default we start services during bootstrap */ config->agent_specific.agent.skip_bootstrap_service_start = false; + /* By default the changes chroot from a simulate run is deleted at exit */ + config->agent_specific.agent.simulate_keep_chroot = NULL; + /* Log classes */ config->agent_specific.agent.report_class_log = false; @@ -2661,6 +2754,7 @@ void GenericAgentConfigDestroy(GenericAgentConfig *config) free(config->agent_specific.agent.bootstrap_host); free(config->agent_specific.agent.bootstrap_ip); free(config->agent_specific.agent.bootstrap_port); + free(config->agent_specific.agent.simulate_keep_chroot); free(config); } } diff --git a/libpromises/generic_agent.h b/libpromises/generic_agent.h index 58f792ab76..595f55be60 100644 --- a/libpromises/generic_agent.h +++ b/libpromises/generic_agent.h @@ -94,6 +94,7 @@ typedef struct bool skip_bootstrap_service_start; char *show_evaluated_classes; char *show_evaluated_variables; + char *simulate_keep_chroot; // --simulate-keep-chroot // BODY AGENT CONTROL bool report_class_log; diff --git a/tests/acceptance/29_simulate_mode/keep_chroot.cf b/tests/acceptance/29_simulate_mode/keep_chroot.cf new file mode 100644 index 0000000000..14aa513aea --- /dev/null +++ b/tests/acceptance/29_simulate_mode/keep_chroot.cf @@ -0,0 +1,86 @@ +body common control +{ + inputs => { + "../default.sub.cf", + }; + bundlesequence => { default("$(this.promise_filename)") }; + version => "1.0"; +} + +bundle agent init +{ + files: + "$(G.testdir)$(const.dirsep)keep-chroot-file" + create => "true", + content => "This is the original content of the file."; +} + +bundle agent test +{ + meta: + "test_soft_fail" + string => "(solaris|aix|hpux|windows)", + # ENT-6540 exotics fail to delete chroot + # On Windows the chroot maps drive letters to a different layout so the + # path of the kept copy checked below does not apply there + meta => { "ENT-6540" }; + + "description" + string => "Test that --simulate-keep-chroot keeps the changes chroot and that it is still deleted by default"; + + vars: + "kept_chroot" string => "$(G.testdir)$(const.dirsep)kept.changes"; + + commands: + # add --verbose here and look at the log for debugging sub policy runs + "$(sys.cf_agent) -Kf $(this.promise_dirname)$(const.dirsep)keep_chroot.cf.sub --simulate=manifest --simulate-keep-chroot=$(kept_chroot) > $(G.testdir)$(const.dirsep)keep.log 2>&1" + contain => in_shell, + comment => "Run sub policy in simulate mode, keeping the changes chroot"; + + "$(sys.cf_agent) -Kf $(this.promise_dirname)$(const.dirsep)keep_chroot.cf.sub --simulate=manifest > $(G.testdir)$(const.dirsep)default.log 2>&1" + contain => in_shell, + comment => "Run sub policy in simulate mode with the default chroot cleanup"; +} + +bundle agent check +{ + vars: + "kept_copy" + string => "$(test.kept_chroot)$(G.testdir)$(const.dirsep)keep-chroot-file"; + + classes: + "kept_copy_has_changes" + expression => strcmp(readfile("$(kept_copy)", 0), + "This is the would-be content of the file."), + if => fileexists("$(kept_copy)"); + + "real_file_untouched" + expression => strcmp(readfile("$(G.testdir)$(const.dirsep)keep-chroot-file", 0), + "This is the original content of the file."), + if => fileexists("$(G.testdir)$(const.dirsep)keep-chroot-file"); + + "keep_notice_logged" + expression => regline(".*Keeping changes chroot.*", + "$(G.testdir)$(const.dirsep)keep.log"), + if => fileexists("$(G.testdir)$(const.dirsep)keep.log"); + + # The default.log check below makes sure this is not evaluated before the + # run without --simulate-keep-chroot has actually happened + "default_chroot_deleted" + expression => strcmp(length(findfiles("$(sys.statedir)$(const.dirsep)*.changes")), "0"), + if => fileexists("$(G.testdir)$(const.dirsep)default.log"); + + "keep_requires_simulate" + expression => not(returnszero("$(sys.cf_agent) -Kf $(this.promise_dirname)$(const.dirsep)keep_chroot.cf.sub --simulate-keep-chroot=$(G.testdir)$(const.dirsep)unused.changes", "noshell")), + if => fileexists("$(G.testdir)$(const.dirsep)default.log"); + + "ok" + expression => and("kept_copy_has_changes", "real_file_untouched", + "keep_notice_logged", "default_chroot_deleted", + "keep_requires_simulate"); + + methods: + "Pass/Fail" + usebundle => dcs_passif("ok", "$(this.promise_filename)"), + inherit => "true"; # We want dcs_passif to inherit bundle scoped classes from our check bundle +} diff --git a/tests/acceptance/29_simulate_mode/keep_chroot.cf.sub b/tests/acceptance/29_simulate_mode/keep_chroot.cf.sub new file mode 100644 index 0000000000..e55972ec63 --- /dev/null +++ b/tests/acceptance/29_simulate_mode/keep_chroot.cf.sub @@ -0,0 +1,17 @@ +bundle common simulate_keep_chroot_promises +{ + vars: + "inputs" slist => { "$(this.promise_dirname)/../default.sub.cf" }; +} + +body common control +{ + inputs => { "@(simulate_keep_chroot_promises.inputs)" }; +} + +bundle agent main +{ + files: + "$(G.testdir)$(const.dirsep)keep-chroot-file" + content => "This is the would-be content of the file."; +} diff --git a/tests/acceptance/29_simulate_mode/keep_chroot_parent_perms.cf b/tests/acceptance/29_simulate_mode/keep_chroot_parent_perms.cf new file mode 100644 index 0000000000..e279264c60 --- /dev/null +++ b/tests/acceptance/29_simulate_mode/keep_chroot_parent_perms.cf @@ -0,0 +1,88 @@ +body common control +{ + inputs => { + "../default.sub.cf", + }; + bundlesequence => { default("$(this.promise_filename)") }; + version => "1.0"; +} + +bundle agent init +{ + vars: + "loose_parent" string => "$(G.testdir)$(const.dirsep)loose"; + "sticky_parent" string => "$(G.testdir)$(const.dirsep)sticky"; + + files: + "$(loose_parent)/." + create => "true", + perms => m("777"); + + "$(sticky_parent)/." + create => "true", + perms => m("1777"); +} + +bundle agent test +{ + meta: + "test_skip_unsupported" + string => "windows", + # The parent directory permission check is Unix-only + comment => "the check is not compiled in on Windows where these + permission bits do not apply"; + + "description" + string => "Test that --simulate-keep-chroot refuses a parent directory writable by other users unless it has the sticky bit set"; + + vars: + "refused" string => "$(init.loose_parent)$(const.dirsep)refused.changes"; + "kept" string => "$(init.sticky_parent)$(const.dirsep)kept.changes"; + + commands: + "$(sys.cf_agent) -Kf $(this.promise_dirname)$(const.dirsep)keep_chroot_parent_perms.cf.sub --simulate=manifest --simulate-keep-chroot=$(refused) > $(G.testdir)$(const.dirsep)parent-loose.log 2>&1" + contain => in_shell, + comment => "Run sub policy with the kept chroot inside a world-writable directory"; + + # The umask makes sure the 0700 permissions are still enforced when the + # sticky bit on the parent directory is what allows the creation. + "{ umask 0777; $(sys.cf_agent) -Kf $(this.promise_dirname)$(const.dirsep)keep_chroot_parent_perms.cf.sub --simulate=manifest --simulate-keep-chroot=$(kept); } > $(G.testdir)$(const.dirsep)parent-sticky.log 2>&1" + contain => in_shell, + comment => "Run sub policy with the kept chroot inside a world-writable directory with the sticky bit"; +} + +bundle agent check +{ + classes: + "loose_parent_error_logged" + expression => regline(".*Refusing to create the directory.*writable by other users.*", + "$(G.testdir)$(const.dirsep)parent-loose.log"), + if => fileexists("$(G.testdir)$(const.dirsep)parent-loose.log"); + + "loose_parent_nothing_created" + expression => not(isdir("$(test.refused)")), + if => fileexists("$(G.testdir)$(const.dirsep)parent-loose.log"); + + "loose_parent_refused" + expression => not(returnszero("$(sys.cf_agent) -Kf $(this.promise_dirname)$(const.dirsep)keep_chroot_parent_perms.cf.sub --simulate=manifest --simulate-keep-chroot=$(test.refused)", "noshell")), + if => fileexists("$(G.testdir)$(const.dirsep)parent-loose.log"); + + "sticky_parent_kept_chroot" + expression => regline(".*Keeping changes chroot.*", + "$(G.testdir)$(const.dirsep)parent-sticky.log"), + if => fileexists("$(G.testdir)$(const.dirsep)parent-sticky.log"); + + "sticky_parent_dir_mode_0700" + expression => strcmp(filestat("$(test.kept)", "permoct"), "700"), + if => isdir("$(test.kept)"); + + "ok" + expression => and("loose_parent_error_logged", "loose_parent_nothing_created", + "loose_parent_refused", "sticky_parent_kept_chroot", + "sticky_parent_dir_mode_0700"); + + methods: + "Pass/Fail" + usebundle => dcs_passif("ok", "$(this.promise_filename)"), + inherit => "true"; # We want dcs_passif to inherit bundle scoped classes from our check bundle +} diff --git a/tests/acceptance/29_simulate_mode/keep_chroot_parent_perms.cf.sub b/tests/acceptance/29_simulate_mode/keep_chroot_parent_perms.cf.sub new file mode 100644 index 0000000000..6b5e77aeb2 --- /dev/null +++ b/tests/acceptance/29_simulate_mode/keep_chroot_parent_perms.cf.sub @@ -0,0 +1,18 @@ +bundle common keep_chroot_parent_perms_promises +{ + vars: + "inputs" slist => { "$(this.promise_dirname)/../default.sub.cf" }; +} + +body common control +{ + inputs => { "@(keep_chroot_parent_perms_promises.inputs)" }; +} + +bundle agent main +{ + files: + "$(G.testdir)$(const.dirsep)keep-chroot-parent-perms-file" + create => "true", + content => "This is the would-be content of the file."; +} diff --git a/tests/acceptance/29_simulate_mode/keep_chroot_path_limits.cf b/tests/acceptance/29_simulate_mode/keep_chroot_path_limits.cf new file mode 100644 index 0000000000..11b2f438c4 --- /dev/null +++ b/tests/acceptance/29_simulate_mode/keep_chroot_path_limits.cf @@ -0,0 +1,110 @@ +body common control +{ + inputs => { + "../default.sub.cf", + }; + bundlesequence => { default("$(this.promise_filename)") }; + version => "1.0"; +} + +bundle agent test +{ + meta: + "test_skip_unsupported" + string => "windows", + comment => "the limits below assume Unix absolute paths"; + + "test_skip_needs_work" + string => "!has_perl"; + + "description" + string => "Test that --simulate-keep-chroot rejects a path longer than the limit it prints, accepts one of exactly that length, and that mapping a too long path into the chroot aborts the run instead of truncating the path"; + + vars: + "toolong_keep" string => "$(G.testdir)$(const.dirsep)path-limits-toolong.changes"; + + commands: + # An argument this long is over the limit on all platforms (the limit is + # PATH_MAX / 2 and the error message prints its value, used by the + # boundary checks below). + "$(sys.cf_agent) -Kf $(this.promise_dirname)$(const.dirsep)keep_chroot_path_limits.cf.sub --simulate=manifest --simulate-keep-chroot=`$(G.perl) -e 'print \"/\", \"a\" x 2999'` > $(G.testdir)$(const.dirsep)pl-reject.log 2>&1" + contain => in_shell, + comment => "Run sub policy with a kept chroot path longer than the limit"; + + # The sub policy promises a path of more than 4000 bytes, which cannot be + # mapped to a path under the chroot within PATH_MAX on any platform. The + # run has to abort -- a truncated mapping would record the copy at a + # different path than reported and could map two long paths to one copy. + "$(sys.cf_agent) -Kf $(this.promise_dirname)$(const.dirsep)keep_chroot_path_limits.cf.sub --simulate=manifest --simulate-keep-chroot=$(toolong_keep) > $(G.testdir)$(const.dirsep)pl-mapped.log 2>&1 || echo SIMULATE_RUN_FAILED >> $(G.testdir)$(const.dirsep)pl-mapped.log" + contain => in_shell, + comment => "Run sub policy that promises a path too long to be mapped into the chroot"; +} + +bundle agent check +{ + vars: + "reject_output" + string => readfile("$(G.testdir)$(const.dirsep)pl-reject.log", 0), + if => fileexists("$(G.testdir)$(const.dirsep)pl-reject.log"); + + classes: + "arg_rejected" + expression => regline(".*Invalid argument for --simulate-keep-chroot, path longer than [0-9]+ bytes.*", + "$(G.testdir)$(const.dirsep)pl-reject.log"), + if => fileexists("$(G.testdir)$(const.dirsep)pl-reject.log"); + + # The limit printed in the message ends up in $(bound[1]) and the runs + # below make sure it is really the enforced one. + "have_bound" + expression => regextract("(?s).*path longer than ([0-9]+) bytes.*", + "$(reject_output)", "bound"); + + # A path of exactly the printed length has to make it past the argument + # check. It fails later (no path component this long can be created), but + # with the error from the creation attempt, not the argument check. + "bound_arg_ran" + expression => not(returnszero("$(sys.cf_agent) -Kf $(this.promise_dirname)$(const.dirsep)keep_chroot_path_limits.cf.sub --simulate=manifest --simulate-keep-chroot=`$(G.perl) -e 'print \"/\", \"a\" x ($(bound[1]) - 1)'` > $(G.testdir)$(const.dirsep)pl-boundary-pass.log 2>&1", "useshell")), + if => "have_bound"; + + "bound_arg_not_rejected" + not => regline(".*Invalid argument for --simulate-keep-chroot.*", + "$(G.testdir)$(const.dirsep)pl-boundary-pass.log"), + if => fileexists("$(G.testdir)$(const.dirsep)pl-boundary-pass.log"); + + "bound_arg_reached_creation" + expression => regline(".*Failed to create the directory.*", + "$(G.testdir)$(const.dirsep)pl-boundary-pass.log"), + if => fileexists("$(G.testdir)$(const.dirsep)pl-boundary-pass.log"); + + # One byte more has to be rejected by the argument check. + "over_bound_arg_rejected" + expression => not(returnszero("$(sys.cf_agent) -Kf $(this.promise_dirname)$(const.dirsep)keep_chroot_path_limits.cf.sub --simulate=manifest --simulate-keep-chroot=`$(G.perl) -e 'print \"/\", \"a\" x $(bound[1])'` > $(G.testdir)$(const.dirsep)pl-boundary-reject.log 2>&1", "useshell")), + if => "have_bound"; + + "over_bound_arg_reject_logged" + expression => regline(".*Invalid argument for --simulate-keep-chroot, path longer than [0-9]+ bytes.*", + "$(G.testdir)$(const.dirsep)pl-boundary-reject.log"), + if => fileexists("$(G.testdir)$(const.dirsep)pl-boundary-reject.log"); + + "mapped_error_logged" + expression => regline(".*too long to be mapped into the changes chroot.*", + "$(G.testdir)$(const.dirsep)pl-mapped.log"), + if => fileexists("$(G.testdir)$(const.dirsep)pl-mapped.log"); + + "mapped_run_failed" + expression => regline("SIMULATE_RUN_FAILED", + "$(G.testdir)$(const.dirsep)pl-mapped.log"), + if => fileexists("$(G.testdir)$(const.dirsep)pl-mapped.log"); + + "ok" + expression => and("arg_rejected", "have_bound", + "bound_arg_ran", "bound_arg_not_rejected", + "bound_arg_reached_creation", + "over_bound_arg_rejected", "over_bound_arg_reject_logged", + "mapped_error_logged", "mapped_run_failed"); + + methods: + "Pass/Fail" + usebundle => dcs_passif("ok", "$(this.promise_filename)"), + inherit => "true"; # We want dcs_passif to inherit bundle scoped classes from our check bundle +} diff --git a/tests/acceptance/29_simulate_mode/keep_chroot_path_limits.cf.sub b/tests/acceptance/29_simulate_mode/keep_chroot_path_limits.cf.sub new file mode 100644 index 0000000000..53deaa4f1b --- /dev/null +++ b/tests/acceptance/29_simulate_mode/keep_chroot_path_limits.cf.sub @@ -0,0 +1,27 @@ +bundle common keep_chroot_path_limits_promises +{ + vars: + "inputs" slist => { "$(this.promise_dirname)/../default.sub.cf" }; +} + +body common control +{ + inputs => { "@(keep_chroot_path_limits_promises.inputs)" }; +} + +bundle agent main +{ + vars: + # A path of more than 4000 bytes, built without external tools so that it + # works in simulate mode: prepended with the chroot directory path it can + # never fit in PATH_MAX (4096 at most on the supported platforms), so + # mapping it has to abort the run. + "a16" string => "aaaaaaaaaaaaaaaa"; + "a240" string => "$(a16)$(a16)$(a16)$(a16)$(a16)$(a16)$(a16)$(a16)$(a16)$(a16)$(a16)$(a16)$(a16)$(a16)$(a16)"; + "a256" string => "$(a240)$(a16)"; + "long" string => "$(a256)$(a256)$(a256)$(a256)$(a256)$(a256)$(a256)$(a256)$(a256)$(a256)$(a256)$(a256)$(a256)$(a256)$(a256)$(a240)"; + + files: + "/$(long)" + create => "true"; +} diff --git a/tests/acceptance/29_simulate_mode/keep_chroot_perms.cf b/tests/acceptance/29_simulate_mode/keep_chroot_perms.cf new file mode 100644 index 0000000000..b9b214da60 --- /dev/null +++ b/tests/acceptance/29_simulate_mode/keep_chroot_perms.cf @@ -0,0 +1,95 @@ +body common control +{ + inputs => { + "../default.sub.cf", + }; + bundlesequence => { default("$(this.promise_filename)") }; + version => "1.0"; +} + +bundle agent init +{ + vars: + "existing" string => "$(G.testdir)$(const.dirsep)existing.changes"; + + files: + "$(existing)/." + create => "true"; +} + +bundle agent test +{ + meta: + "test_skip_unsupported" + string => "windows", + comment => "umask and the 0700 permissions of the kept chroot are Unix + permission semantics"; + + "description" + string => "Test that the changes chroot kept by --simulate-keep-chroot is created with 0700 permissions regardless of umask, that a trailing separator in the path is handled, and that an existing directory is refused"; + + vars: + "kept_umask" string => "$(G.testdir)$(const.dirsep)kept-umask.changes"; + "kept_slash" string => "$(G.testdir)$(const.dirsep)kept-slash.changes"; + + commands: + # The braces make sure the log file is created before umask is changed, + # otherwise it would not be readable by the check bundle below. mkdir(2) + # applies umask to its mode argument, so with an umask this restrictive a + # kept chroot that is not explicitly given its permissions would end up + # with mode 0000. + "{ umask 0777; $(sys.cf_agent) -Kf $(this.promise_dirname)$(const.dirsep)keep_chroot_perms.cf.sub --simulate=manifest --simulate-keep-chroot=$(kept_umask); } > $(G.testdir)$(const.dirsep)perms-umask.log 2>&1" + contain => in_shell, + comment => "Run sub policy keeping the changes chroot under a fully restrictive umask"; + + "{ umask 0777; $(sys.cf_agent) -Kf $(this.promise_dirname)$(const.dirsep)keep_chroot_perms.cf.sub --simulate=manifest --simulate-keep-chroot=$(kept_slash)$(const.dirsep); } > $(G.testdir)$(const.dirsep)perms-slash.log 2>&1" + contain => in_shell, + comment => "Run sub policy with a trailing separator in the kept chroot path"; + + "$(sys.cf_agent) -Kf $(this.promise_dirname)$(const.dirsep)keep_chroot_perms.cf.sub --simulate=manifest --simulate-keep-chroot=$(init.existing) > $(G.testdir)$(const.dirsep)perms-existing.log 2>&1" + contain => in_shell, + comment => "Run sub policy with an already existing directory as the kept chroot path"; +} + +bundle agent check +{ + classes: + "umask_run_kept_chroot" + expression => regline(".*Keeping changes chroot.*", + "$(G.testdir)$(const.dirsep)perms-umask.log"), + if => fileexists("$(G.testdir)$(const.dirsep)perms-umask.log"); + + "umask_dir_mode_0700" + expression => strcmp(filestat("$(test.kept_umask)", "permoct"), "700"), + if => isdir("$(test.kept_umask)"); + + "slash_run_kept_chroot" + expression => regline(".*Keeping changes chroot.*", + "$(G.testdir)$(const.dirsep)perms-slash.log"), + if => fileexists("$(G.testdir)$(const.dirsep)perms-slash.log"); + + # The trailing separator is stripped, the directory is created at the + # normalized path (with the same enforced permissions). + "slash_dir_mode_0700" + expression => strcmp(filestat("$(test.kept_slash)", "permoct"), "700"), + if => isdir("$(test.kept_slash)"); + + "existing_dir_error_logged" + expression => regline(".*Failed to create the directory.*for keeping the changes chroot.*", + "$(G.testdir)$(const.dirsep)perms-existing.log"), + if => fileexists("$(G.testdir)$(const.dirsep)perms-existing.log"); + + "existing_dir_refused" + expression => not(returnszero("$(sys.cf_agent) -Kf $(this.promise_dirname)$(const.dirsep)keep_chroot_perms.cf.sub --simulate=manifest --simulate-keep-chroot=$(init.existing)", "noshell")), + if => fileexists("$(G.testdir)$(const.dirsep)perms-existing.log"); + + "ok" + expression => and("umask_run_kept_chroot", "umask_dir_mode_0700", + "slash_run_kept_chroot", "slash_dir_mode_0700", + "existing_dir_error_logged", "existing_dir_refused"); + + methods: + "Pass/Fail" + usebundle => dcs_passif("ok", "$(this.promise_filename)"), + inherit => "true"; # We want dcs_passif to inherit bundle scoped classes from our check bundle +} diff --git a/tests/acceptance/29_simulate_mode/keep_chroot_perms.cf.sub b/tests/acceptance/29_simulate_mode/keep_chroot_perms.cf.sub new file mode 100644 index 0000000000..11ad8fc74b --- /dev/null +++ b/tests/acceptance/29_simulate_mode/keep_chroot_perms.cf.sub @@ -0,0 +1,18 @@ +bundle common keep_chroot_perms_promises +{ + vars: + "inputs" slist => { "$(this.promise_dirname)/../default.sub.cf" }; +} + +body common control +{ + inputs => { "@(keep_chroot_perms_promises.inputs)" }; +} + +bundle agent main +{ + files: + "$(G.testdir)$(const.dirsep)keep-chroot-perms-file" + create => "true", + content => "This is the would-be content of the file."; +} diff --git a/tests/unit/eval_context_test.c b/tests/unit/eval_context_test.c index 77234beda8..a82d1aa8a3 100644 --- a/tests/unit/eval_context_test.c +++ b/tests/unit/eval_context_test.c @@ -122,6 +122,46 @@ void test_changes_chroot(void) /* Inverse should work as expected */ const char *normal = ToNormalRoot(chrooted); assert_string_equal(normal, "/etc/sysctl.d/00-default.conf"); + + /* The longest path that still fits: with the chroot above the mapped path + * is PATH_MAX - 1 bytes long. */ + const size_t chroot_len = strlen("/changes/go/here/"); + char long_path[PATH_MAX]; + size_t orig_len = PATH_MAX - chroot_len; + memset(long_path, 'a', orig_len); + long_path[0] = '/'; + long_path[orig_len] = '\0'; + chrooted = ToChangesChroot(long_path); + assert_int_equal(strlen(chrooted), PATH_MAX - 1); + assert_true(strncmp(chrooted, "/changes/go/here/", chroot_len) == 0); + + /* One byte more must not be truncated -- the run has to abort cleanly + * with EXIT_FAILURE (in a child process, since ToChangesChroot() does not + * return in that case). */ + orig_len = PATH_MAX - chroot_len + 1; + memset(long_path, 'a', orig_len); + long_path[0] = '/'; + long_path[orig_len] = '\0'; + pid_t child = fork(); + assert_true(child >= 0); + if (child == 0) + { + /* Silence the error message logged before the abort. */ + int devnull = open("/dev/null", O_WRONLY); + if (devnull != -1) + { + dup2(devnull, STDOUT_FILENO); + dup2(devnull, STDERR_FILENO); + } + ToChangesChroot(long_path); + + /* Not reached -- returning means the path was truncated. */ + _exit(0); + } + int status; + assert_int_equal(waitpid(child, &status, 0), child); + assert_true(WIFEXITED(status)); + assert_int_equal(WEXITSTATUS(status), EXIT_FAILURE); #endif }