F 569 : Fix stack buffer overflow in encryption setup

[miyazakh]

Fix stack buffer overflow via unbounded scanf in encryption setup
Add test coverage

Depends on: #211 (Merged)
Depends on: #219

[Copilot]

Pull request overview

Fixes a stack buffer overflow in the encryption CLI setup by replacing unbounded scanf("%s", ...) reads with bounded fgets() reads, and adds regression tests to validate stdin-driven input/output filename paths.

Changes:

• Replace unbounded scanf with bounded fgets for -in / -out prompts in encryption setup.
• Add regression tests that supply input/output filenames via stdin (including a non-EVP path probe).
• Expand an OCSP interop test’s expected error-message matching.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File	Description
tests/ocsp/ocsp-interop-test.sh	Broadens grep pattern to recognize more “missing file” error variants.
tests/encrypt/enc-test.sh	Adds stdin-based regression tests covering the new fgets() input handling.
src/x509/clu_x509_sign.c	Minor formatting cleanup + extends hash-type switch cases under a version gate.
src/crypto/clu_crypto_setup.c	Replaces unsafe scanf("%s") reads with fgets() and newline trimming.

---

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

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.

---

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

[Copilot]

Pull request overview

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

---

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

[dgarske]

🐺 Skoll Code Review

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

Posted findings

• [Medium] Significant code duplication across three fgets+validation blocks — src/crypto/clu_crypto_setup.c:346-490
• [Medium] No test coverage for the 'input too long' fgets truncation path — tests/encrypt/enc-test.sh:188-275
• [Low] Minor: in = inName assignment after the while loop is now redundant — src/crypto/clu_crypto_setup.c:376

Review generated by Skoll via openclaw

[dgarske]

🟡 [Medium] Significant code duplication across three fgets+validation blocks
💡 SUGGEST convention

The fgets-based input-with-validation pattern (fgets → check NULL → check truncation/flush → strip newline → reject empty → assign) is repeated nearly identically three times: once for inName (line 350-374), once for outNameEnc (line 424-446), and once for outNameDec (line 467-489). Each block is ~20 lines of identical logic differing only in the target buffer name. This triplication makes maintenance harder — any future change (e.g. adding a max-retry count or adjusting the error message) must be made in three places.

Suggestion:

Suggested change

	while (ret == 0) {
	Extract a static helper function, e.g.:
	static int wolfCLU_readLine(char *buf, int bufSz, const char *prompt)
	{
	while (1) {
	WOLFCLU_LOG(WOLFCLU_L0, "%s", prompt);
	if (fgets(buf, bufSz, stdin) == NULL)
	return WOLFCLU_FATAL_ERROR;
	if (strchr(buf, '\n') == NULL) {
	int ch;
	do { ch = getchar(); } while (ch != '\n' && ch != EOF);
	wolfCLU_LogError("input too long, please try again");
	continue;
	}
	buf[strcspn(buf, "\r\n")] = '\0';
	if (buf[0] == '\0') {
	wolfCLU_LogError("empty input, please enter a file name");
	continue;
	}
	return WOLFCLU_SUCCESS;
	}
	}
	Then each call site becomes ~5 lines: call the helper, check for error, free+return on failure.

[dgarske]

🟡 [Medium] No test coverage for the 'input too long' fgets truncation path
💡 SUGGEST test

The new tests cover the happy path (valid stdin input), the EOF path (implicitly via pipe termination), and the empty-line rejection + re-prompt path. However, there is no test for the 'input too long' path that triggers the strchr(buf, '\n') == NULL branch and the getchar() flush loop. This is the core buffer-overflow-prevention logic that the PR title highlights. A test could pipe a >256-char filename followed by a valid filename to verify the re-prompt works.

Suggestion:

Suggested change

	# Regression tests for stack buffer overflow fix (scanf -> fgets)
	Add a test such as:
	# Test: input too long is rejected, then valid filename accepted
	LONG_NAME=$(python3 -c "print('A'*300)")
	printf "%s\ncerts/crl.der\n" "$LONG_NAME" | ./wolfssl enc -aes-128-cbc -out test-toolong.enc -k "testpass" > /dev/null 2>&1
	if [ $? != 0 ]; then
	echo "Failed: enc should re-prompt after too-long input"
	exit 99
	fi
	rm -f test-toolong.enc

[dgarske]

🔵 [Low] Minor: in = inName assignment after the while loop is now redundant
🔧 NIT style

The variable in is initialized to inName at declaration (line 66: char* in = inName;). After the while loop on line 376, in = inName is set again, but this is redundant since in already points to inName and hasn't been changed in this code path (the inCheck == 0 guard ensures the in = optarg assignment in the option parsing didn't fire). This is pre-existing from the old code and not introduced by this PR, but since the surrounding code was rewritten it's worth noting.

Recommendation: Can be left as-is for clarity, or removed since in is already inName. Very minor.

[Copilot]

Pull request overview

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

---

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