svn commit: r317240 - head/sys/fs/nfs

[Bruce Evans]

On Fri, 21 Apr 2017, Rick Macklem wrote:

> Log:
>  Set default uid/gid to nobody/nogroup for NFSv4 mapping.
>
>  The default uid/gid for NFSv4 are set by the nfsuserd(8) daemon.
>  However, they were 0 until the nfsuserd(8) was run. Since it is
>  possible to use NFSv4 without running the nfsuserd(8) daemon, set them
>  to nobody/nogroup initially.
>  Without this patch, the values would be set by the nfsuserd(8) daemon
>  and left changed even if the nfsuserd(8) daemon was killed. Also, the default
>  values of 0 meant that setting a group to "wheel" would fail even when
>  done by root and this patch fixes this issue.

Hard-coding these values is wrong, and the actual values seem wronger.

The values in nfsuserd(8) are clearly wrong.  It hard-codes 32767 (with
bogus casts) for both the default uid and the default gid.  These values
are in the user namespace have nothing to do with nobody/nogroup.

>  and left changed even if the nfsuserd(8) daemon was killed. Also, the default
>  ...
> Modified: head/sys/fs/nfs/nfs_commonsubs.c
> ==============================================================================
> --- head/sys/fs/nfs/nfs_commonsubs.c	Fri Apr 21 00:45:44 2017	(r317239)
> +++ head/sys/fs/nfs/nfs_commonsubs.c	Fri Apr 21 01:50:41 2017	(r317240)
> @@ -63,8 +63,8 @@ int nfsrv_useacl = 1;
> struct nfssockreq nfsrv_nfsuserdsock;
> int nfsrv_nfsuserd = 0;
> struct nfsreqhead nfsd_reqq;
> -uid_t nfsrv_defaultuid;
> -gid_t nfsrv_defaultgid;
> +uid_t nfsrv_defaultuid = 65534;
> +gid_t nfsrv_defaultgid = 65533;
> int nfsrv_lease = NFSRV_LEASE;
> int ncl_mbuf_mlen = MLEN;
> int nfsd_enable_stringtouid = 0;

These are the traditional values in /etc/group.  It is bogus but traditional
to have a uid in /etc/group.  nobody doesn't actually have group nogroup.
nobody is also in the correct place (/etc/passwd) provided the sysadmin
didn't meddle with this, but it has group 65534 there.  Group 65533 is
traditionally for kmem and tty in /etc/passwd.

mountd uses the less bogus values with more bogus casts (uid_t)-2 and
(gid_t)-2.  These are obfuscated spellings of 0xfffffffe.  You don't
really want these values to depend on the types because then the values
break when the types are expanded, like they already did for the
expansion to 32 bits.  These values should be reserved in /etc/passwd
and /etc/group, but they still aren't, 20-25 years after the expansion.
You don't want to spell these values in decimal because decimal values
much larger than 65534 are too hard to convert back to hex to see what
they mean.  The bogus casts are to break warnings about converting the
easy-to-remember (but wrong becaus they are negative) values of -2 to
unsigned.

The magic values are still incompletely documented in exports(8) as
pure -2:-2.  It doesn't mention nobody or any other magic numbers or
any bugs in defaulting or specifying the numbers.

Last time I looked, most utilities and even basic library functions
had the usual errors for parsing large and negative values by misusing
atoi() or strtol().  Many can't parse 0xfffffffe or even this value
in decimal, so the value has to be spelled -2 so that undocumented
overflow bugs can convert it to the correct value of 0xffffffffe.  On
64-bit systems, it might be first converted to 0xffffffffffffffffe so
another layer of overflow bugs is needed to get back to 0xfffffffe.
Since library functions are buggy, it is at best unportable to put any
of -2, 0xfffffffe or this value in hex in /etc/passwd or /etc/group.
65534 always worked there since it is much smaller than 32-bit LONG_MAX
and the library was probably never bad enough to use atoi() and 16-bit
ints.  IIRC, -2 doesn't wrok in /etc/passwd, but is the only spelling
that works near mountd.

Checking now shows mountd using many different misparsing methods:
- it uses only atoi() in parescred().  This can read -2 in /etc/exports,
   but misparses the correct value of 0xfffffffe to 0 and the correct
   value of this in decimal to 0x7fffffffl; it silently ignores errors
   in both cases; it blindly assigns to uid_t/gid_t, so -2 becomes the
   correct value
- it uses strtol() with mostly missing and half wrong error checking
   for masklen
- it uses strtoul() with mostly wrong but not so much missing error
   checking for -p.  First it truncates the value to break its subsequent
   range checks...
- its use of atoi() implicitly forces base 10.  This is forced explicitly
   for the strtol() and strtoul().  This makes sure that hex never works.
   Hex is always misparsed as 0, and the resulting garbage after the 0
   is only checked for after the strtoul() call.

Kernel code shouldn't hard-code any user ids, starting with 0 for
root (this is worse than 65534 for nobody since 0 is so special),
but for devices I hard-coded many ids in <sys/conf.h>.  This works
well enough and is better than implicit 0 for root provided everything
uses the #defined values.  It is better than scattered values of
32767, 65534, 65533 and 0xfffffffe and -2.

Bruce