SSHD/Echoserver: Fix memory leaks and public-key lookup

[stenslae]

Fixed: Memory leaks, multiple public-key lookups per user in the echoserver, and authentication privilege errors.

• internal.c: Readability refactor for RSA/ECC keys.
• auth.c: Freed promptData.prompts and promptData.promptLengths on allocation failure pathways.
• echoserver.c: Fixed a copy-paste error and added cleanups on allocations failure.
• Key Hash Lookup Correctness: Updated wsUserAuth in echoserver.c to traverse all
  registered public keys for a matching user name instead of immediately returning an invalid
  key error on the first hash mismatch.
• Stack Buffer Cleanup: Freed keyLoadBuf under WOLFSSH_SMALL_STACK configurations for all early exit paths inside echoserver_test.
• auth.c: Returns success immediately if privilege separation is explicitly disabled.
• auth.c: Limits changing target user/group identifiers to environments where privilege separation or sandboxing is active.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1071

Scan targets checked: wolfssh-bugs, wolfssh-src

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1071

Scan targets checked: wolfssh-bugs, wolfssh-src

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1071

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1071

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

[aidangarske]

🐺 Skoll Code Review

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

Posted findings

• [Medium] Privilege-separation permission behavior lacks regression tests — apps/wolfsshd/auth.c:1853-1961
• [Medium] Multiple public-key lookup fix has no regression coverage — examples/echoserver/echoserver.c:2677-2727
• [Low] New bare scope block violates local C convention — apps/wolfsshd/auth.c:1952-1974

Review generated by Skoll.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1071

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

[aidangarske]

Skoll Code Review

Scan type: review-securityOverall recommendation: APPROVE
Findings: 4 total — 4 posted, 0 skipped
4 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [Low] Mixed pubKeyFmtId/pubKeyId within a single if/else-if free chain is fragile — src/internal.c:14597-14637
• [Low] New privilege-separation-ON tests silently skip when no "sshd" system user exists — apps/wolfsshd/test/test_configuration.c:1679-1822
• [Low] New error paths free host private key material without zeroization — examples/echoserver/echoserver.c:3275-3323
• [Info] internal.c "memory leak fix" is a semantic no-op; no leak existed or was fixed — src/internal.c:14597-14619

Review generated by Skoll

[aidangarske]

🔵 [Low] Mixed pubKeyFmtId/pubKeyId within a single if/else-if free chain is fragile · Logic

Verified by reading the post-change chain at src/internal.c:14597-14637. The PR converted the RSA and ECDSA branches of the key-free chain to test pubKeyFmtId, but left the ED25519 and ML-DSA branches in the same if / else if chain testing pubKeyId. The chain is currently correct only because pubKeyFmtId == pubKeyId for every non-RSA_SHA2 id. This coupling is invisible at the call site: the free chain must mirror the key-init switch in SendKexGetSigningKey, which dispatches on pubKeyId (line 12838), so the free logic now keys off a different field than the init logic. If a future change adds any further pubKeyFmtId remap (for example collapsing ID_X509V3_MLDSA* to ID_MLDSA*, mirroring what is already done for RSA), the RSA/ECDSA branches would silently start matching different ids than the ED25519/ML-DSA branches, producing either a leaked key or a free of an uninitialized union member. This is a maintainability/latent-risk issue, not a live defect.

Fix: Use one field consistently across the whole chain. Since the union member is chosen by the init switch in SendKexGetSigningKey (which dispatches on pubKeyId), keying the free chain on pubKeyId throughout is the lower-risk option; otherwise convert the ED25519/ML-DSA branches to pubKeyFmtId as well and add a comment stating the free chain must mirror the init switch.

[stenslae]

All branches now consistently use pubKeyFmtId. Switching them to pubKeyId would make the RSA branch miss ID_RSA_SHA2_256/512.

[aidangarske]

🔵 [Low] New privilege-separation-ON tests silently skip when no "sshd" system user exists · Missing Tests

Verified: the guard at test_configuration.c:1687-1691 returns WS_SUCCESS when getpwnam("sshd") is NULL, and the auth.c diff confirms SetDefaultUserID only consults getpwnam() on the privilege-separation-ON path (the PRIV_OFF path now short-circuits to getgid()/getuid()). Three of the six new tests — the only ones that exercise the security-relevant branch where privilege separation is ON and setegid/seteuid are actually invoked — bail out with return WS_SUCCESS when getpwnam("sshd") returns NULL, because they cannot construct an auth object without that account. Many CI images and minimal containers have no sshd system account, so in those environments all three tests report PASS while asserting nothing. The result is that the assertions guarding the changed code (that Raise calls setegid/seteuid with the saved uid/gid, that a setegid failure short-circuits seteuid via the new ret == WS_SUCCESS && guard, and that a seteuid failure is reported) may never actually execute in CI, while the suite still shows green. The tests do emit a Log() skip line, so this is not fully silent, but the pass/fail signal does not distinguish "verified" from "not run". Only test_AuthRaisePermissions_offSkipsSyscalls and test_AuthCreateUser_privSepOff (the priv-sep-OFF paths) run unconditionally.

