LinuxLists.cc - [RFC] Implement ambient capability set.

[permalink] [raw]

Subject: Re: [RFC] Implement ambient capability set.

On Wed, Feb 4, 2015 at 10:49 AM, Christoph Lameter <[email protected]> wrote:
> An attempt to implement this. Probably missing some fine points:
>
> Subject: [capabilities] Implement ambient capability set.
>
> DRAFT -- untested -- DRAFT
>
> Implement an ambient capabilty set to allow capabilties
> to be inherited with unix semantics used also for other
> attributes.
>
> Implements PR_CAP_AMBIENT. The second argument to prctl
> is a the capability number and the third the desired state.
> 0 for off. Otherwise on.
>
> Serge:
> A new capability set, pA, is empty by default. You can
> add bits to it using prctl if ns_capable(CAP_SETPCAP) and
> all the new bits are in your pE. Once set, they stay until
> they are removed using prctl. At exec, pA' = pA, and
> fI |= pA (after reading fI from disk but before
> calculating pI').
>
> Since the ambient caps "stay on" cap_inheritable does not
> really matter anymore. Simply set the permitted caps when
> the ambient cap is set.
>
> Signed-off-by: Christoph Lameter <[email protected]>
>
> Index: linux/security/commoncap.c
> ===================================================================
> --- linux.orig/security/commoncap.c 2015-02-04 09:44:25.000000000 -0600
> +++ linux/security/commoncap.c 2015-02-04 12:48:44.100471600 -0600
> @@ -353,7 +353,7 @@ static inline int bprm_caps_from_vfs_cap
> /*
> * pP' = (X & fP) | (pI & fI)

For a final patch, this comment should be fixed.

> */
> - new->cap_permitted.cap[i] =
> + new->cap_permitted.cap[i] = current_cred()->cap_ambient.cap[i] |
> (new->cap_bset.cap[i] & permitted) |
> (new->cap_inheritable.cap[i] & inheritable);

IMO dropping a bit from the permitted set and then calling execve
while ruid != 0 MUST NOT re-add that bit to the permitted set. It
doesn't in current kernels, and changing that is a really bad idea, I
think.

So either we need an assertion that cap_ambiant is a subset of
cap_permitted (and code to make that assertion hold) or we should
change the rule above. I like:

(current_cred()->cap_ambient.cap[i] & permitted)

although I'd also be okay w/ something closer to Andrew's suggestion,
as long as it had the "& permitted" part.

>
> @@ -577,6 +577,7 @@ skip:
> }
>
> new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);
> + new->cap_ambient = old->cap_ambient;
> return 0;
> }
>
> @@ -933,6 +934,20 @@ int cap_task_prctl(int option, unsigned
> new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);
> return commit_creds(new);
>
> + case PR_CAP_AMBIENT:
> + if (!ns_capable(current_user_ns(), CAP_SETPCAP))
> + return -EPERM;

I maintain my assertion that, if we allow root (or, worse, setuid
root) to do this, then someone will come up with the brilliant idea of
writing a program (perhaps called something like seunshare) that does
this, then drops all caps (except ambient caps) and runs something
untrusted. Then we lose. So I'd prefer to check
task_no_new_privs(current) instead of ns_capable(current_user_ns(),
CAP_SETPCAP). I could be talked out of this, though, but I'd want to
see a decent argument why.

In fact, even with your proposal of writing a tool that does this and
then calls a helper, that helper might try to use privilege separation
and open a big hole because clearing pP is no longer sufficient to
drop privileges. Changing the evolution rule as above would fix this.

> +
> + if (!cap_valid(arg2))
> + return -EINVAL;
> +
> + new =prepare_creds();
> + if (arg3 == 0)
> + cap_lower(new->cap_ambient, arg2);
> + else
> + cap_raise(new->cap_ambient, arg2);
> + return commit_creds(new);
> +

This let you add capabilities you don't even have to cap_ambient. I'm
fine with that as long as the cap evolution rule changes, as above.

<bikeshed>
I don't like calling these "ambient". I'd prefer something like
"ambiently inheritable," although that's a bit long-winded.
</bikeshed>

--Andy

2015-02-04 21:16:21

[permalink] [raw]

Subject: Re: [RFC] Implement ambient capability set.

Quoting Andy Lutomirski ([email protected]):
> On Wed, Feb 4, 2015 at 10:49 AM, Christoph Lameter <[email protected]> wrote:
> > An attempt to implement this. Probably missing some fine points:
> >
> > Subject: [capabilities] Implement ambient capability set.
> >
> > DRAFT -- untested -- DRAFT
> >
> > Implement an ambient capabilty set to allow capabilties
> > to be inherited with unix semantics used also for other
> > attributes.
> >
> > Implements PR_CAP_AMBIENT. The second argument to prctl
> > is a the capability number and the third the desired state.
> > 0 for off. Otherwise on.
> >
> > Serge:
> > A new capability set, pA, is empty by default. You can
> > add bits to it using prctl if ns_capable(CAP_SETPCAP) and
> > all the new bits are in your pE. Once set, they stay until
> > they are removed using prctl. At exec, pA' = pA, and
> > fI |= pA (after reading fI from disk but before
> > calculating pI').
> >
> > Since the ambient caps "stay on" cap_inheritable does not
> > really matter anymore. Simply set the permitted caps when
> > the ambient cap is set.
> >
> > Signed-off-by: Christoph Lameter <[email protected]>
> >
> > Index: linux/security/commoncap.c
> > ===================================================================
> > --- linux.orig/security/commoncap.c 2015-02-04 09:44:25.000000000 -0600
> > +++ linux/security/commoncap.c 2015-02-04 12:48:44.100471600 -0600
> > @@ -353,7 +353,7 @@ static inline int bprm_caps_from_vfs_cap
> > /*
> > * pP' = (X & fP) | (pI & fI)
>
> For a final patch, this comment should be fixed.
>
> > */
> > - new->cap_permitted.cap[i] =
> > + new->cap_permitted.cap[i] = current_cred()->cap_ambient.cap[i] |
> > (new->cap_bset.cap[i] & permitted) |
> > (new->cap_inheritable.cap[i] & inheritable);
>
> IMO dropping a bit from the permitted set and then calling execve
> while ruid != 0 MUST NOT re-add that bit to the permitted set. It
> doesn't in current kernels, and changing that is a really bad idea, I
> think.
>
> So either we need an assertion that cap_ambiant is a subset of
> cap_permitted (and code to make that assertion hold) or we should
> change the rule above. I like:
>
> (current_cred()->cap_ambient.cap[i] & permitted)
>
> although I'd also be okay w/ something closer to Andrew's suggestion,
> as long as it had the "& permitted" part.
>
> >
> > @@ -577,6 +577,7 @@ skip:
> > }
> >
> > new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);
> > + new->cap_ambient = old->cap_ambient;
> > return 0;
> > }
> >
> > @@ -933,6 +934,20 @@ int cap_task_prctl(int option, unsigned
> > new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);
> > return commit_creds(new);
> >
> > + case PR_CAP_AMBIENT:
> > + if (!ns_capable(current_user_ns(), CAP_SETPCAP))
> > + return -EPERM;
>
> I maintain my assertion that, if we allow root (or, worse, setuid
> root) to do this, then someone will come up with the brilliant idea of
> writing a program (perhaps called something like seunshare) that does
> this, then drops all caps (except ambient caps) and runs something
> untrusted. Then we lose. So I'd prefer to check

