svn commit: r199983 - in head: lib/libc/stdlib tools/regression/environ

[Sean C. Farley]

On Tue, 1 Dec 2009, M. Warner Losh wrote:

> In message: <alpine.BSF.2.00.0912011514510.84941 at fledge.watson.org>
>            Robert Watson <rwatson at FreeBSD.org> writes:
> : On Mon, 30 Nov 2009, Colin Percival wrote:
> :
> : > Brian Feldman wrote:
> : >>   Do not gratuitously fail *env(3) operations due to corrupt ('='-less)
> : >>   **environ entries.  This puts non-getenv(3) operations in line with
> : >>   getenv(3) in that bad environ entries do not cause all operations to
> : >>   fail.  There is still some inconsistency in that getenv(3) in the
> : >>   absence of any environment-modifying operation does not emit corrupt
> : >>   environ entry warnings.
> : >>
> : >>   I also fixed another inconsistency in getenv(3) where updating the
> : >>   global environ pointer would not be reflected in the return values.
> : >>   It would have taken an intermediary setenv(3)/putenv(3)/unsetenv(3)
> : >>   in order to see the change.
> : >
> : > The FreeBSD Security Team is currently dealing with a security 
> : > issue relating to this code.  Please back out your change (at 
> : > least to getenv.c; I don't particularly care about the regression 
> : > tests) until we've finished, and then submit the patch to us for 
> : > review along with a detailed explanation of what it does.
> : >
> : > We've already had two major security issues arising out of 
> : > getenv.c in the past year, and I'd like to make sure we don't have 
> : > a third.
> :
> : I think it's fair to say that the POSIXization of the environment 
> : code has been an unmitigated disaster, and speaks to the necessity 
> : for careful review of those sorts of code changes.
>
> Why we're not just reverting the whole thing as a bad idea is beyond 
> me.  Clearly the tiny incremental benefits have been far overshadowed 
> by this fiasco.

Which "whole thing"?  The code or the POSIX-compliance?  Technically, it 
is not pure compliance because the code has a few BSD requirements in it 
such as keeping old name=value entries even when new ones are created.

It was my fault for not checking how unsetenv() was used in all of base. 
The change to use unsetenv() in rtld.c was committed just prior to my 
change which introduced a version of unsetenv() that returned an int to 
allow checking.

I am testing a change to have unsetenv() not stop in its attempt to 
unset a variable which should mimic the old behavior.

One difference between our man page and IEEE Std 1003.1-2008 is the part 
concerning that the "environment shall be unchanged" which I will 
introduce to the man page:

     Upon successful completion, zero shall be returned. Otherwise, -1
     shall be returned, errno set to indicate the error, and the
     environment shall be unchanged.

After my change, unsetenv() will not return an error if environ is 
corrupt.  It will wipe the variable.  The only time errors will be 
returned are when the name passed to unsetenv() is invalid or when 
memory allocation fails.

One question is whether the code should abort() when it detects a 
corrupt environ array or do its best to complete the request from the 
caller.

Sean
-- 
scf at FreeBSD.org