pw(8) command does not do any range checking on the uid and gid input, resulting in inconsistencies in the password database. Fix: Patch attached with submission follows: How-To-Repeat: Try adding a too big uid: > root@lab:~ # pw user add -n test1 -u 9999999999999 > root@lab:~ # id test1 > uid=2147483647(test1) gid=1004(test1) groups=1004(test1) An invalid number is also accepted and is interpreted as 0: > root@lab:~ # pw user add -n test2 -u asd9999999999999 > pw: uid `0' has already been allocated The password database can become inconsistent because "pw user del" does not really delete the user: > root@lab:~ # pw user add -n test0 -u 9999999999999999999 > root@lab:~ # id test0 > uid=2147483647(test0) gid=2147483647(test0) groups=2147483647(test0) > root@lab:~ # pw user del test0 > root@lab:~ # pw user del test0 > pw: pw_copy(): No such file or directory > root@lab:~ # id test0 > uid=2147483647(test0) gid=2147483647 groups=2147483647 /etc/passwd does not contain the user test0 but /etc/pwd.db is.
Responsible Changed From-To: freebsd-bugs->mjg I'll take it.
On Wed, 28 Nov 2012, Nikos Vassiliadis wrote: >> Description: > pw(8) command does not do any range checking on the uid and gid input, resulting in inconsistencies in the password database. > ... >> Fix: > > > > > Index: usr.sbin/pw/pw_group.c > =================================================================== > --- usr.sbin/pw/pw_group.c (revision 243652) > +++ usr.sbin/pw/pw_group.c (working copy) > @@ -350,6 +350,8 @@ > */ > if (a_gid != NULL) { > gid = (gid_t) atol(a_gid->val); The behaviour on overflow in atol() is undefined in C, but is benign (the same as for strtol()) in FreeBSD. Then the behaviour on as assignment to `gid' is undefined if sizeof(gid_t) < sizeof(long) (64-bit systems in FreeBSD). > + if (errno == ERANGE || errno == EINVAL) > + errx(EX_DATAERR, "gid %s is invalid", a_gid->val); Checking errno like this depends on atol() being essentially strtol(). When checking like this, it is necessary to set errno to some value (usually 0) different from all the error values that are checked for. The EINVAL check depends on an unportable POSIX feature. Garbage is still not detected on 64-bit systems. > > if ((grp = GETGRGID(gid)) != NULL && getarg(args, 'o') == NULL) > errx(EX_DATAERR, "gid `%ld' has already been allocated", (long) grp->gr_gid); Any program that uses atol() for parsing user input should be rmrf'ed of course. There are a couple of programs than do gid conversions fairly correctly. One is chpass(1). Its conversions are of low quality, but not as low as in pw. It uses strtoul(), but then overflows by assigning to uid_t or gid_t before checking that the value fits. It sets errno before the call and checks ERANGE, but it also bogusly checks for ULONG_MAX and thus bogusly rejects ids with that value. It does some up-front checking to disallow ids that don't start with a digit, and disallows ids in octal or hex. strtoul() would allow leading whitespace and a leading '-', and cal be told to allow any base. For command line args that are numbers represented as strings, I think it is a bug to not allow any format that can be parsed, but for utilities operating on files like /etc/passwd, only the standard format should be allowed. Bruce
On 28 November 2012 13:41, Nikos Vassiliadis <nvass@gmx.com> wrote: > Index: usr.sbin/pw/pw_group.c > =================================================================== > --- usr.sbin/pw/pw_group.c (revision 243652) > +++ usr.sbin/pw/pw_group.c (working copy) > @@ -350,6 +350,8 @@ > */ > if (a_gid != NULL) { > gid = (gid_t) atol(a_gid->val); atoi overflow is considered undefined behavior so the error can not be meaningfully be checked. In particular the compiler may assume the error will never occur and elide the check. Ideally this call is replaced with one of the stro* functions. > + if (errno == ERANGE || errno == EINVAL) > + errx(EX_DATAERR, "gid %s is invalid", a_gid->val); -- Eitan Adler
On Fri, 30 Nov 2012, Eitan Adler wrote: > On 28 November 2012 13:41, Nikos Vassiliadis <nvass@gmx.com> wrote: > >> Index: usr.sbin/pw/pw_group.c >> =================================================================== >> --- usr.sbin/pw/pw_group.c (revision 243652) >> +++ usr.sbin/pw/pw_group.c (working copy) >> @@ -350,6 +350,8 @@ >> */ >> if (a_gid != NULL) { >> gid = (gid_t) atol(a_gid->val); > > atoi overflow is considered undefined behavior so the error can not be > meaningfully be checked. This uses atol(), not atoi(). Unfortunately, the behaviour of both is defined in FreeBSD man pages. (FreeBSD man pages generally don't say what is portable or distinguish FreeBSD extensions, and atoi(3) and atol(3) are not exceptions. The functions are not actually extensions, and the difference is just fuzzy renderings of the C standard in the man pages, with the point about undefined behaviour on error missing but the related point about errno not necessarily being set on error present. Both specify that the behaviour on non-error is that of strtol() (blindly truncated for atoi(), but the man pages say this is what happens on error too, so their saying that errno need not be set on error says less than nothing -- errno can be, and is, set to whatever strtol() sets it to, and errors are only not detected for the blind cast in atoi().) Even the behaviour on cast and assignment is not undefined. It is implementation-defined. > In particular the compiler may assume the > error will never occur and elide the check. Perhaps if the compiler only supports the C standard it can do this (the "need not" wording allows arbitrary clobbering of errno, just like for any function that is not specified to only touch errno for a restricted set of actual errors, so compilers can assume that errno is indeterminate even if it was explicityly set before the call). But this wouldn't be compatible with the man page. > Ideally this call is > replaced with one of the stro* functions. > >> + if (errno == ERANGE || errno == EINVAL) >> + errx(EX_DATAERR, "gid %s is invalid", a_gid->val); Bruce
A commit references this bug: Author: bapt Date: Tue Jul 28 21:11:01 UTC 2015 New revision: 285985 URL: https://svnweb.freebsd.org/changeset/base/285985 Log: Check uid/gid used when creating a user/group are not larger than UID_MAX/GID_MAX PR: 173977 Reported by: nvass@gmx.com Changes: head/usr.sbin/pw/pw.c head/usr.sbin/pw/tests/Makefile head/usr.sbin/pw/tests/pw_groupadd.sh head/usr.sbin/pw/tests/pw_useradd.sh