LD_PRELOAD and RTLD_NEXT support

LD_PRELOAD and RTLD_NEXT support

[Rich Felker]

I've just committed support for LD_PRELOAD (fully disabled for any
suid/sgid binary) and dlsym(RTLD_NEXT, ...). I have only done basic
testing of these features with trivial test apps, so I would very much
appreciate reports of success or failure using these features with
real-world software.

Rich

Re: LD_PRELOAD and RTLD_NEXT support

[Vasiliy Kulikov]

Rich,

On Tue, Aug 16, 2011 at 01:17 -0400, Rich Felker wrote:
> (fully disabled for any suid/sgid binary)

        if ((aux[0]&0x7800)!=0x7800 || aux[AT_UID]!=aux[AT_EUID]
          || aux[AT_GID]!=aux[AT_EGID]) {

Two things here:

1) This check should be extended to support AT_SECURE (dumpable flag,
any LSM security domains, capabilities).

2) As you check for (aux[0] & 0x7800) you assume some of these elements
can be absent.  I feel it's wrong to assume you're not s*id'ed in this
case.  Instead, it's better to check for (getuid()!=geteuid() ||
getgid()!=getegid()).

Thanks,

-- 
Vasiliy

Re: LD_PRELOAD and RTLD_NEXT support

[Rich Felker]

On Tue, Aug 16, 2011 at 10:34:10AM +0400, Vasiliy Kulikov wrote:
> Rich,
> 
> On Tue, Aug 16, 2011 at 01:17 -0400, Rich Felker wrote:
> > (fully disabled for any suid/sgid binary)
> 
>         if ((aux[0]&0x7800)!=0x7800 || aux[AT_UID]!=aux[AT_EUID]
>           || aux[AT_GID]!=aux[AT_EGID]) {
> 
> Two things here:
> 
> 1) This check should be extended to support AT_SECURE (dumpable flag,
> any LSM security domains, capabilities).

Indeed, I'll add this. Do I just check for aux[AT_SECURE] != 0?

> 2) As you check for (aux[0] & 0x7800) you assume some of these elements
> can be absent.  I feel it's wrong to assume you're not s*id'ed in this

You misread the test. Absence of any of the 4 fields causes the
program to be treated as if it were suid.

> case.  Instead, it's better to check for (getuid()!=geteuid() ||
> getgid()!=getegid()).

I don't see how this helps, and this takes us down the glibc path of
abysmal startup times for every tiny program called from ./configure,
which is the reason ./configure takes so damn long...

Rich

Re: LD_PRELOAD and RTLD_NEXT support

[Vasiliy Kulikov]

On Tue, Aug 16, 2011 at 07:47 -0400, Rich Felker wrote:
> > 1) This check should be extended to support AT_SECURE (dumpable flag,
> > any LSM security domains, capabilities).
> 
> Indeed, I'll add this. Do I just check for aux[AT_SECURE] != 0?

Looks like so.

glibc has some crazy dance with these flags and get*id() values, which
we patch in Owl ;-)


> > 2) As you check for (aux[0] & 0x7800) you assume some of these elements
> > can be absent.  I feel it's wrong to assume you're not s*id'ed in this
> 
> You misread the test. Absence of any of the 4 fields causes the
> program to be treated as if it were suid.

Ah, sure.


...btw, I feel it would be cleaner if you check for untrusted environment
at the time of initializing env_* variables.  Currently there is not
much code between env_X assignment and zeroing, but it might be in the
future (with addition of ld features, etc.).

    for (p = argv+i; ... ) {
        if (is_secure_env)
            env_path = ...

-- 
Vasiliy

Re: LD_PRELOAD and RTLD_NEXT support

[Rich Felker]

On Tue, Aug 16, 2011 at 04:46:00PM +0400, Vasiliy Kulikov wrote:
> On Tue, Aug 16, 2011 at 07:47 -0400, Rich Felker wrote:
> > > 1) This check should be extended to support AT_SECURE (dumpable flag,
> > > any LSM security domains, capabilities).
> > 
> > Indeed, I'll add this. Do I just check for aux[AT_SECURE] != 0?
> 
> Looks like so.
> 
> glibc has some crazy dance with these flags and get*id() values, which
> we patch in Owl ;-)

Well glibc tries to be clever and let you use LD_PRELOAD with suid as
long as the library is in a "trusted" path and has the sgid bit set.
To me this seems really misguided; the valid use cases are very few,
and it seems impossible to predict all the future kernel
"enhancements" that might create gaping holes in whatever method you
use to validate... In my opinion, the only safe thing to do when
running with elevated privileges is to completely ignore anything the
user controls in the initial environment.