Fix: Decouple these tests from the host's user database so they always run: either add a wsshd_getpwnam_cb stub (matching the existing wsshd_setegid_cb/wsshd_seteuid_cb pattern this PR already adds to auth.c/auth.h) so SetDefaultUserID can be driven with a synthetic passwd entry, or expose a WOLFSSHD_UNIT_TEST-only setter for auth->sUid/sGid. Failing that, have the runner report skipped tests distinctly from passing ones so a green suite does not imply these branches were verified.

[aidangarske]

🔵 [Low] New error paths free host private key material without zeroization · Zeroization

Verified against the diff and the ES_ERROR macro definition (echoserver.c:2853-2865), which ends in WOLFSSL_RETURN_FROM_THREAD(0) — so each added WFREE is on a genuine early-return path and does execute. Under WOLFSSH_SMALL_STACK, keyLoadBuf is a heap buffer that load_key() / load_key_ed25519() fill with the server's ASN.1 private host key. The PR adds WFREE(keyLoadBuf, NULL, 0) on several new error paths reached after the buffer already holds private key bytes — most clearly the "Couldn't use first/second/Ed25519 key buffer" paths, where load_key() succeeded and only wolfSSH_CTX_UsePrivateKey_buffer() failed. The buffer is released back to the allocator with the key material intact rather than being cleared with ForceZero() first. Mitigating context: this is an example/test program rather than library code, the pre-existing success-path free at line 3439 has the same gap, and reaching these paths requires a key-load failure, so this is a defense-in-depth gap rather than an exploitable flaw. It is listed because the diff introduces additional unzeroized frees of key material on paths that previously leaked the allocation instead.

Fix: Add ForceZero(keyLoadBuf, EXAMPLE_KEYLOAD_BUFFER_SZ); immediately before each WFREE(keyLoadBuf, ...) (including the existing block-end free at line 3439, and the ESP-IDF copy), or factor the free into a small helper that zeroizes then frees so future exit paths inherit the behavior.

[aidangarske]

⚪ [Info] internal.c "memory leak fix" is a semantic no-op; no leak existed or was fixed · Logic

Verified against the diff (HEAD~1..HEAD) and the assignment site. The PR description states this hunk "Fixed a memory leak where RSA/ECC keys in signature blocks were leaked if they matched X.509 format IDs instead of standard SSH format IDs." The change is strictly behavior-preserving. pubKeyFmtId is assigned in exactly one place (src/internal.c:14397-14401): pubKeyFmtId = pubKeyId, then remapped to ID_SSH_RSA only when pubKeyId is ID_RSA_SHA2_256 or ID_RSA_SHA2_512. No other code assigns pubKeyFmtId, and pubKeyId is never reassigned after 14392. Therefore: BEFORE, the RSA branch matched pubKeyId in {ID_SSH_RSA, ID_RSA_SHA2_256, ID_RSA_SHA2_512, ID_X509V3_SSH_RSA}. AFTER, pubKeyFmtId == ID_SSH_RSA holds exactly when pubKeyId is in {ID_SSH_RSA, ID_RSA_SHA2_256, ID_RSA_SHA2_512}, plus pubKeyFmtId == ID_X509V3_SSH_RSA when pubKeyId == ID_X509V3_SSH_RSA — an identical set. For ECDSA, pubKeyFmtId == pubKeyId for every ECDSA and X509V3_ECDSA id (no remap applies), so that branch is identical too. The X.509 ids were already covered by the BEFORE code under the same #ifdef WOLFSSH_CERTS guard. The refactor is harmless and arguably clearer, but it fixes no leak. This is worth flagging so a maintainer does not close a real leak report believing it was addressed here.

Fix: Correct the PR description: this hunk is a readability refactor, not a leak fix. If an actual key leak was observed, capture the reproducing configuration — the fix is not in this hunk and the underlying issue may still be open.

[stenslae]

I didnt see any memory leaks in internal.c related to the pubKeyId... Meant to say the leak in the echoservers. Fixed pr summary.