libclamav: cache EVP_MD handles to fix per-hash provider-fetch churn and a FIPS memory leak - #1774
libclamav: cache EVP_MD handles to fix per-hash provider-fetch churn and a FIPS memory leak#1774Sewci0 wants to merge 3 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a0fdf1c3a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
bfdb1fb to
a980176
Compare
Sewci0
changed the title
d02de5f to
cbe8f6b
Compare
|
This is a significant issue for anyone using this on a fips enabled system. A large scan will quickly run a server out of memory. |
|
@val-ms is there a chance you could look into this please? This is not only an enhancement, currently running the scan on a fips enabled system will break the machine in a matter of seconds. |
jhumlick
left a comment
jhumlick
left a comment
There was a problem hiding this comment.
[P2] The tests do not exercise the actual leaking path
In test_hash_md5_sha1, a NULL result from cl_hash_init("md5") is simply skipped. However, EVP_MD_fetch() == NULL is precisely the FIPS condition that triggered the leak. The test can therefore pass without demonstrating that repeated failures remain bounded.
Recommended coverage:
Run many failing MD5 initializations under the strict-FIPS configuration.
Require every call to return NULL.
Run under LeakSanitizer or Valgrind and verify no per-call allocation remains.
Keep a successful SHA-256 control.
On ordinary non-FIPS CI, assert that MD5 and SHA-1 initialization succeeds instead of silently skipping it.
[P3] Harden the cache’s ownership handling
cli_get_md() ignores the result of EVP_MD_up_ref(). OpenSSL documents that it returns 1 on success and 0 on failure. If it failed, the function would still return the cache-owned pointer; the caller would later free that reference and leave a dangling entry in the cache.
It should return NULL when the reference cannot be acquired:
if (NULL != result && 1 != EVP_MD_up_ref(result)) {
result = NULL;
}
There is also a latent ownership leak if md_cache ever reaches its fixed capacity of eight: a newly fetched object is not stored, but it is still up-referenced before being returned. This is currently unreachable because to_openssl_alg() accepts only five algorithms, but the ownership logic should still distinguish cached and uncached results.
[P3] Remove obsolete local context variables and cleanup calls
Several callers retain:
OSSL_LIB_CTX *ossl_ctx = NULL;
and later call OSSL_LIB_CTX_free(ossl_ctx), even though context creation moved entirely into cli_get_md(). This is harmless because the pointer stays NULL, but removing it would make the new ownership model clearer.
|
@Sewci0 we are going to scramble to assemble a patch release for 1.4.6 and 1.5.4 faster than originally planned. I wish to include your fix (specifically because of the leak fix) in those versions. It doesn't seem fair to ask you to scramble as well, so I am going to handle the requested changes my teammate identified. |
cbe8f6b to
527832a
Compare
|
The force-push was just a rebase to get the latest from main. Will start addressing the review findings next. |
…and a FIPS memory leak On OpenSSL 3, the hashing helpers (cl_hash_init, cl_hash_data, cl_hash_file_fd and their _ex variants) created a fresh OSSL_LIB_CTX and called EVP_MD_fetch() on every hash, then tore both down. A freshly-allocated library context has no method-store cache, so each fetch pays the full provider-bootstrap + method-construction cost (on the order of hundreds of microseconds), and this runs for md5/sha1/sha2-256 on essentially every scanned object (whole-file and section hashes plus the SHA2-256 clean-cache lookup in fmap_get_hash). The per-object hashing plumbing ends up costing far more than the hashing itself, whether or not the host is in FIPS mode. Fetch each FIPS-bypass digest once into a single shared, long-lived non-FIPS OSSL_LIB_CTX and reuse it (a fetched EVP_MD is reference counted and safe to share across threads; the cache is guarded by a mutex). The FIPS-bypass semantics are unchanged -- the shared context is still created with OSSL_LIB_CTX_new() and digests are still fetched with the "-fips" property, so MD5/SHA1 signature hashing still works on FIPS hosts where it can. The FIPS-compliant (non-bypass) path is intentionally not cached: it is still fetched from the default library context on every call so it keeps honoring the current default property query. This also removes the per-hash OSSL_LIB_CTX_new(), which fixes a memory leak. The previous code allocated the context before the fetch and did not free it on the "md == NULL" return path, so on a host whose FIPS policy makes an algorithm unavailable (e.g. MD5), every hashed object leaked an OSSL_LIB_CTX. Because libclamav hashes MD5 for nearly every scanned file, a full scan grew without bound until it was OOM-killed. With a single shared context there is nothing to leak per hash; an unavailable algorithm now degrades to a clean, bounded failure instead of exhausting memory.
…d EVP_MD cache thread-safety Add coverage for the shared EVP_MD cache introduced by the per-hash provider-fetch fix: - test_hash_md5_sha1 verifies cl_hash_init/cl_update_hash/cl_finish_hash return correct MD5 and SHA1 digests. A NULL context is tolerated only where OpenSSL itself cannot fetch the digest (a FIPS policy withholding MD5/SHA1), asked directly via EVP_MD_fetch() so that an ordinary host cannot quietly skip the check; where the digest is withheld, the test instead requires many repeated initializations to keep failing cleanly. - test_hash_fetch_failure_is_bounded exercises the digest-fetch failure path on every host rather than only FIPS ones: an unsupported algorithm name fails at the same early return a FIPS-restricted MD5 does -- the return that used to leak an OSSL_LIB_CTX per call -- with a successful sha2-256 hash on either side as a control. Under the existing Valgrind test this asserts that no per-call allocation survives; against the pre-fix code the same test reports ~2000 definitely-lost blocks, failing "ERROR SUMMARY: 0 errors". - test_hash_cache_threadsafe hammers cl_hash_init() from multiple threads to exercise the cache and its mutex under contention. check_clamav now links OpenSSL::Crypto so the tests can ask OpenSSL directly which digests it is able to supply.
527832a to
e8a274c
Compare
|
Thanks for the review @jhumlick — all three items are addressed in e8a274c. The fixes are folded into the two existing commits, rebased onto current [P2] The tests do not exercise the actual leaking pathAgreed, the skip made it vacuous. Two changes: 1. static bool openssl_has_digest(const char *ossl_alg)
{
EVP_MD *md = EVP_MD_fetch(NULL, ossl_alg, NULL);
...
}So on ordinary non-FIPS CI, MD5/SHA-1 initialization must succeed and produce the right digest. Where the digest genuinely is withheld, the test instead requires 2000 repeated initializations to keep failing cleanly. 2. New Verified on Debian 12 / OpenSSL 3.0.20 via
The ~1,990 lost blocks track the 2000 failing iterations, with To be explicit about the enforcement point: the One limitation worth flagging: I could not force a real strict-FIPS fetch failure portably in a unit test. The bypass context comes from [P3] Harden the cache's ownership handlingDone. done:
/* A cached digest belongs to md_cache, so give the caller its own reference
* to EVP_MD_free(). If that reference can't be acquired, fail instead of
* handing back one the caller would free out from under the cache.
* A digest that was fetched but not cached is already the caller's to free. */
if (cached && NULL != result && 1 != EVP_MD_up_ref(result)) {
cli_errmsg("cli_get_md: Failed to acquire a reference to the %s digest\n", ossl_alg);
result = NULL;
}That also resolves the latent capacity-full case you spotted: a digest fetched but not cached is returned as the caller's own reference with no extra up-ref, so cached and uncached results no longer share ownership logic. [P3] Remove obsolete local context variables and cleanup callsRemoved: 6 dead Two notes
|
|
@val-ms everything's addressed |
Preserve existing libclamav error codes when digest acquisition fails, and emit diagnostics only after releasing the shared digest-cache mutex so application callbacks may safely re-enter hashing. Require MD5, SHA-1, and SHA-256 to remain available through the private non-FIPS context. Add deterministic Linux failure injection for provider-fetch and library-context allocation failures, including Valgrind coverage for bounded failure behavior.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc8f0fb6a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@Sewci0 Our testing indicates that this leak is reached only when the FIPS environment cannot provide MD5 through ClamAV’s separate non-FIPS OpenSSL context—typically because the default provider is absent, cannot be loaded, or is affected by provider configuration such as A correctly configured FIPS installation should permit ClamAV to use the FIPS provider for approved algorithms while obtaining MD5 and SHA-1 from the default provider for signature matching and related hashing. This patch prevents memory growth when digest creation fails, but it does not restore unavailable algorithms; the underlying OpenSSL provider configuration should still be corrected so ClamAV’s MD5- and SHA-1-based features function properly. |
|
@codex review — The latest review feedback has been addressed. Please re-review the current head. |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Agreed, and I don't want that lost: this patch bounds the failure, it doesn't repair it. Where the private-context fetch fails, MD5/SHA-1 signature matching stays broken — it just stops consuming memory without bound. Graceful degradation, not a fix. One refinement on the mechanism, though, because it changes where an operator should be told to look.
|
| condition | EVP_MD_fetch(fresh_ctx, "md5", "-fips") |
|---|---|
| baseline | OK — resolves to provider=default |
OPENSSL_MODULES=/nonexistent |
OK — resolves to provider=default |
explicit OSSL_PROVIDER_load(ctx, "default") |
OK — resolves to provider=default |
And as noted earlier in the thread, OSSL_LIB_CTX_new() doesn't read OPENSSL_CONF either — I couldn't get a config-only change to reach the bypass context at all.
So on a stock OpenSSL 3 build, neither the module search path nor openssl.cnf is reachable from ClamAV's private context. Where the fetch nonetheless fails, the enforcement has to be happening below the library-context layer — a libcrypto applying FIPS policy globally rather than per-context. That would be a property of a particular distro FIPS build rather than of the operator's configuration, which matters here: "correct your provider configuration" isn't actionable if the build doesn't expose a knob for it.
I can't confirm what Canonical's 22.04 FIPS build does from here — no FIPS provider on my test hosts. Happy to run a minimal probe on the affected host and report back; that's what settles whether the right guidance is "fix your configuration" or "this build cannot supply MD5 to a private context at all".
Two things that follow from your position
Both argue in the same direction you are, not against it. If ClamAV requires the default provider inside its private context — which test_hash_md5_sha1 now asserts outright — it seems worth making that requirement explicit rather than implicit:
-
Load the provider explicitly. One
OSSL_PROVIDER_load(nonfips_libctx, "default")instead of relying on fallback activation. It gives a single checkable failure point, and on a host where fallback activation is what's being suppressed it may restore MD5 rather than merely failing cleanly. Worth testing on an affected host before assuming either way. -
Say what actually went wrong. Today the bypass fetch failure is silent in
crypto.c. Through the legacy API the operator sees onlycli_scan_fmap: Error initializing md5 hash context(libclamav/matcher.c:1264); through the_exAPI it becomesCL_EARG→Unsupported hash algorithm: md5, which points at the algorithm name rather than at provider configuration. If the correct remedy is to fix the OpenSSL provider setup, ClamAV should be what tells the operator so — otherwise there's no path from the symptom to that conclusion.
Neither is release-blocking; happy to open them separately once 1.4.6 and 1.5.4 are out.
4 participants
Summary
On OpenSSL 3, libclamav re-acquires the digest implementation on every hash operation.
cl_hash_init(),cl_hash_data(),cl_hash_file_fd()and their*_exvariants each allocate a brand-newOSSL_LIB_CTX(OSSL_LIB_CTX_new()),EVP_MD_fetch()the algorithm with the"-fips"property (so non-approved MD5/SHA1 signature hashing works on FIPS-enabled hosts), then free the digest and the context.Because the provider/method cache lives inside a library context (
OSSL_LIB_CTX(3)), a freshly-allocated context starts with an empty cache, so eachEVP_MD_fetch()pays the full provider-bootstrap + method-construction cost — on the order of hundreds of microseconds — and it runs for md5/sha1/sha2-256 on essentially every scanned object (whole-file and section hashes, plus the SHA2-256 clean-cache lookup infmap_get_hash()). The per-object hashing plumbing ends up costing far more than the hashing itself.This is not FIPS-specific: the throwaway-context path is compiled unconditionally for OpenSSL 3, so it slows scanning whether or not the host is in FIPS mode.
The per-call throwaway context was introduced in 83fd7f1 (#1589, CLAM-2879) to fix a genuine correctness bug — MD5 hashing failing on strict-FIPS hosts when fetched from the default context. That fix is correct in what it does; the only problem is that it does it on every hash instead of once.
Fix
Per the FIPS module guide, a non-default
OSSL_LIB_CTXis the correct way to reach non-FIPS digests — but perEVP_MD_fetch/EVP_DigestInit(3)and the 3.0 migration guide (explicit fetching / performance), a fetchedEVP_MDis a reference-counted object that should be created once and reused, not rebuilt on every operation.So: create the shared non-FIPS
OSSL_LIB_CTXonce and cache each fetched bypassEVP_MDfor the process lifetime, handing callers an owned reference (their existingEVP_MD_free()cleanup is unchanged). This preserves #1589's FIPS-bypass semantics exactly — the shared context is still created withOSSL_LIB_CTX_new()and digests are still fetched with the"-fips"property, so MD5/SHA1 signature hashing keeps working on strict-FIPS hosts.The FIPS-compliant (non-bypass) path is deliberately not cached: it is still fetched from the default library context on every call, so it continues to honor the current default property query — e.g. if the embedding process enables FIPS at runtime, a non-bypass MD5/SHA1 request correctly starts failing instead of returning a stale non-FIPS handle. That default-context fetch is already inexpensive because OpenSSL caches provider methods per library context.
Benchmarks
Isolated micro-benchmark: identical hashing work both ways, differing only in how the digest is obtained — a fresh
OSSL_LIB_CTX+EVP_MD_fetch()per hash vs. fetch-once-and-reuse (this change). OpenSSL 3, aarch64, 200k hashes of a 4 KiB buffer:OSSL_LIB_CTX_new()/EVP_MD_fetch()confirms the bypass path drops from one call per hash (e.g. 5000 fetches for 5000 hashes) to one per algorithm for the entire process (→ 1).libclamav hashes md5/sha1/sha2-256 for essentially every scanned object — whole-file and section hashes, plus the SHA2-256 clean-cache lookup in
fmap_get_hash()— so this per-object overhead is paid throughout a scan. No functional or API change; hashing outputs are identical.Also fixes a memory leak on FIPS hosts
Independent of the performance win, removing the per-hash
OSSL_LIB_CTX_new()fixes a memory leak. The pre-existing code allocates the context before the fetch and does not free it on themd == NULLreturn path incl_hash_init,cl_hash_data, andcl_hash_file_fd(e.g.crypto.c:1866). On a host whose FIPS policy makes MD5 unavailable,EVP_MD_fetch(..., "md5", "-fips")returnsNULL, so every hashed object leaks anOSSL_LIB_CTX— and libclamav hashes MD5 for essentially every scanned file.Reproduced on a FIPS-enabled Ubuntu 22.04 / OpenSSL 3.0.2 host (ClamAV 1.5.3), during a recursive scan:
(SHA-256 succeeds — it is FIPS-approved; MD5 is the one that fails.)
Stock vs. patched, exercising the exact
cl_hash_init(freshOSSL_LIB_CTXper hash) vs. shared-context code paths against a real FIPS OpenSSL 3.0.2 host — 100k MD5 hashes, each returning NULL under FIPS:OSSL_LIB_CTXper hash)The control confirms it is specifically the NULL-fetch path that leaks: when the fetch succeeds (SHA-256, or any algorithm on a non-FIPS host), even the stock per-hash-context churn stays bounded.
With a single shared context there is no per-hash context to leak. On such a host MD5 remains unavailable, but the fetch now fails cleanly and bounded (MD5-based signatures simply don't match) instead of growing without bound — graceful degradation rather than OOM.
References
EVP_MD_fetch/EVP_DigestInit(3)— a fetchedEVP_MDis reference-counted and intended to be fetched once and reused.migration_guide(7)— explicit fetching and its performance characteristics when moving from 1.1.1 to 3.0.fips_module(7)— nondefault library context — the approach Fix issue using non-FIPS algorithsm in some FIPS environments #1589 used to reach non-FIPS digests, preserved here (created once instead of per hash).OSSL_LIB_CTX(3)— the provider/method cache lives in the library context, which is why allocating a fresh one per hash defeats caching.