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

[Bruce Evans]

On Tue, 28 Jul 2015, Baptiste Daroussin wrote:

> Log:
>  Check uid/gid used when creating a user/group are not larger than UID_MAX/GID_MAX
>
>  PR:		173977
>  Reported by:	nvass at gmx.com

This is broken in a different way than before.

> Modified: head/usr.sbin/pw/pw.c
> ==============================================================================
> --- head/usr.sbin/pw/pw.c	Tue Jul 28 20:52:10 2015	(r285984)
> +++ head/usr.sbin/pw/pw.c	Tue Jul 28 21:10:58 2015	(r285985)
> @@ -269,7 +269,7 @@ main(int argc, char *argv[])
> 			}
> 			if (strspn(optarg, "0123456789") != strlen(optarg))
> 				errx(EX_USAGE, "-g expects a number");
> -			id = strtonum(optarg, 0, LONG_MAX, &errstr);
> +			id = strtonum(optarg, 0, GID_MAX, &errstr);

`id' still has type long.  The assignment overflows on 32-bit arches when
the value exceeds 0x7fffffff.  That is for half of all valid values.  pw
is broken in not supporting these values, but at least it detected them
as errors in the previous version.  Old versions implemented this bug
using atoi() with no error checking.

The behaviour in the overflowing cases is implementation-defined.
Normally this converts large values to negative ones.  This defeats
the lower limit of 0.

The change works accidentally on 64-bit arches.  It is assumed that
GID_MAX is representable as long long (since strtonum()'s API is too
broken to use even intmax_t, and uintmax_t is needed).  This assumption
is satisfied now.  It would break if someone expands gid_t to uint64_t.
without expandling long long to int65_t or larger.  Then passing GID_MAX
would change it to an implementation-defined value.  Normally to
INT64_MIN.  This negative, so there are no values in the range.

Similarly for uids.

> Added: head/usr.sbin/pw/tests/pw_groupadd.sh
> ==============================================================================
> --- /dev/null	00:00:00 1970	(empty, because file is newly added)
> +++ head/usr.sbin/pw/tests/pw_groupadd.sh	Tue Jul 28 21:10:58 2015	(r285985)
> @@ -0,0 +1,15 @@
> +# $FreeBSD$
> +
> +# Import helper functions
> +. $(atf_get_srcdir)/helper_functions.shin
> +
> +atf_test_case group_add_gid_too_large
> +group_add_gid_too_large_body() {
> +	populate_etc_skel
> +	atf_check -s exit:64 -e inline:"pw: Bad id '9999999999999': too large\n" \
> +		${PW} groupadd -n test1 -g 9999999999999
> +}

Check for large valid ids on i386 (should succeed, but currently fail),
negative ids (require failure), magic ids like (uid_t)-1 and (uid_t)-2
(should fail, but currently succeed on amd64), and the hex ids (should
succeed, but currently fail).  (uid_t)-1 is special for some syscalls,
so shouldn't be permitted for users.  (uid_t)-2 special for nfs (see
exports(5)).  The magic ids are hard to spell without using hex, but
pw is too broken to accept that.  For 32-bit ids, the above number
should be replaced by 0x100000000 when pw supports hex.  Also check
that 0xffffffff and 0xfffffffe are not too large, but reserved, and
that 0xfffffffd is not too large and not reserved.

Bruce