From owner-cvs-src@FreeBSD.ORG Mon Sep 24 19:11:20 2007 Return-Path: Delivered-To: cvs-src@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id BDC6016A418; Mon, 24 Sep 2007 19:11:20 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail14.syd.optusnet.com.au (mail14.syd.optusnet.com.au [211.29.132.195]) by mx1.freebsd.org (Postfix) with ESMTP id 5DE2613C45D; Mon, 24 Sep 2007 19:11:20 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c220-239-235-248.carlnfd3.nsw.optusnet.com.au (c220-239-235-248.carlnfd3.nsw.optusnet.com.au [220.239.235.248]) by mail14.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id l8OJB20X002245 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 25 Sep 2007 05:11:08 +1000 Date: Tue, 25 Sep 2007 05:11:02 +1000 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: "Sean C. Farley" In-Reply-To: Message-ID: <20070925044617.O54030@delplex.bde.org> References: <200709220230.l8M2UiRK020609@repoman.freebsd.org> <86r6krqbrd.fsf@ds4.des.no> <20070922202914.B90809@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: =?ISO-8859-15?Q?Dag-Erling_Sm=F8rgrav?= , src-committers@freebsd.org, cvs-all@freebsd.org, Bruce Evans , cvs-src@freebsd.org Subject: Re: cvs commit: src/lib/libc/stdlib getenv.c X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 24 Sep 2007 19:11:20 -0000 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