MIME-Version: 1.0
In-Reply-To: <alpine.DEB.2.11.1502041242380.17411@gentwo.org>
References: <alpine.DEB.2.11.1502041242380.17411@gentwo.org>
From: Andy Lutomirski <luto@amacapital.net>
Date: Wed, 4 Feb 2015 12:44:28 -0800
Message-ID: <CALCETrVXmtotCzgw8G4M6u-LrLNMjtzBsTwp4P3M4BLg=muw-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
Content-Length: 4469
Lines: 116

On Wed, Feb 4, 2015 at 10:49 AM, Christoph Lameter <cl@linux.com> 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 <cl@linux.com>
>
> 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
--
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/