| Message ID | 871rn2r8m6.fsf_-_@x220.int.ebiederm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | exec: Remove the computation of bprm->cred | expand |
On Fri, May 29, 2020 at 11:47:29AM -0500, Eric W. Biederman wrote: > Move the computation of creds from prepare_binfmt into begin_new_exec > so that the creds need only be computed once. This is just code > reorganization no semantic changes of any kind are made. > > Moving the computation is safe. I have looked through the kernel and > verified none of the binfmts look at bprm->cred directly, and that > there are no helpers that look at bprm->cred indirectly. Which means > that it is not a problem to compute the bprm->cred later in the > execution flow as it is not used until it becomes current->cred. > > A new function bprm_creds_from_file is added to contain the work that > needs to be done. bprm_creds_from_file first computes which file > bprm->executable or most likely bprm->file that the bprm->creds > will be computed from. > > The funciton bprm_fill_uid is updated to receive the file instead of > accessing bprm->file. The now unnecessary work needed to reset the > bprm->cred->euid, and bprm->cred->egid is removed from brpm_fill_uid. > A small comment to document that bprm_fill_uid now only deals with the > work to handle suid and sgid files. The default case is already > heandled by prepare_exec_creds. > > The function security_bprm_repopulate_creds is renamed > security_bprm_creds_from_file and now is explicitly passed the file > from which to compute the creds. The documentation of the > bprm_creds_from_file security hook is updated to explain when the hook > is called and what it needs to do. The file is passed from > cap_bprm_creds_from_file into get_file_caps so that the caps are > computed for the appropriate file. The now unnecessary work in > cap_bprm_creds_from_file to reset the ambient capabilites has been > removed. A small comment to document that the work of > cap_bprm_creds_from_file is to read capabilities from the files > secureity attribute and derive capabilities from the fact the > user had uid 0 has been added. > > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> This all looks good to me. Small notes below... Reviewed-by: Kees Cook <keescook@chromium.org> > 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. > * @bprm contains the linux_binprm structure. > * Return 0 if the hook is successful and permission is granted. > - * @bprm_repopulate_creds: > - * Assuming that the relevant bits of @bprm->cred->security have been > - * previously set, examine @bprm->file and regenerate them. This is > - * so that the credentials derived from the interpreter the code is > - * actually going to run are used rather than credentials derived > - * from a script. This done because the interpreter binary needs to > - * reopen script, and may end up opening something completely different. > - * This hook may also optionally check permissions (e.g. for > - * transitions between security domains). > - * The hook must set @bprm->active_secureexec to 1 if AT_SECURE should be set to > + * @bprm_creds_from_file: > + * If @file is setpcap, suid, sgid or otherwise marked to change > + * privilege upon exec, update @bprm->cred to reflect that change. > + * This is called after finding the binary that will be executed. > + * without an interpreter. This ensures that the credentials will not > + * be derived from a script that the binary will need to reopen, which > + * when reopend may end up being a completely different file. This > + * hook may also optionally check permissions (e.g. for transitions > + * 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 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 > 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?
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
On Fri, May 29, 2020 at 10:28:41PM -0500, Eric W. Biederman wrote: > 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. Awesome; thanks! > > 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. Okay, cool. I was worried I was missing something in the mess of tiny helper calls. :) > I hope that when the dust clears the function can become a > straightforward implementation of the capability equations. > We will see. Yeah, this looks better and better every day! I'm glad you're able to dig through all of this.
diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c index 53968ea07b57..bc5506619b7e 100644 --- a/fs/binfmt_misc.c +++ b/fs/binfmt_misc.c @@ -192,7 +192,7 @@ static int load_misc_binary(struct linux_binprm *bprm) bprm->interpreter = interp_file; if (fmt->flags & MISC_FMT_CREDENTIALS) - bprm->preserve_creds = 1; + bprm->execfd_creds = 1; retval = 0; ret: diff --git a/fs/exec.c b/fs/exec.c index 0f793536e393..e8599236290d 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -72,6 +72,8 @@ #include <trace/events/sched.h> +static int bprm_creds_from_file(struct linux_binprm *bprm); + int suid_dumpable = 0; static LIST_HEAD(formats); @@ -1304,6 +1306,11 @@ int begin_new_exec(struct linux_binprm * bprm) struct task_struct *me = current; int retval; + /* Once we are committed compute the creds */ + retval = bprm_creds_from_file(bprm); + if (retval) + return retval; + /* * Ensure all future errors are fatal. */ @@ -1354,7 +1361,6 @@ int begin_new_exec(struct linux_binprm * bprm) me->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD | PF_NOFREEZE | PF_NO_SETAFFINITY); flush_thread(); - bprm->per_clear |= bprm->pf_per_clear; me->personality &= ~bprm->per_clear; /* @@ -1365,13 +1371,6 @@ int begin_new_exec(struct linux_binprm * bprm) */ do_close_on_exec(me->files); - /* - * Once here, prepare_binrpm() will not be called any more, so - * the final state of setuid/setgid/fscaps can be merged into the - * secureexec flag. - */ - bprm->secureexec |= bprm->active_secureexec; - if (bprm->secureexec) { /* Make sure parent cannot signal privileged process. */ me->pdeath_signal = 0; @@ -1587,29 +1586,21 @@ static void check_unsafe_exec(struct linux_binprm *bprm) spin_unlock(&p->fs->lock); } -static void bprm_fill_uid(struct linux_binprm *bprm) +static void bprm_fill_uid(struct linux_binprm *bprm, struct file *file) { + /* Handle suid and sgid on files */ struct inode *inode; unsigned int mode; kuid_t uid; kgid_t gid; - /* - * Since this can be called multiple times (via prepare_binprm), - * we must clear any previous work done when setting set[ug]id - * bits from any earlier bprm->file uses (for example when run - * first for a setuid script then again for its interpreter). - */ - bprm->cred->euid = current_euid(); - bprm->cred->egid = current_egid(); - - if (!mnt_may_suid(bprm->file->f_path.mnt)) + if (!mnt_may_suid(file->f_path.mnt)) return; if (task_no_new_privs(current)) return; - inode = bprm->file->f_path.dentry->d_inode; + inode = file->f_path.dentry->d_inode; mode = READ_ONCE(inode->i_mode); if (!(mode & (S_ISUID|S_ISGID))) return; @@ -1629,19 +1620,31 @@ static void bprm_fill_uid(struct linux_binprm *bprm) return; if (mode & S_ISUID) { - bprm->pf_per_clear |= PER_CLEAR_ON_SETID; + bprm->per_clear |= PER_CLEAR_ON_SETID; bprm->cred->euid = uid; } if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) { - bprm->pf_per_clear |= PER_CLEAR_ON_SETID; + bprm->per_clear |= PER_CLEAR_ON_SETID; bprm->cred->egid = gid; } } +/* + * Compute brpm->cred based upon the final binary. + */ +static int bprm_creds_from_file(struct linux_binprm *bprm) +{ + /* Compute creds based on which file? */ + struct file *file = bprm->execfd_creds ? bprm->executable : bprm->file; + + bprm_fill_uid(bprm, file); + return security_bprm_creds_from_file(bprm, file); +} + /* * Fill the binprm structure from the inode. - * Check permissions, then read the first BINPRM_BUF_SIZE bytes + * Read the first BINPRM_BUF_SIZE bytes * * This may be called multiple times for binary chains (scripts for example). */ @@ -1649,20 +1652,6 @@ static int prepare_binprm(struct linux_binprm *bprm) { loff_t pos = 0; - /* Can the interpreter get to the executable without races? */ - if (!bprm->preserve_creds) { - int retval; - - /* Recompute parts of bprm->cred based on bprm->file */ - bprm->active_secureexec = 0; - bprm->pf_per_clear = 0; - bprm_fill_uid(bprm); - retval = security_bprm_repopulate_creds(bprm); - if (retval) - return retval; - } - bprm->preserve_creds = 0; - memset(bprm->buf, 0, BINPRM_BUF_SIZE); return kernel_read(bprm->file, bprm->buf, BINPRM_BUF_SIZE, &pos); } diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h index 50025ead0b72..aece1b340e7d 100644 --- a/include/linux/binfmts.h +++ b/include/linux/binfmts.h @@ -29,13 +29,8 @@ struct linux_binprm { /* Should an execfd be passed to userspace? */ have_execfd:1, - /* It is safe to use the creds of a script (see binfmt_misc) */ - preserve_creds:1, - /* - * True if most recent call to security_bprm_set_creds - * resulted in elevated privileges. - */ - active_secureexec:1, + /* Use the creds of a script (see binfmt_misc) */ + execfd_creds:1, /* * Set by bprm_creds_for_exec hook to indicate a * privilege-gaining exec has happened. Used to set @@ -55,11 +50,6 @@ struct linux_binprm { struct file * file; struct cred *cred; /* new credentials */ int unsafe; /* how unsafe this exec is (mask of LSM_UNSAFE_*) */ - /* - * bits to clear in current->personality - * recalculated for each bprm->file. - */ - unsigned int pf_per_clear; unsigned int per_clear; /* bits to clear in current->personality */ int argc, envc; const char * filename; /* Name of binary as seen by procps */ diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index 1e295ba12c0d..adbc6603abba 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -50,7 +50,7 @@ LSM_HOOK(int, 0, settime, const struct timespec64 *ts, const struct timezone *tz) LSM_HOOK(int, 0, vm_enough_memory, struct mm_struct *mm, long pages) LSM_HOOK(int, 0, bprm_creds_for_exec, struct linux_binprm *bprm) -LSM_HOOK(int, 0, bprm_repopulate_creds, struct linux_binprm *bprm) +LSM_HOOK(int, 0, bprm_creds_from_file, struct linux_binprm *bprm, struct file *file) LSM_HOOK(int, 0, bprm_check_security, struct linux_binprm *bprm) LSM_HOOK(void, LSM_RET_VOID, bprm_committing_creds, struct linux_binprm *bprm) LSM_HOOK(void, LSM_RET_VOID, bprm_committed_creds, struct linux_binprm *bprm) 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. * @bprm contains the linux_binprm structure. * Return 0 if the hook is successful and permission is granted. - * @bprm_repopulate_creds: - * Assuming that the relevant bits of @bprm->cred->security have been - * previously set, examine @bprm->file and regenerate them. This is - * so that the credentials derived from the interpreter the code is - * actually going to run are used rather than credentials derived - * from a script. This done because the interpreter binary needs to - * reopen script, and may end up opening something completely different. - * This hook may also optionally check permissions (e.g. for - * transitions between security domains). - * The hook must set @bprm->active_secureexec to 1 if AT_SECURE should be set to + * @bprm_creds_from_file: + * If @file is setpcap, suid, sgid or otherwise marked to change + * privilege upon exec, update @bprm->cred to reflect that change. + * This is called after finding the binary that will be executed. + * without an interpreter. This ensures that the credentials will not + * be derived from a script that the binary will need to reopen, which + * when reopend may end up being a completely different file. This + * hook may also optionally check permissions (e.g. for transitions + * 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. * @bprm contains the linux_binprm structure. * Return 0 if the hook is successful and permission is granted. diff --git a/include/linux/security.h b/include/linux/security.h index 6dcec9375e8f..8444fae7c5b9 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -140,7 +140,7 @@ extern int cap_capset(struct cred *new, const struct cred *old, const kernel_cap_t *effective, const kernel_cap_t *inheritable, const kernel_cap_t *permitted); -extern int cap_bprm_repopulate_creds(struct linux_binprm *bprm); +extern int cap_bprm_creds_from_file(struct linux_binprm *bprm, struct file *file); extern int cap_inode_setxattr(struct dentry *dentry, const char *name, const void *value, size_t size, int flags); extern int cap_inode_removexattr(struct dentry *dentry, const char *name); @@ -277,7 +277,7 @@ int security_syslog(int type); int security_settime64(const struct timespec64 *ts, const struct timezone *tz); int security_vm_enough_memory_mm(struct mm_struct *mm, long pages); int security_bprm_creds_for_exec(struct linux_binprm *bprm); -int security_bprm_repopulate_creds(struct linux_binprm *bprm); +int security_bprm_creds_from_file(struct linux_binprm *bprm, struct file *file); int security_bprm_check(struct linux_binprm *bprm); void security_bprm_committing_creds(struct linux_binprm *bprm); void security_bprm_committed_creds(struct linux_binprm *bprm); @@ -575,9 +575,10 @@ static inline int security_bprm_creds_for_exec(struct linux_binprm *bprm) return 0; } -static inline int security_bprm_repopulate_creds(struct linux_binprm *bprm) +static inline int security_bprm_creds_from_file(struct linux_binprm *bprm, + struct file *file) { - return cap_bprm_repopulate_creds(bprm); + return cap_bprm_creds_from_file(bprm, file); } static inline int security_bprm_check(struct linux_binprm *bprm) diff --git a/security/commoncap.c b/security/commoncap.c index 6de72d22dc6c..59bf3c1674c8 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -647,7 +647,8 @@ int get_vfs_caps_from_disk(const struct dentry *dentry, struct cpu_vfs_cap_data * its xattrs and, if present, apply them to the proposed credentials being * constructed by execve(). */ -static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_fcap) +static int get_file_caps(struct linux_binprm *bprm, struct file *file, + bool *effective, bool *has_fcap) { int rc = 0; struct cpu_vfs_cap_data vcaps; @@ -657,7 +658,7 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f if (!file_caps_enabled) return 0; - if (!mnt_may_suid(bprm->file->f_path.mnt)) + if (!mnt_may_suid(file->f_path.mnt)) return 0; /* @@ -665,10 +666,10 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f * explicit that capability bits are limited to s_user_ns and its * descendants. */ - if (!current_in_userns(bprm->file->f_path.mnt->mnt_sb->s_user_ns)) + if (!current_in_userns(file->f_path.mnt->mnt_sb->s_user_ns)) return 0; - rc = get_vfs_caps_from_disk(bprm->file->f_path.dentry, &vcaps); + rc = get_vfs_caps_from_disk(file->f_path.dentry, &vcaps); if (rc < 0) { if (rc == -EINVAL) printk(KERN_NOTICE "Invalid argument reading file caps for %s\n", @@ -797,26 +798,27 @@ static inline bool nonroot_raised_pE(struct cred *new, const struct cred *old, } /** - * cap_bprm_repopulate_creds - Set up the proposed credentials for execve(). + * cap_bprm_creds_from_file - Set up the proposed credentials for execve(). * @bprm: The execution parameters, including the proposed creds + * @file: The file to pull the credentials from * * Set up the proposed credentials for a new execution context being * constructed by execve(). The proposed creds in @bprm->cred is altered, * which won't take effect immediately. Returns 0 if successful, -ve on error. */ -int cap_bprm_repopulate_creds(struct linux_binprm *bprm) +int cap_bprm_creds_from_file(struct linux_binprm *bprm, struct file *file) { + /* Process setpcap binaries and capabilities for uid 0 */ const struct cred *old = current_cred(); struct cred *new = bprm->cred; bool effective = false, has_fcap = false, is_setid; int ret; kuid_t root_uid; - new->cap_ambient = old->cap_ambient; if (WARN_ON(!cap_ambient_invariant_ok(old))) return -EPERM; - ret = get_file_caps(bprm, &effective, &has_fcap); + ret = get_file_caps(bprm, file, &effective, &has_fcap); if (ret < 0) return ret; @@ -826,7 +828,7 @@ int cap_bprm_repopulate_creds(struct linux_binprm *bprm) /* if we have fs caps, clear dangerous personality flags */ if (__cap_gained(permitted, new, old)) - bprm->pf_per_clear |= PER_CLEAR_ON_SETID; + bprm->per_clear |= PER_CLEAR_ON_SETID; /* Don't let someone trace a set[ug]id/setpcap binary with the revised * credentials unless they have the appropriate permit. @@ -889,7 +891,7 @@ int cap_bprm_repopulate_creds(struct linux_binprm *bprm) (!__is_real(root_uid, new) && (effective || __cap_grew(permitted, ambient, new)))) - bprm->active_secureexec = 1; + bprm->secureexec = 1; return 0; } @@ -1346,7 +1348,7 @@ 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_repopulate_creds, cap_bprm_repopulate_creds), + LSM_HOOK_INIT(bprm_creds_from_file, cap_bprm_creds_from_file), 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), diff --git a/security/security.c b/security/security.c index b890b7e2a765..259b8e750aa2 100644 --- a/security/security.c +++ b/security/security.c @@ -828,9 +828,9 @@ int security_bprm_creds_for_exec(struct linux_binprm *bprm) return call_int_hook(bprm_creds_for_exec, 0, bprm); } -int security_bprm_repopulate_creds(struct linux_binprm *bprm) +int security_bprm_creds_from_file(struct linux_binprm *bprm, struct file *file) { - return call_int_hook(bprm_repopulate_creds, 0, bprm); + return call_int_hook(bprm_creds_from_file, 0, bprm, file); } int security_bprm_check(struct linux_binprm *bprm)
Move the computation of creds from prepare_binfmt into begin_new_exec so that the creds need only be computed once. This is just code reorganization no semantic changes of any kind are made. Moving the computation is safe. I have looked through the kernel and verified none of the binfmts look at bprm->cred directly, and that there are no helpers that look at bprm->cred indirectly. Which means that it is not a problem to compute the bprm->cred later in the execution flow as it is not used until it becomes current->cred. A new function bprm_creds_from_file is added to contain the work that needs to be done. bprm_creds_from_file first computes which file bprm->executable or most likely bprm->file that the bprm->creds will be computed from. The funciton bprm_fill_uid is updated to receive the file instead of accessing bprm->file. The now unnecessary work needed to reset the bprm->cred->euid, and bprm->cred->egid is removed from brpm_fill_uid. A small comment to document that bprm_fill_uid now only deals with the work to handle suid and sgid files. The default case is already heandled by prepare_exec_creds. The function security_bprm_repopulate_creds is renamed security_bprm_creds_from_file and now is explicitly passed the file from which to compute the creds. The documentation of the bprm_creds_from_file security hook is updated to explain when the hook is called and what it needs to do. The file is passed from cap_bprm_creds_from_file into get_file_caps so that the caps are computed for the appropriate file. The now unnecessary work in cap_bprm_creds_from_file to reset the ambient capabilites has been removed. A small comment to document that the work of cap_bprm_creds_from_file is to read capabilities from the files secureity attribute and derive capabilities from the fact the user had uid 0 has been added. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- fs/binfmt_misc.c | 2 +- fs/exec.c | 63 +++++++++++++++-------------------- include/linux/binfmts.h | 14 ++------ include/linux/lsm_hook_defs.h | 2 +- include/linux/lsm_hooks.h | 22 ++++++------ include/linux/security.h | 9 ++--- security/commoncap.c | 24 +++++++------ security/security.c | 4 +-- 8 files changed, 61 insertions(+), 79 deletions(-)