Well it's sort of the point of this patchset. If you trusted the code
being run, you could assign fI. If you can't be bothered to assign fI,
then you may feel you *need* to run it, but you probably don't really
trust the binary.

> task_no_new_privs(current) instead of ns_capable(current_user_ns(),

.... I'm ok with that. And iiuc it shouldn't get in the way of
Christoph's use case. I'd just rather not have one set of convoluted
new rules now, and the have to relax them later bc it turns out ppl
needed that.

Christoph, would your code run ok under NNP?

> CAP_SETPCAP). I could be talked out of this, though, but I'd want to
> see a decent argument why.
>
> In fact, even with your proposal of writing a tool that does this and
> then calls a helper, that helper might try to use privilege separation
> and open a big hole because clearing pP is no longer sufficient to
> drop privileges. Changing the evolution rule as above would fix this.

Yeah... "because clearing pP is no longer sufficient to drop privileges"
is reasonably convincing.

> > +
> > + if (!cap_valid(arg2))
> > + return -EINVAL;
> > +
> > + new =prepare_creds();
> > + if (arg3 == 0)
> > + cap_lower(new->cap_ambient, arg2);
> > + else
> > + cap_raise(new->cap_ambient, arg2);
> > + return commit_creds(new);
> > +
>
> This let you add capabilities you don't even have to cap_ambient. I'm
> fine with that as long as the cap evolution rule changes, as above.

How about if instead we do restrict it to what's in pP? I don't
want CAP_SETPCAP to become a cheap way to get all caps back. With
or without NNP.

> <bikeshed>
> I don't like calling these "ambient". I'd prefer something like
> "ambiently inheritable," although that's a bit long-winded.
> </bikeshed>
>
> --Andy

2015-02-04 21:24:38

[permalink] [raw]

Subject: Re: [RFC] Implement ambient capability set.

