Fix multiple reported issues

[kareem-wolfssl]

No description provided.

[Copilot]

Pull request overview

This PR hardens several parsing and buffer-handling paths across wolfSSH, addressing correctness and robustness issues in SCP parsing, path normalization, string utilities, KEX message parsing, certificate manager wrappers, and the SSH agent implementation.

Changes:

• Harden SCP parsing by replacing atoi() with strtoull() and adding stricter validation for file sizes and timestamps.
• Add/adjust bounds checks to prevent arithmetic underflow/overflow in path normalization, KEX parsing, and string concatenation helpers.
• Improve agent handling by avoiding accidental secret disclosure and fixing key identity comparison logic.

Reviewed changes

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

Show a summary per file

File	Description
src/wolfscp.c	Tightens SCP numeric parsing (file size + timestamps) with strtoull() and validation.
src/ssh.c	Strengthens wolfSSH_RealPath() bounds checking for segment copies.
src/port.c	Fixes wstrncat() free-space calculation to avoid size underflow scenarios.
src/internal.c	Adds bounds checks when skipping language name-lists during KEXINIT parsing.
src/certman.c	Initializes return value and adds basic NULL-guarding for root CA buffer loading.
src/agent.c	Avoids logging secrets by default, makes lock/unlock return failures on alloc, and fixes key-blob matching logic.

Comments suppressed due to low confidence (1)

src/ssh.c:3842

• The new bounds check doesn’t account for the optional '/' that may be appended when curSz != 1, and the WSTRNCAT() return values are ignored. This can still allow buffer-space miscalculation where the '/' append succeeds but the segment append fails (WSTRNCAT returns NULL), yet the function continues and returns WS_SUCCESS with a truncated path.

            if (segSz > outSz || curSz >= outSz - segSz) {
                return WS_INVALID_PATH_E;
            }

            if (curSz != 1) {

---

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

[lihnucs]

Hi Kareem,

I have reviewed all 7 commits in this PR and confirmed every fix is correct:

• 1c61fe4 (HIGH-01 / ssh.c): segSz > outSz short-circuit correctly prevents the unsigned underflow before the subtraction.
• 3ae9836 (HIGH-02 / agent.c): Size equality check id->keyBlobSz != keyBlobSz before WMEMCMP closes both the OOB read and the prefix-match auth bypass.
• 8ad669a (HIGH-03 / agent.c): All three defects addressed -- SHOW_SECRETS guard for the log, ForceZero before free, and exact-size heap allocation replacing the 32-byte truncating stack buffer.
• fd36f9a (HIGH-05 / wolfscp.c): strtoull + three-part validation (errno, endptr, > UINT32_MAX) correctly rejects -1 and all out-of-range values.
• 712fe71 (MED-11 / internal.c): skipSz <= len - begin guard on both language-list blocks prevents the begin wraparound.
• 9407efa (HIGH-04 / certman.c): NULL guard added before cm->cm dereference, matching the pattern in every other public CERTMAN API.
• 78390be (MED-01 / port.c): The strnlen-style bounded scan is more robust than a plain strlen -- handles the case where s1 already lacks a null terminator within n bytes.

All 131 CI checks are now passing. The fixes are well-constructed and I have no change requests.

One request before merge: could you please add a Reported-by trailer to each commit (or to the PR description) crediting the original reporter? The full preferred format is:

Reported-by: Asif Nadaf <postasif@protonmail.com>
This is the standard git trailer format used in most open-source security fixes and helps ensure the credit is preserved in the git log permanently. I notice a similar commit (989be64 in PR #1080) credited the reporter by name
only -- if that commit is amendable before merge, including the email there as well would be appreciated.

Thank you for the fast turnaround on all nine reports -- this was a thorough and well-executed response.

[kareem-wolfssl]

Thanks for the review and feedback @lihnucs. All of the initial fix commits currently have "Reported-by: Asif Nadaf <postasif@protonmail.com>". This is included on a new line, you will need to expand the commit message to see it. I also amended #1080 to include this in the commit message before it was merged.

[aidangarske]

Skoll Multi-Scan Review

Modes: review + review-securityOverall recommendation: REQUEST_CHANGES
Findings: 2 total — 2 posted, 0 skipped
2 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [High] [review+review-security] wstrncat change breaks RealPath callers that pass remaining capacity — src/port.c:845-850
• [Medium] [review] New SCP numeric parsing lacks regression coverage — src/wolfscp.c:1106-1303

Review generated by Skoll

[aidangarske]

🔴 [High] wstrncat change breaks RealPath callers that pass remaining capacity · Logic

The PR changes wstrncat() to return NULL when s1 is not NUL-terminated within the first n bytes. That matches the helper's documented contract and is safer for callers that pass the full destination size and check the return value, but existing changed-file callers do neither. wolfSSH_RealPath() calls WSTRNCAT(out, ..., outSz - curSz) (src/ssh.c:3843 and src/ssh.c:3846) with remaining capacity instead of the full buffer size while out already holds curSz bytes, and ignores the return. After this PR, for a remote SFTP/SCP path with a long first segment, once curSz >= outSz - curSz the existing terminator is beyond the shortened n, so later appends return NULL even when the final path still fits; wolfSSH_RealPath() still increments curSz and returns WS_SUCCESS with a truncated path (separator/segment omitted). The same new failure mode affects other changed-file SCP path construction such as WSTRNCAT((char*)basePath, "/", sizeof("/")) at src/wolfscp.c:2466. This reaches public wolfSSH_RealPath callers used by SFTP/SCP path handling. Severity note: the review mode rated this High (BLOCK) as a silent path-truncation bug; review-security rated it Medium (CWE-252, unchecked return value); the stricter High rating is kept here.

Fix: Keep the bounded scan, but update affected callers to pass the full destination buffer size and check WSTRNCAT() return values. In wolfSSH_RealPath(), include the optional slash in the bounds check and use explicit length-based appends or call WSTRNCAT(out, ..., outSz) after the existing bounds checks, returning WS_INVALID_PATH_E on NULL:

if (curSz + ((curSz != 1) ? 1 : 0) + segSz >= outSz) {
return WS_INVALID_PATH_E;
}
if (curSz != 1) {
if (WSTRNCAT(out, "/", outSz) == NULL)
return WS_INVALID_PATH_E;
curSz++;
}
if (WSTRNCAT(out, seg, outSz) == NULL)
return WS_INVALID_PATH_E;

Fix the Nucleus SCP basePath append to use the actual basePath capacity. Add a regression test where a valid resolved path grows beyond half of the output buffer but still fits.

[aidangarske]

🟠 [Medium] New SCP numeric parsing lacks regression coverage · test

The PR replaces atoi with strtoull and adds overflow and malformed-field handling for SCP file sizes and timestamps, but the current tests only expose wolfSSH_TestScpGetFileMode; there are no targeted tests for these changed parser paths. This is boundary-heavy protocol parsing, so regressions around negative values, overflow, and partial parses would be easy to miss.

Fix: Add WOLFSSH_TEST_INTERNAL wrappers or ReceiveScpMessage-based tests for file sizes -1, UINT32_MAX, UINT32_MAX + 1, empty/non-numeric fields, timestamp overflow, and malformed timestamp fields.