bin/173977: pw(8) does not do range-checking on UIDs/GUIs from user's input, passwd DB becomes inconsistent

Nikos Vassiliadis nvass at gmx.com
Fri Nov 30 07:09:39 UTC 2012


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


More information about the freebsd-bugs mailing list