From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751559AbcFAUhe (ORCPT ); Wed, 1 Jun 2016 16:37:34 -0400 Received: from emsm-gh1-uea11.nsa.gov ([8.44.101.9]:29315 "EHLO emsm-gh1-uea11.nsa.gov" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750850AbcFAUhb (ORCPT ); Wed, 1 Jun 2016 16:37:31 -0400 X-IronPort-AV: E=Sophos;i="5.26,403,1459814400"; d="scan'208";a="16576159" IronPort-PHdr: =?us-ascii?q?9a23=3AGrQD2BTag6kMCxEdWW0SbGuXFtpsv+yvbD5Q0YIu?= =?us-ascii?q?jvd0So/mwa65YxKN2/xhgRfzUJnB7Loc0qyN4/GmBDNLuMrb+Fk5M7VyFDY9wf?= =?us-ascii?q?0MmAIhBMPXQWbaF9XNKxIAIcJZSVV+9Gu6O0UGUOz3ZlnVv2HgpWVKQka3CwN5?= =?us-ascii?q?K6zPF5LIiIzvjqbpq8yVPloD3WbnKZpJbzyI7izp/vEMhoVjLqtjgjDomVBvP9?= =?us-ascii?q?ps+GVzOFiIlAz97MrjtLRq8iBXpu5zv5UYCfayV+0CQLdZFDUrNXwurI2u7EGb?= =?us-ascii?q?DFiy/H9UbkE62loNWVCEv1nGWcLauzD3p6JG0ymTIMPyQKp8DT+r9Kp6YATjiC?= =?us-ascii?q?4aOTo06iTcg4p7i6cN5FqFnDk3l4rVZpyFceFzdb7HfM8LAG9GUttVWgRfDY6m?= =?us-ascii?q?KYgCFewMOaBftYar4xMspB6zHkGJA/n1yyQA0n3z2rcg0v8JFwjD0Q1mGMgB5i?= =?us-ascii?q?f6ttLwYZwOXPi1wa+A9jDKa/dbyH+p84TTWgwwqvGLG7RreIzezld5RFCNtUmZ?= =?us-ascii?q?tYGwZ2Dd7e8KqWXOqrM6De8=3D?= X-IPAS-Result: =?us-ascii?q?A2G0AwD8Rk9X/wHyM5BdGgEBAQGDH4EBUqVlAQECAQEGkh2?= =?us-ascii?q?EF4YRAoEzTAEBAQEBAQICYieCMIIVAQEBAQMjBCceAggDEAsNCgECAiYCAjEVC?= =?us-ascii?q?QgGAQcFBgIBARkEiA6wN5EvAQEBAQEBAQEBAQEBAQEBAQEggQGEXoI/glaEEhE?= =?us-ascii?q?Bgx2CWQWYN4h4hSiBaYdvhURFhW6JGWKECSAyiHUNFweBFwEBAQ?= Subject: Re: [PATCH] LSM: Reorder security_capset to do access checks properly To: Casey Schaufler , 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: Stephen Smalley Organization: National Security Agency Message-ID: Date: Wed, 1 Jun 2016 16:38:59 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; 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 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... > >> >>> 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. >>> >> >