On Wed, Feb 4, 2015 at 1:16 PM, Serge E. Hallyn <[email protected]> wrote:
> Quoting Andy Lutomirski ([email protected]):
>> On Wed, Feb 4, 2015 at 10:49 AM, Christoph Lameter <[email protected]> wrote:
>> > An attempt to implement this. Probably missing some fine points:
>> >
>> > Subject: [capabilities] Implement ambient capability set.
>> >
>> > DRAFT -- untested -- DRAFT
>> >
>> > Implement an ambient capabilty set to allow capabilties
>> > to be inherited with unix semantics used also for other
>> > attributes.
>> >
>> > Implements PR_CAP_AMBIENT. The second argument to prctl
>> > is a the capability number and the third the desired state.
>> > 0 for off. Otherwise on.
>> >
>> > Serge:
>> > A new capability set, pA, is empty by default. You can
>> > add bits to it using prctl if ns_capable(CAP_SETPCAP) and
>> > all the new bits are in your pE. Once set, they stay until
>> > they are removed using prctl. At exec, pA' = pA, and
>> > fI |= pA (after reading fI from disk but before
>> > calculating pI').
>> >
>> > Since the ambient caps "stay on" cap_inheritable does not
>> > really matter anymore. Simply set the permitted caps when
>> > the ambient cap is set.
>> >
>> > Signed-off-by: Christoph Lameter <[email protected]>
>> >
>> > Index: linux/security/commoncap.c
>> > ===================================================================
>> > --- linux.orig/security/commoncap.c 2015-02-04 09:44:25.000000000 -0600
>> > +++ linux/security/commoncap.c 2015-02-04 12:48:44.100471600 -0600
>> > @@ -353,7 +353,7 @@ static inline int bprm_caps_from_vfs_cap
>> > /*
>> > * pP' = (X & fP) | (pI & fI)
>>
>> For a final patch, this comment should be fixed.
>>
>> > */
>> > - new->cap_permitted.cap[i] =
>> > + new->cap_permitted.cap[i] = current_cred()->cap_ambient.cap[i] |
>> > (new->cap_bset.cap[i] & permitted) |
>> > (new->cap_inheritable.cap[i] & inheritable);
>>
>> IMO dropping a bit from the permitted set and then calling execve
>> while ruid != 0 MUST NOT re-add that bit to the permitted set. It
>> doesn't in current kernels, and changing that is a really bad idea, I
>> think.
>>
>> So either we need an assertion that cap_ambiant is a subset of
>> cap_permitted (and code to make that assertion hold) or we should
>> change the rule above. I like:
>>
>> (current_cred()->cap_ambient.cap[i] & permitted)
>>
>> although I'd also be okay w/ something closer to Andrew's suggestion,
>> as long as it had the "& permitted" part.
>>
>> >
>> > @@ -577,6 +577,7 @@ skip:
>> > }
>> >
>> > new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);
>> > + new->cap_ambient = old->cap_ambient;
>> > return 0;
>> > }
>> >
>> > @@ -933,6 +934,20 @@ int cap_task_prctl(int option, unsigned
>> > new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);
>> > return commit_creds(new);
>> >
>> > + case PR_CAP_AMBIENT:
>> > + if (!ns_capable(current_user_ns(), CAP_SETPCAP))
>> > + return -EPERM;
>>
>> I maintain my assertion that, if we allow root (or, worse, setuid
>> root) to do this, then someone will come up with the brilliant idea of
>> writing a program (perhaps called something like seunshare) that does
>> this, then drops all caps (except ambient caps) and runs something
>> untrusted. Then we lose. So I'd prefer to check
>
> Well it's sort of the point of this patchset. If you trusted the code
> being run, you could assign fI. If you can't be bothered to assign fI,
> then you may feel you *need* to run it, but you probably don't really
> trust the binary.
>
>> task_no_new_privs(current) instead of ns_capable(current_user_ns(),
>
> .... I'm ok with that. And iiuc it shouldn't get in the way of
> Christoph's use case. I'd just rather not have one set of convoluted
> new rules now, and the have to relax them later bc it turns out ppl
> needed that.
>
> Christoph, would your code run ok under NNP?

But someone will want to run *bash* as an untrusted user with, say,
CAP_NET_BIND permitted and ambient. Then that user has a non-empty
ambient set, and they can run a setuid-root program, and who knows
what will go wrong? Requiring no_new_privs would prevent this type of
failure entirely.

If we need to relax that later, it's easy, I think. The rule's not
that convoluted, and there's precedent for having new fancy features
require setting no_new_privs first.

>
>> CAP_SETPCAP). I could be talked out of this, though, but I'd want to
>> see a decent argument why.
>>
>> In fact, even with your proposal of writing a tool that does this and
>> then calls a helper, that helper might try to use privilege separation
>> and open a big hole because clearing pP is no longer sufficient to
>> drop privileges. Changing the evolution rule as above would fix this.
>
> Yeah... "because clearing pP is no longer sufficient to drop privileges"
> is reasonably convincing.
>
>> > +
>> > + if (!cap_valid(arg2))
>> > + return -EINVAL;
>> > +
>> > + new =prepare_creds();
>> > + if (arg3 == 0)
>> > + cap_lower(new->cap_ambient, arg2);
>> > + else
>> > + cap_raise(new->cap_ambient, arg2);
>> > + return commit_creds(new);
>> > +
>>
>> This let you add capabilities you don't even have to cap_ambient. I'm
>> fine with that as long as the cap evolution rule changes, as above.
>
> How about if instead we do restrict it to what's in pP? I don't
> want CAP_SETPCAP to become a cheap way to get all caps back. With
> or without NNP.

We'd also have to modify everything that can change pP to change pA as
well if we went this route. I'd be okay with that, but it would make
the patch much larger, and I'm not entirely sure I see the benefit.
It would keep the number of possible states smaller, which could be
nice.

--Andy

2015-02-04 21:27:48

[permalink] [raw]

Subject: Re: [RFC] Implement ambient capability set.

