From 8023f452a2b507c08a4ce687104dc24d39eb6d93 Mon Sep 17 00:00:00 2001 From: Daniel JB Clark Date: Mon, 17 Aug 2026 08:12:22 -0400 Subject: [PATCH] Handle digest initialization failure when hashing EVP_DigestInit*() failure went unhandled in HashNew(), HashFile() and HashPubKey(), so callers received an all-zero digest indistinguishable from a real one. HashNew() now returns NULL, HashFile() returns false, and HashPubKey() logs. EVP_DigestUpdate(), EVP_DigestFinal*() and fread() failures are handled in the same functions. Reachable in practice, not just in theory: on OpenSSL 3, CryptoDeInitialize() unloads the default provider and every subsequent EVP_DigestInit() fails. tests/unit/hash_init_fail_test.c covers all three, as a separate program because forcing the failure requires draining the OpenSSL 3 providers before any digest has run. Changelog: Fixed hashing silently returning an all-zero digest when the hash algorithm could not be initialized. Ticket: CFE-4717 --- libutils/hash.c | 82 ++++++++++--- libutils/hash.h | 2 +- tests/unit/Makefile.am | 5 +- tests/unit/hash_init_fail_test.c | 202 +++++++++++++++++++++++++++++++ 4 files changed, 275 insertions(+), 16 deletions(-) create mode 100644 tests/unit/hash_init_fail_test.c diff --git a/libutils/hash.c b/libutils/hash.c index a0a1d88c..f43bc874 100644 --- a/libutils/hash.c +++ b/libutils/hash.c @@ -147,11 +147,23 @@ Hash *HashNew(const char *data, const unsigned int length, HashMethod method) Log(LOG_LEVEL_ERR, "Could not allocate openssl hash context"); return NULL; } + if (EVP_DigestInit_ex(context, md, NULL) != 1) + { + Log(LOG_LEVEL_ERR, "Could not initialize openssl hash context"); + EVP_MD_CTX_destroy(context); + return NULL; + } + Hash *hash = HashBasicInit(method); - EVP_DigestInit_ex(context, md, NULL); - EVP_DigestUpdate(context, data, (size_t) length); unsigned int digest_length; - EVP_DigestFinal_ex(context, hash->digest, &digest_length); + if (EVP_DigestUpdate(context, data, (size_t) length) != 1 + || EVP_DigestFinal_ex(context, hash->digest, &digest_length) != 1) + { + Log(LOG_LEVEL_ERR, "Could not compute openssl hash"); + EVP_MD_CTX_destroy(context); + HashDestroy(&hash); + return NULL; + } EVP_MD_CTX_destroy(context); /* Update the printable representation */ HashCalculatePrintableRepresentation(hash); @@ -395,10 +407,11 @@ HashSize HashSizeFromId(HashMethod hash_id) return (hash_id >= HASH_METHOD_NONE) ? CF_NO_HASH : CF_DIGEST_SIZES[hash_id]; } -static void HashFile_Stream( +static bool HashFile_Stream( FILE *const file, unsigned char digest[EVP_MAX_MD_SIZE + 1], - const HashMethod type) + const HashMethod type, + const char *const filename) { assert(file != NULL); const EVP_MD *const md = HashDigestFromId(type); @@ -407,38 +420,73 @@ static void HashFile_Stream( Log(LOG_LEVEL_ERR, "Could not determine function for file hashing (type=%d)", (int) type); - return; + return false; } EVP_MD_CTX *const context = EVP_MD_CTX_new(); if (context == NULL) { Log(LOG_LEVEL_ERR, "Failed to allocate openssl hashing context"); - return; + return false; } - if (EVP_DigestInit(context, md) == 1) + bool success = false; + if (EVP_DigestInit(context, md) != 1) { + Log(LOG_LEVEL_ERR, + "Failed to initialize digest for hashing file '%s'", + filename); + } + else + { + success = true; + unsigned char buffer[1024]; size_t len; - while ((len = fread(buffer, 1, 1024, file))) + while ((len = fread(buffer, 1, sizeof(buffer), file)) > 0) { - EVP_DigestUpdate(context, buffer, len); + if (EVP_DigestUpdate(context, buffer, len) != 1) + { + Log(LOG_LEVEL_ERR, + "Failed to hash contents of file '%s'", + filename); + success = false; + break; + } + } + + /* fread() returns 0 for both EOF and error, so without this a read + * failure part way through yields a well-formed digest of the bytes + * read before it. */ + if (success && ferror(file)) + { + Log(LOG_LEVEL_ERR, + "Failed to read file '%s' for hashing", + filename); + success = false; } unsigned int digest_length; - EVP_DigestFinal(context, digest, &digest_length); + if (success && EVP_DigestFinal(context, digest, &digest_length) != 1) + { + Log(LOG_LEVEL_ERR, + "Failed to finalize digest for hashing file '%s'", + filename); + success = false; + } } EVP_MD_CTX_free(context); + return success; } /** * @param text_mode whether to read the file in text mode or not (binary mode) + * @return whether the file was hashed; on failure digest is left all-zero * @note Reading/writing file in text mode on Windows changes Unix newlines * into Windows newlines. */ -void HashFile( +bool HashFile( const char *const filename, unsigned char digest[EVP_MAX_MD_SIZE + 1], HashMethod type, @@ -464,11 +512,12 @@ void HashFile( "Cannot open file for hashing '%s'. (fopen: %s)", filename, GetErrorStr()); - return; + return false; } - HashFile_Stream(file, digest, type); + const bool success = HashFile_Stream(file, digest, type, filename); fclose(file); + return success; } /*******************************************************************/ @@ -583,6 +632,11 @@ void HashPubKey( unsigned int digest_length; EVP_DigestFinal(context, digest, &digest_length); } + else + { + Log(LOG_LEVEL_ERR, + "Failed to initialize digest for hashing public key"); + } EVP_MD_CTX_free(context); } diff --git a/libutils/hash.h b/libutils/hash.h index 8eef4895..a263ce54 100644 --- a/libutils/hash.h +++ b/libutils/hash.h @@ -150,7 +150,7 @@ HashSize HashSizeFromId(HashMethod hash_id); #define CF_HOSTKEY_STRING_SIZE (4 + 2 * EVP_MAX_MD_SIZE + 1) -void HashFile(const char *filename, unsigned char digest[EVP_MAX_MD_SIZE + 1], HashMethod type, bool text_mode); +bool HashFile(const char *filename, unsigned char digest[EVP_MAX_MD_SIZE + 1], HashMethod type, bool text_mode); void HashString(const char *buffer, int len, unsigned char digest[EVP_MAX_MD_SIZE + 1], HashMethod type); bool HashesMatch( const unsigned char digest1[EVP_MAX_MD_SIZE + 1], diff --git a/tests/unit/Makefile.am b/tests/unit/Makefile.am index c42739b5..1e95fb6a 100644 --- a/tests/unit/Makefile.am +++ b/tests/unit/Makefile.am @@ -93,7 +93,8 @@ endif if WITH_OPENSSL check_PROGRAMS += \ - hash_test + hash_test \ + hash_init_fail_test endif TESTS = $(check_PROGRAMS) @@ -146,6 +147,8 @@ logging_timestamp_test_SOURCES = logging_timestamp_test.c \ hash_test_SOURCES = hash_test.c +hash_init_fail_test_SOURCES = hash_init_fail_test.c + libcompat_test_CPPFLAGS = -I$(top_srcdir)/libcompat -I$(top_srcdir)/libutils libcompat_test_SOURCES = libcompat_test.c diff --git a/tests/unit/hash_init_fail_test.c b/tests/unit/hash_init_fail_test.c new file mode 100644 index 00000000..5874b9e9 --- /dev/null +++ b/tests/unit/hash_init_fail_test.c @@ -0,0 +1,202 @@ +#include + +#include +#include +#include +#include +#include +#include +#include +#if OPENSSL_VERSION_NUMBER >= 0x30000000L +#include +#endif + +/* + * Verifies the handling of EVP_DigestInit() / EVP_DigestInit_ex() failure: + * HashNew() must return NULL, HashFile() must return false, and HashPubKey() + * must report the failure rather than silently returning something that looks + * like success. + * + * This is a separate program from hash_test on purpose. The way to force + * EVP_DigestInit*() to fail from a unit test, without mocking, is to unload + * the OpenSSL 3 providers before anything in the process performs a digest + * operation. Once any digest has run, OpenSSL activates the default + * provider as a fallback, and an explicit load+unload pair no longer + * deactivates it -- appended to the end of hash_test.c, this test would + * find EVP_DigestInit() still succeeding and fail spuriously. + * + * The drain below loads each provider exactly once and unloads it exactly + * once. Never unload more times than loaded: that crashes inside OpenSSL. + * EVP_cleanup() and ERR_free_strings() are no-op macros since OpenSSL 1.1.0 + * and are deliberately not called. + */ + +static bool init_failure_forced = false; + +/* + * HashPubKey() returns void and left the digest zeroed on failure before this + * change too, so the digest alone does not distinguish fixed from unfixed -- + * the error message is the whole of what was added. Asserting on it means + * capturing the log, and there is no public reader for the buffer + * StartLoggingIntoBuffer() fills, so redirect the stream Log() writes to + * instead. HashFile() needs none of this now that it returns bool. + */ +static int stdout_saved = -1; +static char stdout_path[64]; + +static void StartCapturingLog(void) +{ + fflush(stdout); + strlcpy(stdout_path, "/tmp/hash_init_fail_log_XXXXXX", sizeof(stdout_path)); + int fd = mkstemp(stdout_path); + assert_true(fd >= 0); + stdout_saved = dup(STDOUT_FILENO); + assert_true(stdout_saved >= 0); + assert_true(dup2(fd, STDOUT_FILENO) >= 0); + close(fd); +} + +static void StopCapturingLog(char *const buffer, const size_t size) +{ + fflush(stdout); + dup2(stdout_saved, STDOUT_FILENO); + close(stdout_saved); + stdout_saved = -1; + + size_t got = 0; + FILE *file = fopen(stdout_path, "r"); + if (file != NULL) + { + got = fread(buffer, 1, size - 1, file); + fclose(file); + } + buffer[got] = '\0'; + unlink(stdout_path); +} + +static void drain_openssl_providers(void) +{ +#if OPENSSL_VERSION_NUMBER >= 0x30000000L + OSSL_PROVIDER *legacy = OSSL_PROVIDER_load(NULL, "legacy"); + OSSL_PROVIDER *dflt = OSSL_PROVIDER_load(NULL, "default"); + if (legacy != NULL) + { + OSSL_PROVIDER_unload(legacy); + } + if (dflt != NULL) + { + OSSL_PROVIDER_unload(dflt); + } +#endif + + /* Precondition for the tests below: the digest must still be *known*, so + * that the functions under test reach EVP_DigestInit*() rather than + * their earlier md == NULL guard, while initialization itself fails. In + * environments where the drain cannot take effect -- OpenSSL before + * 3.0, a provider activated by openssl.cnf, FIPS -- init_failure_forced + * stays false and the tests skip rather than fail. */ + const EVP_MD *md = EVP_get_digestbyname("sha256"); + if (md == NULL) + { + return; + } + EVP_MD_CTX *context = EVP_MD_CTX_new(); + if (context == NULL) + { + return; + } + init_failure_forced = (EVP_DigestInit_ex(context, md, NULL) != 1); + EVP_MD_CTX_free(context); +} + +static void test_HashNew_returns_NULL_on_init_failure(void) +{ + if (!init_failure_forced) + { + return; + } + static const char message[] = "This is a message"; + Hash *hash = HashNew(message, strlen(message), HASH_METHOD_SHA256); + assert_true(hash == NULL); +} + +static void test_HashFile_reports_init_failure(void) +{ + if (!init_failure_forced) + { + return; + } + static const char message[] = "This is a message"; + char file[] = "/tmp/hash_init_fail_XXXXXX"; + int fd = mkstemp(file); + assert_true(fd >= 0); + ssize_t written = write(fd, message, strlen(message)); + assert_true(written == (ssize_t) strlen(message)); + + unsigned char digest[EVP_MAX_MD_SIZE + 1]; + memset(digest, 0xAA, sizeof(digest)); + + assert_false(HashFile(file, digest, HASH_METHOD_SHA256, false)); + for (size_t i = 0; i < sizeof(digest); i++) + { + assert_int_equal(digest[i], 0); + } + + close(fd); + unlink(file); +} + +static void test_HashPubKey_leaves_zero_digest_on_init_failure(void) +{ + if (!init_failure_forced) + { + return; + } + /* Generating a key needs a live provider, which the drain removed, so + * assemble one directly; HashPubKey() only reads n and e. */ + RSA *rsa = RSA_new(); + assert_true(rsa != NULL); + BIGNUM *n = BN_new(); + BIGNUM *e = BN_new(); + assert_true(n != NULL); + assert_true(e != NULL); + BN_set_word(n, 0xF00DFACE); + BN_set_word(e, RSA_F4); + assert_int_equal(RSA_set0_key(rsa, n, e, NULL), 1); + + unsigned char digest[EVP_MAX_MD_SIZE + 1]; + memset(digest, 0xAA, sizeof(digest)); + char log[4096]; + StartCapturingLog(); + HashPubKey(rsa, digest, HASH_METHOD_SHA256); + StopCapturingLog(log, sizeof(log)); + + for (size_t i = 0; i < sizeof(digest); i++) + { + assert_int_equal(digest[i], 0); + } + /* As above: the digest was already zeroed before this change. */ + assert_true( + strstr(log, "Failed to initialize digest for hashing public key") + != NULL); + + RSA_free(rsa); +} + +int main() +{ + PRINT_TEST_BANNER(); + drain_openssl_providers(); + if (!init_failure_forced) + { + puts("hash_init_fail_test: could not force digest initialization" + " failure in this environment; tests will pass vacuously"); + } + const UnitTest tests[] = + { + unit_test(test_HashNew_returns_NULL_on_init_failure), + unit_test(test_HashFile_reports_init_failure), + unit_test(test_HashPubKey_leaves_zero_digest_on_init_failure), + }; + return run_tests(tests); +}