F 1483 : Fix SHA-1 prefix match overwriting SHA-256/384/512 output by miyazakh · Pull Request #216 · wolfSSL/wolfCLU

miyazakh · 2026-04-02T04:01:34Z

Summary

XSTRNCMP(alg, "sha", 3) in wolfCLU_hash and wolfCLU_hashSetup
matched "sha256", "sha384", and "sha512", causing SHA-1 to overwrite
the correct digest and size
Fixed by adding XSTRLEN(alg) == 3 guard to the SHA-1 branch in
clu_hash.c and clu_hash_setup.c
Fixed algCheck loop in clu_hash_setup.c to use XSTRCMP for
exact matching instead of prefix matching

Depend on : #211 (Fixed)
Depend on : #219

Copilot

Pull request overview

Fixes an algorithm-selection bug in the CLI hash command where SHA-1 prefix matching could incorrectly override SHA-256/384/512 digest output, and tightens algorithm validation to require exact matches.

Changes:

Guard SHA-1 selection in wolfCLU_hash / wolfCLU_hashSetup with XSTRLEN(alg) == 3 so only "sha" maps to SHA-1.
Change hash algorithm validation to exact string matching (XSTRCMP) to avoid prefix-based acceptance.
Broaden OCSP interop test error-message matching; update clu_x509_sign.c hash-type switch version gating / case handling.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
`src/hash/clu_hash.c`	Prevents SHA-1 branch from matching `sha256`/`sha384`/`sha512` by requiring exact `"sha"`.
`src/hash/clu_hash_setup.c`	Requires exact algorithm match during argument validation and ensures SHA-1 size only applies to `"sha"`.
`tests/ocsp/ocsp-interop-test.sh`	Accepts additional common “file not found” phrasing in Test 6 logs.
`src/x509/clu_x509_sign.c`	Adjusts conditional compilation for unsupported hash-type cases in certificate signing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/x509/clu_x509_sign.c

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-07T21:20:36Z

src/hash/clu_hash_setup.c


 #ifndef NO_SHA
-    if (XSTRNCMP(alg, "sha", 3) == 0)
+    if ((XSTRNCMP(alg, "sha", 3) == 0) && (XSTRLEN(alg) == 3))


