fix: harden peer-supplied atoi parsing#1095
Open
MarkAtwood wants to merge 1 commit into
Open
Conversation
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.
MarkAtwood
assigned
MarkAtwood and
ejohnstown and unassigned
wolfSSL-Bot and
MarkAtwood
aidangarske
reviewed
Jul 10, 2026
aidangarske
left a comment
aidangarske
left a comment
Member
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: COMMENT
Findings: 3 total — 3 posted, 0 skipped
Posted findings
- [Medium] VT100 parser still accepts trailing junk after digits —
src/wolfterm.c:326-330 - [Medium] SCP numeric parser lacks regression tests for malformed and overflow fields —
src/wolfscp.c:1075-1131 - [Medium] VT100 argument bounds behavior lacks targeted Windows tests —
src/wolfterm.c:323-360
Review generated by Skoll.
Replace atoi() and strtoull() on peer-controlled numeric fields with bounded, error-reporting parsers. atoi() has undefined behavior on overflow and cannot signal malformed input. strtoull() is C99, so it is unavailable on some ports, and it accepts a leading sign and leading whitespace that the SCP header does not permit. - #2881 src/wolfscp.c: add ScpParseUInt64 helper; parse SCP header file size / mtime / atime with digit validation and overflow rejection against a caller-supplied max, replacing the 3 strtoull call sites from de23a69. Also rejects "+1", " 1" and "\t1" on all three fields, and "-1" on the unbounded scpMTime and scpATime, which had taken the wrapped 0xFFFFFFFFFFFFFFFF. The helper takes the field length, dropping the temporary '\n' written over the separating space at each call site, and with it the <stdint.h> include de23a69 had added for UINT32_MAX. - #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). A param with bytes left over after its digits, such as "12x", clamps to 0 rather than parsing as a short value. strtol is C89 and needs no replacement. Inside USE_WINDOWS_API, so not built on Linux or macOS. Also drop the unused maxIdx from getArgs(). Extend test_ScpGetFileSize and test_ScpGetTimestamp in tests/unit.c for the fields the new parser rejects but strtoull() accepted: a signed or whitespace-padded field, an empty field, a field holding a NUL, and the 64-bit maximum and one past it on the timestamps. Drop the strtoull() and ERANGE references from those test comments. Issue: F-2881, F-2882
ejohnstown
force-pushed
the
fix/wolfssh-atoi_peer_data
branch
from
July 24, 2026 23:52
5405bcb to
46a4988
Compare
ejohnstown
approved these changes
Jul 24, 2026
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.