svn commit: r277652 - in head/usr.sbin/pw: . tests

[Bruce Evans]

On Sat, 24 Jan 2015, Baptiste Daroussin wrote:

> Log:
>  Allow negative numbers in -u and -g options

This is backwards.  ids and gids are non-negative integers that can be
represented in the type uid_t and gid_t, respectively.  All versions of
POSIX require this.  Old versions of POSIX allowed uid_t and gid_t to
be floating point, but didn't allow negative, fractional, infinity or
NaN values.  Not so old versions of POSIX require uid_t and gid_t to
be integer types, but still allow them to be sign.  Negative and values
must not be created.  Unrepresentable values cannot be created of course,
and should be rejected before they damage the password database.  uid_t
and gid_t happened to be unsigned in FreeBSD, so negative values are
unrepresentable so cannot be created.  pw is responsible for creating
ids, so it should reject negative and other unrepresentable ids.  It
has garbage code for both.

Negative ids have historical abuses in places like mountd.  mountd still
hard-codes -2 and -2 for the default uid and gid of an unprivileged user.
It at least casts these values to uid_t and gid_t before using them.
This gives the ids the non-random values of UINT32_MAX-1 if uid_t and
gid_t are uint32_t.  (If uid_t and gid_t were signed, then it would
leave the values as negative, so invalid.)  These magic values may work
better than when ids were 16 bits, since there is less risk of them
conflicting with a normal id.  However, the non-conflict is probably
a bug.  FreeBSD uses the magic ids of 65534 for user nobody: group
nobody.  These would have been (id_t)-2 with 16-bit ids.  They no
longer match, so ls displays (id_t)-2 numerically.  FreeBSD also has
a group nogroup = 65553 that doesn't match the nfs usage.  However2,
in FreeBSD-1 wher ids were 16-bits, nobody was 32767 and nogroup was
32766. so they didn't match nfs for other reasons.  The 2 non-groups
now seem to be just a bug -- FreeBSD-1 didn't have group nobody.
4.4BSD-Lite2 has the same values as FreeBSD-1.

> Modified: head/usr.sbin/pw/pw_group.c
> ==============================================================================
> --- head/usr.sbin/pw/pw_group.c	Sat Jan 24 17:32:45 2015	(r277651)
> +++ head/usr.sbin/pw/pw_group.c	Sat Jan 24 19:13:03 2015	(r277652)
> @@ -68,7 +68,11 @@ pw_group(struct userconf * cnf, int mode
> 	};
>
> 	if (a_gid != NULL) {
> -		if (strspn(a_gid->val, "0123456789") != strlen(a_gid->val))
> +		const char *teststr;
> +		teststr = a_gid->val;
> +		if (*teststr == '-')
> +			teststr++;
> +		if (strspn(teststr, "0123456789") != strlen(teststr))
> 			errx(EX_USAGE, "-g expects a number");
> 	}

Style bugs:
- nested declaration
- no blank line after declarations.

Garbage in the old code includes the primitive strspn() check.  It
continues by misparsing a_gid->value using atoi().  Large values first
overflow atoi(), giving undefined behaviour.  atoi() returns negative
values as negative now that they are allowed.  This overflow on
assignment.  This gives a way of entering the magic nfs ids.  You type
-2, atoi() returns -2, and they value overflows to the correct one on
assignment.  Previously, it was impossible to enter these values on
32-bit arches, but possible by exploiting the overflow bugs on 64-bit
arches.  atoi() uses strtol() with blind casting on both.  On 32-bit
archies, LONG_MAX = INT_MAX and strtol() clamps to this, so overflow
doesn't occur.  Large values are just impossible to specify, and
negative values were impossible to mis-specify.  On 64-bit arches,
large values are accepted by strtol() and the value overflows in the
blind cast.  Then after a few more overflows it is possible for the
magic nfs value to reach storage.  You had to enter it as 4294967294,
since another bug from using atoi() is that it doesn't support hex
numbers.  Now you can exploit the overflows in different ways and enter
the magic number as -2.

Correct parsing using strtoul() (or to support the full complications
of typedefed types that are not completely general but are known to be
integer with only nonnegative values supported, using strtoumax())
would do much more than the strspn() check.  However, a special check
for a minus sign would still be needed, to reject it.  Testing the
leading character as above only works if there is no leading whitespace.

>
>
> Modified: head/usr.sbin/pw/pw_user.c
> ==============================================================================
> --- head/usr.sbin/pw/pw_user.c	Sat Jan 24 17:32:45 2015	(r277651)
> +++ head/usr.sbin/pw/pw_user.c	Sat Jan 24 19:13:03 2015	(r277652)
> @@ -322,7 +322,10 @@ pw_user(struct userconf * cnf, int mode,
> 			a_name = NULL;
> 		}
> 	} else {
> -		if (strspn(a_uid->val, "0123456789") != strlen(a_uid->val))
> +		const char *teststr = a_uid->val;
> +		if (*teststr == '-')
> +			teststr++;
> +		if (strspn(teststr, "0123456789") != strlen(teststr))
> 			errx(EX_USAGE, "-u expects a number");
> 	}

Style bugs: as above, plus initialization in declaration.

pw has bad style generally, and some of the above may be to match this.

> Added: head/usr.sbin/pw/tests/pw_groupshow.sh

The tests are backwards too.

I'm not sure if the negative numbers can reach the password files.  They
might cause real problems there.  Programs like the old version of pw
reading them from there should detect them as errors and either reject
processing of users and groups with them, or just not write back the
garbage.

Bruce