Quoting Andy Lutomirski ([email protected]):
> On Wed, Feb 4, 2015 at 1:16 PM, Serge E. Hallyn <[email protected]> wrote:
> > Quoting Andy Lutomirski ([email protected]):
> >> On Wed, Feb 4, 2015 at 10:49 AM, Christoph Lameter <[email protected]> wrote:
> >> > An attempt to implement this. Probably missing some fine points:
> >> >
> >> > Subject: [capabilities] Implement ambient capability set.
> >> >
> >> > DRAFT -- untested -- DRAFT
> >> >
> >> > Implement an ambient capabilty set to allow capabilties
> >> > to be inherited with unix semantics used also for other
> >> > attributes.
> >> >
> >> > Implements PR_CAP_AMBIENT. The second argument to prctl
> >> > is a the capability number and the third the desired state.
> >> > 0 for off. Otherwise on.
> >> >
> >> > Serge:
> >> > A new capability set, pA, is empty by default. You can
> >> > add bits to it using prctl if ns_capable(CAP_SETPCAP) and
> >> > all the new bits are in your pE. Once set, they stay until
> >> > they are removed using prctl. At exec, pA' = pA, and
> >> > fI |= pA (after reading fI from disk but before
> >> > calculating pI').
> >> >
> >> > Since the ambient caps "stay on" cap_inheritable does not
> >> > really matter anymore. Simply set the permitted caps when
> >> > the ambient cap is set.
> >> >
> >> > Signed-off-by: Christoph Lameter <[email protected]>
> >> >
> >> > Index: linux/security/commoncap.c
> >> > ===================================================================
> >> > --- linux.orig/security/commoncap.c 2015-02-04 09:44:25.000000000 -0600
> >> > +++ linux/security/commoncap.c 2015-02-04 12:48:44.100471600 -0600
> >> > @@ -353,7 +353,7 @@ static inline int bprm_caps_from_vfs_cap
> >> > /*
> >> > * pP' = (X & fP) | (pI & fI)
> >>
> >> For a final patch, this comment should be fixed.
> >>
> >> > */
> >> > - new->cap_permitted.cap[i] =
> >> > + new->cap_permitted.cap[i] = current_cred()->cap_ambient.cap[i] |
> >> > (new->cap_bset.cap[i] & permitted) |
> >> > (new->cap_inheritable.cap[i] & inheritable);
> >>
> >> IMO dropping a bit from the permitted set and then calling execve
> >> while ruid != 0 MUST NOT re-add that bit to the permitted set. It
> >> doesn't in current kernels, and changing that is a really bad idea, I
> >> think.
> >>
> >> So either we need an assertion that cap_ambiant is a subset of
> >> cap_permitted (and code to make that assertion hold) or we should
> >> change the rule above. I like:
> >>
> >> (current_cred()->cap_ambient.cap[i] & permitted)
> >>
> >> although I'd also be okay w/ something closer to Andrew's suggestion,
> >> as long as it had the "& permitted" part.
> >>
> >> >
> >> > @@ -577,6 +577,7 @@ skip:
> >> > }
> >> >
> >> > new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);
> >> > + new->cap_ambient = old->cap_ambient;
> >> > return 0;
> >> > }
> >> >
> >> > @@ -933,6 +934,20 @@ int cap_task_prctl(int option, unsigned
> >> > new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);
> >> > return commit_creds(new);
> >> >
> >> > + case PR_CAP_AMBIENT:
> >> > + if (!ns_capable(current_user_ns(), CAP_SETPCAP))
> >> > + return -EPERM;
> >>
> >> I maintain my assertion that, if we allow root (or, worse, setuid
> >> root) to do this, then someone will come up with the brilliant idea of
> >> writing a program (perhaps called something like seunshare) that does
> >> this, then drops all caps (except ambient caps) and runs something
> >> untrusted. Then we lose. So I'd prefer to check
> >
> > Well it's sort of the point of this patchset. If you trusted the code
> > being run, you could assign fI. If you can't be bothered to assign fI,
> > then you may feel you *need* to run it, but you probably don't really
> > trust the binary.
> >
> >> task_no_new_privs(current) instead of ns_capable(current_user_ns(),
> >
> > .... I'm ok with that. And iiuc it shouldn't get in the way of
> > Christoph's use case. I'd just rather not have one set of convoluted
> > new rules now, and the have to relax them later bc it turns out ppl
> > needed that.
> >
> > Christoph, would your code run ok under NNP?
>
> But someone will want to run *bash* as an untrusted user with, say,
> CAP_NET_BIND permitted and ambient. Then that user has a non-empty
> ambient set, and they can run a setuid-root program, and who knows
> what will go wrong? Requiring no_new_privs would prevent this type of
> failure entirely.
>
> If we need to relax that later, it's easy, I think. The rule's not
> that convoluted, and there's precedent for having new fancy features
> require setting no_new_privs first.
>
> >
> >> CAP_SETPCAP). I could be talked out of this, though, but I'd want to
> >> see a decent argument why.
> >>
> >> In fact, even with your proposal of writing a tool that does this and
> >> then calls a helper, that helper might try to use privilege separation
> >> and open a big hole because clearing pP is no longer sufficient to
> >> drop privileges. Changing the evolution rule as above would fix this.
> >
> > Yeah... "because clearing pP is no longer sufficient to drop privileges"
> > is reasonably convincing.
> >
> >> > +
> >> > + if (!cap_valid(arg2))
> >> > + return -EINVAL;
> >> > +
> >> > + new =prepare_creds();
> >> > + if (arg3 == 0)
> >> > + cap_lower(new->cap_ambient, arg2);
> >> > + else
> >> > + cap_raise(new->cap_ambient, arg2);
> >> > + return commit_creds(new);
> >> > +
> >>
> >> This let you add capabilities you don't even have to cap_ambient. I'm
> >> fine with that as long as the cap evolution rule changes, as above.
> >
> > How about if instead we do restrict it to what's in pP? I don't
> > want CAP_SETPCAP to become a cheap way to get all caps back. With
> > or without NNP.
>
> We'd also have to modify everything that can change pP to change pA as
> well if we went this route. I'd be okay with that, but it would make
> the patch much larger, and I'm not entirely sure I see the benefit.
> It would keep the number of possible states smaller, which could be
> nice.

Do you mean if we didn't require NNP? I'm suggesting that even if
we require NNP we should restrict any new bits added to pA to be
in pP at the prctl call. Then whether or not to drop them from
pA when they are dropped from pP, I'm not yet certain.

2015-02-04 21:32:12

[permalink] [raw]

Subject: Re: [RFC] Implement ambient capability set.

On Wed, Feb 4, 2015 at 1:27 PM, Serge E. Hallyn <[email protected]> wrote:
> Quoting Andy Lutomirski ([email protected]):
>> On Wed, Feb 4, 2015 at 1:16 PM, Serge E. Hallyn <[email protected]> wrote:
>> > Quoting Andy Lutomirski ([email protected]):
>> >> On Wed, Feb 4, 2015 at 10:49 AM, Christoph Lameter <[email protected]> wrote:
>> >> > +
>> >> > + if (!cap_valid(arg2))
>> >> > + return -EINVAL;
>> >> > +
>> >> > + new =prepare_creds();
>> >> > + if (arg3 == 0)
>> >> > + cap_lower(new->cap_ambient, arg2);
>> >> > + else
>> >> > + cap_raise(new->cap_ambient, arg2);
>> >> > + return commit_creds(new);
>> >> > +
>> >>
>> >> This let you add capabilities you don't even have to cap_ambient. I'm
>> >> fine with that as long as the cap evolution rule changes, as above.
>> >
>> > How about if instead we do restrict it to what's in pP? I don't
>> > want CAP_SETPCAP to become a cheap way to get all caps back. With
>> > or without NNP.
>>
>> We'd also have to modify everything that can change pP to change pA as
>> well if we went this route. I'd be okay with that, but it would make
>> the patch much larger, and I'm not entirely sure I see the benefit.
>> It would keep the number of possible states smaller, which could be
>> nice.
>
> Do you mean if we didn't require NNP? I'm suggesting that even if
> we require NNP we should restrict any new bits added to pA to be
> in pP at the prctl call. Then whether or not to drop them from
> pA when they are dropped from pP, I'm not yet certain.

I mean regardless of whether we require NNP.

