Re: Need help in writing a ChangeLog

["Carlos O'Donell" <carlos at systemhalted dot org>]

On Wed, Nov 24, 2010 at 5:57 AM, Pavel Labushev <p.labushev@gmail.com> wrote:
> Thank you for the reply, Carlos.
>
>
> ChangeLog:
>
> 2010-11-19 ?Pavel Labushev ?<p.labushev@gmail.com>
>
> ? ? ? ?* elf/dl-environ.c (_dl_env_sanitize): New function.
> ? ? ? ?* elf/dl-support.c (_dl_non_dynamic_init): If __libc_enable_secure
> ? ? ? ?then sanitize the environment.
> ? ? ? ?* elf/Makefile: add environ to dl-routines list.
> ? ? ? ?* elf/rtld.c (process_envvars): If __libc_enable_secure
> ? ? ? ?then sanitize the environment.
> ? ? ? ?* sysdeps/generic/ldsodefs.h: Add prototype for _dl_env_sanitize.
> ? ? ? ?* sysdeps/generic/unsecvars.h: Define PRIVENV_WHITELIST_FILE and
> ? ? ? ?PRIVENV_BLACKLIST_FILE.
>

Looks great.

> Description:
>
> This patch implements whitelist or blacklist based system-wide environment
> sanitization for suid/sgid processes under centralized control of software
> distributions or system administrators.
>
> The goal is to mitigate the security impact of bugs in any environment
> handling code (that potentially could be exploited in a context of some
> suid/sgid process) by removing every (not) listed environment variable
> before it would be handled.
>
> The patch is mostly based on the existing code in:
> * elf/dl-environ.c (unsetenv)
> * elf/rtld.c (dl_main: the /etc/ld.so.preload parsing code)
>
> How it works:
>
> If the whitelist file /etc/privenv-allow exists and is readable, a suid/sgid
> process unsets every variable if its name is not listed in that file.
>
> Otherwise, if the blacklist file /etc/privenv-deny exists and is readable, a
> suid/sgid process unsets every variable if its name is in the blacklist.
>
> If the file is readable but cannot be read, that could mean an attacker did
> lower RLIMIT_NOFILE or open too much file descriptors inherited after
> execve(2) to avoid the environment sanitization. In such case a suid/sgid
> process performs _exit(EACCES).
>
> (However, an empty file also could not be mapped and read, which, I think,
> is a corner case and should be avoided by writing a single whitespace
> character or a comment like "# Do not remove this line." to the file.
> Another way is to modify _dl_sysdep_read_whole_file function so it would set
> errno to distinguish open(2) and mmap(2) failures. I'm not sure if it's more
> acceptable solution, thus I didn't implement it, but I would in case it is.)
>
> There's no performance or any runtime impact for non-suid/sgid processes.
>
> For suid/sgid processes the performance impact is minimal and comparable to
> the one for parsing of /etc/ld.so.preload and the UNSECURE_ENVVARS
> filtering. If no whitelist nor blacklist file exist, the overhead is only
> two access(2) syscalls.
>
> The sanitization code is invoked very close to the similar UNSECURE_ENVVARS
> filtering code, but before the LD_* environment variables handling code.
> That makes it possible to mitigate the security impact of bugs like the
> recent ones disclosed by Tavis Ormandy in the LD_AUDIT handling code.
>
> The contents of whitelist and blacklist files can be changed by system
> administrators without rebuilding glibc. Therefore they could do more strict
> environment sanitization and change the white/blacklist file contents on
> demand, which is not the case with UNSECURE_ENVVARS.

This is an excellent write up of the description.

You patch lacks two things which are required for changes like this:

* Documentation update.

* Regression test.

We need to tell users how to use it, and prove that it still works
after each release.

>> Have you considered that this could be done in the kernel before the
>> process even starts? As it stands, static programs that have not been
>> rebuilt with this patch are still vulnerable? If you fixed this in the
>> kernel, then it fixes even the old static programs.
>>
>> One might argue that environment sanitation is part of the kernel
>> security services?
>
> The kernel security people I consulted with say it should be in userspace
> because:
> * The kernel haven't to have special cases for environment variables that
> are defined and implemented in userspace.
> * It's more expensive for the kernel to do it, because it has to copy all
> environment variables to the kernel just to inspect them.
>
> And I would add that glibc is supposed to be running on different kernels,
> and it would require far more effort to implement the sanitization in every
> kernel.

Those are excellent arguments for a userspace glibc implementation.

The arguments should be part of your description with the patch.

>> In general you should follow all of the steps in:
>> http://sourceware.org/glibc/wiki/Contribution%20checklist
>>
>> This is significant amount of code, therefore you also need an FSF
>> copyright assignment (mentioned in the checklist).
>
> OK... I'll add copyright notices to the modified files. Should I add it to
> the files where I changed 1-3 lines of code?

In any file that you change you must update the copyright notices.

> And I would like to assign
> copyright, what should I do next?

You have several options when it comes to assigning copyright:

* Assign copyright for just this one change.

* Assign copyright for all future glibc submissions.

You will need an employer disclaimer form if a school or employer owns
work created by you.

What would you like to do?

Cheers,
Carlos.