Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1755070AbbBDV5I (ORCPT <rfc822;w@1wt.eu>);
	Wed, 4 Feb 2015 16:57:08 -0500
Received: from mail-la0-f47.google.com ([209.85.215.47]:60818 "EHLO
	mail-la0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1753469AbbBDV5F (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Wed, 4 Feb 2015 16:57:05 -0500
MIME-Version: 1.0
In-Reply-To: <alpine.DEB.2.11.1502041542260.19035@gentwo.org>
References: <alpine.DEB.2.11.1502041242380.17411@gentwo.org>
 <CALCETrVXmtotCzgw8G4M6u-LrLNMjtzBsTwp4P3M4BLg=muw-g@mail.gmail.com>
 <20150204211617.GA20787@mail.hallyn.com> <alpine.DEB.2.11.1502041542260.19035@gentwo.org>
From: Andy Lutomirski <luto@amacapital.net>
Date: Wed, 4 Feb 2015 13:56:43 -0800
Message-ID: <CALCETrVqVt8kK1AG6YxQbTrNnrRJ0YAZdUzBqPcCxELiO6Ez-g@mail.gmail.com>
Subject: Re: [RFC] Implement ambient capability set.
To: Christoph Lameter <cl@linux.com>
Cc: "Serge E. Hallyn" <serge@hallyn.com>,
        "Andrew G. Morgan" <morgan@kernel.org>,
        Serge Hallyn <serge.hallyn@ubuntu.com>,
        Serge Hallyn <serge.hallyn@canonical.com>,
        Jonathan Corbet <corbet@lwn.net>, Aaron Jones <aaronmdjones@gmail.com>,
        "Ted Ts'o" <tytso@mit.edu>,
        LSM List <linux-security-module@vger.kernel.org>,
        lkml <linux-kernel@vger.kernel.org>,
        Andrew Morton <akpm@linuxfoundation.org>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3947
Lines: 112

On Wed, Feb 4, 2015 at 1:51 PM, Christoph Lameter <cl@linux.com> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/