Skip to content

F 569 : Fix stack buffer overflow in encryption setup#212

Open
miyazakh wants to merge 4 commits intowolfSSL:mainfrom
miyazakh:f-569_stackbuffer_overflow
Open

F 569 : Fix stack buffer overflow in encryption setup#212
miyazakh wants to merge 4 commits intowolfSSL:mainfrom
miyazakh:f-569_stackbuffer_overflow

Conversation

@miyazakh
Copy link
Copy Markdown
Contributor

@miyazakh miyazakh commented Mar 26, 2026

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

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

@miyazakh miyazakh self-assigned this Mar 26, 2026
Copilot AI review requested due to automatic review settings March 26, 2026 22:06
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 AI review requested due to automatic review settings March 27, 2026 00:12
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@miyazakh miyazakh force-pushed the f-569_stackbuffer_overflow branch from 9b1c8c5 to 2a009f1 Compare April 7, 2026 21:02
@miyazakh miyazakh marked this pull request as ready for review April 7, 2026 21:10
Copilot AI review requested due to automatic review settings April 7, 2026 21:10
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

wolfCLU_LogError("input too long, please try again");
continue;
}
inName[strcspn(inName, "\n")] = '\0';
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stripping only \"\\n\" leaves a trailing \"\\r\" on CRLF inputs (common on Windows), which can make filenames fail to open. Use a delimiter set like \"\\r\\n\" here (and similarly for outNameEnc/outNameDec).

Suggested change
inName[strcspn(inName, "\n")] = '\0';
inName[strcspn(inName, "\r\n")] = '\0';

Copilot uses AI. Check for mistakes.
}
inName[strcspn(inName, "\n")] = '\0';
/* Do not accept an empty string as valid input */
if (inName[0] == '\0') {
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty input is silently ignored and the loop re-prompts without any feedback. Consider logging a short message (or re-printing the prompt with a clear hint) so interactive users understand why it’s asking again. Same applies to the empty-string checks for outNameEnc/outNameDec.

Suggested change
if (inName[0] == '\0') {
if (inName[0] == '\0') {
wolfCLU_LogError("empty input is not allowed, please try again");

Copilot uses AI. Check for mistakes.

# Regression tests for stack buffer overflow fix (scanf -> fgets)

# Test: -in not provided, filename supplied via stdin to exercise the inName Path
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests assume the output files don’t already exist. If ./wolfssl enc prompts/refuses on overwrite (or if a previous run left artifacts), the test can hang or behave inconsistently. Consider rm -f of the target outputs before invoking ./wolfssl enc for each case to make runs idempotent.

Suggested change
# Test: -in not provided, filename supplied via stdin to exercise the inName Path
# Test: -in not provided, filename supplied via stdin to exercise the inName Path
rm -f test-stdin-in.enc test-stdin-in.dec

Copilot uses AI. Check for mistakes.
echo "Failed: enc with stdin input (no -in flag)"
exit 99
fi
./wolfssl enc -d -aes-128-cbc -in test-stdin-in.enc -out test-stdin-in.dec -k "testpass" > /dev/null 2>&1
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests assume the output files don’t already exist. If ./wolfssl enc prompts/refuses on overwrite (or if a previous run left artifacts), the test can hang or behave inconsistently. Consider rm -f of the target outputs before invoking ./wolfssl enc for each case to make runs idempotent.

Copilot uses AI. Check for mistakes.
echo "Failed: stdin enc/dec roundtrip mismatch"
exit 99
fi
rm -f test-stdin-in.enc test-stdin-in.dec
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests assume the output files don’t already exist. If ./wolfssl enc prompts/refuses on overwrite (or if a previous run left artifacts), the test can hang or behave inconsistently. Consider rm -f of the target outputs before invoking ./wolfssl enc for each case to make runs idempotent.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants