kern: start tracking cr_gid outside of cr_groups[]

[kevans]

Summary

This is the (mostly) kernel side of de-conflating cr_gid and the
supplemental groups. The pre-existing behavior for getgroups() and
setgroups() is retained to keep the user <-> kernel boundary
functionally the same while we audit use of these syscalls, but we can
remove a lot of the internal special-casing just by reorganizing ucred
like this.

struct xucred has been altered because the cr_gid macro becomes
problematic if ucred has a real cr_gid member but xucred does not. Most
notably, they both also have cr_groups[] members, so the definition
means that we could easily have situations where we end up using the
first supplemental group as the egid in some places. We really can't
change the ABI of xucred, so instead we alias the first member to the
cr_gid name and maintain the status quo.

__FreeBSD_version bumped for the struct ucred ABI break.

For relnotes: downstreams and out-of-tree modules absolutely must fix
any references to cr_groups[0] in their code. These are almost
exclusively incorrect in the new world, and cr_gid should be used
instead. There is a cr_gid macro available in earlier FreeBSD versions
that can be used to avoid having version-dependant conditionals to refer
to the effective group id. Surrounding code may need adjusted if it
peels off the first element of cr_groups and uses the others as the
supplemental groups, since the supplemental groups start at cr_groups[0]
now if &cr_groups[0] != &cr_gid.

Relnotes: yes (see last paragraph)

[kevans]

I'm reviewing {get,set}groups(2) usage in base, and none of this is terribly surprising, though just a few notes:

• we should probably do a pass and stop allocating NGROUPS_MAX + 1, though that doesn't seem terribly high priority
• chroot(8)'s usage is a little surprising, because its use is FreeBSD-native and seems to ignore the historical behavior here. We'll setgroups(2) the -G list, then immediately clobber the first element with a setgid(2) ? chroot(8) looks like it could use a bit of cleanup anyways
• seems to be very little that will actually break when we subsequently change setgroups/getgroups to stop special-casing the first element

[olce]

I'll also check if there are other occurrences we might have missed (hopefully not).

sys/compat/linux/linux_misc.c

1055–1058

I'd simplify this ugly block.

sys/compat/linux/linux_uid16.c

116–119

Same as above.

sys/kern/kern_prot.c

106

Minor. Suggested text could be seen as tautological, YMMV.

514–515

(You'll have to re-fill the comments' paragraph.)

540–541

667–669

(Re-fill the comments' paragraph as necessary.)

Actually, in a subsequent pass, I'll just remove the preallocated array dance, which becomes unnecessary with a separate storage for the egid. Here and below, I'm suggesting the minimal changes for the code to stay correct.

As you can see, I was eyeing at separating the egid : The setcred() interface takes the egid as a separate field, not as the first elements of the groups array, which is really only the set of supplementary groups.

700

701–702

2921

Not sure if this shortcut is really worth it, but in any case, I'd prefer negating the test and returning early rather than adding an indentation level for the current inner block.

sys/rpc/authunix_prot.c

105

cred is a struct xucred here, so this change should be backed out.

(The comment XXXKE should not have been added in the first place, we missed that.)

114

Same as above.

124–126

Same as above.

sys/rpc/svc_auth_unix.c

92–107

Same as above, this must be backed out.

sys/sys/ucred.h

78–80

Given the history, let's be explicit here, out of caution.

And thanks for taking into account my comments. :-)

[olce]

sys/kern/kern_prot.c

741–747

It is very important here that wcred->sc_supp_groups_nb is updated with the value output by groups_normalize().

1266

memcpy() is not supposed to be used on overlap. Anyway, no copying is really necessary.

1275–1276

Keeping this comment does not seem to make much sense now.

[olce]

sys/kern/kern_prot.c

1275–1283

Actually, I would suggest this change, and perhaps adding a comment saying that we pass NULL explicitly in the case of 0 supplementary groups for the sake of normalization/proactive security.

[kevans]

sys/kern/kern_prot.c

701–702

I'm reflowing this into two assignments because it's a bit more clear than the current excessive line-wrapping, IMO.

1266

D'oh.

1275–1283

Adding a comment for you to edit, having fleshed out your commentary in this review a little bit after thinking on it some.

sys/rpc/authunix_prot.c

105

I'm dropping one comment here at the top to point out that it's a struct xucred and dropping the comment while reverting this.

[kevans]

Address review feedback

This seems to fix a bug in one of my VMs that I had only recently noticed where
I'd fail to mount NFS mounts during the initial rc phase -- presumably due to
the sys/rpc bits that I had goofed (both the VM and my VM host (laptop) are
running with the broken change; fixing it in the VM is sufficient to unbreak
it).

[olce]

The fact that this commit also fixes our implementation of Linux's setgroups() should be noted in the commit message.

sys/kern/kern_prot.c

2916–2918

I would move that as the very first statement in this function. In any case, it separates setting cr_ngroups to 0, for the sake of crextend(), to the actual call to it, which feels odd.

