mailing list of musl libc
 help / color / mirror / code / Atom feed
* LD_PRELOAD and RTLD_NEXT support
@ 2011-08-16  5:17 Rich Felker
  2011-08-16  6:34 ` Vasiliy Kulikov
  0 siblings, 1 reply; 7+ messages in thread
From: Rich Felker @ 2011-08-16  5:17 UTC (permalink / raw)
  To: musl

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: LD_PRELOAD and RTLD_NEXT support
  2011-08-16  5:17 LD_PRELOAD and RTLD_NEXT support Rich Felker
@ 2011-08-16  6:34 ` Vasiliy Kulikov
  2011-08-16 11:47   ` Rich Felker
  0 siblings, 1 reply; 7+ messages in thread
From: Vasiliy Kulikov @ 2011-08-16  6:34 UTC (permalink / raw)
  To: musl

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: LD_PRELOAD and RTLD_NEXT support
  2011-08-16  6:34 ` Vasiliy Kulikov
@ 2011-08-16 11:47   ` Rich Felker
  2011-08-16 12:46     ` Vasiliy Kulikov
  0 siblings, 1 reply; 7+ messages in thread
From: Rich Felker @ 2011-08-16 11:47 UTC (permalink / raw)
  To: musl

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: LD_PRELOAD and RTLD_NEXT support
  2011-08-16 11:47   ` Rich Felker
@ 2011-08-16 12:46     ` Vasiliy Kulikov
  2011-08-16 12:59       ` Rich Felker
  2011-08-22 17:02       ` env vars in SUID/SGID programs (was: LD_PRELOAD and RTLD_NEXT support) Solar Designer
  0 siblings, 2 replies; 7+ messages in thread
From: Vasiliy Kulikov @ 2011-08-16 12:46 UTC (permalink / raw)
  To: musl

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: LD_PRELOAD and RTLD_NEXT support
  2011-08-16 12:46     ` Vasiliy Kulikov
@ 2011-08-16 12:59       ` Rich Felker
  2011-08-22 17:02       ` env vars in SUID/SGID programs (was: LD_PRELOAD and RTLD_NEXT support) Solar Designer
  1 sibling, 0 replies; 7+ messages in thread
From: Rich Felker @ 2011-08-16 12:59 UTC (permalink / raw)
  To: musl

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* env vars in SUID/SGID programs (was: LD_PRELOAD and RTLD_NEXT support)
  2011-08-16 12:46     ` Vasiliy Kulikov
  2011-08-16 12:59       ` Rich Felker
@ 2011-08-22 17:02       ` Solar Designer
  2011-08-22 18:13         ` Rich Felker
  1 sibling, 1 reply; 7+ messages in thread
From: Solar Designer @ 2011-08-22 17:02 UTC (permalink / raw)
  To: musl

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: env vars in SUID/SGID programs (was: LD_PRELOAD and RTLD_NEXT support)
  2011-08-22 17:02       ` env vars in SUID/SGID programs (was: LD_PRELOAD and RTLD_NEXT support) Solar Designer
@ 2011-08-22 18:13         ` Rich Felker
  0 siblings, 0 replies; 7+ messages in thread
From: Rich Felker @ 2011-08-22 18:13 UTC (permalink / raw)
  To: musl

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2011-08-22 18:13 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-08-16  5:17 LD_PRELOAD and RTLD_NEXT support Rich Felker
2011-08-16  6:34 ` Vasiliy Kulikov
2011-08-16 11:47   ` Rich Felker
2011-08-16 12:46     ` Vasiliy Kulikov
2011-08-16 12:59       ` Rich Felker
2011-08-22 17:02       ` env vars in SUID/SGID programs (was: LD_PRELOAD and RTLD_NEXT support) Solar Designer
2011-08-22 18:13         ` Rich Felker

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).