Skip to content

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

Open
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_5850
Open

wolfsshd: fail closed when a per-connection privilege drop fails#1067
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_5850

Conversation

@yosuke-wolfssl

Copy link
Copy Markdown
Contributor

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.

@yosuke-wolfssl yosuke-wolfssl self-assigned this Jun 28, 2026
Copilot AI review requested due to automatic review settings June 28, 2026 23:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #1067

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants