Skip to content

Commit

Permalink
fix PTY allocation on Cygwin, broken by sshd split
Browse files Browse the repository at this point in the history
Cygwin doesn't support FD passing and so used to disable post-auth
privilege separation entirely because privsep requires PTY allocation
to happen in the privileged monitor process with the PTY file
descriptors being passed back to the unprivileged process.

This brings back a minimal version of the previous special treatment
for Cygwin (and any other platform that sets DISABLE_FD_PASSING):
privilege separation remains enabled, but PTY allocation happens in
the post-auth user process rather than the monitor.

This either requires PTY allocation to not need privilege to begin
with (this appears to be the case on Cygwin), or the post-auth
privsep process retain privilege (other platforms that set the
DISABLE_FD_PASSING option).

Keeping privileges here is bad, but the non-Cygwin systems that set
DISABLE_FD_PASSING are so deeply legacy that this is likely to be the
least of their problems.
  • Loading branch information
djmdjm committed Jun 13, 2024
1 parent f66d4df commit afe1031
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 1 deletion.
11 changes: 11 additions & 0 deletions session.c
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,17 @@
#include <selinux/selinux.h>
#endif

/*
* Hack for systems that do not support FD passing: allocate PTYs directly
* without calling into the monitor. This requires either the post-auth
* privsep process retain root privileges (see the comment in
* sshd-session:privsep_postauth) or that PTY allocation doesn't require
* privileges to begin with (e.g. Cygwin).
*/
#ifdef DISABLE_FD_PASSING
#define mm_pty_allocate pty_allocate
#endif

#define IS_INTERNAL_SFTP(c) \
(!strncmp(c, INTERNAL_SFTP_NAME, sizeof(INTERNAL_SFTP_NAME) - 1) && \
(c[sizeof(INTERNAL_SFTP_NAME) - 1] == '\0' || \
Expand Down
18 changes: 17 additions & 1 deletion sshd-session.c
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,21 @@ privsep_preauth(struct ssh *ssh)
static void
privsep_postauth(struct ssh *ssh, Authctxt *authctxt)
{
int skip_privdrop = 0;

/*
* Hack for systems that don't support FD passing: retain privileges
* in the post-auth privsep process so it can allocate PTYs directly.
* This is basically equivalent to what we did <= 9.7, which was to
* disable post-auth privsep entriely.
* Cygwin doesn't need to drop privs here although it doesn't support
* fd passing, as AFAIK PTY allocation on this platform doesn't require
* special privileges to begin with.
*/
#if defined(DISABLE_FD_PASSING) && !defined(HAVE_CYGWIN)
skip_privdrop = 1;
#endif

/* New socket pair */
monitor_reinit(pmonitor);

Expand Down Expand Up @@ -406,7 +421,8 @@ privsep_postauth(struct ssh *ssh, Authctxt *authctxt)
reseed_prngs();

/* Drop privileges */
do_setusercontext(authctxt->pw);
if (!skip_privdrop)
do_setusercontext(authctxt->pw);

/* It is safe now to apply the key state */
monitor_apply_keystate(ssh, pmonitor);
Expand Down

0 comments on commit afe1031

Please sign in to comment.