I think that, unless we change the evolution rule, we would need to
drop from pA when bits are dropped from pP to preserve the idea that
dropping bits from pP drops them for good (as long as ruid != 0 or
some securebit is set).

--
Andy Lutomirski
AMA Capital Management, LLC

2015-02-04 21:51:57

[permalink] [raw]

Subject: Re: [RFC] Implement ambient capability set.

On Wed, 4 Feb 2015, Serge E. Hallyn wrote:

> > task_no_new_privs(current) instead of ns_capable(current_user_ns(),
>
> .... I'm ok with that. And iiuc it shouldn't get in the way of
> Christoph's use case. I'd just rather not have one set of convoluted
> new rules now, and the have to relax them later bc it turns out ppl
> needed that.
>
> Christoph, would your code run ok under NNP?

There are still binaries invoked that need more priviledges. Does not
work.

> > In fact, even with your proposal of writing a tool that does this and
> > then calls a helper, that helper might try to use privilege separation
> > and open a big hole because clearing pP is no longer sufficient to
> > drop privileges. Changing the evolution rule as above would fix this.
>
> Yeah... "because clearing pP is no longer sufficient to drop privileges"
> is reasonably convincing.

Well I'd rather have a way to avoid writing a tool. The best would be if
you could just set some caps and that would do it.

> > <bikeshed>
> > I don't like calling these "ambient". I'd prefer something like
> > "ambiently inheritable," although that's a bit long-winded.
> > </bikeshed>

amb_inh?

Fixup patch:

Index: linux/security/commoncap.c
===================================================================
--- linux.orig/security/commoncap.c
+++ linux/security/commoncap.c
@@ -351,9 +351,10 @@ static inline int bprm_caps_from_vfs_cap
__u32 inheritable = caps->inheritable.cap[i];

/*
- * pP' = (X & fP) | (pI & fI)
+ * pP' = (fA & fP) | (X & fP) | (pI & fI)
*/
- new->cap_permitted.cap[i] = current_cred()->cap_ambient.cap[i] |
+ new->cap_permitted.cap[i] =
+ (current_cred()->cap_ambient.cap[i] & permitted) |
(new->cap_bset.cap[i] & permitted) |
(new->cap_inheritable.cap[i] & inheritable);

@@ -453,9 +454,13 @@ static int get_file_caps(struct linux_bi
if (rc == -EINVAL)
printk(KERN_NOTICE "%s: get_vfs_caps_from_disk returned %d for %s\n",
__func__, rc, bprm->filename);
- else if (rc == -ENODATA)
- rc = 0;
- goto out;
+ else if (rc != -ENODATA)
+ goto out;
+ rc = 0;
+ if (!cap_isclear(current_cred()->cap_ambient))
+ goto out;
+ *effective = true;
+ *has_cap = true;
}

rc = bprm_caps_from_vfs_caps(&vcaps, bprm, effective, has_cap);
@@ -941,7 +946,10 @@ int cap_task_prctl(int option, unsigned
if (!cap_valid(arg2))
return -EINVAL;

- new =prepare_creds();
+ if (!ns_capable(current_user_ns(), arg2))
+ return -EPERM;
+
+ new = prepare_creds();
if (arg3 == 0)
cap_lower(new->cap_ambient, arg2);
else

2015-02-04 21:52:04

[permalink] [raw]

Subject: Re: [RFC] Implement ambient capability set.

Quoting Andy Lutomirski ([email protected]):
> On Wed, Feb 4, 2015 at 1:27 PM, Serge E. Hallyn <[email protected]> wrote:
> > Quoting Andy Lutomirski ([email protected]):
> >> On Wed, Feb 4, 2015 at 1:16 PM, Serge E. Hallyn <[email protected]> wrote:
> >> > Quoting Andy Lutomirski ([email protected]):
> >> >> On Wed, Feb 4, 2015 at 10:49 AM, Christoph Lameter <[email protected]> wrote:
> >> >> > +
> >> >> > + if (!cap_valid(arg2))
> >> >> > + return -EINVAL;
> >> >> > +
> >> >> > + new =prepare_creds();
> >> >> > + if (arg3 == 0)
> >> >> > + cap_lower(new->cap_ambient, arg2);
> >> >> > + else
> >> >> > + cap_raise(new->cap_ambient, arg2);
> >> >> > + return commit_creds(new);
> >> >> > +
> >> >>
> >> >> This let you add capabilities you don't even have to cap_ambient. I'm
> >> >> fine with that as long as the cap evolution rule changes, as above.
> >> >
> >> > How about if instead we do restrict it to what's in pP? I don't
> >> > want CAP_SETPCAP to become a cheap way to get all caps back. With
> >> > or without NNP.
> >>
> >> We'd also have to modify everything that can change pP to change pA as
> >> well if we went this route. I'd be okay with that, but it would make
> >> the patch much larger, and I'm not entirely sure I see the benefit.
> >> It would keep the number of possible states smaller, which could be
> >> nice.
> >
> > Do you mean if we didn't require NNP? I'm suggesting that even if
> > we require NNP we should restrict any new bits added to pA to be
> > in pP at the prctl call. Then whether or not to drop them from
> > pA when they are dropped from pP, I'm not yet certain.
>
> I mean regardless of whether we require NNP.
>
> I think that, unless we change the evolution rule, we would need to
> drop from pA when bits are dropped from pP to preserve the idea that
> dropping bits from pP drops them for good (as long as ruid != 0 or
> some securebit is set).

Ok, so iiuc the rules would be:

1. must set nnp and have ns_capable(CAP_SETPCAP) to
call prctl(PR_SET_AMBIENT_WHATEVER)

2. adding bits to pA requires they be in pP at prctl time

3. dropping bits from pP drops them also from pA

4. at exec, fP |= pA; pA' = pA

Christoph, would these suffice for your use caes?

2015-02-04 21:57:08

[permalink] [raw]

Subject: Re: [RFC] Implement ambient capability set.

On Wed, Feb 4, 2015 at 1:51 PM, Christoph Lameter <[email protected]> wrote:
> On Wed, 4 Feb 2015, Serge E. Hallyn wrote:
>
>> > task_no_new_privs(current) instead of ns_capable(current_user_ns(),
>>
>> .... I'm ok with that. And iiuc it shouldn't get in the way of
>> Christoph's use case. I'd just rather not have one set of convoluted
>> new rules now, and the have to relax them later bc it turns out ppl
>> needed that.
>>
>> Christoph, would your code run ok under NNP?
>
> There are still binaries invoked that need more priviledges. Does not
> work.

What do you mean by "need more privileges"? Are they setuid-root or
do they use fP?

>
>> > In fact, even with your proposal of writing a tool that does this and
>> > then calls a helper, that helper might try to use privilege separation
>> > and open a big hole because clearing pP is no longer sufficient to
>> > drop privileges. Changing the evolution rule as above would fix this.
>>
>> Yeah... "because clearing pP is no longer sufficient to drop privileges"
>> is reasonably convincing.
>
> Well I'd rather have a way to avoid writing a tool. The best would be if
> you could just set some caps and that would do it.

However this ends up working, I'll add support to setpriv for it, so
you'll be spared writing the tool if that's acceptable. :)

>
>> > <bikeshed>
>> > I don't like calling these "ambient". I'd prefer something like
>> > "ambiently inheritable," although that's a bit long-winded.
>> > </bikeshed>
>
> amb_inh?
>
> Fixup patch:
>
> Index: linux/security/commoncap.c
> ===================================================================
> --- linux.orig/security/commoncap.c
> +++ linux/security/commoncap.c
> @@ -351,9 +351,10 @@ static inline int bprm_caps_from_vfs_cap
> __u32 inheritable = caps->inheritable.cap[i];
>
> /*
> - * pP' = (X & fP) | (pI & fI)
> + * pP' = (fA & fP) | (X & fP) | (pI & fI)

pA & pP?

> */
> - new->cap_permitted.cap[i] = current_cred()->cap_ambient.cap[i] |
> + new->cap_permitted.cap[i] =
> + (current_cred()->cap_ambient.cap[i] & permitted) |
> (new->cap_bset.cap[i] & permitted) |
> (new->cap_inheritable.cap[i] & inheritable);
>
> @@ -453,9 +454,13 @@ static int get_file_caps(struct linux_bi
> if (rc == -EINVAL)
> printk(KERN_NOTICE "%s: get_vfs_caps_from_disk returned %d for %s\n",
> __func__, rc, bprm->filename);
> - else if (rc == -ENODATA)
> - rc = 0;
> - goto out;
> + else if (rc != -ENODATA)
> + goto out;
> + rc = 0;
> + if (!cap_isclear(current_cred()->cap_ambient))
> + goto out;

Confused. What about effective? Don't we still need to address that?

> + *effective = true;
> + *has_cap = true;
> }
>
> rc = bprm_caps_from_vfs_caps(&vcaps, bprm, effective, has_cap);
> @@ -941,7 +946,10 @@ int cap_task_prctl(int option, unsigned
> if (!cap_valid(arg2))
> return -EINVAL;
>
> - new =prepare_creds();

> + if (!ns_capable(current_user_ns(), arg2))
> + return -EPERM;

I don't see why this is necessary given the change above.

--Andy

> +
> + new = prepare_creds();
> if (arg3 == 0)
> cap_lower(new->cap_ambient, arg2);
> else

--
Andy Lutomirski
AMA Capital Management, LLC

2015-02-04 21:57:38

[permalink] [raw]

Subject: Re: [RFC] Implement ambient capability set.

On Wed, 4 Feb 2015, Andy Lutomirski wrote:

> But someone will want to run *bash* as an untrusted user with, say,
> CAP_NET_BIND permitted and ambient. Then that user has a non-empty
> ambient set, and they can run a setuid-root program, and who knows
> what will go wrong? Requiring no_new_privs would prevent this type of
> failure entirely.
>
> If we need to relax that later, it's easy, I think. The rule's not
> that convoluted, and there's precedent for having new fancy features
> require setting no_new_privs first.

It would make the patch pointless. The case of having to run a setuid root
prpgrams from a shell that has the caps enabled is a routine thing for
testing etc.

2015-02-04 22:01:09

[permalink] [raw]

Subject: Re: [RFC] Implement ambient capability set.

On Wed, 4 Feb 2015, Andy Lutomirski wrote:

> >> Christoph, would your code run ok under NNP?
> >
> > There are still binaries invoked that need more priviledges. Does not
> > work.
>
> What do you mean by "need more privileges"? Are they setuid-root or
> do they use fP?

Both.

> > Well I'd rather have a way to avoid writing a tool. The best would be if
> > you could just set some caps and that would do it.
>
> However this ends up working, I'll add support to setpriv for it, so
> you'll be spared writing the tool if that's acceptable. :)

Sure.

> > __u32 inheritable = caps->inheritable.cap[i];
> >
> > /*
> > - * pP' = (X & fP) | (pI & fI)
> > + * pP' = (fA & fP) | (X & fP) | (pI & fI)
>
> pA & pP?

Ok.

> > + else if (rc != -ENODATA)
> > + goto out;
> > + rc = 0;
> > + if (!cap_isclear(current_cred()->cap_ambient))
> > + goto out;
>
> Confused. What about effective? Don't we still need to address that?

Seems that the caps do not take effect unless I set them. Is there a
better way to do this? Logic is wrong. It must be

if (cap_isclear(..)) goto out

> > + return -EPERM;
>
> I don't see why this is necessary given the change above.

I guess I should repost the whole patch.

2015-02-04 22:02:17

[permalink] [raw]

Subject: Re: [RFC] Implement ambient capability set.

Quoting Serge E. Hallyn ([email protected]):
> Quoting Andy Lutomirski ([email protected]):
> > On Wed, Feb 4, 2015 at 1:27 PM, Serge E. Hallyn <[email protected]> wrote:
> > > Quoting Andy Lutomirski ([email protected]):
> > >> On Wed, Feb 4, 2015 at 1:16 PM, Serge E. Hallyn <[email protected]> wrote:
> > >> > Quoting Andy Lutomirski ([email protected]):
> > >> >> On Wed, Feb 4, 2015 at 10:49 AM, Christoph Lameter <[email protected]> wrote:
> > >> >> > +
> > >> >> > + if (!cap_valid(arg2))
> > >> >> > + return -EINVAL;
> > >> >> > +
> > >> >> > + new =prepare_creds();
> > >> >> > + if (arg3 == 0)
> > >> >> > + cap_lower(new->cap_ambient, arg2);
> > >> >> > + else
> > >> >> > + cap_raise(new->cap_ambient, arg2);
> > >> >> > + return commit_creds(new);
> > >> >> > +
> > >> >>
> > >> >> This let you add capabilities you don't even have to cap_ambient. I'm
> > >> >> fine with that as long as the cap evolution rule changes, as above.
> > >> >
> > >> > How about if instead we do restrict it to what's in pP? I don't
> > >> > want CAP_SETPCAP to become a cheap way to get all caps back. With
> > >> > or without NNP.
> > >>
> > >> We'd also have to modify everything that can change pP to change pA as
> > >> well if we went this route. I'd be okay with that, but it would make
> > >> the patch much larger, and I'm not entirely sure I see the benefit.
> > >> It would keep the number of possible states smaller, which could be
> > >> nice.
> > >
> > > Do you mean if we didn't require NNP? I'm suggesting that even if
> > > we require NNP we should restrict any new bits added to pA to be
> > > in pP at the prctl call. Then whether or not to drop them from
> > > pA when they are dropped from pP, I'm not yet certain.
> >
> > I mean regardless of whether we require NNP.
> >
> > I think that, unless we change the evolution rule, we would need to
> > drop from pA when bits are dropped from pP to preserve the idea that
> > dropping bits from pP drops them for good (as long as ruid != 0 or
> > some securebit is set).
>
> Ok, so iiuc the rules would be:
>
> 1. must set nnp and have ns_capable(CAP_SETPCAP) to
> call prctl(PR_SET_AMBIENT_WHATEVER)
>
> 2. adding bits to pA requires they be in pP at prctl time
>
> 3. dropping bits from pP drops them also from pA
>
> 4. at exec, fP |= pA; pA' = pA

Actually I'm tempted to say that if fP is not empty, then we stick with current
rules for fP/pP and clear out pA. If fP is empty, then fP = pA

Then, if fP is not empty on the file, we either drop nnp at exec (or
don't use nnp at all), or we refuse exec if fP > pA.

We definately do not want to run a file which has capability X
in fP, when X is not in pA.

> Christoph, would these suffice for your use caes?

2015-02-04 22:02:40

[permalink] [raw]

Subject: Re: [RFC] Implement ambient capability set V2

Subject: [capabilities] Implement ambient capability set V2

DRAFT -- untested -- DRAFT

Implement an ambient capabilty set to allow capabilties
to be inherited with unix semantics used also for other
attributes.

Implements PR_CAP_AMBIENT. The second argument to prctl
is a the capability number and the third the desired state.
0 for off. Otherwise on.

Serge:
A new capability set, pA, is empty by default. You can
add bits to it using prctl if ns_capable(CAP_SETPCAP) and
all the new bits are in your pE. Once set, they stay until
they are removed using prctl. At exec, pA' = pA, and
fI |= pA (after reading fI from disk but before
calculating pI').

Since the ambient caps "stay on" cap_inheritable does not
really matter anymore. Simply set the permitted caps when
the ambient cap is set.

Signed-off-by: Christoph Lameter <[email protected]>

Index: linux/security/commoncap.c
===================================================================
--- linux.orig/security/commoncap.c
+++ linux/security/commoncap.c
@@ -351,9 +351,10 @@ static inline int bprm_caps_from_vfs_cap
__u32 inheritable = caps->inheritable.cap[i];

/*
- * pP' = (X & fP) | (pI & fI)
+ * pP' = (pA & fP) | (X & fP) | (pI & fI)
*/
new->cap_permitted.cap[i] =
+ (current_cred()->cap_ambient.cap[i] & permitted) |
(new->cap_bset.cap[i] & permitted) |
(new->cap_inheritable.cap[i] & inheritable);

@@ -453,9 +454,13 @@ static int get_file_caps(struct linux_bi
if (rc == -EINVAL)
printk(KERN_NOTICE "%s: get_vfs_caps_from_disk returned %d for %s\n",
__func__, rc, bprm->filename);
- else if (rc == -ENODATA)
- rc = 0;
- goto out;
+ else if (rc != -ENODATA)
+ goto out;
+ rc = 0;
+ if (cap_isclear(current_cred()->cap_ambient))
+ goto out;
+ *effective = true;
+ *has_cap = true;
}

rc = bprm_caps_from_vfs_caps(&vcaps, bprm, effective, has_cap);
@@ -577,6 +582,7 @@ skip:
}

new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);
+ new->cap_ambient = old->cap_ambient;
return 0;
}

