Added --simulate-keep-chroot option to keep the changes chroot after a run

[djbclark]

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 with mode 0700, so that the permission-mirrored copies of
potentially sensitive system files can neither land in a directory
prepared with looser permissions nor mix with the contents of a
previous run. 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 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.

Requested in #6295.

[CLAassistant]

All committers have signed the CLA.

[djbclark]

Apologies for the force-push churn on this branch — the commit metadata is now correct, and this is what moved and why.

The commit originally carried Ticket: #6295, referring to the discussion this PR implements. I removed that trailer yesterday after convincing myself the number was invented, because GET /repos/cfengine/core/issues/6295 returned 404. That was my mistake: issues are disabled on this repository, #6295 is a Discussion, and the issues endpoint cannot see one. The reference was right all along.

So Ticket: #6295 is restored, and I have added the Changelog: Title line it should have had alongside it per CONTRIBUTING — the title is already a past-tense, user-facing description of the change. I have also added a "Requested in #6295" line to the PR description, which was missing that link entirely.

No code changed in either direction. The tree is byte-identical to what you have already seen; both rewrites touched only the commit message, and I verified the tree hashes match before pushing. Still one commit.

[djbclark]

Please hold off merging this — a review I commissioned has found a buffer overflow in it, and a correction is coming. Flagging now rather than waiting until I have the fix written, so nobody spends time merging it meanwhile.

The defect. The only validation on --simulate-keep-chroot is strlen(optarg) >= PATH_MAX (cf-agent/cf-agent.c:815), which is the wrong bound for what the path is later used for. A path of PATH_MAX - 1 bytes passes it, then drives chroot_len to PATH_MAX in ToChangesChroot(). That function's copy is:

strncpy(chrooted_path + chroot_len + offset, orig_path,
        (PATH_MAX - chroot_len - offset - 1));

With chroot_len == PATH_MAX - 1 and offset == 1, the count evaluates to -1, which as a size_t is SIZE_MAX. The result is an unbounded copy into the static chrooted_path buffer, on the first file the run maps (libpromises/eval_context.c:3897). Confirmed under AddressSanitizer.

The assert in that function does not catch it: it recomputes the same underflowed bound, and the shipped build is -DNDEBUG regardless.

Below that threshold the same bound truncates silently, so two distinct source paths can map to the same chroot path — which matters here specifically, because the tree holds permission-mirrored copies of real system files.

Two other things in the same change that I got wrong, which I'll fix in the same correction:

1. The commit body, the PR description and Retain the --simulate changes chroot after a run (--simulate-keep-chroot) #6295 all say the directory "is created with mode 0700" as the reason sensitive copies cannot land somewhere looser. mkdir(2) applies mode & ~umask, so 0700 is a ceiling, not a guarantee — under umask 0777 the directory is created 0000 and the run then continues happily, leaving an empty unusable tree with no error. That needs an explicit chmod after the mkdir.

2. That same sentence overstates the protection in a second way: mkdir does not inspect the parent. With a writable, non-sticky parent the new 0700 directory can be renamed away and replaced before the copies are written. Either the parent has to be checked, or the claim has to go.

I'd rather say all of this myself than have a reviewer find it. The correction will be a force-push to this branch with the fix and tests; I'll comment again when it's up.

[djbclark]

Now tracked in Jira as CFE-4715.

We couldn't reach the CFE tracker when this PR went up, which is why the commit trailer points at discussion #6295. That's now resolved (creating the Atlassian account wasn't sufficient on its own — I also needed permission in the project itself; thanks to @nickanderson for sorting that out).

The Ticket: trailer will be updated from #6295 to CFE-4715 in the same force-push that carries the correction described in my previous comment, so this PR won't churn twice. The hold-off-merging note still stands until that lands.

Going forward all our CFEngine/libntech reports will go to Jira rather than email or new discussions.

[djbclark]

The fixes are up — this supersedes the hold-off-merging notice above. Force-pushed to f6c06f9.

Every defect listed in that notice is fixed, and the review that produced it found two more things on the way in:

what was wrong	fix
strncpy count PATH_MAX - chroot_len - offset - 1 underflows to SIZE_MAX when chroot_len == PATH_MAX; ASan reports negative-size-param (size=-1). Shorter-but-still-long paths truncated silently instead, so two source files could map onto one copy	ToChangesChroot() now checks the total explicitly before writing anything and aborts with a logged error rather than truncating; the copy is a memcpy of the measured length. The assert that "checked" this is gone — it recomputed the same underflowed bound and the build is -DNDEBUG
the strlen(optarg) >= PATH_MAX option guard bounded the wrong thing — it bounds the chroot prefix, while the overflow is in prefix+suffix	bounded so roughly half the PATH_MAX budget is reserved for the mapped paths, with the printed bound matching the enforced one
mkdir(path, 0700) is masked by umask. Under umask 0777 the agent created the keep tree as mode 0000: an unusable directory, no error reported. The commit message's "created with mode 0700" was simply false	explicit chmod() after a successful mkdir, fatal if it fails
the commit message claimed the copies "can never end up in a directory with looser permissions". mkdir(0700) does not inspect the parent	the tree is now refused inside a group/world-writable parent that lacks the sticky bit, so the claim is enforced rather than asserted. The message has been rewritten to describe only what the code does

Two further defects found while fixing the above, neither of them in the original report:

• A one-byte out-of-bounds write in the fix's own code path. On MinGW the drive-letter bytes were written at chrooted_path[chroot_len] and [chroot_len + 1] before the bound check ran — index PATH_MAX + 1 on a char[PATH_MAX + 1] buffer. Nothing is written now until the total is validated.
• GetParentDirectoryCopy("/opt/keep/") returns the path itself, not its parent (libpromises/files_names.c:296-312). A trailing slash on the option would have made the new parent check silently inspect the directory being created instead of its parent. The path is normalized with DeleteSlash() first.

Tests

Three new acceptance tests in 29_simulate_mode/ (mode under a hostile umask; parent-permission refusal and the sticky-parent acceptance; trailing slash; pre-existing directory; the option bound, re-driven at exactly B and B+1 so printed-versus-enforced is asserted at the byte boundary; and an over-long mapped path aborting instead of truncating), plus a unit case in eval_context_test.c that forks and asserts WEXITSTATUS == EXIT_FAILURE, so it distinguishes a clean abort from silent truncation.

They were checked for discrimination rather than assumed: with the source changes stashed, all three acceptance tests fail and the unit case reports test_changes_chroot: Test failed; with them restored, all pass. Two sub-cases pass either way and are kept deliberately — the pre-existing-directory case is a regression guard, and the bound probe only discriminates where the old and new bounds straddle the probe length.

Full suite: All 68 tests behaved as expected (4 expected failures), make -j2 clean with no new warnings.

On the ASan verification, precisely

The pre-fix negative-size-param (size=-1) and the fixed function's clean abort were both reproduced under AddressSanitizer using a harness built from the verbatim function text of each revision, -DNDEBUG, including the exact boundary (chroot_len + orig_len == PATH_MAX - 1 maps, == PATH_MAX aborts). That verifies the function's logic, not the linked cf-agent object — reconfiguring the tree for a full ASan build would have discarded its known-good configuration. Stating that plainly rather than implying a stronger check than was run.

The Ticket: trailer is now CFE-4715 (https://northerntech.atlassian.net/browse/CFE-4715), replacing the discussion reference, which is what the trailer should have carried from the start.