Re: environ function patch for review

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