Date:	Sat, 14 Mar 2015 14:09:47 -0700
From:	"Andrew G. Morgan" <morgan@...nel.org>
To:	Andy Lutomirski <luto@...capital.net>
Cc:	Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
	"Ted Ts'o" <tytso@....edu>, Andrew Lutomirski <luto@...nel.org>,
	Andrew Morton <akpm@...uxfoundation.org>,
	Michael Kerrisk <mtk.manpages@...il.com>,
	Mimi Zohar <zohar@...ux.vnet.ibm.com>,
	Linux API <linux-api@...r.kernel.org>,
	Austin S Hemmelgarn <ahferroin7@...il.com>,
	linux-security-module <linux-security-module@...r.kernel.org>,
	Aaron Jones <aaronmdjones@...il.com>,
	Christoph Lameter <cl@...ux.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Serge Hallyn <serge.hallyn@...onical.com>,
	Markku Savela <msa@...h.iki.fi>,
	Kees Cook <keescook@...omium.org>,
	Jonathan Corbet <corbet@....net>
Subject: Re: [RFC] capabilities: Ambient capabilities

On Fri, Mar 13, 2015 at 10:57 AM, Andy Lutomirski <luto@...capital.net> wrote:
> On Mar 13, 2015 6:24 AM, "Andrew G. Morgan" <morgan@...nel.org> wrote:
>>
>> > It's to preserve the invariant that pA is always a subset of pI.
>>
>> But since a user can always raise a bit in pI if it is present in pP,
>> what does this invariant add to your model other than inconvenience?
>
> The useful part is that dropping a bit from pI also drops it from pA.
> To keep the model consistent, I also require that you add the bit to
> pI before adding it to pA.

So you are saying that pA is always a strict subset of pI (and pP)?
Then why not explicitly implement it as:

  pA' = (file caps or setuid or setgid ? 0 : pA)
  pP' = (fP & X) | (pI & [fI | (pA' & pP)] )

As it is you have so distributed these constraints that it is hard to
be sure it will remain that way.

>> >> I'm also unclear how you can turn off this new 'feature' for a process
>> >> tree? As it is, the code creates an exploit path for a capable (pP !=
>> >> 0) program with an exploitable flaw to create a privilege escalation
>> >> for an arbitrary child program.
>> >
>> > Huh?  If you exploit the parent, you already win.  Yes, if a kiddie
>> > injects shellcode that does system("/bin/bash") into some pP != 0
>> > program, they don't actually elevate their privileges.  On the other
>> > hand, by the time an attacker injected shellcode for:
>> >
>> > prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_RAISE, CAP_SYS_ADMIN);
>> > system("/bin/bash");
>>
>> Let's call the above two lines [a] and [b]. With this patch, you are
>> encouraging folk to write programs that contain a line like [a]
>> already. So, yes, I am saying that you are creating an exploitable
>> path in these programs that says if someone can inject
>> system("/bin/bash") into these programs they can get a new (because of
>> this patch) privilege escalation.
>>
>> In the prevailing model, this kind of privilege escalation (resulting
>> from naive inheritance) is designed out. I recognize that you want to
>> add it back in, but I am concerned that you are not allowing for the
>> possibility that some folk might still want to still be able to
>> prevent it.
>
> If you have a program that deliberately uses PR_CAP_AMBIENT, then
> setting such a securebit will break the program, so it still doesn't
> buy you anything.

Not if you make the bit lockable (like the other bits). If you want to
run with your model in effect, you lock the enable bit on.

>> > into a target, they can already do whatever they want.
>> >
>> >> While I understand that everyone
>> >> 'knows what they are doing' in implementing this change, I'm convinced
>> >> that folk that are up to no good also do... Why not provide a lockable
>> >> secure bit to selectively disable this support?
>> >
>> > Show me a legitimate use case and I'll gladly implement a secure bit.
>>
>> Thanks. I was kind of hoping that you would add a lockable secure bit
>> that defaults this support to off, but can be used to turn it on with
>> or without a lock. That way, you can avoid disturbing the legacy
>> defaults (no surprises).
>
> I think this thing needs to default on to be useful.
>
>>
>> > In the mean time, I don't even believe that there's a legitimate use
>> > for any of the other secure bits (except keepcaps, and I don't know
>> > why that's a securebit in the first place).
>>
>> Those bits currently make it possible to run a subsystem with no
>> [set]uid-0 support in its process tree.
>
> Not usefully, because even with all the securebits set to their
> non-legacy modes, caps don't inherit, so it doesn't work.  I've tried.

Not sure I follow. They work for a definition if inheritable that you
seem to refuse to accept.

>> > In the mean time, see CVE-2014-3215 for an example of why securebits
>> > are probably more trouble than they're worth.
>>
>> I think it is safe to say that naive privilege inheritance has a fair
>> track record of being exploited orders of magnitude more frequently
>> than this. After all, these are the reasons LD_PRELOAD and shell
>> script setuid bits are suppressed.
>
> I don't know what you mean here by naive privilege inheritance.  The
> examples you're taking about aren't inheritance at all; they're
> exploring privilege *grants* during execve.  My patch deliberately
> leaves grants like that alone.

The pI set is inherited through this exec unmolested. You are adding a
pA set that is passed through exec somewhat unmolested but allows all
descendants to have privilege.

- Naive inheritance is 'if you had a privilege pre-exec any descendant
gets it post-exec'. This is what your pA achieves.

- Inheritance, as it is currently implemented, is implemented as a
handshake between a privileged ancestor (who added a pI bit) and a
descendant that is ready for it (has a fI bit to match).
  - the un-naive aspect (if you prefer 'selective' inheritance) of
this is that only binaries with file capabilities can ever wield
privilege, and while it can be suppressed by an intermediate
descendant the grant of inherited privilege can skip generations.

I think we can agree that the only place that inheritance can occur is
through an exec. So 'grants' through exec is sort of a semantic bit of
smoke masquerading as an argument. Even in the naive inheritance case
you have to explicitly grant some privilege - you are just choosing
different rules.

My Nack remains that you are eliminating the explicit enforcement of
selective inheritance. A lockable secure bit protecting access to your
prctl() function would address this concern.

Thanks

Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists