[1/5] exec: Call cap_bprm_set_creds directly from prepare_binprm
diff mbox series

Message ID 87pnbczyka.fsf_-_@x220.int.ebiederm.org
State New
Headers show
Series
  • exec: Control flow simplifications
Related show

Commit Message

Eric W. Biederman May 9, 2020, 7:40 p.m. UTC
The function cap_bprm_set_creds is the only instance of
security_bprm_set_creds that does something for the primary executable
file and for every interpreter the rest of the implementations of
security_bprm_set_creds do something only for the primary executable
file even if that file is a shell script.

The function cap_bprm_set_creds is also special in that it is called
even when CONFIG_SECURITY is unset.

So calling cap_bprm_set_creds separately to make these two cases explicit,
and allow future changes to take advantages of these differences
to simplify the code.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/exec.c                | 4 ++++
 include/linux/security.h | 2 +-
 security/commoncap.c     | 1 -
 3 files changed, 5 insertions(+), 2 deletions(-)

Comments

Linus Torvalds May 9, 2020, 8:04 p.m. UTC | #1
On Sat, May 9, 2020 at 12:44 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> The function cap_bprm_set_creds is the only instance of
> security_bprm_set_creds that does something for the primary executable
> file and for every interpreter the rest of the implementations of
> security_bprm_set_creds do something only for the primary executable
> file even if that file is a shell script.

Eric, can you please re-write that sentence as something that can be
parsed and understood?

I'm pretty sure that what you are talking about is the whole
"called_set_creds" flag logic, where the logic is that some security
layers only react to the first one, while the capability checks are
done for every one.

But there is no way to realize that from your description above. In
fact, the description above is actively incorrect and misleading,
since you say that "cap_bprm_set_creds is the only instance [..] that
does something for the primary executable"

I think that you mean to say that it does something for *every*
instance of the executable, not just the primary one.

> The function cap_bprm_set_creds is also special in that it is called
> even when CONFIG_SECURITY is unset.
>
> So calling cap_bprm_set_creds separately to make these two cases explicit,
> and allow future changes to take advantages of these differences
> to simplify the code.

I think you need to rename "security_bprm_set_creds()" too, to show
what it does. Since it clearly no longer does that "bprm_set_creds()"
from the common capabilities.

In fact, I think it would probably be good to change the patch too, so
that it is actually understandable what the heck the logic is.

Instead of

        retval = security_bprm_set_creds(bprm);
        if (retval)
                return retval;
        bprm->called_set_creds = 1;
        retval = cap_bprm_set_creds(bprm);
        if (retval)
                return retval;

which makes no sense at all when you read it, do this:

        /* Every instance of the executable gets called for capabilities */
        retval = cap_bprm_set_creds(bprm);
        if (retval)
                return retval;

        /* Other security layers only want the primary executable */
        if (!bprm->called_set_creds) {
                retval = security_primary_bprm_set_creds(bprm);
                if (retval)
                         return retval;
                bprm->called_set_creds = 1;
        }

which now actually describes what is going on.

Then remove the 'called_set_creds' logic from the security layers, and
rename those 'xyz_bprm_set_creds()' to be
'xyz_primary_bprm_set_creds()'.

After that, and with a proper commit message that actually explains
this _properly_, this looks like a cleanup.

Because right now that patch description makes zero sense at all, and
the patch itself results in this insane situation where
"security_bprm_set_creds()" expressly doesn't call the basic
"cap_bprm_set_creds()" at all, which just makes things very very
confusing and the naming actively misleading.

               Linus

Patch
diff mbox series

diff --git a/fs/exec.c b/fs/exec.c
index b0620d5ebc66..765bfd51a546 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1641,6 +1641,10 @@  int prepare_binprm(struct linux_binprm *bprm)
 		return retval;
 	bprm->called_set_creds = 1;
 
+	retval = cap_bprm_set_creds(bprm);
+	if (retval)
+		return retval;
+
 	memset(bprm->buf, 0, BINPRM_BUF_SIZE);
 	return kernel_read(bprm->file, bprm->buf, BINPRM_BUF_SIZE, &pos);
 }
diff --git a/include/linux/security.h b/include/linux/security.h
index a8d9310472df..c1aa1638429a 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -571,7 +571,7 @@  static inline int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
 
 static inline int security_bprm_set_creds(struct linux_binprm *bprm)
 {
-	return cap_bprm_set_creds(bprm);
+	return 0;
 }
 
 static inline int security_bprm_check(struct linux_binprm *bprm)
diff --git a/security/commoncap.c b/security/commoncap.c
index f4ee0ae106b2..3757988abe42 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -1346,7 +1346,6 @@  static struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(ptrace_traceme, cap_ptrace_traceme),
 	LSM_HOOK_INIT(capget, cap_capget),
 	LSM_HOOK_INIT(capset, cap_capset),
-	LSM_HOOK_INIT(bprm_set_creds, cap_bprm_set_creds),
 	LSM_HOOK_INIT(inode_need_killpriv, cap_inode_need_killpriv),
 	LSM_HOOK_INIT(inode_killpriv, cap_inode_killpriv),
 	LSM_HOOK_INIT(inode_getsecurity, cap_inode_getsecurity),