Date: Tue, 25 Sep 2007 05:11:02 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: "Sean C. Farley" <scf@freebsd.org> Cc: =?ISO-8859-15?Q?Dag-Erling_Sm=F8rgrav?= <des@des.no>, src-committers@freebsd.org, cvs-all@freebsd.org, Bruce Evans <brde@optusnet.com.au>, cvs-src@freebsd.org Subject: Re: cvs commit: src/lib/libc/stdlib getenv.c Message-ID: <20070925044617.O54030@delplex.bde.org> In-Reply-To: <alpine.BSF.0.9999.0709220905390.52204@thor.farley.org> References: <200709220230.l8M2UiRK020609@repoman.freebsd.org> <86r6krqbrd.fsf@ds4.des.no> <20070922202914.B90809@besplex.bde.org> <alpine.BSF.0.9999.0709220905390.52204@thor.farley.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 22 Sep 2007, Sean C. Farley wrote:
> On Sat, 22 Sep 2007, Bruce Evans wrote:
>> ...
>> Partial analysis:
>> - the size_t variable must have a small value that is representable
>> as an int (else casting it to int would be a bug and/or printing a
>> line of that length would be a style bug).
>
> What would be a good maximum that would fit style? Although still
> fairly big, NL_TEXTMAX for the entire line looks plausible.
79 less the length of all other text on the line :-).
> Would the best solution be to place a cap on the value? If the value is
> less than INT_MAX, cast it to an int else pass it INT_MAX. Actually, it
I don't remember where the value comes from. If it can be from user input
then it needs to be restricted somewhere, to INT_MAX as a last resort
> looks like the value should never be greater than ARG_MAX if wanting to
> be able to call exec since according to SUSv3 that is the:
> Maximum length of argument to the exec functions including
> environment data.
ARG_MAX can in theory be enormous (too large to be returned in a long
by sysconf()), but in practice it will be much smaller than INT_MAX.
> Hopefully, no environment variables (name=value string) are anywhere
> close in size to size_t. :)
Ah I see where the value comes from. A malicous user could probably
put > INT_MAX bytes in a single string in the environment on machines
with 32-bit ints, 64 bit address space and lots of RAM, and then fork()
but not exec(). That's close enough to user input for me.
> Here is a patch (untested) to at least cast safely. How does this look?
>
> Index: getenv.c
> ===================================================================
> RCS file: /home/ncvs/src/lib/libc/stdlib/getenv.c,v
> retrieving revision 1.12
> diff -u -r1.12 getenv.c
> --- getenv.c 22 Sep 2007 02:30:44 -0000 1.12
> +++ getenv.c 22 Sep 2007 19:05:51 -0000
> @@ -356,8 +356,8 @@
> activeNdx = envVarsTotal - 1;
> if (__findenv(envVars[envNdx].name, nameLen, &activeNdx,
> false) == NULL) {
> - warnx(CorruptEnvFindMsg, (int)nameLen,
> - envVars[envNdx].name);
> + warnx(CorruptEnvFindMsg, nameLen > INT_MAX ? INT_MAX
> :
> + (int)nameLen, envVars[envNdx].name);
> errno = EFAULT;
> goto Failure;
> }
OK (I think one line is only too long/split due to misquoting).
A more refined version would use something like strvis(), and could
use a smaller limit (with long corrupt strings indicated something
likje debuggers print long binary strings) since this this is only
debugging code, but *env.c is already too large for me.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070925044617.O54030>