[PATCH] capabilities not inherited

[PATCH] capabilities not inherited

[Manfred Georg]

Hi,

I was working with passing capabilities through an exec and it
didn't do what I expected it to.  That is, if I set a bit in
the inherited capabilities, it is not "inherited" after an
exec().  After going through the code many times, and still not
understanding it, I hacked together this patch.  It probably
has unforseen side effects and there was probably some
reason it was not done in the first place.

Thanks for the kernel, I have a new found appreciation for it.

Manfred

Patch against 2.6.12-rc6:

Signed-off-by: Manfred Georg <mgeorg@arl.wustl.edu>

diff -uprN -X dontdiff linux-2.6.12-rc6/security/commoncap.c linux/security/commoncap.c
--- linux-2.6.12-rc6/security/commoncap.c	2005-03-02 01:38:07.000000000 -0600
+++ linux/security/commoncap.c	2005-06-08 14:02:21.000000000 -0500
@@ -113,10 +113,11 @@ int cap_bprm_set_security (struct linux_
 {
 	/* Copied from fs/exec.c:prepare_binprm. */

-	/* We don't have VFS support for capabilities yet */
-	cap_clear (bprm->cap_inheritable);
-	cap_clear (bprm->cap_permitted);
-	cap_clear (bprm->cap_effective);
+	bprm->cap_inheritable = current->cap_inheritable;
+	bprm->cap_permitted = cap_intersect(current->cap_inheritable,
+	                                    current->cap_permitted);
+	bprm->cap_effective = cap_intersect(bprm->cap_permitted,
+	                                    current->cap_effective);

 	/*  To support inheritance of root-permissions and suid-root
 	 *  executables under compatibility mode, we raise all three

Re: [PATCH] capabilities not inherited

[Alexander Nyberg]

ons 2005-06-08 klockan 15:27 -0500 skrev Manfred Georg:
> Hi,
> 
> I was working with passing capabilities through an exec and it
> didn't do what I expected it to.  That is, if I set a bit in
> the inherited capabilities, it is not "inherited" after an
> exec().  After going through the code many times, and still not
> understanding it, I hacked together this patch.  It probably
> has unforseen side effects and there was probably some
> reason it was not done in the first place.

Please read the thread at
http://www.ussg.iu.edu/hypermail/linux/kernel/0503.1/1571.html

Basically it says that because a broken interface was accepted from the
beginning it can't be changed due to the security aspect.

The whole thing sucks, sorry.

Re: [PATCH] capabilities not inherited

[Manfred Georg]

On Wed, 8 Jun 2005, Alexander Nyberg wrote:

> ons 2005-06-08 klockan 15:27 -0500 skrev Manfred Georg:
>> I was working with passing capabilities through an exec and it
>> didn't do what I expected it to.  That is, if I set a bit in
>> the inherited capabilities, it is not "inherited" after an
>> exec().  After going through the code many times, and still not
>> understanding it, I hacked together this patch.  It probably
>> has unforseen side effects and there was probably some
>> reason it was not done in the first place.
>
> Please read the thread at
> http://www.ussg.iu.edu/hypermail/linux/kernel/0503.1/1571.html
>
> Basically it says that because a broken interface was accepted from the
> beginning it can't be changed due to the security aspect.

Ok, that's what I figured, however, there seems to be some framework
for configuring different security modules.  Why isn't there one
that enables the non-broken interface?  feature creep?

> The whole thing sucks, sorry.
yep. :(
Especially since the current functionality doesn't make the
system any more secure.

Manfred

Re: [PATCH] capabilities not inherited

[Chris Wright]

* Manfred Georg (mgeorg@arl.wustl.edu) wrote:
> I was working with passing capabilities through an exec and it
> didn't do what I expected it to.  That is, if I set a bit in
> the inherited capabilities, it is not "inherited" after an
> exec().  After going through the code many times, and still not
> understanding it, I hacked together this patch.  It probably
> has unforseen side effects and there was probably some
> reason it was not done in the first place.

True to both.  If you'd like to work with this, check the archives for
similar patches.  Most recent in a thread from Alex Nyberg starting
here:

http://marc.theaimsgroup.com/?l=linux-kernel&m=111062795600730&w=2

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] capabilities not inherited

[Alexander Nyberg]

ons 2005-06-08 klockan 13:44 -0700 skrev Chris Wright:
> * Manfred Georg (mgeorg@arl.wustl.edu) wrote:
> > I was working with passing capabilities through an exec and it
> > didn't do what I expected it to.  That is, if I set a bit in
> > the inherited capabilities, it is not "inherited" after an
> > exec().  After going through the code many times, and still not
> > understanding it, I hacked together this patch.  It probably
> > has unforseen side effects and there was probably some
> > reason it was not done in the first place.
> 
> True to both.  If you'd like to work with this, check the archives for
> similar patches.  Most recent in a thread from Alex Nyberg starting
> here:
> 
> http://marc.theaimsgroup.com/?l=linux-kernel&m=111062795600730&w=2
> 

btw since the last discussion was about not changing the existing
interface and thus exposing security flaws, what about introducing
another prctrl that says maybe PRCTRL_ACROSS_EXECVE?

Any new user-space applications must understand the implications of
using it so it's safe in that aspect. Yes?

(yeah it's rather silly since there already is an unused
keep_capabilities flag but that would change old interfaces so ok)

Re: [PATCH] capabilities not inherited

[Manfred Georg]

On Wed, 8 Jun 2005, Alexander Nyberg wrote:
> btw since the last discussion was about not changing the existing
> interface and thus exposing security flaws, what about introducing
> another prctrl that says maybe PRCTRL_ACROSS_EXECVE?

Wasn't the original inherited set supposed take care of that?

> Any new user-space applications must understand the implications of
> using it so it's safe in that aspect. Yes?

As far as I can tell, applying the patch from the earlier discussion
and setting the inherited set has the same, "I really meant to do this"
effect as what you propose.

> (yeah it's rather silly since there already is an unused
> keep_capabilities flag but that would change old interfaces so ok)

Isn't the keep_capabilities flag related to setuid() ? or did I miss
something.

Manfred

Re: [PATCH] capabilities not inherited

[Alexander Nyberg]

ons 2005-06-08 klockan 16:33 -0500 skrev Manfred Georg:
> On Wed, 8 Jun 2005, Alexander Nyberg wrote:
> > btw since the last discussion was about not changing the existing
> > interface and thus exposing security flaws, what about introducing
> > another prctrl that says maybe PRCTRL_ACROSS_EXECVE?
> 
> Wasn't the original inherited set supposed take care of that?

Yes

> > Any new user-space applications must understand the implications of
> > using it so it's safe in that aspect. Yes?
> 
> As far as I can tell, applying the patch from the earlier discussion
> and setting the inherited set has the same, "I really meant to do this"
> effect as what you propose.

But since it didn't do what it was supposed to from the beginning some
people don't want to change it because it might open up security holes
and I can understand that. So maybe we need some new flag for
applications to say "I really want to do this" and noone can say we
break old interfaces (mess upon mess...)

> > (yeah it's rather silly since there already is an unused
> > keep_capabilities flag but that would change old interfaces so ok)
> 
> Isn't the keep_capabilities flag related to setuid() ? or did I miss
> something.
> 

Yeah it's related to changing user IDs, it's me who is lost, sorry.

And I still think having apps being able to keep a chain of capabilities
would be very useful, most noticeable pam_cap so that normal users can
have capabilities like chroot, running SCHED_FIFO or SCHED_RR, mlock
etc. But let's see what Chris has to say about another flag for explicit
inheritance.

Re: [PATCH] capabilities not inherited

[Chris Wright]

* Manfred Georg (mgeorg@arl.wustl.edu) wrote:
> 
> On Wed, 8 Jun 2005, Alexander Nyberg wrote:
> >btw since the last discussion was about not changing the existing
> >interface and thus exposing security flaws, what about introducing
> >another prctrl that says maybe PRCTRL_ACROSS_EXECVE?
> 
> Wasn't the original inherited set supposed take care of that?

The filesystem part was quite integral to the original intent.

> >Any new user-space applications must understand the implications of
> >using it so it's safe in that aspect. Yes?
> 
> As far as I can tell, applying the patch from the earlier discussion
> and setting the inherited set has the same, "I really meant to do this"
> effect as what you propose.
> 
> >(yeah it's rather silly since there already is an unused
> >keep_capabilities flag but that would change old interfaces so ok)
> 
> Isn't the keep_capabilities flag related to setuid() ? or did I miss
> something.

Yes, it is, but it's tempting to reuse to really keep them. I think
that's the point.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] capabilities not inherited

[Chris Wright]

* Alexander Nyberg (alexn@telia.com) wrote:
> btw since the last discussion was about not changing the existing
> interface and thus exposing security flaws, what about introducing
> another prctrl that says maybe PRCTRL_ACROSS_EXECVE?

It's not ideal (as you mention, mess upon mess), but maybe it is the
sanest way to go forward.

> Any new user-space applications must understand the implications of
> using it so it's safe in that aspect. Yes?

At least less-likely to surprise ;-)

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] capabilities not inherited

[Lee Revell]

On Wed, 2005-06-08 at 14:59 -0700, Chris Wright wrote:
> * Alexander Nyberg (alexn@telia.com) wrote:
> > btw since the last discussion was about not changing the existing
> > interface and thus exposing security flaws, what about introducing
> > another prctrl that says maybe PRCTRL_ACROSS_EXECVE?
> 
> It's not ideal (as you mention, mess upon mess), but maybe it is the
> sanest way to go forward.
> 
> > Any new user-space applications must understand the implications of
> > using it so it's safe in that aspect. Yes?
> 
> At least less-likely to surprise ;-)

Any new user-space application developers that think about using
capabilities for anything should run away screaming.  When the JACK
developers proposed extending the mechanism to meet our needs, we were
basically told the capabilities subsystem is deeply broken and that we'd
have to rewrite the subsystem to do anything useful.  We ended up
shoehorning LSM and finally rlimits into doing what we need.  Please see
various "realtime LSM" threads for more (a LOT more) on the topic.

Lee

Re: [PATCH] capabilities not inherited

[David Wagner]

Alexander Nyberg  wrote:
>btw since the last discussion was about not changing the existing
>interface and thus exposing security flaws, what about introducing
>another prctrl that says maybe PRCTRL_ACROSS_EXECVE?

Not sure if I understand the semantics you are proposing.

I remember that the sendmail attack involved the attacker clearing
its SETUID capability bit, then execing sendmail.  Sendmail, the victim,
got executed with fewer capabilities than it expected, and this caused it
to fail (in particular, sendmail's attempt to drop privileges silently
failed) -- leading to a security hole.  Will your proposal prevent such
attacks?  I'm worried.

>Any new user-space applications must understand the implications of
>using it so it's safe in that aspect. Yes?

Not clear.  Suppose Alice exec()s Bob.  

Does your scheme protect Alice against a malicious Bob?  Yes, because
Alice has to know about PRCTRL_ACROSS_EXECVE to use it.

Does your scheme protect Bob against a malicious Alice?  Not clear.
If Alice is the only who has to set PRCTRL_ACROSS_EXECVE, then Bob might
not know about this flag and thus might be surprised by the implicatiohns
of this flag.  Consequently, I can imagine this flag might allow Alice
to attack Bob by exec()ing Bob with a different set of capabilities than
Bob was expecting.  Does this sound right?

But maybe I'm not thinking clearly enough about this.  This is tricky
stuff.

Re: [PATCH] capabilities not inherited

[Alexander Nyberg]

tor 2005-06-09 klockan 02:59 +0000 skrev David Wagner:
> Alexander Nyberg  wrote:
> >btw since the last discussion was about not changing the existing
> >interface and thus exposing security flaws, what about introducing
> >another prctrl that says maybe PRCTRL_ACROSS_EXECVE?
> 
> Not sure if I understand the semantics you are proposing.
> 
> I remember that the sendmail attack involved the attacker clearing
> its SETUID capability bit, then execing sendmail.  Sendmail, the victim,
> got executed with fewer capabilities than it expected, and this caused it
> to fail (in particular, sendmail's attempt to drop privileges silently
> failed) -- leading to a security hole.  Will your proposal prevent such
> attacks?  I'm worried.

I'll look this up but it sounds very weird and I don't see how this
would happen with this change.

> >Any new user-space applications must understand the implications of
> >using it so it's safe in that aspect. Yes?
> 
> Not clear.  Suppose Alice exec()s Bob.  
> 
> Does your scheme protect Alice against a malicious Bob?  Yes, because
> Alice has to know about PRCTRL_ACROSS_EXECVE to use it.

Yeah but for capabilities to be useful it needs to be recursive too, ie.
Alice runs Bob that in turn runs Joe. Joe should now have the
capabilities that Alice had if Alice had set PCRTL_ACROSS_EXECVE and Bob
did not drop the PRCTL flag nor zeroed the inheritable mask.

> Does your scheme protect Bob against a malicious Alice?  Not clear.
> If Alice is the only who has to set PRCTRL_ACROSS_EXECVE, then Bob might
> not know about this flag and thus might be surprised by the implicatiohns
> of this flag.  Consequently, I can imagine this flag might allow Alice
> to attack Bob by exec()ing Bob with a different set of capabilities than
> Bob was expecting.  Does this sound right?

That would require Alice to already have the capabilities. No extra
capabilities are gained by exec'ing, it's only about inheriting the
existing ones.

Re: [PATCH] capabilities not inherited

[David Wagner]

Alexander Nyberg  wrote:
>tor 2005-06-09 klockan 02:59 +0000 skrev David Wagner:
>> [...] the sendmail attack [...]
>
>I'll look this up but it sounds very weird and I don't see how this
>would happen with this change.

Yup, it was a weird one indeed -- which is part of why I'm concerned.
Take a look at the attack again, then re-read my message.  Maybe my
concerns will make more sense once you refresh your memory about the
setuid capabilities attack?  If not, feel free to ask again, and I'll
try to elaborate.  Here is a pointer to one description of that attack:
    http://www.cs.berkeley.edu/~daw/papers/setuid-usenix02.pdf
    (jump straight to Section 7.1) 

Re: [PATCH] capabilities not inherited

[Lee Revell]

On Thu, 2005-06-09 at 14:55 +0000, David Wagner wrote:
> Alexander Nyberg  wrote:
> >tor 2005-06-09 klockan 02:59 +0000 skrev David Wagner:
> >> [...] the sendmail attack [...]
> >
> >I'll look this up but it sounds very weird and I don't see how this
> >would happen with this change.
> 
> Yup, it was a weird one indeed -- which is part of why I'm concerned.
> Take a look at the attack again, then re-read my message.  Maybe my
> concerns will make more sense once you refresh your memory about the
> setuid capabilities attack?  If not, feel free to ask again, and I'll
> try to elaborate.  Here is a pointer to one description of that attack:
>     http://www.cs.berkeley.edu/~daw/papers/setuid-usenix02.pdf
>     (jump straight to Section 7.1) 

Thanks for the link, I wish I had that during the realtime LSM debate,
when people were actually recommending that jackd use setuid to grant RT
scheduling ability to clients.

Lee