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.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1067

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1067

⚠️ An internal error occurred during the automated review. This error has been logged. Please contact the Fenrir team if you need assistance.

Error: OperationalError

[yosuke-wolfssl]

Hi @aidangarske . could you please review this ?

[aidangarske]

Is there any way we can do this test without adding fault injections into the src?

[yosuke-wolfssl]

Thanks, I dropped the in-source fault injection entirely. auth.c and include.am
are now back to master (no callbacks, no WOLFSSHD_FAULT_INJECT, no faultinj daemon
build).

The test now drives the stock wolfsshd binary with a small LD_PRELOAD interposer
(apps/wolfsshd/test/sshd_privdrop_preload.c) that returns EPERM from
setregid()/setreuid() only when WOLFSSHD_FAULT_PRIVDROP is set, forwarding the real
call otherwise. It's compiled on-demand by the test, Linux-only (SKIPs where
DYLD stripping applies), and never touches the shipped daemon or library — so we're
now testing the actual production binary, not a special build.

[yosuke-wolfssl]

Hi @aidangarske ,
Sorry to bother you again, could you review this please ?