Fix: Test memory leak and Failing test when md5 and sha are enabled

[aidankeefe2022]

Description

Fixed a memory leak where if _ret is already failing, the rngInit function will not run, causing rngFree to free garbage data. The fix was to have rngInit always run to match with rngFree by swapping ExpectInt to DoExpectInt. Fixed failing test where WC_SIG_MIN_HASH_TYPE was not properly set for --enable-sha and --enable-md5, so we skip under those conditions.

Found while testing wolfCLU

Testing

Code no longer leaks, and code no longer fails with the wrong code for the test when SHA and MD5 are enabled

[github-actions]

MemBrowse Memory Report

gcc-arm-cortex-m4-dtls13

• FLASH: .text +64 B (+0.0%, 181,092 B / 1,048,576 B, total: 17% used)

gcc-arm-cortex-m4-openssl-compat

• FLASH: .text +64 B (+0.0%, 771,412 B / 1,048,576 B, total: 74% used)

gcc-arm-cortex-m4-pq

• FLASH: .text +64 B (+0.0%, 280,000 B / 1,048,576 B, total: 27% used)
  No memory changes detected for:
• gcc-arm-cortex-m0plus
• gcc-arm-cortex-m3
• gcc-arm-cortex-m4
• gcc-arm-cortex-m4-baremetal
• gcc-arm-cortex-m4-crypto-only
• gcc-arm-cortex-m4-min-ecc
• gcc-arm-cortex-m4-pkcs7
• gcc-arm-cortex-m4-rsa-only
• gcc-arm-cortex-m4-sp-math
• gcc-arm-cortex-m4-tls12
• gcc-arm-cortex-m4-tls13
• gcc-arm-cortex-m7
• gcc-arm-cortex-m7-pq
• gcc-arm-cortex-m7-tls13
• linuxkm-pie
• linuxkm-standard
• stm32-sim-stm32h753

[Frauschi]

🐺 Skoll Code Review

Overall recommendation: APPROVE
Findings: 6 total — 2 posted, 4 skipped

Posted findings

• [Medium] Weak-hash guard is skipped even when WC_SIG_MIN_HASH_TYPE raises the floor, and silently self-destructs if the default ever moves into a header — tests/api/test_signature.c:338
• [Medium] Lowered-floor branch of wc_SignatureCheckHashStrength() has no coverage at all — tests/api/test_signature.c:338

Skipped findings

• [Low] PR description attributes the failure to --enable-sha/--enable-md5, but the trigger is --enable-wolfclu
• [Low] Two different init/free-pairing conventions now coexist in this file
• [Low] Weak-hash assertions skipped for any WC_SIG_MIN_HASH_TYPE override, including stronger floors
• [Info] Guard depends on WC_SIG_MIN_HASH_TYPE's default living in signature.c, not a public header

Review generated by Skoll via Claude/Codex

[Frauschi]

🟡 [Medium] Weak-hash guard is skipped even when WC_SIG_MIN_HASH_TYPE raises the floor, and silently self-destructs if the default ever moves into a header
💡 SUGGEST test

The new guard #if !defined(NO_SHA) && !defined(WC_SIG_MIN_HASH_TYPE) drops the SHA-1-rejection assertion whenever the macro is defined at all, not just when it is defined to something weaker than SHA-256. If a downstream config sets WC_SIG_MIN_HASH_TYPE=WC_HASH_TYPE_SHA384 (a stricter floor), wc_SignatureCheckHashStrength() still rejects SHA-1 with BAD_FUNC_ARG, but the test is skipped anyway — coverage is lost for no reason.

There is a second, more fragile coupling. The default #ifndef WC_SIG_MIN_HASH_TYPE / #define WC_SIG_MIN_HASH_TYPE WC_HASH_TYPE_SHA256 / #endif currently lives in wolfcrypt/src/signature.c:62-64, not in wolfssl/wolfcrypt/signature.h. The test only compiles the assertion because that default is invisible to this translation unit. The moment anyone moves that default into signature.h (a natural refactor — signature.h is included at line 31 of this file, before the guard), WC_SIG_MIN_HASH_TYPE becomes defined in every build and both assertion blocks become permanently dead code, with no compile error and no test failure to signal it.

A size comparison against the configured floor keeps coverage in every config and removes the dependence on where the default happens to be defined.

Suggestion:

Suggested change

	#if !defined(NO_SHA) && !defined(WC_SIG_MIN_HASH_TYPE)
	/* Near the top of the file, mirroring wolfcrypt/src/signature.c: */
	#ifndef WC_SIG_MIN_HASH_TYPE
	#define TEST_SIG_MIN_HASH_TYPE WC_HASH_TYPE_SHA256
	#else
	#define TEST_SIG_MIN_HASH_TYPE WC_SIG_MIN_HASH_TYPE
	#endif
	/* At the call site: */
	#ifndef NO_SHA
	/* Hash weaker than WC_SIG_MIN_HASH_TYPE is rejected by
	* wc_SignatureCheckHashStrength(). wolfCLU with MD5 compiled in
	* lowers the floor to MD5, which lets SHA-1 through. */
	if (wc_HashGetDigestSize(TEST_SIG_MIN_HASH_TYPE) >
	WC_SHA_DIGEST_SIZE) {
	ExpectIntEQ(wc_SignatureVerify(WC_HASH_TYPE_SHA, sig_type,
	data, data_len, sig, sig_len, &ecc, key_len),
	WC_NO_ERR_TRACE(BAD_FUNC_ARG));
	}
	#endif

[aidankeefe2022]

merged the two issues into one fix

[Frauschi]

🟡 [Medium] Lowered-floor branch of wc_SignatureCheckHashStrength() has no coverage at all
💡 SUGGEST test

Both new guards are bare #if ... #endif with no #else. In exactly the configuration that motivated this PR (wolfCLU with MD5, which sets -DWC_SIG_MIN_HASH_TYPE=WC_HASH_TYPE_MD5 at configure.ac:12052), the test now asserts nothing about hash-strength enforcement — it does not verify that SHA-1 is accepted under the lowered floor, only that it is no longer checked.

That matters because this file is explicitly a decision-coverage suite (see the header comment at lines 209-227 about independence pairs). Silently skipping the only assertion that touches wc_SignatureCheckHashStrength() means a regression that made the floor unconditionally SHA-256 — breaking wolfCLU's MD5 support, which is precisely the behaviour this override exists to preserve — would not be caught by any test.

Suggestion:

Suggested change

	#if !defined(NO_SHA) && !defined(WC_SIG_MIN_HASH_TYPE)
	#if !defined(NO_SHA) && !defined(WC_SIG_MIN_HASH_TYPE)
	ExpectIntEQ(wc_SignatureVerify(WC_HASH_TYPE_SHA, sig_type,
	data, data_len, sig, sig_len, &ecc, key_len),
	WC_NO_ERR_TRACE(BAD_FUNC_ARG));
	#elif !defined(NO_SHA)
	/* Floor lowered (e.g. wolfCLU with MD5): SHA-1 must get past the
	* strength check and fail on the signature itself instead. */
	ExpectIntNE(wc_SignatureVerify(WC_HASH_TYPE_SHA, sig_type,
	data, data_len, sig, sig_len, &ecc, key_len),
	WC_NO_ERR_TRACE(BAD_FUNC_ARG));
	#endif

[aidankeefe2022]

merged the two issues into one fix

[aidankeefe2022]

Retest this please