environ function patch for review
jilles at stack.nl
Sat Dec 5 21:47:56 UTC 2009
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 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, 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).
More information about the freebsd-current