Date: Fri, 21 Apr 2017 11:41:40 +0000 From: Rick Macklem <rmacklem@uoguelph.ca> To: Bruce Evans <brde@optusnet.com.au>, Rick Macklem <rmacklem@FreeBSD.org> Cc: "src-committers@freebsd.org" <src-committers@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org> Subject: Re: svn commit: r317240 - head/sys/fs/nfs Message-ID: <YTXPR01MB01896B8154F83A3CFFA31FB9DD1A0@YTXPR01MB0189.CANPRD01.PROD.OUTLOOK.COM> In-Reply-To: <20170421141623.A1141@besplex.bde.org> References: <201704210150.v3L1ofgl014509@repo.freebsd.org>, <20170421141623.A1141@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Yes, I agree that hardcoded values are bad. Thanks for pointing out the constants in sys/conf.h, I can use those instea= d. sys/conf.h does not have an entry for "nogroup", which is what has been use= d by nfsuserd since it was written. Is adding an entry to sys/conf.h for GID_NOGROUP ok to do? rick ps: I hope you don't mind the top post... ________________________________________ From: Bruce Evans <brde@optusnet.com.au> Sent: Friday, April 21, 2017 1:19:25 AM To: Rick Macklem Cc: src-committers@freebsd.org; svn-src-all@freebsd.org; svn-src-head@freeb= sd.org Subject: Re: svn commit: r317240 - head/sys/fs/nfs 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 de= fault > 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 de= fault > ... > Modified: head/sys/fs/nfs/nfs_commonsubs.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > --- head/sys/fs/nfs/nfs_commonsubs.c Fri Apr 21 00:45:44 2017 (r3= 17239) > +++ head/sys/fs/nfs/nfs_commonsubs.c Fri Apr 21 01:50:41 2017 (r3= 17240) > @@ -63,8 +63,8 @@ int nfsrv_useacl =3D 1; > struct nfssockreq nfsrv_nfsuserdsock; > int nfsrv_nfsuserd =3D 0; > struct nfsreqhead nfsd_reqq; > -uid_t nfsrv_defaultuid; > -gid_t nfsrv_defaultgid; > +uid_t nfsrv_defaultuid =3D 65534; > +gid_t nfsrv_defaultgid =3D 65533; > int nfsrv_lease =3D NFSRV_LEASE; > int ncl_mbuf_mlen =3D MLEN; > int nfsd_enable_stringtouid =3D 0; These are the traditional values in /etc/group. It is bogus but traditiona= l 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?YTXPR01MB01896B8154F83A3CFFA31FB9DD1A0>