Date: Fri, 30 Nov 2012 09:09:21 +0200 From: Nikos Vassiliadis <nvass@gmx.com> To: Bruce Evans <brde@optusnet.com.au> Cc: freebsd-bugs@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: <50B85BA1.9030502@gmx.com> In-Reply-To: <20121129090618.F1410@besplex.bde.org> References: <201211281841.qASIfkx4033378@red.freebsd.org> <20121129090618.F1410@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 11/29/2012 12:32 AM, Bruce Evans wrote:
> 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.
Thanks for the recommendations Bruce,
I'll try to come up with something new soon.
Nikos
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?50B85BA1.9030502>