mountd(8): parsecred(): Remove "duplicate compression"

[olce]

Summary

No functional change (intended).

This code dates back to 4.4BSD, became wrong after some getgrouplist()
(nssswitch-related) change in 2007, was fixed only in 2020 and since
then underwent cosmetic changes.

It is likely that in fact it never served any useful purpose in FreeBSD,
except perhaps at the very beginning. It's certainly not the case
today: NFS credentials are only used to check for file accesses, whose
group is checked against all groups of a credentials indiscriminately
(except for the real GID). Consequently, having a single duplicate,
which the code would actually remove only if in the first supplementary
group slot, doesn't change behavior. Moreover, we are going to regain
one slot in a subsequent commit.

[rmacklem]

I will look at the patches later, but just in case you are not aware of
why a lot of this weirdness exists in the code...

Sun RPC has a gid plus a list of additional groups (MAX 16) on the wire.
Broken code existed for a long time, that did the following:

cr_gid = gid
copy groups list

Step two overwrote the first gid, since cr_gid was just cr_groups[0] for BSD

The workaround was to put the same gid# in gid and element 0 of the Sun RPC
on-the-wire gid list.

Are any of these broken systems still out there (several very old variants of *BSD)?
If they are, you can easily break interoperability if you take this "put the same gid
in gid and element 0 of the gid list on the wire".
--> The code that looks for and gets rid of the duplicate on the receiving side

probably doesn't matter.

I will look at the patches later, rick
ps: If the gid is not duplicated on-the-wire, then there are up to 17 groups in

the RPC request.

[rmacklem]

usr.sbin/mountd/mountd.c

3616

This should now be

gid_t groups[NGROUPS_MAX];

3648

As noted for the other patch, this should now be

ngroups = NGROUPS_MAX;

since there is no chance it will be reduced by one.

3653

You might want to put a check here for

if (groups[0] == groups[1])
     syslog(LOG_ERR, "Group[0] == Group[1], pleas email [e-mail]");

so that any case where this occurs gets logged?
(I might do this as a first patch and see if anyone
reports the log message before moving to this patch.)

But this is up to you.

[olce]

In D46920#1069916, @rmacklem wrote:

Sun RPC has a gid plus a list of additional groups (MAX 16) on the wire.
Broken code existed for a long time, that did the following:

cr_gid = gid
copy groups list

Step two overwrote the first gid, since cr_gid was just cr_groups[0] for BSD

The workaround was to put the same gid# in gid and element 0 of the Sun RPC
on-the-wire gid list.

I wasn't aware, thanks for providing these elements. That said, what you describe is only a problem if that Sun RPC code is used verbatim on a BSD, which has not been the case for a long time on FreeBSD AFAICT, and the remote doesn't put the same GID in the first element of the groups array.

Are any of these broken systems still out there (several very old variants of *BSD)?
If they are, you can easily break interoperability if you take this "put the same gid
in gid and element 0 of the gid list on the wire".

I see. However, I think the situation here in mountd is the opposite: We were just getting rid of one occurrence of a GID if both present as the effective GID (slot 0) and the first supplementary group (slot 1), and my point is that doing so is unnecessary (as explained in the commit message).

--> The code that looks for and gets rid of the duplicate on the receiving side

probably doesn't matter.

Not sure why you refer to the receiving side here. mountd, AFAICT, is always the sending side for such information. Or are you referring to some implementations of getgrouplist() such as NIS, or some other method?

usr.sbin/mountd/mountd.c

3616

