rpc.lockd: avoid embedding assumptions about cr_groups[0]

[kevans]

Summary

sys/ucred.h provides a cr_gid macro that should be used to reference the
egid element of an xucred, so let's use that.

While we're here, avoid assuming that the first element is the egid and
include it in the group list unless it *does* match the egid. This is
not a functional change today: the egid is always the first group in
the list, but we may want to consider changing that some day.

[olce]

With this change, we would not send in the list of supplementary groups to the lock server the egid if it is also present there. This does not have an effect most of the time, as the distinction egid/supplementary group ID matters only for processes, but it might have one if some special security policy (such as some MAC custom one if the server is FreeBSD) messes around, e.g., denies access based on supplementary group membership (I agree that's highly hypothetical at this stage). Still, for this reason and perhaps also for clarity, I would prefer testing just whether cr_gid and groups[0] live at the same address.

usr.sbin/rpc.lockd/kern.c

245

As per the general comment.

245

struct xucred is passed by the kernel, so should always contain at least one group. But better be safe than sorry. The use of cr_groups[0] before, now cr_gid on xucred actually assumes that, so we should probably test for that separately.

[kevans]

usr.sbin/rpc.lockd/kern.c

245

Hmm, I was trying to avoid the assumption that the group list always has at least one element going forward in case we do not want to add the egid back in there if it gets separated out into cr_gid -- this path would then not *need* to change, but we *could* simplify it by removing the new logic.

[olce]

usr.sbin/rpc.lockd/kern.c

245

Ah yes, sure. To avoid this problem, we can first test for groups == &xucred->cr_gid and only then assert (and have a different assertion per branch).

[kevans]

Strengthen our check to cr_gid == &cr_groups[0]

Language in both the commit message and comment have been adjusted from
discussing 'matching' the egid to the stronger 'is actually the egid'. We now
assert that ngroups > 0 if the egid is the first element as well.

This avoids dropping the egid from the supplementary group list in the future
if cr_groups[0] is *not* the egid but it was explicitly added to the list and
happens to be the first element.

[kevans]

Ping

[rmacklem]

I'll admit I cannot remember what olce@ decided.
Note that Sun RPC will always fill in at least one gid
in the RPC request. (The RPC request has a gid and
a gid list in it. The list can be 0 length, but there will
always be 1 gid.)