From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.7 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, MENTIONS_GIT_HOSTING,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 95D8CC433E0 for ; Sat, 30 May 2020 03:32:39 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 57B2C208E4 for ; Sat, 30 May 2020 03:32:39 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728762AbgE3Dci (ORCPT ); Fri, 29 May 2020 23:32:38 -0400 Received: from out03.mta.xmission.com ([166.70.13.233]:57630 "EHLO out03.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728406AbgE3Dch (ORCPT ); Fri, 29 May 2020 23:32:37 -0400 Received: from in01.mta.xmission.com ([166.70.13.51]) by out03.mta.xmission.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1jesEd-0002Yt-NU; Fri, 29 May 2020 21:32:35 -0600 Received: from ip68-227-160-95.om.om.cox.net ([68.227.160.95] helo=x220.xmission.com) by in01.mta.xmission.com with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.87) (envelope-from ) id 1jesEc-0001y8-Q0; Fri, 29 May 2020 21:32:35 -0600 From: ebiederm@xmission.com (Eric W. Biederman) To: Kees Cook Cc: linux-kernel@vger.kernel.org, Linus Torvalds , Oleg Nesterov , Jann Horn , Greg Ungerer , Rob Landley , Bernd Edlinger , linux-fsdevel@vger.kernel.org, Al Viro , Alexey Dobriyan , Andrew Morton , Casey Schaufler , linux-security-module@vger.kernel.org, James Morris , "Serge E. Hallyn" , Andy Lutomirski References: <87h7wujhmz.fsf@x220.int.ebiederm.org> <87sgga6ze4.fsf@x220.int.ebiederm.org> <87v9l4zyla.fsf_-_@x220.int.ebiederm.org> <877dx822er.fsf_-_@x220.int.ebiederm.org> <87k10wysqz.fsf_-_@x220.int.ebiederm.org> <87d06mr8ps.fsf_-_@x220.int.ebiederm.org> <871rn2r8m6.fsf_-_@x220.int.ebiederm.org> <202005291406.52E27AF8@keescook> Date: Fri, 29 May 2020 22:28:41 -0500 In-Reply-To: <202005291406.52E27AF8@keescook> (Kees Cook's message of "Fri, 29 May 2020 14:24:46 -0700") Message-ID: <875zcenlsm.fsf@x220.int.ebiederm.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1jesEc-0001y8-Q0;;;mid=<875zcenlsm.fsf@x220.int.ebiederm.org>;;;hst=in01.mta.xmission.com;;;ip=68.227.160.95;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX19+t4UEQteAa1xI/LIkN0ljhGBleMTZt7U= X-SA-Exim-Connect-IP: 68.227.160.95 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [PATCH 2/2] exec: Compute file based creds only once X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in01.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Kees Cook writes: > On Fri, May 29, 2020 at 11:47:29AM -0500, Eric W. Biederman wrote: >> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >> index cd3dd0afceb5..37bb3df751c6 100644 >> --- a/include/linux/lsm_hooks.h >> +++ b/include/linux/lsm_hooks.h >> @@ -44,18 +44,18 @@ >> * request libc enable secure mode. >> - * The hook must set @bprm->pf_per_clear to the personality flags that >> + * The hook must set @bprm->per_clear to the personality flags that > > Here and the other per_clear comment have language that doesn't quite > line up with how hooks should deal with the bits. They should not "set > it to" the personality flags they want clear, they need to "add the > bits" they want to see cleared. i.e I don't want something thinking > they're the only one touching per_clear, so they should never do: > bprm->per_clear = PER_CLEAR_ON_SETID; > but always: > bprm->per_clear |= PER_CLEAR_ON_SETID; > > How about: > > The hook must set @bprm->per_clear with any personality flag bits that Sounds good: The range-diff winds up being: 1: c9258ef4879b ! 1: a7868323c263 exec: Add a per bprm->file version of per_clear @@ Commit message History Tree: git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git Fixes: 1bb0fa189c6a ("[PATCH] NX: clean up legacy binary support") + Reviewed-by: Kees Cook Signed-off-by: "Eric W. Biederman" ## fs/exec.c ## @@ include/linux/lsm_hooks.h * transitions between security domains). * The hook must set @bprm->active_secureexec to 1 if AT_SECURE should be set to * request libc enable secure mode. -+ * The hook must set @bprm->pf_per_clear to the personality flags that -+ * should be cleared from current->personality. ++ * The hook must add to @bprm->pf_per_clear any personality flags that ++ * should be cleared from current->personality. * @bprm contains the linux_binprm structure. * Return 0 if the hook is successful and permission is granted. * @bprm_check_security: 2: e6f20c69b96e ! 2: 56305aa9b6fa exec: Compute file based creds only once @@ Commit message secureity attribute and derive capabilities from the fact the user had uid 0 has been added. + Reviewed-by: Kees Cook Signed-off-by: "Eric W. Biederman" ## fs/binfmt_misc.c ## @@ include/linux/lsm_hooks.h + * between security domains). + * The hook must set @bprm->secureexec to 1 if AT_SECURE should be set to * request libc enable secure mode. -- * The hook must set @bprm->pf_per_clear to the personality flags that -+ * The hook must set @bprm->per_clear to the personality flags that - * should be cleared from current->personality. +- * The hook must add to @bprm->pf_per_clear any personality flags that ++ * The hook must add to @bprm->per_clear any personality flags that + * should be cleared from current->personality. * @bprm contains the linux_binprm structure. * Return 0 if the hook is successful and permission is granted. >> diff --git a/security/commoncap.c b/security/commoncap.c > > Not about this patch, but while looking through this file, I see: > > int cap_bprm_set_creds(struct linux_binprm *bprm) > { > ... > *capability manipulations* > > if (WARN_ON(!cap_ambient_invariant_ok(new))) > return -EPERM; > > if (nonroot_raised_pE(new, old, root_uid, has_fcap)) { > ret = audit_log_bprm_fcaps(bprm, new, old); > if (ret < 0) > return ret; > } > > new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS); > > if (WARN_ON(!cap_ambient_invariant_ok(new))) > return -EPERM; > ... > } > > The cap_ambient_invariant_ok() test is needlessly repeated: it doesn't > examine securebits, and nonroot_raised_pE appears to have no > side-effects. > > One of those can be dropped, yes? That is what it looks like to me. I am hoping to take a deep dive into this function after I finish with bprm_fill_uid (the patches that were dropped). My brain bends on little details like is_setid not testing if the excutable was suid or sgid, but instead is testing something close but unrelated. I hope that when the dust clears the function can become a straightforward implementation of the capability equations. We will see. Eric