Handle digest initialization failure when hashing

[djbclark]

Three functions in this file called EVP_DigestInit() or
EVP_DigestInit_ex() without handling failure, each in a different way:

• HashFile() and HashPubKey() zero their output digest and fill it in
  only on success, so on failure the caller received an all-zero
  digest indistinguishable from a real one. Both return void, so the
  digest buffer alone carried no success indication, and neither
  logged anything.
• HashNew() ignored the return value entirely and then called
  EVP_DigestUpdate() and EVP_DigestFinal_ex() on an uninitialized
  context, returning a non-NULL Hash whose digest is meaningless.

Each has a sibling in this same file that already does the right thing,
which is what the fixes are modelled on: HashString() logs in exactly
this case, and HashNewFromDescriptor() logs, destroys the context and
returns NULL. The asymmetry dates from the commit that first moved
these functions into libntech and looks like an oversight rather than a
decision.

HashFile() and HashPubKey() now log an error. HashNew() now logs and
returns NULL, matching HashNewFromDescriptor(); its callers already
handle NULL, which it returns on four other paths. HashBasicInit() moved
below the check so the failure path has nothing to free.

The failure is reachable in practice, not just in theory. On OpenSSL 3,
CryptoDeInitialize() unloads the default provider and every subsequent
EVP_DigestInit() fails; anything hashing after that point was silently
affected. That is how this was found. HashPubKey() is the most serious
of the three, since a public key digest that silently becomes a constant
is an identity that collides across hosts. In cfengine/core that digest
reaches TrustKey/GetPubkeyDigest (cf-key), PolicyHubUpdateKeys,
sys.key_digest, cf-execd's mail header, and the localhost entry of
lastseen's Address2Hostkey. The TLS trust-on-first-use path hashes peer
keys via HashNewFromKey(), which already failed closed before this
change.

A test is included. tests/unit/hash_init_fail_test.c drains the OpenSSL 3
default provider before any digest use, which makes EVP_DigestInit_ex()
fail while EVP_get_digestbyname() still succeeds, so the new branch is
what runs rather than the pre-existing md == NULL guard. It asserts that
HashNew() returns NULL and that HashFile() and HashPubKey() report the
failure.

It is a separate program rather than a case in hash_test.c because the
drain only works before any EVP digest operation has run: once one has,
OpenSSL activates the default provider as a fallback that an explicit
load and unload no longer removes, and the assertions would pass
spuriously against the very change they are meant to hold. Where the
drain cannot take effect at all -- OpenSSL before 3.0, a provider
activated by configuration, FIPS -- the test detects that and skips
rather than failing.

Note that the zeroed digest from HashFile() and HashPubKey() is not new;
what this change adds for those two is the report of it, so that is what
the test asserts. Against the unfixed file all three cases fail.

Making failure detectable by the callers of HashFile() and HashPubKey()
rather than merely visible in the log would mean changing the return
types of two void functions with callers across both repositories, and
is deliberately left out of this change. The return values of
EVP_DigestUpdate() and EVP_DigestFinal() also remain unchecked
throughout this file, as before; checking them is a separate change with
a different failure signature, since a failure after a successful init
yields a hash of partial data rather than an all-zero digest.

Fixes #290. Reviewed on a fork PR first: djbclark#1.

[mender-test-bot]

There was an error running your pipeline, see logs for details.

[CLAassistant]

All committers have signed the CLA.

[mender-test-bot]

There was an error running your pipeline, see logs for details.

[djbclark]

I've force-pushed a correction. Since that hides the change from anyone who already read the first version, here is exactly what was wrong and what moved.

Two claims in the original were wrong, and both were mine.

1. "No unit test: forcing EVP_DigestInit() to fail requires deinitializing the crypto library, which lives in cfengine/core's libpromises." This is false. It can be done from libntech alone, and the PR now includes the test: tests/unit/hash_init_fail_test.c loads the OpenSSL 3 providers and unloads them before any digest operation runs, which makes EVP_DigestInit_ex() fail while EVP_get_digestbyname() still succeeds — so it exercises the new branch rather than the pre-existing md == NULL guard.

   It is a separate test program rather than a case appended to hash_test.c, and that detail matters: once any EVP digest operation has run in the process, OpenSSL activates the default provider as a fallback that an explicit load/unload pair no longer removes. Inside hash_test.c the assertions would pass spuriously against the very change they are meant to hold. Where the drain cannot take effect at all (OpenSSL < 3.0, a provider activated by configuration, FIPS) the test detects that and skips rather than failing.

