From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751470AbcFAUpd (ORCPT ); Wed, 1 Jun 2016 16:45:33 -0400 Received: from nm30-vm0.bullet.mail.bf1.yahoo.com ([98.139.213.126]:48430 "EHLO nm30-vm0.bullet.mail.bf1.yahoo.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750807AbcFAUpb (ORCPT ); Wed, 1 Jun 2016 16:45:31 -0400 X-Yahoo-Newman-Id: 212968.51462.bm@smtp115.mail.bf1.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: NzO4GtUVM1klLm2NmaaLvi7yuIOHrjMRLmBE7LpkmhJvbSf DnlsjbISuRoIEq4iZzJQfYDZkVpwPp3ZMk9m1kEpsOldOxuOOMivUn8_PPeb 3_w2tdTJ6iTTzIu5pEW_QgwNAJq3WmYkMXGy0tonsL.7w44ZGLduW8B3EjOb 6u6dQsAsnn8UC9M2jO90qupcAt_zD0UTiDD0.czgiQBT4lBjm6b2.ut_rHwO n63bpo0yq.EW5CmX.ArVvJv1maOCrmNPxYb06KQ6AM505KRMSL1tlv3BgbI8 6Tm9KbH3Oo6npIilRENuKjoBEVgtcVFlv16v1RKg6FyBuLaZKvvdyzoAdJyZ 85i9e7Kbdw3H5pSwfbG2Puu.pSBzozkj81gXhj_T2Xw.ielp097.fAZM07AV CoQIqvOV5Fq6.Wy4SuBVfktVw25z3s0xGvSnqo9y764Pw9gyF_hHvVstVxaP vJEx9fRjCvatOoNvfJOB2bt4g5jhOxVHEna3KHNt0rJwD2u3hlrDZcEWTV8p 3HC6is.Q0dBXIrwQtbkmL3gt4K5xI18Q3qufVM7P1Vl1osep2BModAm_0IA- - X-Yahoo-SMTP: OIJXglSswBDfgLtXluJ6wiAYv6_cnw-- Subject: Re: [PATCH] LSM: Reorder security_capset to do access checks properly To: Stephen Smalley , LSM , James Morris References: <0243a591-a6e1-8827-3f03-884c3ad331d0@schaufler-ca.com> <439266f9-6431-4c59-db2e-7eb48d49aaf8@tycho.nsa.gov> Cc: LKLM , SE Linux From: Casey Schaufler Message-ID: Date: Wed, 1 Jun 2016 13:45:22 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 6/1/2016 1:38 PM, Stephen Smalley wrote: > On 06/01/2016 04:30 PM, Casey Schaufler wrote: >> On 6/1/2016 1:06 PM, Stephen Smalley wrote: >>> On 06/01/2016 03:27 PM, Casey Schaufler wrote: >>>> Subject: [PATCH] LSM: Reorder security_capset to do access checks properly >>>> >>>> The security module hooks that check whether a process should >>>> be able to set a new capset are currently called after the new >>>> values are set in cap_capset(). This change reverses the order. >>>> The capability module no longer adds cap_capset to the module list. >>>> Instead, it is invoked directly by the LSM infrastructure. >>>> This isn't an approach that generalizes well. >>> Is this change necessary? The fact that cap_capset() modifies new >>> before the other hooks are called does no harm, because if any hook >>> returns an error, then the caller returns that error and never commits >>> the new cred. It is actually possibly beneficial for the other security >>> hooks to be called after cap_capset() so that they can adjust the new >>> values if desired (e.g. to reduce them) before they are finally committed. >> The existing code will set the new credential values before the >> security modules do their checks. Even if it's harmless, it's sloppy. >> Currently there's only one caller, but with Serge's work on ns_capabilities >> I'm looking to make this safer. > It's intentional. cap_capset() does two things: it validates the > proposed capability sets (a permission check, returning -EPERM on > failure) and if valid under its own logic, it then updates new. But the > update does not take effect until the caller of security_capset() calls > commit_creds() and that only happens if all of the hooks pass. By > moving cap_capset() to the end, you are reversing the order of checks > from the norm (DAC before MAC) and you aren't allowing other security > modules to vet and possibly reduce new further. Also, it is obvious > from the patch below that doing so requires a massive hack to what was > otherwise working fine for stacking. > > If you are going to insist on reversing the order, then I think you need > to split security_capset into two hooks, one which only validates and > one which sets, and only use your alternative ordering for the latter. > But that's a lot of work for no apparent gain... Point. I'll drop this. We'll worry about it if it ever actually becomes an issue. Thanks for the comments. > >>>> Signed-off-by: Casey Schaufler >>>> --- >>>> security/commoncap.c | 2 +- >>>> security/security.c | 24 ++++++++++++++++++++++-- >>>> 2 files changed, 23 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/security/commoncap.c b/security/commoncap.c >>>> index 48071ed..f5bce18 100644 >>>> --- a/security/commoncap.c >>>> +++ b/security/commoncap.c >>>> @@ -1073,7 +1073,7 @@ struct security_hook_list capability_hooks[] = { >>>> LSM_HOOK_INIT(ptrace_access_check, cap_ptrace_access_check), >>>> LSM_HOOK_INIT(ptrace_traceme, cap_ptrace_traceme), >>>> LSM_HOOK_INIT(capget, cap_capget), >>>> - LSM_HOOK_INIT(capset, cap_capset), >>>> + /* Carefull! Do not include cap_capset! */ >>>> LSM_HOOK_INIT(bprm_set_creds, cap_bprm_set_creds), >>>> LSM_HOOK_INIT(bprm_secureexec, cap_bprm_secureexec), >>>> LSM_HOOK_INIT(inode_need_killpriv, cap_inode_need_killpriv), >>>> diff --git a/security/security.c b/security/security.c >>>> index 92cd1d8..1610be8 100644 >>>> --- a/security/security.c >>>> +++ b/security/security.c >>>> @@ -177,8 +177,28 @@ int security_capset(struct cred *new, const struct cred *old, >>>> const kernel_cap_t *inheritable, >>>> const kernel_cap_t *permitted) >>>> { >>>> - return call_int_hook(capset, 0, new, old, >>>> - effective, inheritable, permitted); >>>> + struct security_hook_list *hp; >>>> + int rc; >>>> + >>>> + /* >>>> + * Special case handling because the "new" capabilities >>>> + * should not be set until it has been determined that >>>> + * all modules approve of the change. Passing NULL pointers >>>> + * to all modules except the capabilty module as it is >>>> + * expected that only the capability modules needs the >>>> + * result pointers. >>>> + * >>>> + * cap_capset() must not be in the capability module hook list! >>>> + */ >>>> + list_for_each_entry(hp, &security_hook_heads.capset, list) { >>>> + rc = hp->hook.capset(new, old, NULL, NULL, NULL); >>>> + if (rc != 0) >>>> + return rc; >>>> + } >>>> + /* >>>> + * Call cap_capset now to update the new capset. >>>> + */ >>>> + return cap_capset(new, old, effective, inheritable, permitted); >>>> } >>>> >>>> int security_capable(const struct cred *cred, struct user_namespace *ns, >>>> >>>> _______________________________________________ >>>> Selinux mailing list >>>> Selinux@tycho.nsa.gov >>>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. >>>> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov. >>>> >