Re: environ function patch for review

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.

> 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.

> 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?

Sean
   1. http://opengroup.org/onlinepubs/009695399/functions/getenv.html
-- 
scf_at_FreeBSD.org