wrongly auxvt in __uClibc_main

2012-11-26 Thread Filippo ARCIDIACONO

Folks,
we have seen some issues recently in uClibc related to the way the aux 
vect is managed inside __uClibc_main.


Running a set-[user,group]-ID ELF binaries with some 'unsecure' 
environment variable set (i.e LD_LIBRARY_PATH or LD_PRELOAD), auxvt is 
wrongly set in __uClibc_main.


The problem is caused by the call to _dl_unsetenv function in ldso.c 
that pops the specific env variables
from the stack adding a corresponding number of NULL entries at the end 
of the environments stack.
To be clearer, at the beginning the stack looks like as expected 
(assuming M env variables and N aux_vect entries)


argc
argv[0]
...
argv[argc-1]
NULL /* separator */
envp[0]
...
LD_LIBRARY_PATH
...
envp[M-1]
NULL /* separator */
aux_vect[0]
...
aux_vect[N-1]
NULL /* separator */

After the LD_LIBRARY_PATH has been removed by _dl_unsetenv, the stack 
will look like to libc


argc
argv[0]
...
argv[argc-1]
NULL /* separator */
envp[0]
...
envp[M-2]
NULL /* filled by ld.so */
NULL /* separator */
auxvect[0]
...
aux_vect[N-1]
NULL /* separator */

Currently, __uClibc_main assumes that there is just one NULL separator 
between environments and aux_vect entries, as from the extract below:


aux_dat = (unsigned long*)__environ;
while (*aux_dat) {
aux_dat++;
}
aux_dat++; -- This move the aux_dat of just 1 position to skip the 
NULL separator


Due to this, the local auxvt array (initialised by memset) won't be 
filled with proper values.

All the following code that use the auxvt variable is wrong.

For example, the check for the SUID binaries will be affected, as it 
needs to verify AT_UID, AT_GID, AT-EUID and AT_EGID entries (all NULL)


A potential dirty fix or work-around would be to replace aux_dat++ 
statement with while (!(*aux_dat++)) to skip all the NULL entries, but 
doing so there is no way to detect the case where aux_vect is not present.


So, the proper fix we have in mind is to completely remove the aux vect 
handling from libc and keep it solely into the dynamic linker (as 
actually glibc is doing). If libc needs to check some values from aux 
vect, it can refer to globals exported by the ld.so.


Feedback are really welcome,
Regards,
Filippo
___
uClibc mailing list
uClibc@uclibc.org
http://lists.busybox.net/mailman/listinfo/uclibc


Re: wrongly auxvt in __uClibc_main

2012-11-26 Thread Rich Felker
On Mon, Nov 26, 2012 at 04:45:18PM +0100, Filippo ARCIDIACONO wrote:
 Folks,
 we have seen some issues recently in uClibc related to the way the
 aux vect is managed inside __uClibc_main.
 
 Running a set-[user,group]-ID ELF binaries with some 'unsecure'
 environment variable set (i.e LD_LIBRARY_PATH or LD_PRELOAD), auxvt
 is wrongly set in __uClibc_main.
 
 The problem is caused by the call to _dl_unsetenv function in ldso.c
 that pops the specific env variables
 from the stack adding a corresponding number of NULL entries at the
 end of the environments stack.

I don't know if anybody wants to tackle this, but I believe the
current _dl_unsetenv behavior is non-conforming (the implementation
does not have a license to unset environment variables at will). The
correct behavior is just ignoring the variables in the current (suid)
process, but still making them available to the application which
might want to query them with getenv or pass them on to child
processes. There is some concern about the safety of the latter, but
the issue only arises if the suid program has elevated itself to
having both read and effective uid privileged. If the real/effective
ids still differ, the child will ignore the environment variables, and
if they process has fully dropped privileges down to that of the
invoking user (the original real uid), then it's appropriate that the
environment variables be honored.

I don't believe whoever originally added the _dl_unsetenv logic
thought these issues out well...

Rich
___
uClibc mailing list
uClibc@uclibc.org
http://lists.busybox.net/mailman/listinfo/uclibc


Re: wrongly auxvt in __uClibc_main

2012-11-26 Thread Carmelo AMOROSO
On 26/11/2012 20.45, Rich Felker wrote:
 On Mon, Nov 26, 2012 at 04:45:18PM +0100, Filippo ARCIDIACONO wrote:
 Folks,
 we have seen some issues recently in uClibc related to the way the
 aux vect is managed inside __uClibc_main.

 Running a set-[user,group]-ID ELF binaries with some 'unsecure'
 environment variable set (i.e LD_LIBRARY_PATH or LD_PRELOAD), auxvt
 is wrongly set in __uClibc_main.

 The problem is caused by the call to _dl_unsetenv function in ldso.c
 that pops the specific env variables
 from the stack adding a corresponding number of NULL entries at the
 end of the environments stack.
 

Hi Rich,

 I don't know if anybody wants to tackle this, but I believe the
 current _dl_unsetenv behavior is non-conforming (the implementation
 does not have a license to unset environment variables at will). The
 correct behavior is just ignoring the variables in the current (suid)
 process, but still making them available to the application which
 might want to query them with getenv or pass them on to child
 processes. There is some concern about the safety of the latter, but
 the issue only arises if the suid program has elevated itself to
 having both read and effective uid privileged. If the real/effective
 ids still differ, the child will ignore the environment variables, and
 if they process has fully dropped privileges down to that of the
 invoking user (the original real uid), then it's appropriate that the
 environment variables be honored.

I see your concerns, currently I have not a clear idea.

 
 I don't believe whoever originally added the _dl_unsetenv logic
 thought these issues out well...
 

anyway there is another case that is showing the same issue anyway. If
one of the constructor of a dependent shared libraries called the C
library unsetenv, the side effect would be the same, that is aux vect
invalid if used into the __uClibc_main.

Beside the issue with suid programs, also the actual value of the
pagesize used by the running kernel could be affected.

So I think that there are still more valid reason for redesigning this
core piece of code.

 Rich

Cheers,
Carmelo

 ___
 uClibc mailing list
 uClibc@uclibc.org
 http://lists.busybox.net/mailman/listinfo/uclibc
 

___
uClibc mailing list
uClibc@uclibc.org
http://lists.busybox.net/mailman/listinfo/uclibc