wolfsshd: fail closed when a per-connection privilege drop fails

[yosuke-wolfssl]

wolfsshd: fail closed when a per-connection privilege drop fails

Summary

Fixes a privilege-handling defect in wolfsshd where a failed per-connection
drop to the authenticated user's uid/gid could leave the connection handler
running at an elevated privilege level (root, or the sshd-daemon uid) instead
of terminating.

Addressed by f_5850.

Background

When servicing a channel request, the subsystem handlers
(SHELL_Subsystem, SCP_Subsystem, SFTP_Subsystem) drop privileges to the
authenticated user via wolfSSHD_AuthReducePermissionsUser(). On failure, the
old code attempted a fallback to wolfSSHD_AuthReducePermissions() and then
did return WS_FATAL_ERROR:

if (wolfSSHD_AuthReducePermissionsUser(conn->auth, pPasswd->pw_uid,
        pPasswd->pw_gid) != WS_SUCCESS) {
    wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Error setting user ID");
    if (wolfSSHD_AuthReducePermissions(conn->auth) != WS_SUCCESS) {
        exit(1);
    }
    return WS_FATAL_ERROR;
}

Two problems combined into a privilege-management hole:

1. The fallback is a no-op when privilege separation is off. With
   UsePrivilegeSeparation set to WOLFSSHD_PRIV_OFF,
   wolfSSHD_AuthReducePermissions() performs no uid change and returns
   WS_SUCCESS. So after a failed drop-to-user, the fallback "succeeds"
   without lowering anything.

2. HandleConnection discarded the SHELL_Subsystem return value. The
   WS_FATAL_ERROR was never captured, so ret stayed WS_SUCCESS. The
   post-switch error check consults wolfSSH_get_error(ssh), which a kernel
   setreuid()/setregid() failure does not set, so wolfSSH_shutdown and up
   to ten wolfSSH_worker iterations would execute while the effective uid was
   still above the authenticated user.

Triggering the drop failure itself requires an environmental condition that
forces setreuid/setregid to fail (e.g. RLIMIT_NPROC saturation at the
target uid, an LSM denial, or user-namespace mapping limits), so this is not
routinely reachable in default deployments — but when it does occur the handler
must fail closed.

Fix

• Every per-connection privilege-drop failure path now calls exit(1)
  directly instead of attempting the no-op fallback and returning. This is
  strictly stronger than propagating an error into the connection handler:
  the per-connection process terminates before any work runs at the wrong
  privilege level. Applied consistently across SHELL_Subsystem (the forked
  child branch and the parent branch, plus the dup2/setgroups/chroot
  failure paths), SCP_Subsystem, and SFTP_Subsystem.

• HandleConnection now captures the return value at both subsystem call
  sites (ret = SHELL_Subsystem(...) for the shell and exec sessions), so a
  failure is reflected in ret as well (defense in depth; exit(1) already
  makes the path unreachable).

• In the SHELL_Subsystem parent branch the forkpty child has already been
  spawned, so it is killed with kill(childPid, SIGKILL) before exit(1) to
  guarantee deterministic teardown rather than relying on pty/pipe closure,
  matching the existing unexpected-error handling later in the same function.

The daemon's own startup drop in main() is unchanged: it already fails closed
by setting ret = WS_FATAL_ERROR, which prevents the listen/accept loop from
starting.

Testing

• wolfsshd builds cleanly with the project configuration.
• No functional behavior change on the success path; only the
  privilege-drop-failure paths are affected, and they now terminate the
  per-connection process.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1067

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

[aidangarske]

🐺 Skoll Code Review

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

Posted findings

• [Medium] Add regression coverage for fail-closed subsystem paths — apps/wolfsshd/wolfsshd.c:595-601

Review generated by Skoll.

[aidangarske]

🟡 [Medium] Add regression coverage for fail-closed subsystem paths
💡 SUGGEST test

The PR changes several privilege-drop failure paths from returning WS_FATAL_ERROR after a fallback attempt to calling exit(1), and it changes HandleConnection to retain SHELL_Subsystem return values. Existing tests cover wolfSSHD_AuthReducePermissionsUser() itself, but I did not find tests that exercise the subsystem call sites or verify that HandleConnection no longer discards shell/exec subsystem failures. Without a regression test around these changed paths, a future edit could accidentally reintroduce a return path into the connection handler after a failed per-connection privilege drop.

Suggestion:

Suggested change

	if (wolfSSHD_AuthReducePermissionsUser(conn->auth, pPasswd->pw_uid,
	Add a focused regression test using the existing `WOLFSSHD_UNIT_TEST` privilege-drop stubs. Run the subsystem failure case in a child process and assert that it exits non-zero, and add coverage that a `SHELL_Subsystem` failure is assigned into `ret` at the shell and exec call sites.