Now that exact matching is the goal, consider replacing the XSTRNCMP(..., 3) && XSTRLEN(alg) == 3 pattern with a single exact comparison (e.g., XSTRCMP(alg, \"sha\") == 0). It’s simpler, more readable, and avoids a redundant length check.

Suggested change

if ((XSTRNCMP(alg, "sha", 3) == 0) && (XSTRLEN(alg) == 3))

if (XSTRCMP(alg, "sha") == 0)

Copilot · 2026-04-07T21:20:37Z

src/hash/clu_hash.c

+    if (ret == WOLFCLU_SUCCESS && XSTRNCMP(alg, "sha", 3) == 0
+            && XSTRLEN(alg) == 3) {


Same as in setup: since this branch should only run for the literal algorithm name, prefer an exact compare (e.g., XSTRCMP(alg, \"sha\") == 0) over XSTRNCMP + length guard. This reduces complexity and makes the intent unambiguous.

Suggested change

if (ret == WOLFCLU_SUCCESS && XSTRNCMP(alg, "sha", 3) == 0

&& XSTRLEN(alg) == 3) {

if (ret == WOLFCLU_SUCCESS && XSTRCMP(alg, "sha") == 0) {

src/hash/clu_hash_setup.c

dgarske

🐺 Skoll Code Review

Overall recommendation: APPROVE
Findings: 2 total — 2 posted, 0 skipped

Posted findings

[Medium] Remaining XSTRNCMP calls could be made consistent with the fix — src/hash/clu_hash.c:104
[Low] The fix is correct but the if-chain ordering is fragile — src/hash/clu_hash.c:108-127

Review generated by Skoll via openclaw

dgarske · 2026-04-10T17:01:50Z

src/hash/clu_hash.c

@@ -121,7 +121,7 @@ int wolfCLU_hash(WOLFSSL_BIO* bioIn, WOLFSSL_BIO* bioOut, const char* alg,
    }


🟡 [Medium] Remaining XSTRNCMP calls could be made consistent with the fix
💡 SUGGEST convention

The PR correctly changes XSTRNCMP(alg, "sha", 3) to XSTRCMP(alg, "sha") to prevent the SHA-1 branch from matching sha256/sha384/sha512. However, other algorithm checks in both wolfCLU_hash() and wolfCLU_hashSetup() still use XSTRNCMP (e.g., XSTRNCMP(alg, "md5", 3) at clu_hash.c:104, XSTRNCMP(alg, "md5", 3) at clu_hash_setup.c:138). While these are functionally safe today because none of those prefixes collide with other algorithm names, wolfCLU_hash() is a public function that accepts alg from external callers (e.g., clu_alg_hash.c). Switching all comparisons to XSTRCMP would be more defensive and consistent with the spirit of this fix — preventing future prefix-collision bugs if new algorithms are added.

Suggestion:

Suggested change

}

#ifndef NO_MD5

if (ret == WOLFCLU_SUCCESS && XSTRCMP(alg, "md5") == 0) {

ret = wc_Md5Hash(input, inputSz, output);

}

#endif

/* And similarly for sha256, sha384, sha512, blake2b, base64enc, base64dec

in both clu_hash.c and clu_hash_setup.c */

dgarske · 2026-04-10T17:01:50Z

src/hash/clu_hash.c

@@ -121,7 +121,7 @@ int wolfCLU_hash(WOLFSSL_BIO* bioIn, WOLFSSL_BIO* bioOut, const char* alg,
    }


🔵 [Low] The fix is correct but the if-chain ordering is fragile
🔧 NIT style

In wolfCLU_hash(), all the hash algorithm branches are independent if statements (not else if). With the old XSTRNCMP(alg, "sha", 3) code, if alg was "sha256", both the sha256 branch (line 109) and the sha branch (line 124) would fire because ret was still WOLFCLU_SUCCESS (wc_Sha256Hash returns 0 on success). The SHA-1 branch would overwrite the SHA-256 output with a smaller (20-byte) digest. The fix to use XSTRCMP is correct. As a minor readability note, using else if for this chain would make it obvious that only one branch should ever execute, preventing similar issues in the future. This is not blocking since the fix as-is is correct.

Suggestion:

Suggested change

}

/* The #ifdef guards make else-if impossible across preprocessor blocks,

so XSTRCMP is the right fix here. No change needed. */

Copilot AI review requested due to automatic review settings April 2, 2026 04:01

miyazakh self-assigned this Apr 2, 2026

Copilot started reviewing on behalf of miyazakh April 2, 2026 04:02 View session

Copilot AI reviewed Apr 2, 2026

View reviewed changes

src/x509/clu_x509_sign.c Show resolved Hide resolved

miyazakh force-pushed the f-1483_sha1prefix branch from e43bb6a to 1a11f72 Compare April 7, 2026 21:13

miyazakh marked this pull request as ready for review April 7, 2026 21:13

Copilot AI review requested due to automatic review settings April 7, 2026 21:13

Copilot AI reviewed Apr 7, 2026

View reviewed changes

Copilot started reviewing on behalf of miyazakh April 7, 2026 22:26 View session

miyazakh added 2 commits April 9, 2026 10:12

fix f-1483 sha1 prefix substring matching

de71361

Addressed Copilot review comments

e1ea92c

miyazakh force-pushed the f-1483_sha1prefix branch from 1a11f72 to e1ea92c Compare April 9, 2026 01:22

miyazakh assigned wolfSSL-Bot and unassigned miyazakh Apr 9, 2026

dgarske reviewed Apr 10, 2026

View reviewed changes

dgarske assigned miyazakh and unassigned wolfSSL-Bot Apr 10, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

F 1483 : Fix SHA-1 prefix match overwriting SHA-256/384/512 output#216

F 1483 : Fix SHA-1 prefix match overwriting SHA-256/384/512 output#216
miyazakh wants to merge 2 commits intowolfSSL:mainfrom
miyazakh:f-1483_sha1prefix

miyazakh commented Apr 2, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 7, 2026

Uh oh!

miyazakh Apr 9, 2026

Uh oh!

Copilot AI Apr 7, 2026

Uh oh!

miyazakh Apr 9, 2026

Uh oh!

Uh oh!

dgarske left a comment

Uh oh!

dgarske Apr 10, 2026

Uh oh!

dgarske Apr 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	if ((XSTRNCMP(alg, "sha", 3) == 0) && (XSTRLEN(alg) == 3))
	if (XSTRCMP(alg, "sha") == 0)

		if (ret == WOLFCLU_SUCCESS && XSTRNCMP(alg, "sha", 3) == 0
		&& XSTRLEN(alg) == 3) {

	if (ret == WOLFCLU_SUCCESS && XSTRNCMP(alg, "sha", 3) == 0
	&& XSTRLEN(alg) == 3) {
	if (ret == WOLFCLU_SUCCESS && XSTRCMP(alg, "sha") == 0) {

		@@ -121,7 +121,7 @@ int wolfCLU_hash(WOLFSSL_BIO* bioIn, WOLFSSL_BIO* bioOut, const char* alg,
		}

-    }
+#ifndef NO_MD5
+    if (ret == WOLFCLU_SUCCESS && XSTRCMP(alg, "md5") == 0) {
+        ret = wc_Md5Hash(input, inputSz, output);
+    }
+#endif
+/* And similarly for sha256, sha384, sha512, blake2b, base64enc, base64dec
+   in both clu_hash.c and clu_hash_setup.c */

	}
	/* The #ifdef guards make else-if impossible across preprocessor blocks,
	so XSTRCMP is the right fix here. No change needed. */

Conversation

miyazakh commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

miyazakh Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

miyazakh Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dgarske left a comment

Choose a reason for hiding this comment

🐺 Skoll Code Review

Posted findings

Uh oh!

dgarske Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

dgarske Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

miyazakh commented Apr 2, 2026 •

edited

Loading