getgrouplist() can return up to NGROUPS_MAX + 1 groups (well, that's in fact only the minimum maximum figure), so why would you want to change that?

3648

I don't think so. Please see D46921 and the new comments there.

3653

These two groups can be the same and for a good reason. POSIX mandates a special (non-)treatment for supplementary groups: They must not be affected by GID changes prompted by setgid(), setegid(), etc. Which means, that, when changing credentials in such ways, one can lose access to files having the real GID as their group. If this behavior is not desired, the usual idiom is to add that real GID *also* in the list of supplementary groups. We should not remove this possibility.

[rmacklem]

usr.sbin/mountd/mountd.c

3648

I'll look there, but if you are not doing deduplication,
then no more than NGROUPSS_MAX can be passed
into the kernel for exports.

Why get one more and then throw it away?
The NGROUPS_MAX + 1 was because deduplication
might reduce the size by 1.

3653

This case is not affected by setuid()/setgid() etc. They are simply the
values acquired from the password/group list in the exports file.
For this case, duplication just wastes storage space.

It was more important before FreeBSD was able to handle large
group lists. Now, the only effect of deduplication is that it might
avoid a malloc/free in the crget/crfree.

[rmacklem]

In D46920#1070863, @olce wrote:

In D46920#1069916, @rmacklem wrote:

Sun RPC has a gid plus a list of additional groups (MAX 16) on the wire.
Broken code existed for a long time, that did the following:

cr_gid = gid
copy groups list

Step two overwrote the first gid, since cr_gid was just cr_groups[0] for BSD

The workaround was to put the same gid# in gid and element 0 of the Sun RPC
on-the-wire gid list.

I wasn't aware, thanks for providing these elements. That said, what you describe is only a problem if that Sun RPC code is used verbatim on a BSD, which has not been the case for a long time on FreeBSD AFAICT, and the remote doesn't put the same GID in the first element of the groups array.

Are any of these broken systems still out there (several very old variants of *BSD)?
If they are, you can easily break interoperability if you take this "put the same gid
in gid and element 0 of the gid list on the wire".

I see. However, I think the situation here in mountd is the opposite: We were just getting rid of one occurrence of a GID if both present as the effective GID (slot 0) and the first supplementary group (slot 1), and my point is that doing so is unnecessary (as explained in the commit message).

--> The code that looks for and gets rid of the duplicate on the receiving side

probably doesn't matter.

Not sure why you refer to the receiving side here. mountd, AFAICT, is always the sending side for such information. Or are you referring to some implementations of getgrouplist() such as NIS, or some other method?

Yes. The historical background wass just FYI for whoever was reading this
and wondering why the code would check for a duplicate.
I agree with you that the duplicate is unlikely to still exist in passwd/group databases
used by FreeBSD, so removing the code seems reasonable and does simplify the
code slightly.

However, since the deduplication does not fix any extant bug I am aware of, I do
not have a strong opinion w.r.t. whether or not the deduplication should be removed.

[olce]

usr.sbin/mountd/mountd.c

3648

I'd really like to clear this up, as I might be missing something important (but seriously doubt that).

The latest version of the export structure (struct export_args; you introduced it in 2020) does not have any limit to the number of groups it can pass, as the storage for the groups array effectively has to be allocated separately (but then, of course, the kernel will enforce the ngroups_max + 1 limit). Passing NGROUPS_MAX + 1 groups (effective GID + supplementary groups) has thus always been possible. Why limit to anything less than that?

Additionally, this structure doesn't have a separate field for the effective GID, so relies on the traditional BSD convention for passing it (as the first group in the groups array; this was also the case for the older struct xucred). So NGROUPS_MAX + 1 has always been the total number of groups, i.e., the effective GID and supplementary groups.

I'm not getting "one more group". All groups (effective GID + supplementary groups) are returned by getgrouplist(). And I'm not throwing away anything, *on the contrary* I'm just preserving the exact list as specified in both /etc/passwd and /etc/group (if using the files NSS provider).

So, could you please explain what you mean exactly by "why get one more", and "then throw it away"?

3653

This case is not affected by setuid()/setgid() etc.

What is affected by these functions is outside the scope of NFS, but is affected by the content of the supplementary group list, which is established independently of NFS. In other words, the effective GID (e.g., from /etc/passwd) may or may not be duplicated in the supplementary groups (e.g., from /etc/groups) independently of NFS use. We just have to cope with both scenarios, which I think are equally valid, in NFS.

I agree that the internal struct ucred used by NFS is only used to check for file access, which doesn't depend on whether the effective GID is duplicated, and is never the subject of a call to setgid() and co. that could change its effective GID, but that is not my point here.

They are simply the values acquired from the password/group list in the exports file. For this case, duplication just wastes storage space.
Now, the only effect of deduplication is that it might avoid a malloc/free in the crget/crfree.

True, but only in the case of having exactly 16 supplementary groups and also only if the effective GID is listed as the first supplementary group (but not the second, or any other position, which makes this process rather fragile). I suspect this doesn't matter in practice, that's why I chose to remove this "compression" instead of making it more robust (i.e., immune to the position of the duplicate).

[olce]

usr.sbin/mountd/mountd.c

3648

I'd really like to clear this up, as I might be missing something important (but seriously doubt that).

For the record, seems the misunderstanding came from the NGROUPS_MAX limitation in nmount(2) for the numbers of groups passed through the export option. This is addressed separately by the new D47013 in this revision series.