Date: Sat, 5 Dec 2009 22:47:52 +0100 From: Jilles Tjoelker <jilles@stack.nl> To: "Sean C. Farley" <scf@FreeBSD.org> Cc: freebsd-current@FreeBSD.org Subject: Re: environ function patch for review Message-ID: <20091205214752.GA45968@stack.nl> In-Reply-To: <alpine.BSF.2.00.0912041131390.50034@thor.farley.org> References: <alpine.BSF.2.00.0912031730030.18865@thor.farley.org> <20091204161031.GA37372@stack.nl> <alpine.BSF.2.00.0912041131390.50034@thor.farley.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20091205214752.GA45968>