Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161069AbbBCUNe (ORCPT ); Tue, 3 Feb 2015 15:13:34 -0500 Received: from mail-la0-f54.google.com ([209.85.215.54]:58969 "EHLO mail-la0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756239AbbBCUN2 (ORCPT ); Tue, 3 Feb 2015 15:13:28 -0500 MIME-Version: 1.0 In-Reply-To: References: <54CFB9B8.8020701@schaufler-ca.com> <20150202180806.GE24351@ubuntumail> <54CFE3E8.2030402@schaufler-ca.com> <20150203155122.GD2923@mail.hallyn.com> <54D0F94D.3050704@schaufler-ca.com> <20150203172837.GC4748@mail.hallyn.com> <54D10A50.5030707@schaufler-ca.com> From: Andy Lutomirski Date: Tue, 3 Feb 2015 12:13:06 -0800 Message-ID: Subject: Re: [capabilities] Allow normal inheritance for a configurable set of capabilities To: Christoph Lameter Cc: Casey Schaufler , "Serge E. Hallyn" , Serge Hallyn , Serge Hallyn , Jonathan Corbet , Aaron Jones , "Ted Ts'o" , LSM List , "linux-kernel@vger.kernel.org" , Andrew Morton Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4573 Lines: 110 On Tue, Feb 3, 2015 at 11:45 AM, Christoph Lameter 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/