fix: harden peer-supplied atoi parsing#1095
Open
MarkAtwood wants to merge 1 commit into
Open
Conversation
Replace atoi() on peer-controlled numeric fields with bounded, error-reporting parsers. atoi() has undefined behavior on overflow and cannot signal malformed input. - #2881 src/wolfscp.c: add ScpParseUInt64 helper; parse SCP header file size / mtime / atime with digit validation and overflow rejection (returns WS_SCP_BAD_MSG_E), replacing 3 atoi call sites. - #2882 src/wolfterm.c: add parseArg helper using strtol; clamp invalid/out-of-range VT100 params to 0, replacing 2 atoi call sites (blocks peer "-1" wrapping to 0xFFFFFFFF).
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens parsing of peer-controlled numeric fields by removing atoi() usage and replacing it with bounded, error-reporting parsing helpers to prevent overflow UB and malformed-input acceptance in SCP and Windows console/VT100 handling.
Changes:
src/wolfscp.c: IntroducesScpParseUInt64()and replaces SCP header numeric parsing for file size and timestamps.src/wolfterm.c: IntroducesparseArg()(strtol-based) and replacesatoi()when parsing CSI/VT100 parameter arguments.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/wolfterm.c | Replaces atoi() with a bounded strtol-based helper for VT100/CSI argument parsing under USE_WINDOWS_API. |
| src/wolfscp.c | Adds a digit-validating, overflow-checked unsigned parser and uses it for SCP size/mtime/atime parsing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| char* endp; | ||
| long v = strtol((const char*)s, &endp, 10); | ||
| if (endp == (const char*)s || v < 0 || v > 65535L) { |
Comment on lines
+1085
to
+1086
| if (val > (max - d) / 10) | ||
| return WS_SCP_BAD_MSG_E; |
Comment on lines
+1071
to
+1074
| /* Parse a base-10 unsigned integer from a newline- or NUL-terminated | ||
| * string (the SCP header field). Rejects empty input, any non-digit | ||
| * character, and values that overflow max. Returns WS_SUCCESS on | ||
| * success, WS_SCP_BAD_MSG_E on malformed/out-of-range input. */ |
Comment on lines
+1128
to
+1132
| word64 scpFileSz64 = 0; | ||
| ret = ScpParseUInt64((char *)(buf + idx), 0xFFFFFFFFUL, | ||
| &scpFileSz64); | ||
| ssh->scpFileSz = (word32)scpFileSz64; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces
atoi()on peer-controlled numeric fields with bounded, error-reporting parsers.atoi()has undefined behavior on overflow (C99 7.20.1) and cannot signal malformed input.ScpParseUInt64helper and replace the 3atoicall sites parsing the SCP header file size (word32), mtime, and atime (word64). The helper validates every character is a digit, rejects empty input, and rejects values that overflow the field max, returningWS_SCP_BAD_MSG_E. Prevents a malicious peer from injecting garbage/overflowing sizes and timestamps.parseArghelper usingstrtoland replace the 2atoicall sites parsing VT100 console parameters. Clamps invalid or out-of-range params (outside 0..65535) to 0, blocking a peer-supplied-1from wrapping to0xFFFFFFFF. This code is inside the existingUSE_WINDOWS_APIblock.Build-verified:
./autogen.sh && ./configure --with-wolfssl=... --enable-all && make -j8completes clean (exit 0) with both fixes applied; wolfscp.c compiled and ran through the build. The wolfterm.c change is compiled only underUSE_WINDOWS_API, so it was source-analyzed on the Linux build.Reported by Fenrir static analysis.