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 Code Review

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

Posted findings

• [Medium] ML-DSA-only builds do not advertise publickey auth by default — src/internal.c:17875-17877
• [Medium] TPM host key API leaves ctx mutated when registration fails — src/ssh.c:540-543
• [Medium] New ML-DSA ASN.1 public-key path bypasses caller output buffer — src/ssh.c:1878-1892

Review generated by Skoll.

[aidangarske]

🟡 [Medium] ML-DSA-only builds do not advertise publickey auth by default
💡 SUGGEST bug

The PR adds ML-DSA public-key authentication paths and compiles DoUserAuthRequestPublicKey() when !defined(WOLFSSH_NO_MLDSA), but the default allowed-auth list still adds publickey only when RSA or ECDSA is enabled. In an ML-DSA-only build, a server can now process ML-DSA public-key auth, but a normal USERAUTH_FAILURE response will omit publickey, so clients may never attempt the newly supported method unless an application overrides userAuthTypesCb.

Recommendation: Update the compile-time gate to include the public-key algorithms now supported by this file, at least ML-DSA, and add a regression test for an ML-DSA-only default auth list.

[aidangarske]

🟡 [Medium] TPM host key API leaves ctx mutated when registration fails
💡 SUGGEST api

The new API stores ctx->tpmDev and ctx->tpmKey before checking whether wolfSSH_SetHostTpmKey() succeeds. If registration fails, for example because privateKeyCount is already at WOLFSSH_MAX_PVT_KEYS, the function returns an error but leaves the context partially configured. A later call with a different TPM key then hits the ctx->tpmKey != NULL && ctx->tpmKey != key guard and fails even though the original setup did not succeed.

Recommendation: Commit TPM device/key pointers only after the host-key slot registration succeeds, or explicitly roll them back on failure.

[aidangarske]

🟡 [Medium] New ML-DSA ASN.1 public-key path bypasses caller output buffer
💡 SUGGEST api

The ML-DSA public-key branch always allocates a new output buffer and assigns it to *out. That means a caller-provided buffer is ignored, undersized caller buffers do not get WS_BUFFER_E, and the original *out pointer can be overwritten. This is in newly added ML-DSA handling and should either follow the existing wolfSSH_ReadKey_buffer_ex() output-buffer contract or be explicitly documented and tested as allocate-only.

Recommendation: Compute the required size first, use *out when it is non-NULL and large enough, return WS_BUFFER_E when it is too small, and allocate only when *out == NULL.