Date: Thu, 29 Nov 2012 09:32:26 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Nikos Vassiliadis <nvass@gmx.com> Cc: freebsd-bugs@freebsd.org, freebsd-gnats-submit@freebsd.org Subject: Re: bin/173977: pw(8) does not do range-checking on UIDs/GUIs from user's input, passwd DB becomes inconsistent Message-ID: <20121129090618.F1410@besplex.bde.org> In-Reply-To: <201211281841.qASIfkx4033378@red.freebsd.org> References: <201211281841.qASIfkx4033378@red.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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:
>
>
> Patch attached with submission follows:
>
> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121129090618.F1410>