xinstall: Validate numeric uids/gids

[jlduran]

Summary

Extend the fix 4b04f5d7e8a2 ("install: Fix METALOG ouptut for numeric -o
and -g args") to validate the uid/gid falls between a valid range.

[jlduran]

I personally prefer to commit the fix and the test together, however I have been instructed in the past to do it separately. If this is still the best practice, I will split it upon commit.

[emaste]

@guest-patmaddox has a proposal for this also, in https://github.com/freebsd/freebsd-src/pull/1552
PR: 283355

[emaste]

I personally prefer to commit the fix and the test together

That is my preference as well

[jlduran]

In D48587#1108227, @emaste wrote:

@guest-patmaddox has a proposal for this also, in https://github.com/freebsd/freebsd-src/pull/1552
PR: 283355

Oh, thanks for the heads up!

[emaste]

Note that we sort-of rely on install not validating -o and -g in -U/-M mode for some uses (see discussion in D48506). In practice it wouldn't matter because uname root = uid 0 and gname wheel = gid 0 is generally going to be true regardless of whether we use the host's passwd file or (as we should) the target's, but for this to be done properly we need to plumb dbdir to certctl and anything else calling install -U -M. @jrtc27 offered to do that if nobody gets to it first. I will try to find time to look at it but also won't mind if someone gets to it first.

[markj]

In D48587#1108226, @jlduran wrote:

I personally prefer to commit the fix and the test together, however I have been instructed in the past to do it separately. If this is still the best practice, I will split it upon commit.

I'm not aware of any particular guidance along these lines, aside from the general, "keep commits small self-contained." I often commit tests together with functional changes. The one reason I might avoid that is if I want to ensure that a bug fix can be backported easily, e.g., in response to a secteam@ report.

[jlduran]

In D48587#1108230, @emaste wrote:

Note that we sort-of rely on install not validating -o and -g in -U/-M mode for some uses (see discussion in D48506).

Yes, I think this should be a feature and not a bug. -U and -o/-g without -M makes no sense.

[jlduran]

In D48587#1108227, @emaste wrote:

@guest-patmaddox has a proposal for this also, in https://github.com/freebsd/freebsd-src/pull/1552
PR: 283355

This revision proposes a different approach, more in-line with the current status, and avoids producing an invalid spec file when bogus uids/gids are used (e.g., -1). If it turns out this is the desired approach, I will also document it in the man page, as it is ambiguous at the moment.

[markj]

usr.bin/xinstall/xinstall.c

330

Should we compare with GID_MAX instead?

347

Compare with UID_MAX instead?

[jlduran]

Address suggestions:

• Use a more appropriate GID_/UID_MAX, rather than raw UINT_MAX

[markj]

usr.bin/xinstall/xinstall.c

341

What if the gid_from_group() call above succeeds? Aren't we overwriting its output value here?

[jlduran]

Address fixes:

• Keep the logic as close to the original as possible
• Add a couple more tests

[markj]

usr.bin/xinstall/xinstall.c

333

If group != NULL and dounpriv is true, we can leave gid uninitialized, I think.

[jlduran]

Address suggestions:

• Do not initialize uid/gid when -g/-o and -U are passed (the way it currently is)