From owner-freebsd-current@FreeBSD.ORG Sat Dec 5 21:47:56 2009 Return-Path: Delivered-To: freebsd-current@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 3359F106566B; Sat, 5 Dec 2009 21:47:56 +0000 (UTC) (envelope-from jilles@crab.stack.nl) Received: from crab.stack.nl (crab.stack.nl [131.155.140.134]) by mx1.freebsd.org (Postfix) with ESMTP id BDAC08FC08; Sat, 5 Dec 2009 21:47:55 +0000 (UTC) Received: by crab.stack.nl (Postfix, from userid 1677) id 73FAD5C43; Sat, 5 Dec 2009 22:47:52 +0100 (CET) Date: Sat, 5 Dec 2009 22:47:52 +0100 From: Jilles Tjoelker To: "Sean C. Farley" Message-ID: <20091205214752.GA45968@stack.nl> References: <20091204161031.GA37372@stack.nl> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.20 (2009-06-14) Cc: freebsd-current@FreeBSD.org Subject: Re: environ function patch for review X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 05 Dec 2009 21:47:56 -0000 On Fri, Dec 04, 2009 at 12:02:25PM -0600, Sean C. Farley wrote: > On Fri, 4 Dec 2009, Jilles Tjoelker wrote: > > On Thu, Dec 03, 2009 at 06:04:47PM -0600, Sean C. Farley wrote: > >> Regarding the recent security issue with the unsetenv() calls in > >> rtld, I have made a patch[1] I would like reviewed prior to commit. > >> It changes the behavior of all the *env() routines that cause an > >> internal environment to be created. This is putenv(), setenv() and > >> unsetenv(). getenv() will not cause an internal environment to be > >> created. I have tested the patch without the rltd fix, and it > >> prevents the security issue. > >> Instead of returning an error when tripping upon a corrupt > >> environment, it will return an error when the caller passes bad > >> argument(s) (EINVAL) or if unable to allocate memory (ENOMEM). > >> Except for the possibility for ENOMEM, this should make the behavior > >> the same as FreeBSD 6 and below. > > It would be very nice to avoid ENOMEM on unsetenv() somehow, such as > > by rewriting environ in place. Neither FreeBSD6 nor POSIX mention the > > possibility of ENOMEM on unsetenv(). The only error listed by POSIX is > > an invalid variable name (unsetting a variable that does not exist is > > not an error), so it seems reasonable to assume unsetenv() is > > successful if passed a valid constant variable name. > I agree that they do not mention the possibility of ENOMEM with > unsetenv() but only in the unsetenv() man pages. From OpenGroup's > getenv() page[1], there is a constraint of a conforming application not > altering environ. The "rationale" section states: > This constraint allows the implementation to properly manage the > memory it allocates, either by using allocated storage for all > variables (copying them on the first invocation of setenv() or > unsetenv()), ... > From this, it appears that unsetenv() is allowed to set errno to ENOMEM. > At least, it should be able to do that from that rationale. That POSIX allows something does not mean it is a good idea. > > If unsetenv() has to copy the environment, there cannot be any > > setenv() or putenv() strings in it. So it seems correct to remove the > > pointer from environ without doing anything else, instead. > I can add this, however, I was trying to keep the complexity down a bit. > Since the copying would normally only occur once in the lifetime of an > application and not necessarily by unsetenv(), most of the time > unsetenv() will take that code path one time if at all. But that one time may be when trying to unset a possibly dangerous environment variable... I cannot exclude the possibility of setting up rlimits and environment such that the environment cannot be copied while the exploit still works. > > If the environment has already been copied, the unsetenv() should > > proceed without needing to allocate any memory. This seems to be the > > case. > Yes, unsetenv() only allocates memory upon its first call using an > environ that has not been previously copied. > How did the patches look otherwise, or are you suggesting this change to > unsetenv() in place of the patch to continue even after discovering a > corrupt environ? I think that patch is needed as well, as setenv()/putenv() should probably not fail for petty reasons either (although they can always fail because of ENOMEM). -- Jilles Tjoelker