Re: [capabilities] Allow normal inheritance for a configurable set of capabilities

[Andy Lutomirski]

On Tue, Feb 3, 2015 at 11:45 AM, Christoph Lameter <cl@xxxxxxxxx> wrote:
> On Tue, 3 Feb 2015, Casey Schaufler wrote:
>
>> > (I wasn't going to ask bc I assumed not, but heck maybe you're bored
>> > on a desert island or snowed in and just looking for an excuse to hack :)
>>
>> Not at all bored, but I think this could be important.
>
> Ok here is a draft of a patch that follows these ideas.
>
> It also adds an ambient field and sets the field if a new capability
>
>         CAP_AMBIENT_MASK
>
> is set. The perm calcualtion is as suggested by Serge.
>
>
> If CAP_AMBIENT_MASK is set the inheritable caps become the ambient ones.
>
> If it is not set then the ambient caps are copied from the parent.
>
>
> DRAFT --- not a working patch:
>
> Index: linux/include/linux/capability.h
> ===================================================================
> --- linux.orig/include/linux/capability.h       2015-02-03 13:25:03.000000000 -0600
> +++ linux/include/linux/capability.h    2015-02-03 13:39:23.385424676 -0600
> @@ -29,6 +29,7 @@ struct cpu_vfs_cap_data {
>         __u32 magic_etc;
>         kernel_cap_t permitted;
>         kernel_cap_t inheritable;
> +       kernel_cap_t ambient;
>  };
>
>  #define _USER_CAP_HEADER_SIZE  (sizeof(struct __user_cap_header_struct))
> Index: linux/include/uapi/linux/capability.h
> ===================================================================
> --- linux.orig/include/uapi/linux/capability.h  2014-07-10 16:10:29.814424392 -0500
> +++ linux/include/uapi/linux/capability.h       2015-02-03 13:26:13.231081452 -0600
> @@ -351,8 +351,10 @@ struct vfs_cap_data {
>
>  #define CAP_AUDIT_READ         37
>
> +/* Set the current inheritable mask as the ambient inheritable mask */
> +#define CAP_AMBIENT_MASK       38
>
> -#define CAP_LAST_CAP         CAP_AUDIT_READ
> +#define CAP_LAST_CAP         CAP_AMBIENT_MASK
>
>  #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
>
> Index: linux/security/commoncap.c
> ===================================================================
> --- linux.orig/security/commoncap.c     2015-02-03 13:25:03.000000000 -0600
> +++ linux/security/commoncap.c  2015-02-03 13:43:16.317859741 -0600
> @@ -349,17 +349,24 @@ static inline int bprm_caps_from_vfs_cap
>         CAP_FOR_EACH_U32(i) {
>                 __u32 permitted = caps->permitted.cap[i];
>                 __u32 inheritable = caps->inheritable.cap[i];
> +               __u32 ambient = caps->ambient.cap[i];
>
>                 /*
>                  * pP' = (X & fP) | (pI & fI)
>                  */
>                 new->cap_permitted.cap[i] =
>                         (new->cap_bset.cap[i] & permitted) |
> -                       (new->cap_inheritable.cap[i] & inheritable);
> +                       (new->cap_inheritable.cap[i] & inheritable) |
> +                       (ambient & inheritable);

Is there a clear reason why no non-permitted bits can be inheritable?
If not, then I think this should be (ambient & inheritable &
permitted).

Do we need to think about the effective mask here?  What happens when
we exec a setuid program or a program with a non-empty fP set?  I
think that, in these cases, we should strongly consider clearing the
ambient set.  For a different approach, see below.

>
>                 if (permitted & ~new->cap_permitted.cap[i])
>                         /* insufficient to execute correctly */
>                         ret = -EPERM;
> +
> +               if (capable(CAP_AMBIENT_MASK))
> +                       new->cap_ambient.cap[i] = inheritable;
> +               else
> +                       new->cap_ambient.cap[i] = ambient;

IMO this is really weird.  I don't think that the presence of an
effective cap should change the cap equations.  (Also, that should be
nsown_capable.)

Can we please make this an explicit opt-in?  For example, allow a
process to set an ambient cap bit if that bit is both permitted and
inheritable.  I'd prefer having it be a single control, though -- just
prctl(PR_SET_ALWAYS_INHERIT_CAPS, 1, 0, 0, 0) would set a single bit
that would cause all inheritable bits to be treated as ambient.

Here's a slight variant that might be more clearly safe: add an
inherited per-process bit that causes all files to act as though fI is
the full set.  Only allow setting that bit if no_new_privs is set.

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