ssh: sshd-session: properly save off the privileged gid
ClosedPublic
Actions

Authored by kevans on Wed, Aug 6, 5:56 AM.

Details

Reviewers

emaste
des
markj
jlduran

Commits

rG239e8c98636a: ssh: sshd-session: properly save off the privileged gid

Summary

Current and traditional FreeBSD behavior means that getegid() here is
the first element in the prior setgroups() call, if any, so we may
inadvertently wipe out our rgid with the unprivileged gid. This is
rendered somewhat harmless by the fact that we're losing the privileged
gid -- we'll still regain it as the egid in restore_uid() later by way
of restoring saved_egroups, rather than by intentionally restoring it
from getgid().

This will be promptly reverted if we can get setgroups(2)/getgroups(2)
change in FreeBSD 15.0, but it seemed wise to get this technically
correct for previous branches.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 65998
Build 62881: arc lint + arc unit

Event Timeline

kevans created this revision.Wed, Aug 6, 5:56 AM

Herald added a subscriber: imp. · View Herald TranscriptWed, Aug 6, 5:56 AM

kevans requested review of this revision.Wed, Aug 6, 5:56 AM

Harbormaster completed remote builds in B65998: Diff 159830.Wed, Aug 6, 5:56 AM

kevans retitled this revision from sshd-session: properly save off the privileged gid to ssh: sshd-session: properly save off the privileged gid.Wed, Aug 6, 4:28 PM

kevans edited the summary of this revision. (Show Details)

emaste added a reviewer: jlduran.Wed, Aug 6, 4:41 PM

Should we attempt to upstream this change?
From my brief testing this might also affect NetBSD, although they currently use the non-portable version of this file.

@kevans just to confirm, the phrase: "This will be promptly reverted if we can get setgroups(2)/getgroups(2) change in FreeBSD 15.0" essentially means we'll start defining _POSIX_SAVED_IDS (in unistd.h)? If this is so, this should not be reverted, because it will simply take the path where SAVED_IDS_WORK_WITH_SETEUID is defined?

@emaste, I can try upstreaming this revision (if there are no objections, of course), as well as other fixes that landed recently (chiefly D51810 and D51809), maybe also a few simple improvements from D51642 to reduce our diffs.

This revision is now accepted and ready to land.Sat, Aug 9, 6:30 AM

In D51753#1184355, @jlduran wrote:

Should we attempt to upstream this change?
From my brief testing this might also affect NetBSD, although they currently use the non-portable version of this file.

NetBSD's setgroups() doesn't set the egid as FreeBSD's does, so it should not be affected. The only other platform I've found to be affected is macOS.

@kevans just to confirm, the phrase: "This will be promptly reverted if we can get setgroups(2)/getgroups(2) change in FreeBSD 15.0" essentially means we'll start defining _POSIX_SAVED_IDS (in unistd.h)? If this is so, this should not be reverted, because it will simply take the path where SAVED_IDS_WORK_WITH_SETEUID is defined?

Not quite- those macros are unrelated to this particular problem, which is that the egid is influenced by setgroups(). Those macros are more about whether you can recover your previous gid if it was the saved one.

@emaste, I can try upstreaming this revision (if there are no objections, of course), as well as other fixes that landed recently (chiefly D51810 and D51809), maybe also a few simple improvements from D51642 to reduce our diffs.

For this one in particular, my hope is to just MFC it, patch the port, then fix 15.0 and drop the local patch before upstream has to care.

In D51753#1184367, @kevans wrote:

In D51753#1184355, @jlduran wrote:

Should we attempt to upstream this change?
From my brief testing this might also affect NetBSD, although they currently use the non-portable version of this file.

NetBSD's setgroups() doesn't set the egid as FreeBSD's does, so it should not be affected. The only other platform I've found to be affected is macOS.

@kevans just to confirm, the phrase: "This will be promptly reverted if we can get setgroups(2)/getgroups(2) change in FreeBSD 15.0" essentially means we'll start defining _POSIX_SAVED_IDS (in unistd.h)? If this is so, this should not be reverted, because it will simply take the path where SAVED_IDS_WORK_WITH_SETEUID is defined?

Not quite- those macros are unrelated to this particular problem, which is that the egid is influenced by setgroups(). Those macros are more about whether you can recover your previous gid if it was the saved one.

@emaste, I can try upstreaming this revision (if there are no objections, of course), as well as other fixes that landed recently (chiefly D51810 and D51809), maybe also a few simple improvements from D51642 to reduce our diffs.

For this one in particular, my hope is to just MFC it, patch the port, then fix 15.0 and drop the local patch before upstream has to care.