diff --git a/cf-agent/verify_files_utils.c b/cf-agent/verify_files_utils.c index 008c7ec469..c29b315ab4 100644 --- a/cf-agent/verify_files_utils.c +++ b/cf-agent/verify_files_utils.c @@ -3690,7 +3690,6 @@ static PromiseResult VerifyFileIntegrity(EvalContext *ctx, const char *file, con { assert(attr != NULL); unsigned char digest1[EVP_MAX_MD_SIZE + 1]; - unsigned char digest2[EVP_MAX_MD_SIZE + 1]; if ((attr->change.report_changes != FILE_CHANGE_REPORT_CONTENT_CHANGE) && (attr->change.report_changes != FILE_CHANGE_REPORT_ALL)) { @@ -3698,27 +3697,22 @@ static PromiseResult VerifyFileIntegrity(EvalContext *ctx, const char *file, con } memset(digest1, 0, EVP_MAX_MD_SIZE + 1); - memset(digest2, 0, EVP_MAX_MD_SIZE + 1); PromiseResult result = PROMISE_RESULT_NOOP; bool changed = false; - if (attr->change.hash == HASH_METHOD_BEST) - { - HashFile(file, digest1, HASH_METHOD_MD5, false); - HashFile(file, digest2, HASH_METHOD_SHA1, false); - changed = (changed || - FileChangesCheckAndUpdateHash(ctx, file, digest1, HASH_METHOD_MD5, attr, pp, &result)); - changed = (changed || - FileChangesCheckAndUpdateHash(ctx, file, digest2, HASH_METHOD_SHA1, attr, pp, &result)); - } - else - { - HashFile(file, digest1, attr->change.hash, false); + /* "best" resolves to a single strong hash (SHA-512) in both Community and + * Enterprise (see GetBestFileChangeHashMethod()), so HASH_METHOD_BEST is + * never a runtime hash value here. Previously the Community stub returned + * HASH_METHOD_BEST and this function checked MD5 and SHA1 in sequence with + * a short-circuiting ||, which skipped updating the second hash once the + * first reported a change -- leaving it stale and firing a phantom + * "Content changed" on the next run (CFE-3725). */ + assert(attr->change.hash != HASH_METHOD_BEST); - changed = (changed || - FileChangesCheckAndUpdateHash(ctx, file, digest1, attr->change.hash, attr, pp, &result)); - } + HashFile(file, digest1, attr->change.hash, false); + + changed = FileChangesCheckAndUpdateHash(ctx, file, digest1, attr->change.hash, attr, pp, &result); if (changed && MakingInternalChanges(ctx, pp, attr, &result, "record integrity changes in '%s'", file)) { diff --git a/libpromises/attributes.c b/libpromises/attributes.c index ddc3138e8e..8240f3b2a8 100644 --- a/libpromises/attributes.c +++ b/libpromises/attributes.c @@ -876,7 +876,10 @@ FileRename GetRenameConstraints(const EvalContext *ctx, const Promise *pp) ENTERPRISE_FUNC_0ARG_DEFINE_STUB(HashMethod, GetBestFileChangeHashMethod) { - return HASH_METHOD_BEST; + /* "best" resolves to the strongest available hash (SHA-512), matching the + * Enterprise implementation of this function. The legacy MD5+SHA1 pairing + * was neither the "best" pair of algorithms nor FIPS-approved. */ + return HASH_METHOD_SHA512; } FileChange GetChangeMgtConstraints(const EvalContext *ctx, const Promise *pp) diff --git a/tests/acceptance/10_files/02_maintain/changes_best_hash_no_double_detection.cf b/tests/acceptance/10_files/02_maintain/changes_best_hash_no_double_detection.cf new file mode 100644 index 0000000000..e457b5df88 --- /dev/null +++ b/tests/acceptance/10_files/02_maintain/changes_best_hash_no_double_detection.cf @@ -0,0 +1,103 @@ +# Test that a files 'changes' body with hash => "best" reports a content change +# exactly ONCE, with no phantom re-detection on the following run (CFE-3725). +# +# hash => "best" now resolves to a single strong hash (SHA-512). Previously +# Community "best" checked MD5 and SHA1 in sequence with a short-circuit: when +# MD5 detected a change the SHA1 update was skipped, leaving the stored SHA1 +# stale, so the NEXT run fired a second "Content changed" via the SHA1 check. +# With a single hash a content change is logged exactly once. +# +# Runs, all inside one agent invocation: +# init - baseline an (empty) file with hash => "best" +# test - change the content, then re-check (run 2), then re-check again (run 3) +# check - the change log must contain EXACTLY ONE "Content changed" for the file +# (before the fix there are two: run 2 real + run 3 phantom). +body common control +{ + inputs => { "../../default.sub.cf", "check_file_changes_log.cf.sub" }; + bundlesequence => { default("$(this.promise_filename)") }; + version => "1.0"; +} + +bundle agent init +{ + files: + # Baseline the file with hash => "best" (records the stored hash). + "$(G.testfile).best" + create => "true", + changes => changes_best; +} + +bundle agent test +{ + meta: + "test_skip_unsupported" string => "windows"; + + files: + # Change the content so the next check detects a difference. + "$(G.testfile).best" edit_line => insert_marker; + + methods: + # Run 2: re-check the now-changed file -> reports the change (once). + "run2" usebundle => recheck_best; + + # Run 3: CFEngine skips a bundle it already ran, so a duplicate bundle + # gives us a third run. No further content change -> must report nothing. + "run3" usebundle => recheck_best_again; +} + +bundle edit_line insert_marker +{ + insert_lines: + "changed content for CFE-3725"; +} + +bundle agent recheck_best +{ + files: + "$(G.testfile).best" changes => changes_best; +} + +bundle agent recheck_best_again +{ + files: + "$(G.testfile).best" changes => changes_best; +} + +body changes changes_best +{ + hash => "best"; + report_changes => "content"; + update_hashes => "yes"; +} + +bundle agent check +{ + methods: + # Expected: exactly one "Content changed" entry for our file. check_file_changes_log + # reads the real $(sys.workdir)/state/file_changes.log, strips unstable fields + # (timestamp/promise-id/path) down to ",C,Content changed", and diffs. + "any" + usebundle => file_make( + "$(G.testfile).expected", "TEST.cfengine.best,C,Content changed" + ); + + "any" + usebundle => check_file_changes_log( + "$(G.testfile).expected", "cfe3725_log_ok", "cfe3725_log_fail", "" + ); + + reports: + # Decide the outcome from the classes that check_file_changes_log sets in + # the methods promises above. Guarding the reports directly (instead of + # via an intermediate "ok" class in a classes promise, which would be + # evaluated a pass earlier) means exactly one of Pass/FAIL is emitted. + cfe3725_log_ok.!cfe3725_log_fail:: + "$(this.promise_filename) Pass"; + + cfe3725_log_fail|!cfe3725_log_ok:: + "$(this.promise_filename) FAIL: expected exactly one 'Content changed' for the file (hash=>best must not re-detect on the next run)"; +} + +### PROJECT_ID: core +### CATEGORY_ID: 27 diff --git a/tests/acceptance/28_inform_testing/01_files/changes01.cf.expected b/tests/acceptance/28_inform_testing/01_files/changes01.cf.expected index 5f30ec8e07..c6f69a3b72 100644 --- a/tests/acceptance/28_inform_testing/01_files/changes01.cf.expected +++ b/tests/acceptance/28_inform_testing/01_files/changes01.cf.expected @@ -1,2 +1 @@ - info: Stored md5 hash for '/tmp/TEST.cfengine' (MD5=d41d8cd98f00b204e9800998ecf8427e) - info: Stored sha1 hash for '/tmp/TEST.cfengine' (SHA=da39a3ee5e6b4b0d3255bfef95601890afd80709) + info: Stored sha512 hash for '/tmp/TEST.cfengine' (SHA=cf83e1357eefb8bdf1542850d66d8007d620e4050b5715dc83f4a921d36ce9ce47d0d13c5d85f2b0ff8318d2877eec2f63b931bd47417a81a538327af927da3e)