@@ -933,6 +939,23 @@ int cap_task_prctl(int option, unsigned
new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);
return commit_creds(new);

+ case PR_CAP_AMBIENT:
+ if (!ns_capable(current_user_ns(), CAP_SETPCAP))
+ return -EPERM;
+
+ if (!cap_valid(arg2))
+ return -EINVAL;
+
+ if (!ns_capable(current_user_ns(), arg2))
+ return -EPERM;
+
+ new = prepare_creds();
+ if (arg3 == 0)
+ cap_lower(new->cap_ambient, arg2);
+ else
+ cap_raise(new->cap_ambient, arg2);
+ return commit_creds(new);
+
default:
/* No functionality available - continue with default */
return -ENOSYS;
Index: linux/include/linux/cred.h
===================================================================
--- linux.orig/include/linux/cred.h
+++ linux/include/linux/cred.h
@@ -122,6 +122,7 @@ struct cred {
kernel_cap_t cap_permitted; /* caps we're permitted */
kernel_cap_t cap_effective; /* caps we can actually use */
kernel_cap_t cap_bset; /* capability bounding set */
+ kernel_cap_t cap_ambient; /* Ambient capability set */
#ifdef CONFIG_KEYS
unsigned char jit_keyring; /* default keyring to attach requested
* keys to */
Index: linux/include/uapi/linux/prctl.h
===================================================================
--- linux.orig/include/uapi/linux/prctl.h
+++ linux/include/uapi/linux/prctl.h
@@ -185,4 +185,7 @@ struct prctl_mm_map {
#define PR_MPX_ENABLE_MANAGEMENT 43
#define PR_MPX_DISABLE_MANAGEMENT 44

+/* Control the ambient capability set */
+#define PR_CAP_AMBIENT 45
+
#endif /* _LINUX_PRCTL_H */

2015-02-04 22:30:21

[permalink] [raw]

Subject: Re: [RFC] Implement ambient capability set.

On Wed, Feb 4, 2015 at 1:57 PM, Christoph Lameter <[email protected]> wrote:
> On Wed, 4 Feb 2015, Andy Lutomirski wrote:
>
>> But someone will want to run *bash* as an untrusted user with, say,
>> CAP_NET_BIND permitted and ambient. Then that user has a non-empty
>> ambient set, and they can run a setuid-root program, and who knows
>> what will go wrong? Requiring no_new_privs would prevent this type of
>> failure entirely.
>>
>> If we need to relax that later, it's easy, I think. The rule's not
>> that convoluted, and there's precedent for having new fancy features
>> require setting no_new_privs first.
>
> It would make the patch pointless. The case of having to run a setuid root
> prpgrams from a shell that has the caps enabled is a routine thing for
> testing etc.
>

That's unfortunate. In that case, we need to figure out what happens
when you run such a setuid root program. I think the answer should be
that pA gets cleared, and that pA also gets cleared if you run a
program that has file caps.

--Andy

2015-02-04 22:33:01

[permalink] [raw]

Subject: Re: [RFC] Implement ambient capability set.

On Wed, Feb 4, 2015 at 2:02 PM, Serge E. Hallyn <[email protected]> wrote:
> Quoting Serge E. Hallyn ([email protected]):
>> Quoting Andy Lutomirski ([email protected]):
>> > On Wed, Feb 4, 2015 at 1:27 PM, Serge E. Hallyn <[email protected]> wrote:
>> > > Quoting Andy Lutomirski ([email protected]):
>> > >> On Wed, Feb 4, 2015 at 1:16 PM, Serge E. Hallyn <[email protected]> wrote:
>> > >> > Quoting Andy Lutomirski ([email protected]):
>> > >> >> On Wed, Feb 4, 2015 at 10:49 AM, Christoph Lameter <[email protected]> wrote:
>> > >> >> > +
>> > >> >> > + if (!cap_valid(arg2))
>> > >> >> > + return -EINVAL;
>> > >> >> > +
>> > >> >> > + new =prepare_creds();
>> > >> >> > + if (arg3 == 0)
>> > >> >> > + cap_lower(new->cap_ambient, arg2);
>> > >> >> > + else
>> > >> >> > + cap_raise(new->cap_ambient, arg2);
>> > >> >> > + return commit_creds(new);
>> > >> >> > +
>> > >> >>
>> > >> >> This let you add capabilities you don't even have to cap_ambient. I'm
>> > >> >> fine with that as long as the cap evolution rule changes, as above.
>> > >> >
>> > >> > How about if instead we do restrict it to what's in pP? I don't
>> > >> > want CAP_SETPCAP to become a cheap way to get all caps back. With
>> > >> > or without NNP.
>> > >>
>> > >> We'd also have to modify everything that can change pP to change pA as
>> > >> well if we went this route. I'd be okay with that, but it would make
>> > >> the patch much larger, and I'm not entirely sure I see the benefit.
>> > >> It would keep the number of possible states smaller, which could be
>> > >> nice.
>> > >
>> > > Do you mean if we didn't require NNP? I'm suggesting that even if
>> > > we require NNP we should restrict any new bits added to pA to be
>> > > in pP at the prctl call. Then whether or not to drop them from
>> > > pA when they are dropped from pP, I'm not yet certain.
>> >
>> > I mean regardless of whether we require NNP.
>> >
>> > I think that, unless we change the evolution rule, we would need to
>> > drop from pA when bits are dropped from pP to preserve the idea that
>> > dropping bits from pP drops them for good (as long as ruid != 0 or
>> > some securebit is set).
>>
>> Ok, so iiuc the rules would be:
>>
>> 1. must set nnp and have ns_capable(CAP_SETPCAP) to
>> call prctl(PR_SET_AMBIENT_WHATEVER)
>>
>> 2. adding bits to pA requires they be in pP at prctl time
>>
>> 3. dropping bits from pP drops them also from pA

I'm still unconvinced that 2 and 3 is better than using pP & pA in
execve, but I could go either way on that.

>>
>> 4. at exec, fP |= pA; pA' = pA
>
> Actually I'm tempted to say that if fP is not empty, then we stick with current
> rules for fP/pP and clear out pA. If fP is empty, then fP = pA
>
> Then, if fP is not empty on the file, we either drop nnp at exec (or
> don't use nnp at all), or we refuse exec if fP > pA.

We can't drop nnp at exec.

>
> We definately do not want to run a file which has capability X
> in fP, when X is not in pA.

Confused. This will break everything, including Christoph's use case.
The status quo for running ping from bash has pA == 0 and fP != 0.

--Andy

2015-02-05 00:03:13