| Message ID | 161377734508.87807.8537642254664217815.stgit@sifl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Split security_task_getsecid() into subj and obj variants | expand |
On Fri, 19 Feb 2021, Paul Moore wrote: > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > index c119736ca56ac..39d501261108d 100644 > --- a/drivers/android/binder.c > +++ b/drivers/android/binder.c > @@ -2700,7 +2700,7 @@ static void binder_transaction(struct binder_proc *proc, > u32 secid; > size_t added_size; > > - security_task_getsecid(proc->tsk, &secid); > + security_task_getsecid_subj(proc->tsk, &secid); > ret = security_secid_to_secctx(secid, &secctx, &secctx_sz); > if (ret) { > return_error = BR_FAILED_REPLY; Can someone from the Android project confirm this is correct for binder?
On Fri, Feb 19, 2021 at 9:57 PM James Morris <jmorris@namei.org> wrote: > On Fri, 19 Feb 2021, Paul Moore wrote: > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > > index c119736ca56ac..39d501261108d 100644 > > --- a/drivers/android/binder.c > > +++ b/drivers/android/binder.c > > @@ -2700,7 +2700,7 @@ static void binder_transaction(struct binder_proc *proc, > > u32 secid; > > size_t added_size; > > > > - security_task_getsecid(proc->tsk, &secid); > > + security_task_getsecid_subj(proc->tsk, &secid); > > ret = security_secid_to_secctx(secid, &secctx, &secctx_sz); > > if (ret) { > > return_error = BR_FAILED_REPLY; > > Can someone from the Android project confirm this is correct for binder? Yes, please take a look Android folks. As I mentioned previously, review of the binder changes is one area where I think some extra review is needed; I'm just not confident enough in my understanding of binder.
On 2/19/21 3:29 PM, Paul Moore wrote: > Of the three LSMs that implement the security_task_getsecid() LSM > hook, all three LSMs provide the task's objective security > credentials. This turns out to be unfortunate as most of the hook's > callers seem to expect the task's subjective credentials, although > a small handful of callers do correctly expect the objective > credentials. > > This patch is the first step towards fixing the problem: it splits > the existing security_task_getsecid() hook into two variants, one > for the subjective creds, one for the objective creds. > > void security_task_getsecid_subj(struct task_struct *p, > u32 *secid); > void security_task_getsecid_obj(struct task_struct *p, > u32 *secid); > > While this patch does fix all of the callers to use the correct > variant, in order to keep this patch focused on the callers and to > ease review, the LSMs continue to use the same implementation for > both hooks. The net effect is that this patch should not change > the behavior of the kernel in any way, it will be up to the latter > LSM specific patches in this series to change the hook > implementations and return the correct credentials. > > Signed-off-by: Paul Moore <paul@paul-moore.com> So far this looks good. I want to take another stab at it and give it some testing > --- > drivers/android/binder.c | 2 +- > include/linux/cred.h | 2 +- > include/linux/lsm_hook_defs.h | 5 ++++- > include/linux/lsm_hooks.h | 8 ++++++-- > include/linux/security.h | 10 ++++++++-- > kernel/audit.c | 4 ++-- > kernel/auditfilter.c | 3 ++- > kernel/auditsc.c | 8 ++++---- > net/netlabel/netlabel_unlabeled.c | 2 +- > net/netlabel/netlabel_user.h | 2 +- > security/apparmor/lsm.c | 3 ++- > security/integrity/ima/ima_appraise.c | 2 +- > security/integrity/ima/ima_main.c | 14 +++++++------- > security/security.c | 13 ++++++++++--- > security/selinux/hooks.c | 3 ++- > security/smack/smack_lsm.c | 3 ++- > 16 files changed, 54 insertions(+), 30 deletions(-) > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > index c119736ca56ac..39d501261108d 100644 > --- a/drivers/android/binder.c > +++ b/drivers/android/binder.c > @@ -2700,7 +2700,7 @@ static void binder_transaction(struct binder_proc *proc, > u32 secid; > size_t added_size; > > - security_task_getsecid(proc->tsk, &secid); > + security_task_getsecid_subj(proc->tsk, &secid); > ret = security_secid_to_secctx(secid, &secctx, &secctx_sz); > if (ret) { > return_error = BR_FAILED_REPLY; > diff --git a/include/linux/cred.h b/include/linux/cred.h > index 18639c069263f..42b9d88d9a565 100644 > --- a/include/linux/cred.h > +++ b/include/linux/cred.h > @@ -140,7 +140,7 @@ struct cred { > struct key *request_key_auth; /* assumed request_key authority */ > #endif > #ifdef CONFIG_SECURITY > - void *security; /* subjective LSM security */ > + void *security; /* LSM security */ > #endif > struct user_struct *user; /* real user ID subscription */ > struct user_namespace *user_ns; /* user_ns the caps and keyrings are relative to. */ > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h > index dfd261dcbcb04..1490a185135a0 100644 > --- a/include/linux/lsm_hook_defs.h > +++ b/include/linux/lsm_hook_defs.h > @@ -200,7 +200,10 @@ LSM_HOOK(int, 0, task_fix_setgid, struct cred *new, const struct cred * old, > LSM_HOOK(int, 0, task_setpgid, struct task_struct *p, pid_t pgid) > LSM_HOOK(int, 0, task_getpgid, struct task_struct *p) > LSM_HOOK(int, 0, task_getsid, struct task_struct *p) > -LSM_HOOK(void, LSM_RET_VOID, task_getsecid, struct task_struct *p, u32 *secid) > +LSM_HOOK(void, LSM_RET_VOID, task_getsecid_subj, > + struct task_struct *p, u32 *secid) > +LSM_HOOK(void, LSM_RET_VOID, task_getsecid_obj, > + struct task_struct *p, u32 *secid) > LSM_HOOK(int, 0, task_setnice, struct task_struct *p, int nice) > LSM_HOOK(int, 0, task_setioprio, struct task_struct *p, int ioprio) > LSM_HOOK(int, 0, task_getioprio, struct task_struct *p) > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index bdfc8a76a4f79..13d2a9a6f2014 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -706,8 +706,12 @@ > * @p. > * @p contains the task_struct for the process. > * Return 0 if permission is granted. > - * @task_getsecid: > - * Retrieve the security identifier of the process @p. > + * @task_getsecid_subj: > + * Retrieve the subjective security identifier of the process @p. > + * @p contains the task_struct for the process and place is into @secid. > + * In case of failure, @secid will be set to zero. > + * @task_getsecid_obj: > + * Retrieve the objective security identifier of the process @p. > * @p contains the task_struct for the process and place is into @secid. > * In case of failure, @secid will be set to zero. > * > diff --git a/include/linux/security.h b/include/linux/security.h > index b0d14f04b16de..1826bb0cea825 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -406,7 +406,8 @@ int security_task_fix_setgid(struct cred *new, const struct cred *old, > int security_task_setpgid(struct task_struct *p, pid_t pgid); > int security_task_getpgid(struct task_struct *p); > int security_task_getsid(struct task_struct *p); > -void security_task_getsecid(struct task_struct *p, u32 *secid); > +void security_task_getsecid_subj(struct task_struct *p, u32 *secid); > +void security_task_getsecid_obj(struct task_struct *p, u32 *secid); > int security_task_setnice(struct task_struct *p, int nice); > int security_task_setioprio(struct task_struct *p, int ioprio); > int security_task_getioprio(struct task_struct *p); > @@ -1084,7 +1085,12 @@ static inline int security_task_getsid(struct task_struct *p) > return 0; > } > > -static inline void security_task_getsecid(struct task_struct *p, u32 *secid) > +static inline void security_task_getsecid_subj(struct task_struct *p, u32 *secid) > +{ > + *secid = 0; > +} > + > +static inline void security_task_getsecid_obj(struct task_struct *p, u32 *secid) > { > *secid = 0; > } > diff --git a/kernel/audit.c b/kernel/audit.c > index 1ffc2e059027d..8e725db6ecb02 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -2132,7 +2132,7 @@ int audit_log_task_context(struct audit_buffer *ab) > int error; > u32 sid; > > - security_task_getsecid(current, &sid); > + security_task_getsecid_subj(current, &sid); > if (!sid) > return 0; > > @@ -2353,7 +2353,7 @@ int audit_signal_info(int sig, struct task_struct *t) > audit_sig_uid = auid; > else > audit_sig_uid = uid; > - security_task_getsecid(current, &audit_sig_sid); > + security_task_getsecid_subj(current, &audit_sig_sid); > } > > return audit_signal_info_syscall(t); > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c > index 333b3bcfc5458..db2c6b59dfc33 100644 > --- a/kernel/auditfilter.c > +++ b/kernel/auditfilter.c > @@ -1359,7 +1359,8 @@ int audit_filter(int msgtype, unsigned int listtype) > case AUDIT_SUBJ_SEN: > case AUDIT_SUBJ_CLR: > if (f->lsm_rule) { > - security_task_getsecid(current, &sid); > + security_task_getsecid_subj(current, > + &sid); > result = security_audit_rule_match(sid, > f->type, f->op, f->lsm_rule); > } > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index ce8c9e2279ba9..3bfbecca4664a 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -667,7 +667,7 @@ static int audit_filter_rules(struct task_struct *tsk, > logged upon error */ > if (f->lsm_rule) { > if (need_sid) { > - security_task_getsecid(tsk, &sid); > + security_task_getsecid_subj(tsk, &sid); > need_sid = 0; > } > result = security_audit_rule_match(sid, f->type, > @@ -2400,7 +2400,7 @@ void __audit_ptrace(struct task_struct *t) > context->target_auid = audit_get_loginuid(t); > context->target_uid = task_uid(t); > context->target_sessionid = audit_get_sessionid(t); > - security_task_getsecid(t, &context->target_sid); > + security_task_getsecid_obj(t, &context->target_sid); > memcpy(context->target_comm, t->comm, TASK_COMM_LEN); > } > > @@ -2427,7 +2427,7 @@ int audit_signal_info_syscall(struct task_struct *t) > ctx->target_auid = audit_get_loginuid(t); > ctx->target_uid = t_uid; > ctx->target_sessionid = audit_get_sessionid(t); > - security_task_getsecid(t, &ctx->target_sid); > + security_task_getsecid_obj(t, &ctx->target_sid); > memcpy(ctx->target_comm, t->comm, TASK_COMM_LEN); > return 0; > } > @@ -2448,7 +2448,7 @@ int audit_signal_info_syscall(struct task_struct *t) > axp->target_auid[axp->pid_count] = audit_get_loginuid(t); > axp->target_uid[axp->pid_count] = t_uid; > axp->target_sessionid[axp->pid_count] = audit_get_sessionid(t); > - security_task_getsecid(t, &axp->target_sid[axp->pid_count]); > + security_task_getsecid_obj(t, &axp->target_sid[axp->pid_count]); > memcpy(axp->target_comm[axp->pid_count], t->comm, TASK_COMM_LEN); > axp->pid_count++; > > diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c > index ccb4916428116..3e6ac9b790b15 100644 > --- a/net/netlabel/netlabel_unlabeled.c > +++ b/net/netlabel/netlabel_unlabeled.c > @@ -1539,7 +1539,7 @@ int __init netlbl_unlabel_defconf(void) > /* Only the kernel is allowed to call this function and the only time > * it is called is at bootup before the audit subsystem is reporting > * messages so don't worry to much about these values. */ > - security_task_getsecid(current, &audit_info.secid); > + security_task_getsecid_subj(current, &audit_info.secid); > audit_info.loginuid = GLOBAL_ROOT_UID; > audit_info.sessionid = 0; > > diff --git a/net/netlabel/netlabel_user.h b/net/netlabel/netlabel_user.h > index 3c67afce64f12..b9ba8112b3c52 100644 > --- a/net/netlabel/netlabel_user.h > +++ b/net/netlabel/netlabel_user.h > @@ -34,7 +34,7 @@ > static inline void netlbl_netlink_auditinfo(struct sk_buff *skb, > struct netlbl_audit *audit_info) > { > - security_task_getsecid(current, &audit_info->secid); > + security_task_getsecid_subj(current, &audit_info->secid); > audit_info->loginuid = audit_get_loginuid(current); > audit_info->sessionid = audit_get_sessionid(current); > } > diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c > index 1b0aba8eb7235..15e37b9132679 100644 > --- a/security/apparmor/lsm.c > +++ b/security/apparmor/lsm.c > @@ -1243,7 +1243,8 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = { > > LSM_HOOK_INIT(task_free, apparmor_task_free), > LSM_HOOK_INIT(task_alloc, apparmor_task_alloc), > - LSM_HOOK_INIT(task_getsecid, apparmor_task_getsecid), > + LSM_HOOK_INIT(task_getsecid_subj, apparmor_task_getsecid), > + LSM_HOOK_INIT(task_getsecid_obj, apparmor_task_getsecid), > LSM_HOOK_INIT(task_setrlimit, apparmor_task_setrlimit), > LSM_HOOK_INIT(task_kill, apparmor_task_kill), > > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c > index 8361941ee0a12..afa4923dbd33d 100644 > --- a/security/integrity/ima/ima_appraise.c > +++ b/security/integrity/ima/ima_appraise.c > @@ -75,7 +75,7 @@ int ima_must_appraise(struct inode *inode, int mask, enum ima_hooks func) > if (!ima_appraise) > return 0; > > - security_task_getsecid(current, &secid); > + security_task_getsecid_subj(current, &secid); > return ima_match_policy(inode, current_cred(), secid, func, mask, > IMA_APPRAISE | IMA_HASH, NULL, NULL, NULL); > } > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index f87cb29329e91..97a6913bb3d86 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -391,7 +391,7 @@ int ima_file_mmap(struct file *file, unsigned long prot) > u32 secid; > > if (file && (prot & PROT_EXEC)) { > - security_task_getsecid(current, &secid); > + security_task_getsecid_subj(current, &secid); > return process_measurement(file, current_cred(), secid, NULL, > 0, MAY_EXEC, MMAP_CHECK); > } > @@ -429,7 +429,7 @@ int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot) > !(prot & PROT_EXEC) || (vma->vm_flags & VM_EXEC)) > return 0; > > - security_task_getsecid(current, &secid); > + security_task_getsecid_subj(current, &secid); > inode = file_inode(vma->vm_file); > action = ima_get_action(inode, current_cred(), secid, MAY_EXEC, > MMAP_CHECK, &pcr, &template, 0); > @@ -469,7 +469,7 @@ int ima_bprm_check(struct linux_binprm *bprm) > int ret; > u32 secid; > > - security_task_getsecid(current, &secid); > + security_task_getsecid_subj(current, &secid); > ret = process_measurement(bprm->file, current_cred(), secid, NULL, 0, > MAY_EXEC, BPRM_CHECK); > if (ret) > @@ -494,7 +494,7 @@ int ima_file_check(struct file *file, int mask) > { > u32 secid; > > - security_task_getsecid(current, &secid); > + security_task_getsecid_subj(current, &secid); > return process_measurement(file, current_cred(), secid, NULL, 0, > mask & (MAY_READ | MAY_WRITE | MAY_EXEC | > MAY_APPEND), FILE_CHECK); > @@ -679,7 +679,7 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id, > > /* Read entire file for all partial reads. */ > func = read_idmap[read_id] ?: FILE_CHECK; > - security_task_getsecid(current, &secid); > + security_task_getsecid_subj(current, &secid); > return process_measurement(file, current_cred(), secid, NULL, > 0, MAY_READ, func); > } > @@ -722,7 +722,7 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size, > } > > func = read_idmap[read_id] ?: FILE_CHECK; > - security_task_getsecid(current, &secid); > + security_task_getsecid_subj(current, &secid); > return process_measurement(file, current_cred(), secid, buf, size, > MAY_READ, func); > } > @@ -859,7 +859,7 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size, > * buffer measurements. > */ > if (func) { > - security_task_getsecid(current, &secid); > + security_task_getsecid_subj(current, &secid); > action = ima_get_action(inode, current_cred(), secid, 0, func, > &pcr, &template, keyring); > if (!(action & IMA_MEASURE)) > diff --git a/security/security.c b/security/security.c > index 401663b5b70ea..85e504df051b3 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -1757,12 +1757,19 @@ int security_task_getsid(struct task_struct *p) > return call_int_hook(task_getsid, 0, p); > } > > -void security_task_getsecid(struct task_struct *p, u32 *secid) > +void security_task_getsecid_subj(struct task_struct *p, u32 *secid) > { > *secid = 0; > - call_void_hook(task_getsecid, p, secid); > + call_void_hook(task_getsecid_subj, p, secid); > } > -EXPORT_SYMBOL(security_task_getsecid); > +EXPORT_SYMBOL(security_task_getsecid_subj); > + > +void security_task_getsecid_obj(struct task_struct *p, u32 *secid) > +{ > + *secid = 0; > + call_void_hook(task_getsecid_obj, p, secid); > +} > +EXPORT_SYMBOL(security_task_getsecid_obj); > > int security_task_setnice(struct task_struct *p, int nice) > { > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index af2994adf9dd1..f311541c4972e 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -7143,7 +7143,8 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { > LSM_HOOK_INIT(task_setpgid, selinux_task_setpgid), > LSM_HOOK_INIT(task_getpgid, selinux_task_getpgid), > LSM_HOOK_INIT(task_getsid, selinux_task_getsid), > - LSM_HOOK_INIT(task_getsecid, selinux_task_getsecid), > + LSM_HOOK_INIT(task_getsecid_subj, selinux_task_getsecid), > + LSM_HOOK_INIT(task_getsecid_obj, selinux_task_getsecid), > LSM_HOOK_INIT(task_setnice, selinux_task_setnice), > LSM_HOOK_INIT(task_setioprio, selinux_task_setioprio), > LSM_HOOK_INIT(task_getioprio, selinux_task_getioprio), > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > index f69c3dd9a0c67..2bb354ef2c4a9 100644 > --- a/security/smack/smack_lsm.c > +++ b/security/smack/smack_lsm.c > @@ -4755,7 +4755,8 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = { > LSM_HOOK_INIT(task_setpgid, smack_task_setpgid), > LSM_HOOK_INIT(task_getpgid, smack_task_getpgid), > LSM_HOOK_INIT(task_getsid, smack_task_getsid), > - LSM_HOOK_INIT(task_getsecid, smack_task_getsecid), > + LSM_HOOK_INIT(task_getsecid_subj, smack_task_getsecid), > + LSM_HOOK_INIT(task_getsecid_obj, smack_task_getsecid), > LSM_HOOK_INIT(task_setnice, smack_task_setnice), > LSM_HOOK_INIT(task_setioprio, smack_task_setioprio), > LSM_HOOK_INIT(task_getioprio, smack_task_getioprio), >
On Sun, Feb 21, 2021 at 7:51 AM John Johansen <john.johansen@canonical.com> wrote: > On 2/19/21 3:29 PM, Paul Moore wrote: > > Of the three LSMs that implement the security_task_getsecid() LSM > > hook, all three LSMs provide the task's objective security > > credentials. This turns out to be unfortunate as most of the hook's > > callers seem to expect the task's subjective credentials, although > > a small handful of callers do correctly expect the objective > > credentials. > > > > This patch is the first step towards fixing the problem: it splits > > the existing security_task_getsecid() hook into two variants, one > > for the subjective creds, one for the objective creds. > > > > void security_task_getsecid_subj(struct task_struct *p, > > u32 *secid); > > void security_task_getsecid_obj(struct task_struct *p, > > u32 *secid); > > > > While this patch does fix all of the callers to use the correct > > variant, in order to keep this patch focused on the callers and to > > ease review, the LSMs continue to use the same implementation for > > both hooks. The net effect is that this patch should not change > > the behavior of the kernel in any way, it will be up to the latter > > LSM specific patches in this series to change the hook > > implementations and return the correct credentials. > > > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > So far this looks good. I want to take another stab at it and give > it some testing Thanks John, I appreciate the extra set of eyes. Let me know if you run across anything wonky.
On Fri, 2021-02-19 at 18:29 -0500, Paul Moore wrote: > Of the three LSMs that implement the security_task_getsecid() LSM > hook, all three LSMs provide the task's objective security > credentials. This turns out to be unfortunate as most of the hook's > callers seem to expect the task's subjective credentials, although > a small handful of callers do correctly expect the objective > credentials. > > This patch is the first step towards fixing the problem: it splits > the existing security_task_getsecid() hook into two variants, one > for the subjective creds, one for the objective creds. > > void security_task_getsecid_subj(struct task_struct *p, > u32 *secid); > void security_task_getsecid_obj(struct task_struct *p, > u32 *secid); > > While this patch does fix all of the callers to use the correct > variant, in order to keep this patch focused on the callers and to > ease review, the LSMs continue to use the same implementation for > both hooks. The net effect is that this patch should not change > the behavior of the kernel in any way, it will be up to the latter > LSM specific patches in this series to change the hook > implementations and return the correct credentials. > > Signed-off-by: Paul Moore <paul@paul-moore.com> Thanks, Paul. Acked-by: Mimi Zohar <zohar@linux.ibm.com> (IMA)
On Sun, Feb 21, 2021 at 7:51 AM John Johansen <john.johansen@canonical.com> wrote: > On 2/19/21 3:29 PM, Paul Moore wrote: > > Of the three LSMs that implement the security_task_getsecid() LSM > > hook, all three LSMs provide the task's objective security > > credentials. This turns out to be unfortunate as most of the hook's > > callers seem to expect the task's subjective credentials, although > > a small handful of callers do correctly expect the objective > > credentials. > > > > This patch is the first step towards fixing the problem: it splits > > the existing security_task_getsecid() hook into two variants, one > > for the subjective creds, one for the objective creds. > > > > void security_task_getsecid_subj(struct task_struct *p, > > u32 *secid); > > void security_task_getsecid_obj(struct task_struct *p, > > u32 *secid); > > > > While this patch does fix all of the callers to use the correct > > variant, in order to keep this patch focused on the callers and to > > ease review, the LSMs continue to use the same implementation for > > both hooks. The net effect is that this patch should not change > > the behavior of the kernel in any way, it will be up to the latter > > LSM specific patches in this series to change the hook > > implementations and return the correct credentials. > > > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > So far this looks good. I want to take another stab at it and give > it some testing Checking in as I know you said you needed to fix/release the AppArmor patch in this series ... is this patch still looking okay to you? If so, can I get an ACK at least on this patch?
On Sat, Feb 20, 2021 at 3:45 PM Paul Moore <paul@paul-moore.com> wrote: > > On Fri, Feb 19, 2021 at 9:57 PM James Morris <jmorris@namei.org> wrote: > > On Fri, 19 Feb 2021, Paul Moore wrote: > > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > > > index c119736ca56ac..39d501261108d 100644 > > > --- a/drivers/android/binder.c > > > +++ b/drivers/android/binder.c > > > @@ -2700,7 +2700,7 @@ static void binder_transaction(struct binder_proc *proc, > > > u32 secid; > > > size_t added_size; > > > > > > - security_task_getsecid(proc->tsk, &secid); > > > + security_task_getsecid_subj(proc->tsk, &secid); > > > ret = security_secid_to_secctx(secid, &secctx, &secctx_sz); > > > if (ret) { > > > return_error = BR_FAILED_REPLY; > > > > Can someone from the Android project confirm this is correct for binder? This looks correct to me. > > Yes, please take a look Android folks. As I mentioned previously, > review of the binder changes is one area where I think some extra > review is needed; I'm just not confident enough in my understanding of > binder. > > -- > paul moore > www.paul-moore.com
On Thu, Mar 4, 2021 at 5:04 AM Jeffrey Vander Stoep <jeffv@google.com> wrote: > On Sat, Feb 20, 2021 at 3:45 PM Paul Moore <paul@paul-moore.com> wrote: > > On Fri, Feb 19, 2021 at 9:57 PM James Morris <jmorris@namei.org> wrote: > > > On Fri, 19 Feb 2021, Paul Moore wrote: > > > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > > > > index c119736ca56ac..39d501261108d 100644 > > > > --- a/drivers/android/binder.c > > > > +++ b/drivers/android/binder.c > > > > @@ -2700,7 +2700,7 @@ static void binder_transaction(struct binder_proc *proc, > > > > u32 secid; > > > > size_t added_size; > > > > > > > > - security_task_getsecid(proc->tsk, &secid); > > > > + security_task_getsecid_subj(proc->tsk, &secid); > > > > ret = security_secid_to_secctx(secid, &secctx, &secctx_sz); > > > > if (ret) { > > > > return_error = BR_FAILED_REPLY; > > > > > > Can someone from the Android project confirm this is correct for binder? > > This looks correct to me. Thanks for the verification. Should I assume the SELinux specific binder changes looked okay too? https://lore.kernel.org/selinux/84053ed8-4778-f246-2177-cf5c1b9516a9@canonical.com/T/#m4ae49d4a5a62d600fa3f3b1a5bba2d6611b1051c
On 2021-02-19 18:29, Paul Moore wrote: > Of the three LSMs that implement the security_task_getsecid() LSM > hook, all three LSMs provide the task's objective security > credentials. This turns out to be unfortunate as most of the hook's > callers seem to expect the task's subjective credentials, although > a small handful of callers do correctly expect the objective > credentials. > > This patch is the first step towards fixing the problem: it splits > the existing security_task_getsecid() hook into two variants, one > for the subjective creds, one for the objective creds. > > void security_task_getsecid_subj(struct task_struct *p, > u32 *secid); > void security_task_getsecid_obj(struct task_struct *p, > u32 *secid); > > While this patch does fix all of the callers to use the correct > variant, in order to keep this patch focused on the callers and to > ease review, the LSMs continue to use the same implementation for > both hooks. The net effect is that this patch should not change > the behavior of the kernel in any way, it will be up to the latter > LSM specific patches in this series to change the hook > implementations and return the correct credentials. > > Signed-off-by: Paul Moore <paul@paul-moore.com> Audit: Acked-by: Richard Guy Briggs <rgb@redhat.com> Reviewed-by: Richard Guy Briggs <rgb@redhat.com> > --- > drivers/android/binder.c | 2 +- > include/linux/cred.h | 2 +- > include/linux/lsm_hook_defs.h | 5 ++++- > include/linux/lsm_hooks.h | 8 ++++++-- > include/linux/security.h | 10 ++++++++-- > kernel/audit.c | 4 ++-- > kernel/auditfilter.c | 3 ++- > kernel/auditsc.c | 8 ++++---- > net/netlabel/netlabel_unlabeled.c | 2 +- > net/netlabel/netlabel_user.h | 2 +- > security/apparmor/lsm.c | 3 ++- > security/integrity/ima/ima_appraise.c | 2 +- > security/integrity/ima/ima_main.c | 14 +++++++------- > security/security.c | 13 ++++++++++--- > security/selinux/hooks.c | 3 ++- > security/smack/smack_lsm.c | 3 ++- > 16 files changed, 54 insertions(+), 30 deletions(-) > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > index c119736ca56ac..39d501261108d 100644 > --- a/drivers/android/binder.c > +++ b/drivers/android/binder.c > @@ -2700,7 +2700,7 @@ static void binder_transaction(struct binder_proc *proc, > u32 secid; > size_t added_size; > > - security_task_getsecid(proc->tsk, &secid); > + security_task_getsecid_subj(proc->tsk, &secid); > ret = security_secid_to_secctx(secid, &secctx, &secctx_sz); > if (ret) { > return_error = BR_FAILED_REPLY; > diff --git a/include/linux/cred.h b/include/linux/cred.h > index 18639c069263f..42b9d88d9a565 100644 > --- a/include/linux/cred.h > +++ b/include/linux/cred.h > @@ -140,7 +140,7 @@ struct cred { > struct key *request_key_auth; /* assumed request_key authority */ > #endif > #ifdef CONFIG_SECURITY > - void *security; /* subjective LSM security */ > + void *security; /* LSM security */ > #endif > struct user_struct *user; /* real user ID subscription */ > struct user_namespace *user_ns; /* user_ns the caps and keyrings are relative to. */ > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h > index dfd261dcbcb04..1490a185135a0 100644 > --- a/include/linux/lsm_hook_defs.h > +++ b/include/linux/lsm_hook_defs.h > @@ -200,7 +200,10 @@ LSM_HOOK(int, 0, task_fix_setgid, struct cred *new, const struct cred * old, > LSM_HOOK(int, 0, task_setpgid, struct task_struct *p, pid_t pgid) > LSM_HOOK(int, 0, task_getpgid, struct task_struct *p) > LSM_HOOK(int, 0, task_getsid, struct task_struct *p) > -LSM_HOOK(void, LSM_RET_VOID, task_getsecid, struct task_struct *p, u32 *secid) > +LSM_HOOK(void, LSM_RET_VOID, task_getsecid_subj, > + struct task_struct *p, u32 *secid) > +LSM_HOOK(void, LSM_RET_VOID, task_getsecid_obj, > + struct task_struct *p, u32 *secid) > LSM_HOOK(int, 0, task_setnice, struct task_struct *p, int nice) > LSM_HOOK(int, 0, task_setioprio, struct task_struct *p, int ioprio) > LSM_HOOK(int, 0, task_getioprio, struct task_struct *p) > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index bdfc8a76a4f79..13d2a9a6f2014 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -706,8 +706,12 @@ > * @p. > * @p contains the task_struct for the process. > * Return 0 if permission is granted. > - * @task_getsecid: > - * Retrieve the security identifier of the process @p. > + * @task_getsecid_subj: > + * Retrieve the subjective security identifier of the process @p. > + * @p contains the task_struct for the process and place is into @secid. > + * In case of failure, @secid will be set to zero. > + * @task_getsecid_obj: > + * Retrieve the objective security identifier of the process @p. > * @p contains the task_struct for the process and place is into @secid. > * In case of failure, @secid will be set to zero. > * > diff --git a/include/linux/security.h b/include/linux/security.h > index b0d14f04b16de..1826bb0cea825 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -406,7 +406,8 @@ int security_task_fix_setgid(struct cred *new, const struct cred *old, > int security_task_setpgid(struct task_struct *p, pid_t pgid); > int security_task_getpgid(struct task_struct *p); > int security_task_getsid(struct task_struct *p); > -void security_task_getsecid(struct task_struct *p, u32 *secid); > +void security_task_getsecid_subj(struct task_struct *p, u32 *secid); > +void security_task_getsecid_obj(struct task_struct *p, u32 *secid); > int security_task_setnice(struct task_struct *p, int nice); > int security_task_setioprio(struct task_struct *p, int ioprio); > int security_task_getioprio(struct task_struct *p); > @@ -1084,7 +1085,12 @@ static inline int security_task_getsid(struct task_struct *p) > return 0; > } > > -static inline void security_task_getsecid(struct task_struct *p, u32 *secid) > +static inline void security_task_getsecid_subj(struct task_struct *p, u32 *secid) > +{ > + *secid = 0; > +} > + > +static inline void security_task_getsecid_obj(struct task_struct *p, u32 *secid) > { > *secid = 0; > } > diff --git a/kernel/audit.c b/kernel/audit.c > index 1ffc2e059027d..8e725db6ecb02 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -2132,7 +2132,7 @@ int audit_log_task_context(struct audit_buffer *ab) > int error; > u32 sid; > > - security_task_getsecid(current, &sid); > + security_task_getsecid_subj(current, &sid); > if (!sid) > return 0; > > @@ -2353,7 +2353,7 @@ int audit_signal_info(int sig, struct task_struct *t) > audit_sig_uid = auid; > else > audit_sig_uid = uid; > - security_task_getsecid(current, &audit_sig_sid); > + security_task_getsecid_subj(current, &audit_sig_sid); > } > > return audit_signal_info_syscall(t); > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c > index 333b3bcfc5458..db2c6b59dfc33 100644 > --- a/kernel/auditfilter.c > +++ b/kernel/auditfilter.c > @@ -1359,7 +1359,8 @@ int audit_filter(int msgtype, unsigned int listtype) > case AUDIT_SUBJ_SEN: > case AUDIT_SUBJ_CLR: > if (f->lsm_rule) { > - security_task_getsecid(current, &sid); > + security_task_getsecid_subj(current, > + &sid); > result = security_audit_rule_match(sid, > f->type, f->op, f->lsm_rule); > } > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index ce8c9e2279ba9..3bfbecca4664a 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -667,7 +667,7 @@ static int audit_filter_rules(struct task_struct *tsk, > logged upon error */ > if (f->lsm_rule) { > if (need_sid) { > - security_task_getsecid(tsk, &sid); > + security_task_getsecid_subj(tsk, &sid); > need_sid = 0; > } > result = security_audit_rule_match(sid, f->type, > @@ -2400,7 +2400,7 @@ void __audit_ptrace(struct task_struct *t) > context->target_auid = audit_get_loginuid(t); > context->target_uid = task_uid(t); > context->target_sessionid = audit_get_sessionid(t); > - security_task_getsecid(t, &context->target_sid); > + security_task_getsecid_obj(t, &context->target_sid); > memcpy(context->target_comm, t->comm, TASK_COMM_LEN); > } > > @@ -2427,7 +2427,7 @@ int audit_signal_info_syscall(struct task_struct *t) > ctx->target_auid = audit_get_loginuid(t); > ctx->target_uid = t_uid; > ctx->target_sessionid = audit_get_sessionid(t); > - security_task_getsecid(t, &ctx->target_sid); > + security_task_getsecid_obj(t, &ctx->target_sid); > memcpy(ctx->target_comm, t->comm, TASK_COMM_LEN); > return 0; > } > @@ -2448,7 +2448,7 @@ int audit_signal_info_syscall(struct task_struct *t) > axp->target_auid[axp->pid_count] = audit_get_loginuid(t); > axp->target_uid[axp->pid_count] = t_uid; > axp->target_sessionid[axp->pid_count] = audit_get_sessionid(t); > - security_task_getsecid(t, &axp->target_sid[axp->pid_count]); > + security_task_getsecid_obj(t, &axp->target_sid[axp->pid_count]); > memcpy(axp->target_comm[axp->pid_count], t->comm, TASK_COMM_LEN); > axp->pid_count++; > > diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c > index ccb4916428116..3e6ac9b790b15 100644 > --- a/net/netlabel/netlabel_unlabeled.c > +++ b/net/netlabel/netlabel_unlabeled.c > @@ -1539,7 +1539,7 @@ int __init netlbl_unlabel_defconf(void) > /* Only the kernel is allowed to call this function and the only time > * it is called is at bootup before the audit subsystem is reporting > * messages so don't worry to much about these values. */ > - security_task_getsecid(current, &audit_info.secid); > + security_task_getsecid_subj(current, &audit_info.secid); > audit_info.loginuid = GLOBAL_ROOT_UID; > audit_info.sessionid = 0; > > diff --git a/net/netlabel/netlabel_user.h b/net/netlabel/netlabel_user.h > index 3c67afce64f12..b9ba8112b3c52 100644 > --- a/net/netlabel/netlabel_user.h > +++ b/net/netlabel/netlabel_user.h > @@ -34,7 +34,7 @@ > static inline void netlbl_netlink_auditinfo(struct sk_buff *skb, > struct netlbl_audit *audit_info) > { > - security_task_getsecid(current, &audit_info->secid); > + security_task_getsecid_subj(current, &audit_info->secid); > audit_info->loginuid = audit_get_loginuid(current); > audit_info->sessionid = audit_get_sessionid(current); > } > diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c > index 1b0aba8eb7235..15e37b9132679 100644 > --- a/security/apparmor/lsm.c > +++ b/security/apparmor/lsm.c > @@ -1243,7 +1243,8 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = { > > LSM_HOOK_INIT(task_free, apparmor_task_free), > LSM_HOOK_INIT(task_alloc, apparmor_task_alloc), > - LSM_HOOK_INIT(task_getsecid, apparmor_task_getsecid), > + LSM_HOOK_INIT(task_getsecid_subj, apparmor_task_getsecid), > + LSM_HOOK_INIT(task_getsecid_obj, apparmor_task_getsecid), > LSM_HOOK_INIT(task_setrlimit, apparmor_task_setrlimit), > LSM_HOOK_INIT(task_kill, apparmor_task_kill), > > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c > index 8361941ee0a12..afa4923dbd33d 100644 > --- a/security/integrity/ima/ima_appraise.c > +++ b/security/integrity/ima/ima_appraise.c > @@ -75,7 +75,7 @@ int ima_must_appraise(struct inode *inode, int mask, enum ima_hooks func) > if (!ima_appraise) > return 0; > > - security_task_getsecid(current, &secid); > + security_task_getsecid_subj(current, &secid); > return ima_match_policy(inode, current_cred(), secid, func, mask, > IMA_APPRAISE | IMA_HASH, NULL, NULL, NULL); > } > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index f87cb29329e91..97a6913bb3d86 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -391,7 +391,7 @@ int ima_file_mmap(struct file *file, unsigned long prot) > u32 secid; > > if (file && (prot & PROT_EXEC)) { > - security_task_getsecid(current, &secid); > + security_task_getsecid_subj(current, &secid); > return process_measurement(file, current_cred(), secid, NULL, > 0, MAY_EXEC, MMAP_CHECK); > } > @@ -429,7 +429,7 @@ int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot) > !(prot & PROT_EXEC) || (vma->vm_flags & VM_EXEC)) > return 0; > > - security_task_getsecid(current, &secid); > + security_task_getsecid_subj(current, &secid); > inode = file_inode(vma->vm_file); > action = ima_get_action(inode, current_cred(), secid, MAY_EXEC, > MMAP_CHECK, &pcr, &template, 0); > @@ -469,7 +469,7 @@ int ima_bprm_check(struct linux_binprm *bprm) > int ret; > u32 secid; > > - security_task_getsecid(current, &secid); > + security_task_getsecid_subj(current, &secid); > ret = process_measurement(bprm->file, current_cred(), secid, NULL, 0, > MAY_EXEC, BPRM_CHECK); > if (ret) > @@ -494,7 +494,7 @@ int ima_file_check(struct file *file, int mask) > { > u32 secid; > > - security_task_getsecid(current, &secid); > + security_task_getsecid_subj(current, &secid); > return process_measurement(file, current_cred(), secid, NULL, 0, > mask & (MAY_READ | MAY_WRITE | MAY_EXEC | > MAY_APPEND), FILE_CHECK); > @@ -679,7 +679,7 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id, > > /* Read entire file for all partial reads. */ > func = read_idmap[read_id] ?: FILE_CHECK; > - security_task_getsecid(current, &secid); > + security_task_getsecid_subj(current, &secid); > return process_measurement(file, current_cred(), secid, NULL, > 0, MAY_READ, func); > } > @@ -722,7 +722,7 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size, > } > > func = read_idmap[read_id] ?: FILE_CHECK; > - security_task_getsecid(current, &secid); > + security_task_getsecid_subj(current, &secid); > return process_measurement(file, current_cred(), secid, buf, size, > MAY_READ, func); > } > @@ -859,7 +859,7 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size, > * buffer measurements. > */ > if (func) { > - security_task_getsecid(current, &secid); > + security_task_getsecid_subj(current, &secid); > action = ima_get_action(inode, current_cred(), secid, 0, func, > &pcr, &template, keyring); > if (!(action & IMA_MEASURE)) > diff --git a/security/security.c b/security/security.c > index 401663b5b70ea..85e504df051b3 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -1757,12 +1757,19 @@ int security_task_getsid(struct task_struct *p) > return call_int_hook(task_getsid, 0, p); > } > > -void security_task_getsecid(struct task_struct *p, u32 *secid) > +void security_task_getsecid_subj(struct task_struct *p, u32 *secid) > { > *secid = 0; > - call_void_hook(task_getsecid, p, secid); > + call_void_hook(task_getsecid_subj, p, secid); > } > -EXPORT_SYMBOL(security_task_getsecid); > +EXPORT_SYMBOL(security_task_getsecid_subj); > + > +void security_task_getsecid_obj(struct task_struct *p, u32 *secid) > +{ > + *secid = 0; > + call_void_hook(task_getsecid_obj, p, secid); > +} > +EXPORT_SYMBOL(security_task_getsecid_obj); > > int security_task_setnice(struct task_struct *p, int nice) > { > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index af2994adf9dd1..f311541c4972e 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -7143,7 +7143,8 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { > LSM_HOOK_INIT(task_setpgid, selinux_task_setpgid), > LSM_HOOK_INIT(task_getpgid, selinux_task_getpgid), > LSM_HOOK_INIT(task_getsid, selinux_task_getsid), > - LSM_HOOK_INIT(task_getsecid, selinux_task_getsecid), > + LSM_HOOK_INIT(task_getsecid_subj, selinux_task_getsecid), > + LSM_HOOK_INIT(task_getsecid_obj, selinux_task_getsecid), > LSM_HOOK_INIT(task_setnice, selinux_task_setnice), > LSM_HOOK_INIT(task_setioprio, selinux_task_setioprio), > LSM_HOOK_INIT(task_getioprio, selinux_task_getioprio), > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > index f69c3dd9a0c67..2bb354ef2c4a9 100644 > --- a/security/smack/smack_lsm.c > +++ b/security/smack/smack_lsm.c > @@ -4755,7 +4755,8 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = { > LSM_HOOK_INIT(task_setpgid, smack_task_setpgid), > LSM_HOOK_INIT(task_getpgid, smack_task_getpgid), > LSM_HOOK_INIT(task_getsid, smack_task_getsid), > - LSM_HOOK_INIT(task_getsecid, smack_task_getsecid), > + LSM_HOOK_INIT(task_getsecid_subj, smack_task_getsecid), > + LSM_HOOK_INIT(task_getsecid_obj, smack_task_getsecid), > LSM_HOOK_INIT(task_setnice, smack_task_setnice), > LSM_HOOK_INIT(task_setioprio, smack_task_setioprio), > LSM_HOOK_INIT(task_getioprio, smack_task_getioprio), > > -- > Linux-audit mailing list > Linux-audit@redhat.com > https://listman.redhat.com/mailman/listinfo/linux-audit - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
On Mon, Mar 8, 2021 at 2:25 PM Richard Guy Briggs <rgb@redhat.com> wrote: > On 2021-02-19 18:29, Paul Moore wrote: > > Of the three LSMs that implement the security_task_getsecid() LSM > > hook, all three LSMs provide the task's objective security > > credentials. This turns out to be unfortunate as most of the hook's > > callers seem to expect the task's subjective credentials, although > > a small handful of callers do correctly expect the objective > > credentials. > > > > This patch is the first step towards fixing the problem: it splits > > the existing security_task_getsecid() hook into two variants, one > > for the subjective creds, one for the objective creds. > > > > void security_task_getsecid_subj(struct task_struct *p, > > u32 *secid); > > void security_task_getsecid_obj(struct task_struct *p, > > u32 *secid); > > > > While this patch does fix all of the callers to use the correct > > variant, in order to keep this patch focused on the callers and to > > ease review, the LSMs continue to use the same implementation for > > both hooks. The net effect is that this patch should not change > > the behavior of the kernel in any way, it will be up to the latter > > LSM specific patches in this series to change the hook > > implementations and return the correct credentials. > > > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > Audit: Acked-by: Richard Guy Briggs <rgb@redhat.com> > Reviewed-by: Richard Guy Briggs <rgb@redhat.com> Thanks Richard, I added your review tag to the LSM, SELinux, and Smack patches.
On Wed, Mar 3, 2021 at 7:44 PM Paul Moore <paul@paul-moore.com> wrote: > On Sun, Feb 21, 2021 at 7:51 AM John Johansen > <john.johansen@canonical.com> wrote: > > On 2/19/21 3:29 PM, Paul Moore wrote: > > > Of the three LSMs that implement the security_task_getsecid() LSM > > > hook, all three LSMs provide the task's objective security > > > credentials. This turns out to be unfortunate as most of the hook's > > > callers seem to expect the task's subjective credentials, although > > > a small handful of callers do correctly expect the objective > > > credentials. > > > > > > This patch is the first step towards fixing the problem: it splits > > > the existing security_task_getsecid() hook into two variants, one > > > for the subjective creds, one for the objective creds. > > > > > > void security_task_getsecid_subj(struct task_struct *p, > > > u32 *secid); > > > void security_task_getsecid_obj(struct task_struct *p, > > > u32 *secid); > > > > > > While this patch does fix all of the callers to use the correct > > > variant, in order to keep this patch focused on the callers and to > > > ease review, the LSMs continue to use the same implementation for > > > both hooks. The net effect is that this patch should not change > > > the behavior of the kernel in any way, it will be up to the latter > > > LSM specific patches in this series to change the hook > > > implementations and return the correct credentials. > > > > > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > > > So far this looks good. I want to take another stab at it and give > > it some testing > > Checking in as I know you said you needed to fix/release the AppArmor > patch in this series ... is this patch still looking okay to you? If > so, can I get an ACK at least on this patch? Hi John, Any objections if I merge the LSM, SELinux, and Smack patches into the selinux/next tree so that we can start getting some wider testing? If I leave out my poor attempt at an AppArmor patch, the current in-tree AppArmor code should behave exactly as it does today with the apparmor_task_getsecid() function handling both the subjective and objective creds. I can always merge the AppArmor patch later when you have it ready, or you can merge it via your AppArmor tree at a later date.
On 2/19/21 3:29 PM, Paul Moore wrote: > Of the three LSMs that implement the security_task_getsecid() LSM > hook, all three LSMs provide the task's objective security > credentials. This turns out to be unfortunate as most of the hook's > callers seem to expect the task's subjective credentials, although > a small handful of callers do correctly expect the objective > credentials. > > This patch is the first step towards fixing the problem: it splits > the existing security_task_getsecid() hook into two variants, one > for the subjective creds, one for the objective creds. > > void security_task_getsecid_subj(struct task_struct *p, > u32 *secid); > void security_task_getsecid_obj(struct task_struct *p, > u32 *secid); > > While this patch does fix all of the callers to use the correct > variant, in order to keep this patch focused on the callers and to > ease review, the LSMs continue to use the same implementation for > both hooks. The net effect is that this patch should not change > the behavior of the kernel in any way, it will be up to the latter > LSM specific patches in this series to change the hook > implementations and return the correct credentials. > > Signed-off-by: Paul Moore <paul@paul-moore.com> Reviewed-by: John Johansen <john.johansen@canonical.com> > --- > drivers/android/binder.c | 2 +- > include/linux/cred.h | 2 +- > include/linux/lsm_hook_defs.h | 5 ++++- > include/linux/lsm_hooks.h | 8 ++++++-- > include/linux/security.h | 10 ++++++++-- > kernel/audit.c | 4 ++-- > kernel/auditfilter.c | 3 ++- > kernel/auditsc.c | 8 ++++---- > net/netlabel/netlabel_unlabeled.c | 2 +- > net/netlabel/netlabel_user.h | 2 +- > security/apparmor/lsm.c | 3 ++- > security/integrity/ima/ima_appraise.c | 2 +- > security/integrity/ima/ima_main.c | 14 +++++++------- > security/security.c | 13 ++++++++++--- > security/selinux/hooks.c | 3 ++- > security/smack/smack_lsm.c | 3 ++- > 16 files changed, 54 insertions(+), 30 deletions(-) > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > index c119736ca56ac..39d501261108d 100644 > --- a/drivers/android/binder.c > +++ b/drivers/android/binder.c > @@ -2700,7 +2700,7 @@ static void binder_transaction(struct binder_proc *proc, > u32 secid; > size_t added_size; > > - security_task_getsecid(proc->tsk, &secid); > + security_task_getsecid_subj(proc->tsk, &secid); > ret = security_secid_to_secctx(secid, &secctx, &secctx_sz); > if (ret) { > return_error = BR_FAILED_REPLY; > diff --git a/include/linux/cred.h b/include/linux/cred.h > index 18639c069263f..42b9d88d9a565 100644 > --- a/include/linux/cred.h > +++ b/include/linux/cred.h > @@ -140,7 +140,7 @@ struct cred { > struct key *request_key_auth; /* assumed request_key authority */ > #endif > #ifdef CONFIG_SECURITY > - void *security; /* subjective LSM security */ > + void *security; /* LSM security */ > #endif > struct user_struct *user; /* real user ID subscription */ > struct user_namespace *user_ns; /* user_ns the caps and keyrings are relative to. */ > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h > index dfd261dcbcb04..1490a185135a0 100644 > --- a/include/linux/lsm_hook_defs.h > +++ b/include/linux/lsm_hook_defs.h > @@ -200,7 +200,10 @@ LSM_HOOK(int, 0, task_fix_setgid, struct cred *new, const struct cred * old, > LSM_HOOK(int, 0, task_setpgid, struct task_struct *p, pid_t pgid) > LSM_HOOK(int, 0, task_getpgid, struct task_struct *p) > LSM_HOOK(int, 0, task_getsid, struct task_struct *p) > -LSM_HOOK(void, LSM_RET_VOID, task_getsecid, struct task_struct *p, u32 *secid) > +LSM_HOOK(void, LSM_RET_VOID, task_getsecid_subj, > + struct task_struct *p, u32 *secid) > +LSM_HOOK(void, LSM_RET_VOID, task_getsecid_obj, > + struct task_struct *p, u32 *secid) > LSM_HOOK(int, 0, task_setnice, struct task_struct *p, int nice) > LSM_HOOK(int, 0, task_setioprio, struct task_struct *p, int ioprio) > LSM_HOOK(int, 0, task_getioprio, struct task_struct *p) > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index bdfc8a76a4f79..13d2a9a6f2014 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -706,8 +706,12 @@ > * @p. > * @p contains the task_struct for the process. > * Return 0 if permission is granted. > - * @task_getsecid: > - * Retrieve the security identifier of the process @p. > + * @task_getsecid_subj: > + * Retrieve the subjective security identifier of the process @p. > + * @p contains the task_struct for the process and place is into @secid. > + * In case of failure, @secid will be set to zero. > + * @task_getsecid_obj: > + * Retrieve the objective security identifier of the process @p. > * @p contains the task_struct for the process and place is into @secid. > * In case of failure, @secid will be set to zero. > * > diff --git a/include/linux/security.h b/include/linux/security.h > index b0d14f04b16de..1826bb0cea825 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -406,7 +406,8 @@ int security_task_fix_setgid(struct cred *new, const struct cred *old, > int security_task_setpgid(struct task_struct *p, pid_t pgid); > int security_task_getpgid(struct task_struct *p); > int security_task_getsid(struct task_struct *p); > -void security_task_getsecid(struct task_struct *p, u32 *secid); > +void security_task_getsecid_subj(struct task_struct *p, u32 *secid); > +void security_task_getsecid_obj(struct task_struct *p, u32 *secid); > int security_task_setnice(struct task_struct *p, int nice); > int security_task_setioprio(struct task_struct *p, int ioprio); > int security_task_getioprio(struct task_struct *p); > @@ -1084,7 +1085,12 @@ static inline int security_task_getsid(struct task_struct *p) > return 0; > } > > -static inline void security_task_getsecid(struct task_struct *p, u32 *secid) > +static inline void security_task_getsecid_subj(struct task_struct *p, u32 *secid) > +{ > + *secid = 0; > +} > + > +static inline void security_task_getsecid_obj(struct task_struct *p, u32 *secid) > { > *secid = 0; > } > diff --git a/kernel/audit.c b/kernel/audit.c > index 1ffc2e059027d..8e725db6ecb02 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -2132,7 +2132,7 @@ int audit_log_task_context(struct audit_buffer *ab) > int error; > u32 sid; > > - security_task_getsecid(current, &sid); > + security_task_getsecid_subj(current, &sid); > if (!sid) > return 0; > > @@ -2353,7 +2353,7 @@ int audit_signal_info(int sig, struct task_struct *t) > audit_sig_uid = auid; > else > audit_sig_uid = uid; > - security_task_getsecid(current, &audit_sig_sid); > + security_task_getsecid_subj(current, &audit_sig_sid); > } > > return audit_signal_info_syscall(t); > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c > index 333b3bcfc5458..db2c6b59dfc33 100644 > --- a/kernel/auditfilter.c > +++ b/kernel/auditfilter.c > @@ -1359,7 +1359,8 @@ int audit_filter(int msgtype, unsigned int listtype) > case AUDIT_SUBJ_SEN: > case AUDIT_SUBJ_CLR: > if (f->lsm_rule) { > - security_task_getsecid(current, &sid); > + security_task_getsecid_subj(current, > + &sid); > result = security_audit_rule_match(sid, > f->type, f->op, f->lsm_rule); > } > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index ce8c9e2279ba9..3bfbecca4664a 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -667,7 +667,7 @@ static int audit_filter_rules(struct task_struct *tsk, > logged upon error */ > if (f->lsm_rule) { > if (need_sid) { > - security_task_getsecid(tsk, &sid); > + security_task_getsecid_subj(tsk, &sid); > need_sid = 0; > } > result = security_audit_rule_match(sid, f->type, > @@ -2400,7 +2400,7 @@ void __audit_ptrace(struct task_struct *t) > context->target_auid = audit_get_loginuid(t); > context->target_uid = task_uid(t); > context->target_sessionid = audit_get_sessionid(t); > - security_task_getsecid(t, &context->target_sid); > + security_task_getsecid_obj(t, &context->target_sid); > memcpy(context->target_comm, t->comm, TASK_COMM_LEN); > } > > @@ -2427,7 +2427,7 @@ int audit_signal_info_syscall(struct task_struct *t) > ctx->target_auid = audit_get_loginuid(t); > ctx->target_uid = t_uid; > ctx->target_sessionid = audit_get_sessionid(t); > - security_task_getsecid(t, &ctx->target_sid); > + security_task_getsecid_obj(t, &ctx->target_sid); > memcpy(ctx->target_comm, t->comm, TASK_COMM_LEN); > return 0; > } > @@ -2448,7 +2448,7 @@ int audit_signal_info_syscall(struct task_struct *t) > axp->target_auid[axp->pid_count] = audit_get_loginuid(t); > axp->target_uid[axp->pid_count] = t_uid; > axp->target_sessionid[axp->pid_count] = audit_get_sessionid(t); > - security_task_getsecid(t, &axp->target_sid[axp->pid_count]); > + security_task_getsecid_obj(t, &axp->target_sid[axp->pid_count]); > memcpy(axp->target_comm[axp->pid_count], t->comm, TASK_COMM_LEN); > axp->pid_count++; > > diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c > index ccb4916428116..3e6ac9b790b15 100644 > --- a/net/netlabel/netlabel_unlabeled.c > +++ b/net/netlabel/netlabel_unlabeled.c > @@ -1539,7 +1539,7 @@ int __init netlbl_unlabel_defconf(void) > /* Only the kernel is allowed to call this function and the only time > * it is called is at bootup before the audit subsystem is reporting > * messages so don't worry to much about these values. */ > - security_task_getsecid(current, &audit_info.secid); > + security_task_getsecid_subj(current, &audit_info.secid); > audit_info.loginuid = GLOBAL_ROOT_UID; > audit_info.sessionid = 0; > > diff --git a/net/netlabel/netlabel_user.h b/net/netlabel/netlabel_user.h > index 3c67afce64f12..b9ba8112b3c52 100644 > --- a/net/netlabel/netlabel_user.h > +++ b/net/netlabel/netlabel_user.h > @@ -34,7 +34,7 @@ > static inline void netlbl_netlink_auditinfo(struct sk_buff *skb, > struct netlbl_audit *audit_info) > { > - security_task_getsecid(current, &audit_info->secid); > + security_task_getsecid_subj(current, &audit_info->secid); > audit_info->loginuid = audit_get_loginuid(current); > audit_info->sessionid = audit_get_sessionid(current); > } > diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c > index 1b0aba8eb7235..15e37b9132679 100644 > --- a/security/apparmor/lsm.c > +++ b/security/apparmor/lsm.c > @@ -1243,7 +1243,8 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = { > > LSM_HOOK_INIT(task_free, apparmor_task_free), > LSM_HOOK_INIT(task_alloc, apparmor_task_alloc), > - LSM_HOOK_INIT(task_getsecid, apparmor_task_getsecid), > + LSM_HOOK_INIT(task_getsecid_subj, apparmor_task_getsecid), > + LSM_HOOK_INIT(task_getsecid_obj, apparmor_task_getsecid), > LSM_HOOK_INIT(task_setrlimit, apparmor_task_setrlimit), > LSM_HOOK_INIT(task_kill, apparmor_task_kill), > > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c > index 8361941ee0a12..afa4923dbd33d 100644 > --- a/security/integrity/ima/ima_appraise.c > +++ b/security/integrity/ima/ima_appraise.c > @@ -75,7 +75,7 @@ int ima_must_appraise(struct inode *inode, int mask, enum ima_hooks func) > if (!ima_appraise) > return 0; > > - security_task_getsecid(current, &secid); > + security_task_getsecid_subj(current, &secid); > return ima_match_policy(inode, current_cred(), secid, func, mask, > IMA_APPRAISE | IMA_HASH, NULL, NULL, NULL); > } > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index f87cb29329e91..97a6913bb3d86 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -391,7 +391,7 @@ int ima_file_mmap(struct file *file, unsigned long prot) > u32 secid; > > if (file && (prot & PROT_EXEC)) { > - security_task_getsecid(current, &secid); > + security_task_getsecid_subj(current, &secid); > return process_measurement(file, current_cred(), secid, NULL, > 0, MAY_EXEC, MMAP_CHECK); > } > @@ -429,7 +429,7 @@ int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot) > !(prot & PROT_EXEC) || (vma->vm_flags & VM_EXEC)) > return 0; > > - security_task_getsecid(current, &secid); > + security_task_getsecid_subj(current, &secid); > inode = file_inode(vma->vm_file); > action = ima_get_action(inode, current_cred(), secid, MAY_EXEC, > MMAP_CHECK, &pcr, &template, 0); > @@ -469,7 +469,7 @@ int ima_bprm_check(struct linux_binprm *bprm) > int ret; > u32 secid; > > - security_task_getsecid(current, &secid); > + security_task_getsecid_subj(current, &secid); > ret = process_measurement(bprm->file, current_cred(), secid, NULL, 0, > MAY_EXEC, BPRM_CHECK); > if (ret) > @@ -494,7 +494,7 @@ int ima_file_check(struct file *file, int mask) > { > u32 secid; > > - security_task_getsecid(current, &secid); > + security_task_getsecid_subj(current, &secid); > return process_measurement(file, current_cred(), secid, NULL, 0, > mask & (MAY_READ | MAY_WRITE | MAY_EXEC | > MAY_APPEND), FILE_CHECK); > @@ -679,7 +679,7 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id, > > /* Read entire file for all partial reads. */ > func = read_idmap[read_id] ?: FILE_CHECK; > - security_task_getsecid(current, &secid); > + security_task_getsecid_subj(current, &secid); > return process_measurement(file, current_cred(), secid, NULL, > 0, MAY_READ, func); > } > @@ -722,7 +722,7 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size, > } > > func = read_idmap[read_id] ?: FILE_CHECK; > - security_task_getsecid(current, &secid); > + security_task_getsecid_subj(current, &secid); > return process_measurement(file, current_cred(), secid, buf, size, > MAY_READ, func); > } > @@ -859,7 +859,7 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size, > * buffer measurements. > */ > if (func) { > - security_task_getsecid(current, &secid); > + security_task_getsecid_subj(current, &secid); > action = ima_get_action(inode, current_cred(), secid, 0, func, > &pcr, &template, keyring); > if (!(action & IMA_MEASURE)) > diff --git a/security/security.c b/security/security.c > index 401663b5b70ea..85e504df051b3 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -1757,12 +1757,19 @@ int security_task_getsid(struct task_struct *p) > return call_int_hook(task_getsid, 0, p); > } > > -void security_task_getsecid(struct task_struct *p, u32 *secid) > +void security_task_getsecid_subj(struct task_struct *p, u32 *secid) > { > *secid = 0; > - call_void_hook(task_getsecid, p, secid); > + call_void_hook(task_getsecid_subj, p, secid); > } > -EXPORT_SYMBOL(security_task_getsecid); > +EXPORT_SYMBOL(security_task_getsecid_subj); > + > +void security_task_getsecid_obj(struct task_struct *p, u32 *secid) > +{ > + *secid = 0; > + call_void_hook(task_getsecid_obj, p, secid); > +} > +EXPORT_SYMBOL(security_task_getsecid_obj); > > int security_task_setnice(struct task_struct *p, int nice) > { > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index af2994adf9dd1..f311541c4972e 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -7143,7 +7143,8 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { > LSM_HOOK_INIT(task_setpgid, selinux_task_setpgid), > LSM_HOOK_INIT(task_getpgid, selinux_task_getpgid), > LSM_HOOK_INIT(task_getsid, selinux_task_getsid), > - LSM_HOOK_INIT(task_getsecid, selinux_task_getsecid), > + LSM_HOOK_INIT(task_getsecid_subj, selinux_task_getsecid), > + LSM_HOOK_INIT(task_getsecid_obj, selinux_task_getsecid), > LSM_HOOK_INIT(task_setnice, selinux_task_setnice), > LSM_HOOK_INIT(task_setioprio, selinux_task_setioprio), > LSM_HOOK_INIT(task_getioprio, selinux_task_getioprio), > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > index f69c3dd9a0c67..2bb354ef2c4a9 100644 > --- a/security/smack/smack_lsm.c > +++ b/security/smack/smack_lsm.c > @@ -4755,7 +4755,8 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = { > LSM_HOOK_INIT(task_setpgid, smack_task_setpgid), > LSM_HOOK_INIT(task_getpgid, smack_task_getpgid), > LSM_HOOK_INIT(task_getsid, smack_task_getsid), > - LSM_HOOK_INIT(task_getsecid, smack_task_getsecid), > + LSM_HOOK_INIT(task_getsecid_subj, smack_task_getsecid), > + LSM_HOOK_INIT(task_getsecid_obj, smack_task_getsecid), > LSM_HOOK_INIT(task_setnice, smack_task_setnice), > LSM_HOOK_INIT(task_setioprio, smack_task_setioprio), > LSM_HOOK_INIT(task_getioprio, smack_task_getioprio), >
On 3/9/21 4:28 PM, Paul Moore wrote: > On Wed, Mar 3, 2021 at 7:44 PM Paul Moore <paul@paul-moore.com> wrote: >> On Sun, Feb 21, 2021 at 7:51 AM John Johansen >> <john.johansen@canonical.com> wrote: >>> On 2/19/21 3:29 PM, Paul Moore wrote: >>>> Of the three LSMs that implement the security_task_getsecid() LSM >>>> hook, all three LSMs provide the task's objective security >>>> credentials. This turns out to be unfortunate as most of the hook's >>>> callers seem to expect the task's subjective credentials, although >>>> a small handful of callers do correctly expect the objective >>>> credentials. >>>> >>>> This patch is the first step towards fixing the problem: it splits >>>> the existing security_task_getsecid() hook into two variants, one >>>> for the subjective creds, one for the objective creds. >>>> >>>> void security_task_getsecid_subj(struct task_struct *p, >>>> u32 *secid); >>>> void security_task_getsecid_obj(struct task_struct *p, >>>> u32 *secid); >>>> >>>> While this patch does fix all of the callers to use the correct >>>> variant, in order to keep this patch focused on the callers and to >>>> ease review, the LSMs continue to use the same implementation for >>>> both hooks. The net effect is that this patch should not change >>>> the behavior of the kernel in any way, it will be up to the latter >>>> LSM specific patches in this series to change the hook >>>> implementations and return the correct credentials. >>>> >>>> Signed-off-by: Paul Moore <paul@paul-moore.com> >>> >>> So far this looks good. I want to take another stab at it and give >>> it some testing >> >> Checking in as I know you said you needed to fix/release the AppArmor >> patch in this series ... is this patch still looking okay to you? If >> so, can I get an ACK at least on this patch? > > Hi John, > > Any objections if I merge the LSM, SELinux, and Smack patches into the > selinux/next tree so that we can start getting some wider testing? If > I leave out my poor attempt at an AppArmor patch, the current in-tree > AppArmor code should behave exactly as it does today with the > apparmor_task_getsecid() function handling both the subjective and > objective creds. I can always merge the AppArmor patch later when you > have it ready, or you can merge it via your AppArmor tree at a later > date. > I have some questions around selinux and binder but I don't have any objections to you merging, we can always drop fixes on top if they are necessary
On Fri, Mar 5, 2021 at 12:44 AM Paul Moore <paul@paul-moore.com> wrote: > > On Thu, Mar 4, 2021 at 5:04 AM Jeffrey Vander Stoep <jeffv@google.com> wrote: > > On Sat, Feb 20, 2021 at 3:45 PM Paul Moore <paul@paul-moore.com> wrote: > > > On Fri, Feb 19, 2021 at 9:57 PM James Morris <jmorris@namei.org> wrote: > > > > On Fri, 19 Feb 2021, Paul Moore wrote: > > > > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > > > > > index c119736ca56ac..39d501261108d 100644 > > > > > --- a/drivers/android/binder.c > > > > > +++ b/drivers/android/binder.c > > > > > @@ -2700,7 +2700,7 @@ static void binder_transaction(struct binder_proc *proc, > > > > > u32 secid; > > > > > size_t added_size; > > > > > > > > > > - security_task_getsecid(proc->tsk, &secid); > > > > > + security_task_getsecid_subj(proc->tsk, &secid); > > > > > ret = security_secid_to_secctx(secid, &secctx, &secctx_sz); > > > > > if (ret) { > > > > > return_error = BR_FAILED_REPLY; > > > > > > > > Can someone from the Android project confirm this is correct for binder? > > > > This looks correct to me. > > Thanks for the verification. Should I assume the SELinux specific > binder changes looked okay too? > Yes, those also look good to me. > https://lore.kernel.org/selinux/84053ed8-4778-f246-2177-cf5c1b9516a9@canonical.com/T/#m4ae49d4a5a62d600fa3f3b1a5bba2d6611b1051c > > -- > paul moore > www.paul-moore.com
On Tue, Mar 9, 2021 at 8:03 PM John Johansen <john.johansen@canonical.com> wrote: > On 2/19/21 3:29 PM, Paul Moore wrote: > > Of the three LSMs that implement the security_task_getsecid() LSM > > hook, all three LSMs provide the task's objective security > > credentials. This turns out to be unfortunate as most of the hook's > > callers seem to expect the task's subjective credentials, although > > a small handful of callers do correctly expect the objective > > credentials. > > > > This patch is the first step towards fixing the problem: it splits > > the existing security_task_getsecid() hook into two variants, one > > for the subjective creds, one for the objective creds. > > > > void security_task_getsecid_subj(struct task_struct *p, > > u32 *secid); > > void security_task_getsecid_obj(struct task_struct *p, > > u32 *secid); > > > > While this patch does fix all of the callers to use the correct > > variant, in order to keep this patch focused on the callers and to > > ease review, the LSMs continue to use the same implementation for > > both hooks. The net effect is that this patch should not change > > the behavior of the kernel in any way, it will be up to the latter > > LSM specific patches in this series to change the hook > > implementations and return the correct credentials. > > > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > Reviewed-by: John Johansen <john.johansen@canonical.com> Thanks John, I know you're swamped these days.
On Wed, Mar 10, 2021 at 3:21 AM Jeffrey Vander Stoep <jeffv@google.com> wrote: > On Fri, Mar 5, 2021 at 12:44 AM Paul Moore <paul@paul-moore.com> wrote: > > > > On Thu, Mar 4, 2021 at 5:04 AM Jeffrey Vander Stoep <jeffv@google.com> wrote: > > > On Sat, Feb 20, 2021 at 3:45 PM Paul Moore <paul@paul-moore.com> wrote: > > > > On Fri, Feb 19, 2021 at 9:57 PM James Morris <jmorris@namei.org> wrote: > > > > > On Fri, 19 Feb 2021, Paul Moore wrote: > > > > > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > > > > > > index c119736ca56ac..39d501261108d 100644 > > > > > > --- a/drivers/android/binder.c > > > > > > +++ b/drivers/android/binder.c > > > > > > @@ -2700,7 +2700,7 @@ static void binder_transaction(struct binder_proc *proc, > > > > > > u32 secid; > > > > > > size_t added_size; > > > > > > > > > > > > - security_task_getsecid(proc->tsk, &secid); > > > > > > + security_task_getsecid_subj(proc->tsk, &secid); > > > > > > ret = security_secid_to_secctx(secid, &secctx, &secctx_sz); > > > > > > if (ret) { > > > > > > return_error = BR_FAILED_REPLY; > > > > > > > > > > Can someone from the Android project confirm this is correct for binder? > > > > > > This looks correct to me. > > > > Thanks for the verification. Should I assume the SELinux specific > > binder changes looked okay too? > > > Yes, those also look good to me. Thanks, that binder changes were the one area I wasn't 100% on, I appreciate the verification.
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index c119736ca56ac..39d501261108d 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -2700,7 +2700,7 @@ static void binder_transaction(struct binder_proc *proc, u32 secid; size_t added_size; - security_task_getsecid(proc->tsk, &secid); + security_task_getsecid_subj(proc->tsk, &secid); ret = security_secid_to_secctx(secid, &secctx, &secctx_sz); if (ret) { return_error = BR_FAILED_REPLY; diff --git a/include/linux/cred.h b/include/linux/cred.h index 18639c069263f..42b9d88d9a565 100644 --- a/include/linux/cred.h +++ b/include/linux/cred.h @@ -140,7 +140,7 @@ struct cred { struct key *request_key_auth; /* assumed request_key authority */ #endif #ifdef CONFIG_SECURITY - void *security; /* subjective LSM security */ + void *security; /* LSM security */ #endif struct user_struct *user; /* real user ID subscription */ struct user_namespace *user_ns; /* user_ns the caps and keyrings are relative to. */ diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index dfd261dcbcb04..1490a185135a0 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -200,7 +200,10 @@ LSM_HOOK(int, 0, task_fix_setgid, struct cred *new, const struct cred * old, LSM_HOOK(int, 0, task_setpgid, struct task_struct *p, pid_t pgid) LSM_HOOK(int, 0, task_getpgid, struct task_struct *p) LSM_HOOK(int, 0, task_getsid, struct task_struct *p) -LSM_HOOK(void, LSM_RET_VOID, task_getsecid, struct task_struct *p, u32 *secid) +LSM_HOOK(void, LSM_RET_VOID, task_getsecid_subj, + struct task_struct *p, u32 *secid) +LSM_HOOK(void, LSM_RET_VOID, task_getsecid_obj, + struct task_struct *p, u32 *secid) LSM_HOOK(int, 0, task_setnice, struct task_struct *p, int nice) LSM_HOOK(int, 0, task_setioprio, struct task_struct *p, int ioprio) LSM_HOOK(int, 0, task_getioprio, struct task_struct *p) diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index bdfc8a76a4f79..13d2a9a6f2014 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -706,8 +706,12 @@ * @p. * @p contains the task_struct for the process. * Return 0 if permission is granted. - * @task_getsecid: - * Retrieve the security identifier of the process @p. + * @task_getsecid_subj: + * Retrieve the subjective security identifier of the process @p. + * @p contains the task_struct for the process and place is into @secid. + * In case of failure, @secid will be set to zero. + * @task_getsecid_obj: + * Retrieve the objective security identifier of the process @p. * @p contains the task_struct for the process and place is into @secid. * In case of failure, @secid will be set to zero. * diff --git a/include/linux/security.h b/include/linux/security.h index b0d14f04b16de..1826bb0cea825 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -406,7 +406,8 @@ int security_task_fix_setgid(struct cred *new, const struct cred *old, int security_task_setpgid(struct task_struct *p, pid_t pgid); int security_task_getpgid(struct task_struct *p); int security_task_getsid(struct task_struct *p); -void security_task_getsecid(struct task_struct *p, u32 *secid); +void security_task_getsecid_subj(struct task_struct *p, u32 *secid); +void security_task_getsecid_obj(struct task_struct *p, u32 *secid); int security_task_setnice(struct task_struct *p, int nice); int security_task_setioprio(struct task_struct *p, int ioprio); int security_task_getioprio(struct task_struct *p); @@ -1084,7 +1085,12 @@ static inline int security_task_getsid(struct task_struct *p) return 0; } -static inline void security_task_getsecid(struct task_struct *p, u32 *secid) +static inline void security_task_getsecid_subj(struct task_struct *p, u32 *secid) +{ + *secid = 0; +} + +static inline void security_task_getsecid_obj(struct task_struct *p, u32 *secid) { *secid = 0; } diff --git a/kernel/audit.c b/kernel/audit.c index 1ffc2e059027d..8e725db6ecb02 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -2132,7 +2132,7 @@ int audit_log_task_context(struct audit_buffer *ab) int error; u32 sid; - security_task_getsecid(current, &sid); + security_task_getsecid_subj(current, &sid); if (!sid) return 0; @@ -2353,7 +2353,7 @@ int audit_signal_info(int sig, struct task_struct *t) audit_sig_uid = auid; else audit_sig_uid = uid; - security_task_getsecid(current, &audit_sig_sid); + security_task_getsecid_subj(current, &audit_sig_sid); } return audit_signal_info_syscall(t); diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c index 333b3bcfc5458..db2c6b59dfc33 100644 --- a/kernel/auditfilter.c +++ b/kernel/auditfilter.c @@ -1359,7 +1359,8 @@ int audit_filter(int msgtype, unsigned int listtype) case AUDIT_SUBJ_SEN: case AUDIT_SUBJ_CLR: if (f->lsm_rule) { - security_task_getsecid(current, &sid); + security_task_getsecid_subj(current, + &sid); result = security_audit_rule_match(sid, f->type, f->op, f->lsm_rule); } diff --git a/kernel/auditsc.c b/kernel/auditsc.c index ce8c9e2279ba9..3bfbecca4664a 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -667,7 +667,7 @@ static int audit_filter_rules(struct task_struct *tsk, logged upon error */ if (f->lsm_rule) { if (need_sid) { - security_task_getsecid(tsk, &sid); + security_task_getsecid_subj(tsk, &sid); need_sid = 0; } result = security_audit_rule_match(sid, f->type, @@ -2400,7 +2400,7 @@ void __audit_ptrace(struct task_struct *t) context->target_auid = audit_get_loginuid(t); context->target_uid = task_uid(t); context->target_sessionid = audit_get_sessionid(t); - security_task_getsecid(t, &context->target_sid); + security_task_getsecid_obj(t, &context->target_sid); memcpy(context->target_comm, t->comm, TASK_COMM_LEN); } @@ -2427,7 +2427,7 @@ int audit_signal_info_syscall(struct task_struct *t) ctx->target_auid = audit_get_loginuid(t); ctx->target_uid = t_uid; ctx->target_sessionid = audit_get_sessionid(t); - security_task_getsecid(t, &ctx->target_sid); + security_task_getsecid_obj(t, &ctx->target_sid); memcpy(ctx->target_comm, t->comm, TASK_COMM_LEN); return 0; } @@ -2448,7 +2448,7 @@ int audit_signal_info_syscall(struct task_struct *t) axp->target_auid[axp->pid_count] = audit_get_loginuid(t); axp->target_uid[axp->pid_count] = t_uid; axp->target_sessionid[axp->pid_count] = audit_get_sessionid(t); - security_task_getsecid(t, &axp->target_sid[axp->pid_count]); + security_task_getsecid_obj(t, &axp->target_sid[axp->pid_count]); memcpy(axp->target_comm[axp->pid_count], t->comm, TASK_COMM_LEN); axp->pid_count++; diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c index ccb4916428116..3e6ac9b790b15 100644 --- a/net/netlabel/netlabel_unlabeled.c +++ b/net/netlabel/netlabel_unlabeled.c @@ -1539,7 +1539,7 @@ int __init netlbl_unlabel_defconf(void) /* Only the kernel is allowed to call this function and the only time * it is called is at bootup before the audit subsystem is reporting * messages so don't worry to much about these values. */ - security_task_getsecid(current, &audit_info.secid); + security_task_getsecid_subj(current, &audit_info.secid); audit_info.loginuid = GLOBAL_ROOT_UID; audit_info.sessionid = 0; diff --git a/net/netlabel/netlabel_user.h b/net/netlabel/netlabel_user.h index 3c67afce64f12..b9ba8112b3c52 100644 --- a/net/netlabel/netlabel_user.h +++ b/net/netlabel/netlabel_user.h @@ -34,7 +34,7 @@ static inline void netlbl_netlink_auditinfo(struct sk_buff *skb, struct netlbl_audit *audit_info) { - security_task_getsecid(current, &audit_info->secid); + security_task_getsecid_subj(current, &audit_info->secid); audit_info->loginuid = audit_get_loginuid(current); audit_info->sessionid = audit_get_sessionid(current); } diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 1b0aba8eb7235..15e37b9132679 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -1243,7 +1243,8 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(task_free, apparmor_task_free), LSM_HOOK_INIT(task_alloc, apparmor_task_alloc), - LSM_HOOK_INIT(task_getsecid, apparmor_task_getsecid), + LSM_HOOK_INIT(task_getsecid_subj, apparmor_task_getsecid), + LSM_HOOK_INIT(task_getsecid_obj, apparmor_task_getsecid), LSM_HOOK_INIT(task_setrlimit, apparmor_task_setrlimit), LSM_HOOK_INIT(task_kill, apparmor_task_kill), diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 8361941ee0a12..afa4923dbd33d 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -75,7 +75,7 @@ int ima_must_appraise(struct inode *inode, int mask, enum ima_hooks func) if (!ima_appraise) return 0; - security_task_getsecid(current, &secid); + security_task_getsecid_subj(current, &secid); return ima_match_policy(inode, current_cred(), secid, func, mask, IMA_APPRAISE | IMA_HASH, NULL, NULL, NULL); } diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index f87cb29329e91..97a6913bb3d86 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -391,7 +391,7 @@ int ima_file_mmap(struct file *file, unsigned long prot) u32 secid; if (file && (prot & PROT_EXEC)) { - security_task_getsecid(current, &secid); + security_task_getsecid_subj(current, &secid); return process_measurement(file, current_cred(), secid, NULL, 0, MAY_EXEC, MMAP_CHECK); } @@ -429,7 +429,7 @@ int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot) !(prot & PROT_EXEC) || (vma->vm_flags & VM_EXEC)) return 0; - security_task_getsecid(current, &secid); + security_task_getsecid_subj(current, &secid); inode = file_inode(vma->vm_file); action = ima_get_action(inode, current_cred(), secid, MAY_EXEC, MMAP_CHECK, &pcr, &template, 0); @@ -469,7 +469,7 @@ int ima_bprm_check(struct linux_binprm *bprm) int ret; u32 secid; - security_task_getsecid(current, &secid); + security_task_getsecid_subj(current, &secid); ret = process_measurement(bprm->file, current_cred(), secid, NULL, 0, MAY_EXEC, BPRM_CHECK); if (ret) @@ -494,7 +494,7 @@ int ima_file_check(struct file *file, int mask) { u32 secid; - security_task_getsecid(current, &secid); + security_task_getsecid_subj(current, &secid); return process_measurement(file, current_cred(), secid, NULL, 0, mask & (MAY_READ | MAY_WRITE | MAY_EXEC | MAY_APPEND), FILE_CHECK); @@ -679,7 +679,7 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id, /* Read entire file for all partial reads. */ func = read_idmap[read_id] ?: FILE_CHECK; - security_task_getsecid(current, &secid); + security_task_getsecid_subj(current, &secid); return process_measurement(file, current_cred(), secid, NULL, 0, MAY_READ, func); } @@ -722,7 +722,7 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size, } func = read_idmap[read_id] ?: FILE_CHECK; - security_task_getsecid(current, &secid); + security_task_getsecid_subj(current, &secid); return process_measurement(file, current_cred(), secid, buf, size, MAY_READ, func); } @@ -859,7 +859,7 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size, * buffer measurements. */ if (func) { - security_task_getsecid(current, &secid); + security_task_getsecid_subj(current, &secid); action = ima_get_action(inode, current_cred(), secid, 0, func, &pcr, &template, keyring); if (!(action & IMA_MEASURE)) diff --git a/security/security.c b/security/security.c index 401663b5b70ea..85e504df051b3 100644 --- a/security/security.c +++ b/security/security.c @@ -1757,12 +1757,19 @@ int security_task_getsid(struct task_struct *p) return call_int_hook(task_getsid, 0, p); } -void security_task_getsecid(struct task_struct *p, u32 *secid) +void security_task_getsecid_subj(struct task_struct *p, u32 *secid) { *secid = 0; - call_void_hook(task_getsecid, p, secid); + call_void_hook(task_getsecid_subj, p, secid); } -EXPORT_SYMBOL(security_task_getsecid); +EXPORT_SYMBOL(security_task_getsecid_subj); + +void security_task_getsecid_obj(struct task_struct *p, u32 *secid) +{ + *secid = 0; + call_void_hook(task_getsecid_obj, p, secid); +} +EXPORT_SYMBOL(security_task_getsecid_obj); int security_task_setnice(struct task_struct *p, int nice) { diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index af2994adf9dd1..f311541c4972e 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -7143,7 +7143,8 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(task_setpgid, selinux_task_setpgid), LSM_HOOK_INIT(task_getpgid, selinux_task_getpgid), LSM_HOOK_INIT(task_getsid, selinux_task_getsid), - LSM_HOOK_INIT(task_getsecid, selinux_task_getsecid), + LSM_HOOK_INIT(task_getsecid_subj, selinux_task_getsecid), + LSM_HOOK_INIT(task_getsecid_obj, selinux_task_getsecid), LSM_HOOK_INIT(task_setnice, selinux_task_setnice), LSM_HOOK_INIT(task_setioprio, selinux_task_setioprio), LSM_HOOK_INIT(task_getioprio, selinux_task_getioprio), diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index f69c3dd9a0c67..2bb354ef2c4a9 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -4755,7 +4755,8 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(task_setpgid, smack_task_setpgid), LSM_HOOK_INIT(task_getpgid, smack_task_getpgid), LSM_HOOK_INIT(task_getsid, smack_task_getsid), - LSM_HOOK_INIT(task_getsecid, smack_task_getsecid), + LSM_HOOK_INIT(task_getsecid_subj, smack_task_getsecid), + LSM_HOOK_INIT(task_getsecid_obj, smack_task_getsecid), LSM_HOOK_INIT(task_setnice, smack_task_setnice), LSM_HOOK_INIT(task_setioprio, smack_task_setioprio), LSM_HOOK_INIT(task_getioprio, smack_task_getioprio),
Of the three LSMs that implement the security_task_getsecid() LSM hook, all three LSMs provide the task's objective security credentials. This turns out to be unfortunate as most of the hook's callers seem to expect the task's subjective credentials, although a small handful of callers do correctly expect the objective credentials. This patch is the first step towards fixing the problem: it splits the existing security_task_getsecid() hook into two variants, one for the subjective creds, one for the objective creds. void security_task_getsecid_subj(struct task_struct *p, u32 *secid); void security_task_getsecid_obj(struct task_struct *p, u32 *secid); While this patch does fix all of the callers to use the correct variant, in order to keep this patch focused on the callers and to ease review, the LSMs continue to use the same implementation for both hooks. The net effect is that this patch should not change the behavior of the kernel in any way, it will be up to the latter LSM specific patches in this series to change the hook implementations and return the correct credentials. Signed-off-by: Paul Moore <paul@paul-moore.com> --- drivers/android/binder.c | 2 +- include/linux/cred.h | 2 +- include/linux/lsm_hook_defs.h | 5 ++++- include/linux/lsm_hooks.h | 8 ++++++-- include/linux/security.h | 10 ++++++++-- kernel/audit.c | 4 ++-- kernel/auditfilter.c | 3 ++- kernel/auditsc.c | 8 ++++---- net/netlabel/netlabel_unlabeled.c | 2 +- net/netlabel/netlabel_user.h | 2 +- security/apparmor/lsm.c | 3 ++- security/integrity/ima/ima_appraise.c | 2 +- security/integrity/ima/ima_main.c | 14 +++++++------- security/security.c | 13 ++++++++++--- security/selinux/hooks.c | 3 ++- security/smack/smack_lsm.c | 3 ++- 16 files changed, 54 insertions(+), 30 deletions(-)