2. "it feeds lastseen and the TLS paths" overstated the impact. The TLS trust-on-first-use path hashes peer keys through HashNewFromKey(), which already returned NULL on EVP_DigestInit_ex failure before this change — it fails closed, and HashPubKey is not on it. There are no HashPubKey calls in libcfnet or cf-serverd at all. What HashPubKey's digest actually reaches in cfengine/core is TrustKey/GetPubkeyDigest (cf-key), PolicyHubUpdateKeys, sys.key_digest, cf-execd's mail header, and the localhost branch of lastseen's Address2Hostkey. The commit message now says that instead.

I've left a matching correction on #290, whose "worst-case impact" paragraph put it more strongly still.

The C change itself is unchanged — byte-identical to what you have already seen. Only the commit message and the new test are different.

One scope note now stated explicitly in the commit rather than left implicit: EVP_DigestUpdate() and EVP_DigestFinal() returns remain unchecked throughout this file, as before. That is a different failure signature — a failure after a successful init yields a hash of partial data, not an all-zero digest — and I'd rather it were a separate change than smuggled into this one. Happy to do it as a follow-up.

Verified in both directions: against this branch the suite is 40/40 with the new test passing; against the file as it is on master the new test fails all three of its cases (exit 3).

[djbclark]

Now tracked in Jira as CFE-4717.

Issue #290 said I'd move this into Jira once access was sorted — that's now done. (For anyone hitting the same thing: creating the Atlassian account wasn't sufficient, I also needed to be granted permission in the project. Thanks to @nickanderson for the help.)

The Jira ticket carries the corrected impact assessment rather than the original one — the all-zero digest is a colliding lookup handle, not a bypassed cryptographic gate — and it records that the "no unit test is possible" claim was wrong, since this PR now carries hash_init_fail_test.c.

Nothing about the code changes here; this is a tracker reference only.

[mender-test-bot]

There was an error running your pipeline, see logs for details.

[larsewi]

Thanks for submitting the patch @djbclark. Just a few things. Could you change the signature of HashFile_Stream and HashFile to return boolean (false on error)? Also, the test seems a bit overkill, for testing an error path that should never happen. How did you come across it? Was it through static analysis, or did this actually happen to you? Last but not least, would you mind shortening the commit message a bit? This is pretty simple change and a few lines should be sufficient.

[larsewi]

Why did you move this to one line above?

Hash *hash = HashBasicInit(method);

[larsewi]

EVP_DigestUpdate and EVP_DigestFinal_ex can also return error. Maybe add check here as well?

[larsewi]

It would be even better to change the signature of this function to return boolean (false on error). This way the callers could handle the error. Not just print it.

[mender-test-bot]

There was an error running your pipeline, see logs for details.

[djbclark]

All addressed in 8023f45. Yes, AI-assisted — and noted on verbosity, I will keep these tighter.

Signatures. HashFile_Stream and HashFile now return bool.

Left HashPubKey as void on purpose: cfengine/core redefines it in three test files (tests/unit/lastseen_test.c, tests/unit/lastseen_migration_test.c, tests/load/lastseen_load.c), so changing it breaks core's build until a matching core PR lands. HashFile has no such stubs — verified by building core against this patch, all ten callers plus core's unit tests clean. Say the word and I will do HashPubKey and HashString in a follow-up.

EVP_DigestUpdate/EVP_DigestFinal_ex. Checked now, in HashNew and HashFile_Stream. Not in the other four functions yet — happy to do the whole file if you prefer.

HashBasicInit. Moved below the init check, so the failure path has nothing to free.

fread. Also added, since the loop was being rewritten anyway: fread returns 0 for both EOF and error, so a read failure part way through gave a well-formed digest of a prefix of the file. That branch is not covered by a test — I could not force it portably without exposing HashFile_Stream.

The test. Not static analysis, and not the field: we hit it building the --simulate work in #6293/#6294, writing unit tests for a schema linter over that output — hashes silently all-zero. The cause was CryptoDeInitialize() unloading the OpenSSL 3 default provider, after which every EVP_DigestInit() fails. One CryptoDeInitialize() away, not unreachable, which is why I wanted it pinned.

Your first request is what shrank it: with HashFile returning bool that case is one assert_false, no log capture. The rest is the provider drain plus the capture that only HashPubKey still needs — if that returns bool too, ~45 more lines go.

Commit message. 70 lines -> 19.

[larsewi]

Feel free to change the signature of HashPubKey too and create a matching core PR. We can easily test those two PRs together.

[larsewi]

I would prefer it if you freed heap allocated data here and do an early return. The success variable does save you a few lines of code, but makes the code harder to read / reason about. Early return will also result in less nesting.

[larsewi]

Same here, I would do this check inside the while loop and do an early return in case of failure.

[olehermanse]

See: cfengine/core#6293 (comment)