Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   ebiederm@xmission.com (Eric W. Biederman)
To:     Kees Cook <keescook@chromium.org>
Cc:     linux-kernel@vger.kernel.org,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Oleg Nesterov <oleg@redhat.com>, Jann Horn <jannh@google.com>,
        Greg Ungerer <gerg@linux-m68k.org>,
        Rob Landley <rob@landley.net>,
        Bernd Edlinger <bernd.edlinger@hotmail.de>,
        linux-fsdevel@vger.kernel.org, Al Viro <viro@ZenIV.linux.org.uk>,
        Alexey Dobriyan <adobriyan@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Casey Schaufler <casey@schaufler-ca.com>,
        linux-security-module@vger.kernel.org,
        James Morris <jmorris@namei.org>,
        "Serge E. Hallyn" <serge@hallyn.com>,
        Andy Lutomirski <luto@amacapital.net>
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
Subject: Re: [PATCH 2/2] exec: Compute file based creds only once
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Kees Cook <keescook@chromium.org> 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 <keescook@chromium.org>
         Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
     
      ## 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 <keescook@chromium.org>
         Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
     
      ## 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