Date: Sat, 1 Dec 2012 12:02:01 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Eitan Adler <lists@eitanadler.com> Cc: freebsd-bugs@freebsd.org, Nikos Vassiliadis <nvass@gmx.com>, bug-followup@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: <20121201113232.T903@besplex.bde.org> In-Reply-To: <CAF6rxgnbi6jmG-Y0JKyp5tD2US5YTdoATSfq7MF9D6kEyZy%2BBg@mail.gmail.com> References: <201211281841.qASIfkx4033378@red.freebsd.org> <CAF6rxgnbi6jmG-Y0JKyp5tD2US5YTdoATSfq7MF9D6kEyZy%2BBg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 30 Nov 2012, Eitan Adler wrote:
> On 28 November 2012 13:41, Nikos Vassiliadis <nvass@gmx.com> wrote:
>
>> 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);
>
> atoi overflow is considered undefined behavior so the error can not be
> meaningfully be checked.
This uses atol(), not atoi().
Unfortunately, the behaviour of both is defined in FreeBSD man pages.
(FreeBSD man pages generally don't say what is portable or distinguish
FreeBSD extensions, and atoi(3) and atol(3) are not exceptions. The
functions are not actually extensions, and the difference is just fuzzy
renderings of the C standard in the man pages, with the point about
undefined behaviour on error missing but the related point about errno
not necessarily being set on error present. Both specify that the
behaviour on non-error is that of strtol() (blindly truncated for atoi(),
but the man pages say this is what happens on error too, so their
saying that errno need not be set on error says less than nothing --
errno can be, and is, set to whatever strtol() sets it to, and errors
are only not detected for the blind cast in atoi().)
Even the behaviour on cast and assignment is not undefined. It is
implementation-defined.
> In particular the compiler may assume the
> error will never occur and elide the check.
Perhaps if the compiler only supports the C standard it can do this
(the "need not" wording allows arbitrary clobbering of errno, just like
for any function that is not specified to only touch errno for a
restricted set of actual errors, so compilers can assume that errno
is indeterminate even if it was explicityly set before the call). But
this wouldn't be compatible with the man page.
> Ideally this call is
> replaced with one of the stro* functions.
>
>> + if (errno == ERANGE || errno == EINVAL)
>> + errx(EX_DATAERR, "gid %s is invalid", a_gid->val);
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121201113232.T903>