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>