[kevans]

sys/kern/kern_prot.c

2916–2918

How does cr_ngroups get cleared to 0, then, if we're doing a crsetgroups with ngrp == 0? The comment above the assignment doesn't inspire confidence that we wouldn't end up in a scenario wherre cr_ngroups > 0 with no groups et.

[olce]

sys/kern/kern_prot.c

701–702

I usually prefer to use the ternary operator when both branches are just assignments to the same variable (to make the unconditional assignment stand out), but that's fine.

2916–2918

Oh, I mixed up variables it seems... Pilot error.

[kevans]

sys/kern/kern_prot.c

2799

I was going to commit this, and on final review I started wondering: this assertion has me questioning if we should consider some other mechanism to flag that groups have been set, now that cr_ngroups == 0 is a valid immutable condition. We lose the ability to detect that kind of error with this change.

Would it be worth adding a CRED_FLAG bit for this to track that out-of-band? I don't necessarily think that's a blocker for this, but something we should do sooner rather than later.

[kevans]

sys/kern/kern_prot.c

2933

This seems like it might be able to go away if we're confident that all of the cr_groups[0] references are fixed.

[kevans]

Fix crsetgroups_fallback()

crsetgroups_fallback() would previously have had the side effect of setting the
egid, but won't anymore with this change -- ensure we set it, and we can decide
if the callers should be converted to an explicit cr_gid set + cr_setgroups()
later.

While we're here, update ucred(9) to drop a mention of touching cr_groups[0].

[olce]

sys/kern/kern_prot.c

2933

I don't think it can. I introduced this function for the sake of NFS-related code, which maintains credentials in a variety of structures that store the effective GID altogether with supplementary groups in a single array. My reasoning was that auditing all places and code to be sure that these arrays could not be empty was too much work and for arguably a non-durable result. Departing from this scheme feels like a security hazard.

We now have that crsetgroups() only sets supplementary groups (so the name could be misleading now; I'll be in favor of renaming it, but perhaps in a separate commit if this would not be MFCable). In that vein, crsetgroups_fallback() should now be the one to be named crsetgroups(). As this name is quite overloaded, and anyway doing that would require first renaming the existing one, perhaps we should instead rename crsetgroups_fallback() to something like crsetgroups_and_egid().

2936–2942

This function was indeed missed.

The proposed change alone does not work. Basically, now that crsetgroups() only sets supplementary groups, we are stuffing the egid in there but we should not.

I've suggested a change, where crsetgroups() is called before setting cr_gid just for the sake of the execution flow reaching assertions in that function sooner.

[kevans]

sys/kern/kern_prot.c

2936–2942

Presumably we still need to set cr->cr_ngroups = 0; in the ngrp == 0 case, right? It looks like some of the callers operate on crget(), but nfs in particular seems to be duping an existing ucred.

[kevans]

More correct fix for crsetgroups_fallback -- we shouldn't adopt the egid into
the supplementary groups in the new cr_groups scheme, we should peel it off and
set the rest as cr_groups.

[kevans]

Convert one crsetgroups() caller to crsetgroups_fallback() in nfsserver

We want the effective GID set, so we should use crsetgroups_fallback() now that
crsetgroups() won't set that. We know that credanon was already populated,
though, so we'll just use its GID as the fallback.

[kevans]

*sigh* sorry for the churn. _fallbaxk is wrong because these arw purely
supplementary groups. Copy the gid.

[olce]

I've spent time examining again all callers of crsetgroups*() functions, and have a couple of suggested changes, some of which are additional bug fixes. One bug fix is in sys/kern/kern_prot.c:kern_setcred() (I've added an inline comment for it), and the others are in sys/rpc/svc_auth.c. Since it is annoying to send suggestions through Phab (and is not even readily possible for files that are not already part of the review), I have been put in a single patch here: https://people.freebsd.org/~olce/Phabricator/kevans-D51489_diff_159387-suggested_changes.patch.

Given the time spent on this, a Co-authored-by: olce tag would be appreciated (although there's no denying that you did the bulk of the work).

sys/kern/kern_prot.c

785–790

These two cases have to become independent, as we do not set the egid anymore with crsetgroups_internal(). Please see suggested patch.

2933

I don't think it can. I introduced this function for the sake of NFS-related code, which maintains credentials in a variety of structures that store the effective GID altogether with supplementary groups in a single array. My reasoning was that auditing all places and code to be sure that these arrays could not be empty was too much work and for arguably a non-durable result. Departing from this scheme feels like a security hazard.

And, for the record, I now vaguely recall that some of these arrays are in fact filled up by data coming from userland, which could be buggy or could be exploited, escalating a userland problem (arguably relatively severe, if groups can be manipulated) into a kernel problem.

2936–2942

Presumably we still need to set cr->cr_ngroups = 0; in the ngrp == 0 case, right?

Right, I missed that.

[kevans]

Incorporate Olivier's patch