> ....btw, I feel it would be cleaner if you check for untrusted environment
> at the time of initializing env_* variables.  Currently there is not
> much code between env_X assignment and zeroing, but it might be in the
> future (with addition of ld features, etc.).
> 
>     for (p = argv+i; ... ) {
>         if (is_secure_env)
>             env_path = ...

Notice the problem is that this code is in the loop that's responsible
for *finding* auxv. For now I've just moved the code closer together,
but if you think it would help, I might first load the env vars into
temp variables and switch the conditionals for secure mode, so that
env_* would never get loaded in the suid/sgid/caps case.

Rich

env vars in SUID/SGID programs (was: LD_PRELOAD and RTLD_NEXT support)

[Solar Designer]

Rich, Vasiliy -

On Tue, Aug 16, 2011 at 04:46:00PM +0400, Vasiliy Kulikov wrote:
> ...btw, I feel it would be cleaner if you check for untrusted environment
> at the time of initializing env_* variables.  Currently there is not
> much code between env_X assignment and zeroing, but it might be in the
> future (with addition of ld features, etc.).
> 
>     for (p = argv+i; ... ) {
>         if (is_secure_env)
>             env_path = ...

I just took a look at recent musl, and I don't see any safety from env
vars used elsewhere in musl.

If a SUID/SGID program uses execvp(3), what do we want to happen?

Also, for env vars that musl ignores because of SUID/SGID'ness of the
current program, shouldn't it also unset them?  This is what we're doing
in glibc on Owl, because of sudo-like programs (where another program is
invoked next - still privileged, but no longer detected as such by libc).

I think these potential precautions I am talking about above should
apply for both static and dynamic binaries.  That is, they should be in
musl's program startup code that is used in both cases.

What do you think?

Alexander

Re: env vars in SUID/SGID programs (was: LD_PRELOAD and RTLD_NEXT support)

[Rich Felker]

On Mon, Aug 22, 2011 at 09:02:06PM +0400, Solar Designer wrote:
> Rich, Vasiliy -
> 
> On Tue, Aug 16, 2011 at 04:46:00PM +0400, Vasiliy Kulikov wrote:
> > ...btw, I feel it would be cleaner if you check for untrusted environment
> > at the time of initializing env_* variables.  Currently there is not
> > much code between env_X assignment and zeroing, but it might be in the
> > future (with addition of ld features, etc.).
> > 
> >     for (p = argv+i; ... ) {
> >         if (is_secure_env)
> >             env_path = ...
> 
> I just took a look at recent musl, and I don't see any safety from env
> vars used elsewhere in musl.
> 
> If a SUID/SGID program uses execvp(3), what do we want to happen?
> 
> Also, for env vars that musl ignores because of SUID/SGID'ness of the
> current program, shouldn't it also unset them?  This is what we're doing
> in glibc on Owl, because of sudo-like programs (where another program is
> invoked next - still privileged, but no longer detected as such by libc).
> 
> I think these potential precautions I am talking about above should
> apply for both static and dynamic binaries.  That is, they should be in
> musl's program startup code that is used in both cases.

The startup code (__libc_start_main) is the right place for this
check, and to merge nik'a vdso support for static-linked programs we
need auxv parsing here anyway, so it's minimal added cost. I just
haven't done it yet, partly for some silly micro-opt reasons that are
hard to explain -- basically there's some mildly ugly code I should
remove at the same time because there will no longer be any cases
where it could actually help, and I want to make sure that's cleaned
up right.

As for what specifically should be done, that's less clear to me.
Surely any use of environment vars (TMP?) in musl itself should be
conditional on !suid, but I don't see any allowance in the
specification for environment variables to "disappear" when an
suid/sgid program is exec'd. While unsetting them may help work around
some buggy suid programs that exec other programs without sanitizing
the environment, (1) it can't help with vars that libc doesn't know
about, and (2) it prevents legitimate, correct suid programs from
analyzing the environment and copying through or otherwise using some
security-related variables they might want to be able to see.
Especially for the normal suid behavior (just dropping privs and
running as the real uid after performing a single privileged
operation) it's desirable for the real user's environment (e.g. tmp
file location) to have effect.

Ultimately, suid programs that intend to invoke other programs which
aren't aware of having elevated privileges MUST perform the equivalent
of clearenv()/environ=0;, possibly after backing up the environment if
they need to examine it themselves... Anything else is dangerous,
whatever libc does.

Rich