linux-audit.redhat.com archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH 0/4] Split security_task_getsecid() into subj and obj variants
@ 2021-02-19 23:28 Paul Moore
  2021-02-19 23:29 ` [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants Paul Moore
                   ` (4 more replies)
  0 siblings, 5 replies; 39+ messages in thread
From: Paul Moore @ 2021-02-19 23:28 UTC (permalink / raw)
  To: Casey Schaufler, John Johansen
  Cc: selinux, linux-security-module, linux-audit

As discussed briefly on the list (lore link below), we are a little
sloppy when it comes to using task credentials, mixing both the
subjective and object credentials.  This patch set attempts to fix
this by replacing security_task_getsecid() with two new hooks that
return either the subjective (_subj) or objective (_obj) credentials.

https://lore.kernel.org/linux-security-module/806848326.0ifERbkFSE@x2/T/

Casey and John, I made a quick pass through the Smack and AppArmor
code in an effort to try and do the right thing, but I will admit
that I haven't tested those changes, just the SELinux code.  I
would really appreciate your help in reviewing those changes.  If
you find it easier, feel free to wholesale replace my Smack/AppArmor
patch with one of your own.

---

Paul Moore (4):
      lsm: separate security_task_getsecid() into subjective and objective variants
      selinux: clarify task subjective and objective credentials
      smack: differentiate between subjective and objective task credentials
      apparmor: differentiate between subjective and objective task credentials


 security/apparmor/domain.c       |  2 +-
 security/apparmor/include/cred.h | 19 +++++--
 security/apparmor/include/task.h |  3 +-
 security/apparmor/lsm.c          | 23 ++++++---
 security/apparmor/task.c         | 23 +++++++--
 security/selinux/hooks.c         | 85 ++++++++++++++++++--------------
 security/smack/smack.h           | 18 ++++++-
 security/smack/smack_lsm.c       | 40 ++++++++++-----
 8 files changed, 147 insertions(+), 66 deletions(-)

--
Signature

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants
  2021-02-19 23:28 [RFC PATCH 0/4] Split security_task_getsecid() into subj and obj variants Paul Moore
@ 2021-02-19 23:29 ` Paul Moore
  2021-02-20  2:55   ` James Morris
                     ` (4 more replies)
  2021-02-19 23:29 ` [RFC PATCH 2/4] selinux: clarify task subjective and objective credentials Paul Moore
                   ` (3 subsequent siblings)
  4 siblings, 5 replies; 39+ messages in thread
From: Paul Moore @ 2021-02-19 23:29 UTC (permalink / raw)
  To: Casey Schaufler, John Johansen
  Cc: selinux, linux-security-module, linux-audit

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(-)

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


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [RFC PATCH 2/4] selinux: clarify task subjective and objective credentials
  2021-02-19 23:28 [RFC PATCH 0/4] Split security_task_getsecid() into subj and obj variants Paul Moore
  2021-02-19 23:29 ` [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants Paul Moore
@ 2021-02-19 23:29 ` Paul Moore
  2021-02-21 12:55   ` John Johansen
                     ` (2 more replies)
  2021-02-19 23:29 ` [RFC PATCH 3/4] smack: differentiate between subjective and objective task credentials Paul Moore
                   ` (2 subsequent siblings)
  4 siblings, 3 replies; 39+ messages in thread
From: Paul Moore @ 2021-02-19 23:29 UTC (permalink / raw)
  To: Casey Schaufler, John Johansen
  Cc: selinux, linux-security-module, linux-audit

SELinux has a function, task_sid(), which returns the task's
objective credentials, but unfortunately is used in a few places
where the subjective task credentials should be used.  Most notably
in the new security_task_getsecid_subj() LSM hook.

This patch fixes this and attempts to make things more obvious by
introducing a new function, task_sid_subj(), and renaming the
existing task_sid() function to task_sid_obj().

Signed-off-by: Paul Moore <paul@paul-moore.com>
---
 security/selinux/hooks.c |   85 +++++++++++++++++++++++++++-------------------
 1 file changed, 49 insertions(+), 36 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f311541c4972e..1c53000d28e37 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -229,10 +229,23 @@ static inline u32 cred_sid(const struct cred *cred)
 	return tsec->sid;
 }
 
+/*
+ * get the subjective security ID of a task
+ */
+static inline u32 task_sid_subj(const struct task_struct *task)
+{
+	u32 sid;
+
+	rcu_read_lock();
+	sid = cred_sid(rcu_dereference(task->cred));
+	rcu_read_unlock();
+	return sid;
+}
+
 /*
  * get the objective security ID of a task
  */
-static inline u32 task_sid(const struct task_struct *task)
+static inline u32 task_sid_obj(const struct task_struct *task)
 {
 	u32 sid;
 
@@ -2034,11 +2047,8 @@ static inline u32 open_file_to_av(struct file *file)
 
 static int selinux_binder_set_context_mgr(struct task_struct *mgr)
 {
-	u32 mysid = current_sid();
-	u32 mgrsid = task_sid(mgr);
-
 	return avc_has_perm(&selinux_state,
-			    mysid, mgrsid, SECCLASS_BINDER,
+			    current_sid(), task_sid_obj(mgr), SECCLASS_BINDER,
 			    BINDER__SET_CONTEXT_MGR, NULL);
 }
 
@@ -2046,8 +2056,8 @@ static int selinux_binder_transaction(struct task_struct *from,
 				      struct task_struct *to)
 {
 	u32 mysid = current_sid();
-	u32 fromsid = task_sid(from);
-	u32 tosid = task_sid(to);
+	u32 fromsid = task_sid_subj(from);
+	u32 tosid = task_sid_subj(to);
 	int rc;
 
 	if (mysid != fromsid) {
@@ -2066,11 +2076,9 @@ static int selinux_binder_transaction(struct task_struct *from,
 static int selinux_binder_transfer_binder(struct task_struct *from,
 					  struct task_struct *to)
 {
-	u32 fromsid = task_sid(from);
-	u32 tosid = task_sid(to);
-
 	return avc_has_perm(&selinux_state,
-			    fromsid, tosid, SECCLASS_BINDER, BINDER__TRANSFER,
+			    task_sid_subj(from), task_sid_obj(to),
+			    SECCLASS_BINDER, BINDER__TRANSFER,
 			    NULL);
 }
 
@@ -2078,7 +2086,7 @@ static int selinux_binder_transfer_file(struct task_struct *from,
 					struct task_struct *to,
 					struct file *file)
 {
-	u32 sid = task_sid(to);
+	u32 sid = task_sid_subj(to);
 	struct file_security_struct *fsec = selinux_file(file);
 	struct dentry *dentry = file->f_path.dentry;
 	struct inode_security_struct *isec;
@@ -2114,10 +2122,10 @@ static int selinux_binder_transfer_file(struct task_struct *from,
 }
 
 static int selinux_ptrace_access_check(struct task_struct *child,
-				     unsigned int mode)
+				       unsigned int mode)
 {
 	u32 sid = current_sid();
-	u32 csid = task_sid(child);
+	u32 csid = task_sid_obj(child);
 
 	if (mode & PTRACE_MODE_READ)
 		return avc_has_perm(&selinux_state,
@@ -2130,15 +2138,15 @@ static int selinux_ptrace_access_check(struct task_struct *child,
 static int selinux_ptrace_traceme(struct task_struct *parent)
 {
 	return avc_has_perm(&selinux_state,
-			    task_sid(parent), current_sid(), SECCLASS_PROCESS,
-			    PROCESS__PTRACE, NULL);
+			    task_sid_subj(parent), task_sid_obj(current),
+			    SECCLASS_PROCESS, PROCESS__PTRACE, NULL);
 }
 
 static int selinux_capget(struct task_struct *target, kernel_cap_t *effective,
 			  kernel_cap_t *inheritable, kernel_cap_t *permitted)
 {
 	return avc_has_perm(&selinux_state,
-			    current_sid(), task_sid(target), SECCLASS_PROCESS,
+			    current_sid(), task_sid_obj(target), SECCLASS_PROCESS,
 			    PROCESS__GETCAP, NULL);
 }
 
@@ -2263,7 +2271,7 @@ static u32 ptrace_parent_sid(void)
 	rcu_read_lock();
 	tracer = ptrace_parent(current);
 	if (tracer)
-		sid = task_sid(tracer);
+		sid = task_sid_obj(tracer);
 	rcu_read_unlock();
 
 	return sid;
@@ -3916,7 +3924,7 @@ static int selinux_file_send_sigiotask(struct task_struct *tsk,
 				       struct fown_struct *fown, int signum)
 {
 	struct file *file;
-	u32 sid = task_sid(tsk);
+	u32 sid = task_sid_obj(tsk);
 	u32 perm;
 	struct file_security_struct *fsec;
 
@@ -4135,47 +4143,52 @@ static int selinux_kernel_load_data(enum kernel_load_data_id id, bool contents)
 static int selinux_task_setpgid(struct task_struct *p, pid_t pgid)
 {
 	return avc_has_perm(&selinux_state,
-			    current_sid(), task_sid(p), SECCLASS_PROCESS,
+			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
 			    PROCESS__SETPGID, NULL);
 }
 
 static int selinux_task_getpgid(struct task_struct *p)
 {
 	return avc_has_perm(&selinux_state,
-			    current_sid(), task_sid(p), SECCLASS_PROCESS,
+			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
 			    PROCESS__GETPGID, NULL);
 }
 
 static int selinux_task_getsid(struct task_struct *p)
 {
 	return avc_has_perm(&selinux_state,
-			    current_sid(), task_sid(p), SECCLASS_PROCESS,
+			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
 			    PROCESS__GETSESSION, NULL);
 }
 
-static void selinux_task_getsecid(struct task_struct *p, u32 *secid)
+static void selinux_task_getsecid_subj(struct task_struct *p, u32 *secid)
+{
+	*secid = task_sid_subj(p);
+}
+
+static void selinux_task_getsecid_obj(struct task_struct *p, u32 *secid)
 {
-	*secid = task_sid(p);
+	*secid = task_sid_obj(p);
 }
 
 static int selinux_task_setnice(struct task_struct *p, int nice)
 {
 	return avc_has_perm(&selinux_state,
-			    current_sid(), task_sid(p), SECCLASS_PROCESS,
+			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
 			    PROCESS__SETSCHED, NULL);
 }
 
 static int selinux_task_setioprio(struct task_struct *p, int ioprio)
 {
 	return avc_has_perm(&selinux_state,
-			    current_sid(), task_sid(p), SECCLASS_PROCESS,
+			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
 			    PROCESS__SETSCHED, NULL);
 }
 
 static int selinux_task_getioprio(struct task_struct *p)
 {
 	return avc_has_perm(&selinux_state,
-			    current_sid(), task_sid(p), SECCLASS_PROCESS,
+			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
 			    PROCESS__GETSCHED, NULL);
 }
 
@@ -4206,7 +4219,7 @@ static int selinux_task_setrlimit(struct task_struct *p, unsigned int resource,
 	   upon context transitions.  See selinux_bprm_committing_creds. */
 	if (old_rlim->rlim_max != new_rlim->rlim_max)
 		return avc_has_perm(&selinux_state,
-				    current_sid(), task_sid(p),
+				    current_sid(), task_sid_obj(p),
 				    SECCLASS_PROCESS, PROCESS__SETRLIMIT, NULL);
 
 	return 0;
@@ -4215,21 +4228,21 @@ static int selinux_task_setrlimit(struct task_struct *p, unsigned int resource,
 static int selinux_task_setscheduler(struct task_struct *p)
 {
 	return avc_has_perm(&selinux_state,
-			    current_sid(), task_sid(p), SECCLASS_PROCESS,
+			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
 			    PROCESS__SETSCHED, NULL);
 }
 
 static int selinux_task_getscheduler(struct task_struct *p)
 {
 	return avc_has_perm(&selinux_state,
-			    current_sid(), task_sid(p), SECCLASS_PROCESS,
+			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
 			    PROCESS__GETSCHED, NULL);
 }
 
 static int selinux_task_movememory(struct task_struct *p)
 {
 	return avc_has_perm(&selinux_state,
-			    current_sid(), task_sid(p), SECCLASS_PROCESS,
+			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
 			    PROCESS__SETSCHED, NULL);
 }
 
@@ -4248,14 +4261,14 @@ static int selinux_task_kill(struct task_struct *p, struct kernel_siginfo *info,
 	else
 		secid = cred_sid(cred);
 	return avc_has_perm(&selinux_state,
-			    secid, task_sid(p), SECCLASS_PROCESS, perm, NULL);
+			    secid, task_sid_obj(p), SECCLASS_PROCESS, perm, NULL);
 }
 
 static void selinux_task_to_inode(struct task_struct *p,
 				  struct inode *inode)
 {
 	struct inode_security_struct *isec = selinux_inode(inode);
-	u32 sid = task_sid(p);
+	u32 sid = task_sid_obj(p);
 
 	spin_lock(&isec->lock);
 	isec->sclass = inode_mode_to_security_class(inode->i_mode);
@@ -6148,7 +6161,7 @@ static int selinux_msg_queue_msgrcv(struct kern_ipc_perm *msq, struct msg_msg *m
 	struct ipc_security_struct *isec;
 	struct msg_security_struct *msec;
 	struct common_audit_data ad;
-	u32 sid = task_sid(target);
+	u32 sid = task_sid_subj(target);
 	int rc;
 
 	isec = selinux_ipc(msq);
@@ -7143,8 +7156,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_subj, selinux_task_getsecid),
-	LSM_HOOK_INIT(task_getsecid_obj, selinux_task_getsecid),
+	LSM_HOOK_INIT(task_getsecid_subj, selinux_task_getsecid_subj),
+	LSM_HOOK_INIT(task_getsecid_obj, selinux_task_getsecid_obj),
 	LSM_HOOK_INIT(task_setnice, selinux_task_setnice),
 	LSM_HOOK_INIT(task_setioprio, selinux_task_setioprio),
 	LSM_HOOK_INIT(task_getioprio, selinux_task_getioprio),

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [RFC PATCH 3/4] smack: differentiate between subjective and objective task credentials
  2021-02-19 23:28 [RFC PATCH 0/4] Split security_task_getsecid() into subj and obj variants Paul Moore
  2021-02-19 23:29 ` [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants Paul Moore
  2021-02-19 23:29 ` [RFC PATCH 2/4] selinux: clarify task subjective and objective credentials Paul Moore
@ 2021-02-19 23:29 ` Paul Moore
  2021-02-21 12:56   ` John Johansen
                     ` (2 more replies)
  2021-02-19 23:29 ` [RFC PATCH 4/4] apparmor: " Paul Moore
  2021-02-20  1:49 ` [RFC PATCH 0/4] Split security_task_getsecid() into subj and obj variants Casey Schaufler
  4 siblings, 3 replies; 39+ messages in thread
From: Paul Moore @ 2021-02-19 23:29 UTC (permalink / raw)
  To: Casey Schaufler, John Johansen
  Cc: selinux, linux-security-module, linux-audit

With the split of the security_task_getsecid() into subjective and
objective variants it's time to update Smack to ensure it is using
the correct task creds.

Signed-off-by: Paul Moore <paul@paul-moore.com>
---
 security/smack/smack.h     |   18 +++++++++++++++++-
 security/smack/smack_lsm.c |   40 +++++++++++++++++++++++++++-------------
 2 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/security/smack/smack.h b/security/smack/smack.h
index a9768b12716bf..08f9cb80655ce 100644
--- a/security/smack/smack.h
+++ b/security/smack/smack.h
@@ -383,7 +383,23 @@ static inline struct smack_known *smk_of_task(const struct task_smack *tsp)
 	return tsp->smk_task;
 }
 
-static inline struct smack_known *smk_of_task_struct(
+static inline struct smack_known *smk_of_task_struct_subj(
+						const struct task_struct *t)
+{
+	struct smack_known *skp;
+	const struct cred *cred;
+
+	rcu_read_lock();
+
+	cred = rcu_dereference(t->cred);
+	skp = smk_of_task(smack_cred(cred));
+
+	rcu_read_unlock();
+
+	return skp;
+}
+
+static inline struct smack_known *smk_of_task_struct_obj(
 						const struct task_struct *t)
 {
 	struct smack_known *skp;
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 2bb354ef2c4a9..ea1a82742e8ba 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -159,7 +159,7 @@ static int smk_bu_current(char *note, struct smack_known *oskp,
 static int smk_bu_task(struct task_struct *otp, int mode, int rc)
 {
 	struct task_smack *tsp = smack_cred(current_cred());
-	struct smack_known *smk_task = smk_of_task_struct(otp);
+	struct smack_known *smk_task = smk_of_task_struct_obj(otp);
 	char acc[SMK_NUM_ACCESS_TYPE + 1];
 
 	if (rc <= 0)
@@ -479,7 +479,7 @@ static int smack_ptrace_access_check(struct task_struct *ctp, unsigned int mode)
 {
 	struct smack_known *skp;
 
-	skp = smk_of_task_struct(ctp);
+	skp = smk_of_task_struct_obj(ctp);
 
 	return smk_ptrace_rule_check(current, skp, mode, __func__);
 }
@@ -2031,7 +2031,7 @@ static int smk_curacc_on_task(struct task_struct *p, int access,
 				const char *caller)
 {
 	struct smk_audit_info ad;
-	struct smack_known *skp = smk_of_task_struct(p);
+	struct smack_known *skp = smk_of_task_struct_subj(p);
 	int rc;
 
 	smk_ad_init(&ad, caller, LSM_AUDIT_DATA_TASK);
@@ -2076,15 +2076,29 @@ static int smack_task_getsid(struct task_struct *p)
 }
 
 /**
- * smack_task_getsecid - get the secid of the task
- * @p: the object task
+ * smack_task_getsecid_subj - get the subjective secid of the task
+ * @p: the task
  * @secid: where to put the result
  *
- * Sets the secid to contain a u32 version of the smack label.
+ * Sets the secid to contain a u32 version of the task's subjective smack label.
+ */
+static void smack_task_getsecid_subj(struct task_struct *p, u32 *secid)
+{
+	struct smack_known *skp = smk_of_task_struct_subj(p);
+
+	*secid = skp->smk_secid;
+}
+
+/**
+ * smack_task_getsecid_obj - get the objective secid of the task
+ * @p: the task
+ * @secid: where to put the result
+ *
+ * Sets the secid to contain a u32 version of the task's objective smack label.
  */
-static void smack_task_getsecid(struct task_struct *p, u32 *secid)
+static void smack_task_getsecid_obj(struct task_struct *p, u32 *secid)
 {
-	struct smack_known *skp = smk_of_task_struct(p);
+	struct smack_known *skp = smk_of_task_struct_obj(p);
 
 	*secid = skp->smk_secid;
 }
@@ -2172,7 +2186,7 @@ static int smack_task_kill(struct task_struct *p, struct kernel_siginfo *info,
 {
 	struct smk_audit_info ad;
 	struct smack_known *skp;
-	struct smack_known *tkp = smk_of_task_struct(p);
+	struct smack_known *tkp = smk_of_task_struct_obj(p);
 	int rc;
 
 	if (!sig)
@@ -2210,7 +2224,7 @@ static int smack_task_kill(struct task_struct *p, struct kernel_siginfo *info,
 static void smack_task_to_inode(struct task_struct *p, struct inode *inode)
 {
 	struct inode_smack *isp = smack_inode(inode);
-	struct smack_known *skp = smk_of_task_struct(p);
+	struct smack_known *skp = smk_of_task_struct_obj(p);
 
 	isp->smk_inode = skp;
 	isp->smk_flags |= SMK_INODE_INSTANT;
@@ -3481,7 +3495,7 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
  */
 static int smack_getprocattr(struct task_struct *p, char *name, char **value)
 {
-	struct smack_known *skp = smk_of_task_struct(p);
+	struct smack_known *skp = smk_of_task_struct_subj(p);
 	char *cp;
 	int slen;
 
@@ -4755,8 +4769,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_subj, smack_task_getsecid),
-	LSM_HOOK_INIT(task_getsecid_obj, smack_task_getsecid),
+	LSM_HOOK_INIT(task_getsecid_subj, smack_task_getsecid_subj),
+	LSM_HOOK_INIT(task_getsecid_obj, smack_task_getsecid_obj),
 	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


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [RFC PATCH 4/4] apparmor: differentiate between subjective and objective task credentials
  2021-02-19 23:28 [RFC PATCH 0/4] Split security_task_getsecid() into subj and obj variants Paul Moore
                   ` (2 preceding siblings ...)
  2021-02-19 23:29 ` [RFC PATCH 3/4] smack: differentiate between subjective and objective task credentials Paul Moore
@ 2021-02-19 23:29 ` Paul Moore
  2021-02-21 12:57   ` John Johansen
  2021-02-20  1:49 ` [RFC PATCH 0/4] Split security_task_getsecid() into subj and obj variants Casey Schaufler
  4 siblings, 1 reply; 39+ messages in thread
From: Paul Moore @ 2021-02-19 23:29 UTC (permalink / raw)
  To: Casey Schaufler, John Johansen
  Cc: selinux, linux-security-module, linux-audit

With the split of the security_task_getsecid() into subjective and
objective variants it's time to update AppArmor to ensure it is
using the correct task creds.

Signed-off-by: Paul Moore <paul@paul-moore.com>
---
 security/apparmor/domain.c       |    2 +-
 security/apparmor/include/cred.h |   19 ++++++++++++++++---
 security/apparmor/include/task.h |    3 ++-
 security/apparmor/lsm.c          |   23 +++++++++++++++--------
 security/apparmor/task.c         |   23 ++++++++++++++++++++---
 5 files changed, 54 insertions(+), 16 deletions(-)

diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index f919ebd042fd2..9ed00b8dcdf0c 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -67,7 +67,7 @@ static int may_change_ptraced_domain(struct aa_label *to_label,
 	tracer = ptrace_parent(current);
 	if (tracer)
 		/* released below */
-		tracerl = aa_get_task_label(tracer);
+		tracerl = aa_get_task_label_subj(tracer);
 
 	/* not ptraced */
 	if (!tracer || unconfined(tracerl))
diff --git a/security/apparmor/include/cred.h b/security/apparmor/include/cred.h
index 0b9ae4804ef73..43c21ef5568ab 100644
--- a/security/apparmor/include/cred.h
+++ b/security/apparmor/include/cred.h
@@ -64,14 +64,27 @@ static inline struct aa_label *aa_get_newest_cred_label(const struct cred *cred)
 }
 
 /**
- * __aa_task_raw_label - retrieve another task's label
+ * __aa_task_raw_label_subj - retrieve another task's subjective label
  * @task: task to query  (NOT NULL)
  *
- * Returns: @task's label without incrementing its ref count
+ * Returns: @task's subjective label without incrementing its ref count
  *
  * If @task != current needs to be called in RCU safe critical section
  */
-static inline struct aa_label *__aa_task_raw_label(struct task_struct *task)
+static inline struct aa_label *__aa_task_raw_label_subj(struct task_struct *task)
+{
+	return aa_cred_raw_label(rcu_dereference(task->cred));
+}
+
+/**
+ * __aa_task_raw_label_obj - retrieve another task's objective label
+ * @task: task to query  (NOT NULL)
+ *
+ * Returns: @task's objective label without incrementing its ref count
+ *
+ * If @task != current needs to be called in RCU safe critical section
+ */
+static inline struct aa_label *__aa_task_raw_label_obj(struct task_struct *task)
 {
 	return aa_cred_raw_label(__task_cred(task));
 }
diff --git a/security/apparmor/include/task.h b/security/apparmor/include/task.h
index f13d12373b25e..27a2961558555 100644
--- a/security/apparmor/include/task.h
+++ b/security/apparmor/include/task.h
@@ -33,7 +33,8 @@ int aa_replace_current_label(struct aa_label *label);
 int aa_set_current_onexec(struct aa_label *label, bool stack);
 int aa_set_current_hat(struct aa_label *label, u64 token);
 int aa_restore_previous_label(u64 cookie);
-struct aa_label *aa_get_task_label(struct task_struct *task);
+struct aa_label *aa_get_task_label_subj(struct task_struct *task);
+struct aa_label *aa_get_task_label_obj(struct task_struct *task);
 
 /**
  * aa_free_task_ctx - free a task_ctx
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 15e37b9132679..38430851675b9 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -119,7 +119,7 @@ static int apparmor_ptrace_access_check(struct task_struct *child,
 	int error;
 
 	tracer = __begin_current_label_crit_section();
-	tracee = aa_get_task_label(child);
+	tracee = aa_get_task_label_obj(child);
 	error = aa_may_ptrace(tracer, tracee,
 			(mode & PTRACE_MODE_READ) ? AA_PTRACE_READ
 						  : AA_PTRACE_TRACE);
@@ -135,7 +135,7 @@ static int apparmor_ptrace_traceme(struct task_struct *parent)
 	int error;
 
 	tracee = __begin_current_label_crit_section();
-	tracer = aa_get_task_label(parent);
+	tracer = aa_get_task_label_subj(parent);
 	error = aa_may_ptrace(tracer, tracee, AA_PTRACE_TRACE);
 	aa_put_label(tracer);
 	__end_current_label_crit_section(tracee);
@@ -719,9 +719,16 @@ static void apparmor_bprm_committed_creds(struct linux_binprm *bprm)
 	return;
 }
 
-static void apparmor_task_getsecid(struct task_struct *p, u32 *secid)
+static void apparmor_task_getsecid_subj(struct task_struct *p, u32 *secid)
 {
-	struct aa_label *label = aa_get_task_label(p);
+	struct aa_label *label = aa_get_task_label_subj(p);
+	*secid = label->secid;
+	aa_put_label(label);
+}
+
+static void apparmor_task_getsecid_obj(struct task_struct *p, u32 *secid)
+{
+	struct aa_label *label = aa_get_task_label_obj(p);
 	*secid = label->secid;
 	aa_put_label(label);
 }
@@ -750,7 +757,7 @@ static int apparmor_task_kill(struct task_struct *target, struct kernel_siginfo
 		 * Dealing with USB IO specific behavior
 		 */
 		cl = aa_get_newest_cred_label(cred);
-		tl = aa_get_task_label(target);
+		tl = aa_get_task_label_obj(target);
 		error = aa_may_signal(cl, tl, sig);
 		aa_put_label(cl);
 		aa_put_label(tl);
@@ -758,7 +765,7 @@ static int apparmor_task_kill(struct task_struct *target, struct kernel_siginfo
 	}
 
 	cl = __begin_current_label_crit_section();
-	tl = aa_get_task_label(target);
+	tl = aa_get_task_label_obj(target);
 	error = aa_may_signal(cl, tl, sig);
 	aa_put_label(tl);
 	__end_current_label_crit_section(cl);
@@ -1243,8 +1250,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_subj, apparmor_task_getsecid),
-	LSM_HOOK_INIT(task_getsecid_obj, apparmor_task_getsecid),
+	LSM_HOOK_INIT(task_getsecid_subj, apparmor_task_getsecid_subj),
+	LSM_HOOK_INIT(task_getsecid_obj, apparmor_task_getsecid_obj),
 	LSM_HOOK_INIT(task_setrlimit, apparmor_task_setrlimit),
 	LSM_HOOK_INIT(task_kill, apparmor_task_kill),
 
diff --git a/security/apparmor/task.c b/security/apparmor/task.c
index d17130ee6795d..c03c8e3928055 100644
--- a/security/apparmor/task.c
+++ b/security/apparmor/task.c
@@ -16,17 +16,34 @@
 #include "include/task.h"
 
 /**
- * aa_get_task_label - Get another task's label
+ * aa_get_task_label_subj - Get another task's subjective label
  * @task: task to query  (NOT NULL)
  *
  * Returns: counted reference to @task's label
  */
-struct aa_label *aa_get_task_label(struct task_struct *task)
+struct aa_label *aa_get_task_label_subj(struct task_struct *task)
 {
 	struct aa_label *p;
 
 	rcu_read_lock();
-	p = aa_get_newest_label(__aa_task_raw_label(task));
+	p = aa_get_newest_label(__aa_task_raw_label_subj(task));
+	rcu_read_unlock();
+
+	return p;
+}
+
+/**
+ * aa_get_task_label_obj - Get another task's objective label
+ * @task: task to query  (NOT NULL)
+ *
+ * Returns: counted reference to @task's label
+ */
+struct aa_label *aa_get_task_label_obj(struct task_struct *task)
+{
+	struct aa_label *p;
+
+	rcu_read_lock();
+	p = aa_get_newest_label(__aa_task_raw_label_obj(task));
 	rcu_read_unlock();
 
 	return p;

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 0/4] Split security_task_getsecid() into subj and obj variants
  2021-02-19 23:28 [RFC PATCH 0/4] Split security_task_getsecid() into subj and obj variants Paul Moore
                   ` (3 preceding siblings ...)
  2021-02-19 23:29 ` [RFC PATCH 4/4] apparmor: " Paul Moore
@ 2021-02-20  1:49 ` Casey Schaufler
  2021-02-20 14:41   ` Paul Moore
  4 siblings, 1 reply; 39+ messages in thread
From: Casey Schaufler @ 2021-02-20  1:49 UTC (permalink / raw)
  To: Paul Moore, John Johansen; +Cc: selinux, linux-security-module, linux-audit

On 2/19/2021 3:28 PM, Paul Moore wrote:
> As discussed briefly on the list (lore link below), we are a little
> sloppy when it comes to using task credentials, mixing both the
> subjective and object credentials.  This patch set attempts to fix
> this by replacing security_task_getsecid() with two new hooks that
> return either the subjective (_subj) or objective (_obj) credentials.
>
> https://lore.kernel.org/linux-security-module/806848326.0ifERbkFSE@x2/T/
>
> Casey and John, I made a quick pass through the Smack and AppArmor
> code in an effort to try and do the right thing, but I will admit
> that I haven't tested those changes, just the SELinux code.  I
> would really appreciate your help in reviewing those changes.  If
> you find it easier, feel free to wholesale replace my Smack/AppArmor
> patch with one of your own.

A quick test pass didn't show up anything obviously
amiss with the Smack changes. I have will do some more
through inspection, but they look fine so far. 

>
> ---
>
> Paul Moore (4):
>       lsm: separate security_task_getsecid() into subjective and objective variants
>       selinux: clarify task subjective and objective credentials
>       smack: differentiate between subjective and objective task credentials
>       apparmor: differentiate between subjective and objective task credentials
>
>
>  security/apparmor/domain.c       |  2 +-
>  security/apparmor/include/cred.h | 19 +++++--
>  security/apparmor/include/task.h |  3 +-
>  security/apparmor/lsm.c          | 23 ++++++---
>  security/apparmor/task.c         | 23 +++++++--
>  security/selinux/hooks.c         | 85 ++++++++++++++++++--------------
>  security/smack/smack.h           | 18 ++++++-
>  security/smack/smack_lsm.c       | 40 ++++++++++-----
>  8 files changed, 147 insertions(+), 66 deletions(-)
>
> --
> Signature

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants
  2021-02-19 23:29 ` [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants Paul Moore
@ 2021-02-20  2:55   ` James Morris
  2021-02-20 14:44     ` Paul Moore
  2021-02-21 12:51   ` John Johansen
                     ` (3 subsequent siblings)
  4 siblings, 1 reply; 39+ messages in thread
From: James Morris @ 2021-02-20  2:55 UTC (permalink / raw)
  To: Paul Moore; +Cc: selinux, linux-security-module, John Johansen, linux-audit

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?

-- 
James Morris
<jmorris@namei.org>

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 0/4] Split security_task_getsecid() into subj and obj variants
  2021-02-20  1:49 ` [RFC PATCH 0/4] Split security_task_getsecid() into subj and obj variants Casey Schaufler
@ 2021-02-20 14:41   ` Paul Moore
  2021-02-22 23:58     ` Casey Schaufler
  0 siblings, 1 reply; 39+ messages in thread
From: Paul Moore @ 2021-02-20 14:41 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: selinux, linux-security-module, John Johansen, linux-audit

On Fri, Feb 19, 2021 at 8:49 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 2/19/2021 3:28 PM, Paul Moore wrote:
> > As discussed briefly on the list (lore link below), we are a little
> > sloppy when it comes to using task credentials, mixing both the
> > subjective and object credentials.  This patch set attempts to fix
> > this by replacing security_task_getsecid() with two new hooks that
> > return either the subjective (_subj) or objective (_obj) credentials.
> >
> > https://lore.kernel.org/linux-security-module/806848326.0ifERbkFSE@x2/T/
> >
> > Casey and John, I made a quick pass through the Smack and AppArmor
> > code in an effort to try and do the right thing, but I will admit
> > that I haven't tested those changes, just the SELinux code.  I
> > would really appreciate your help in reviewing those changes.  If
> > you find it easier, feel free to wholesale replace my Smack/AppArmor
> > patch with one of your own.
>
> A quick test pass didn't show up anything obviously
> amiss with the Smack changes. I have will do some more
> through inspection, but they look fine so far.

Thanks for testing it out and giving it a look.  Beyond the Smack
specific changes, I'm also interested in making sure all the hook
callers are correct; I believe I made the correct substitutions, but a
second (or third (or fourth ...)) set of eyes is never a bad idea.

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants
  2021-02-20  2:55   ` James Morris
@ 2021-02-20 14:44     ` Paul Moore
  2021-03-04 10:04       ` Jeffrey Vander Stoep
  0 siblings, 1 reply; 39+ messages in thread
From: Paul Moore @ 2021-02-20 14:44 UTC (permalink / raw)
  To: linux-security-module, selinux; +Cc: John Johansen, James Morris, linux-audit

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.

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants
  2021-02-19 23:29 ` [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants Paul Moore
  2021-02-20  2:55   ` James Morris
@ 2021-02-21 12:51   ` John Johansen
  2021-02-21 22:09     ` Paul Moore
  2021-03-04  0:44     ` Paul Moore
  2021-02-24 16:49   ` Mimi Zohar
                     ` (2 subsequent siblings)
  4 siblings, 2 replies; 39+ messages in thread
From: John Johansen @ 2021-02-21 12:51 UTC (permalink / raw)
  To: Paul Moore, Casey Schaufler; +Cc: selinux, linux-security-module, linux-audit

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),
> 

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 2/4] selinux: clarify task subjective and objective credentials
  2021-02-19 23:29 ` [RFC PATCH 2/4] selinux: clarify task subjective and objective credentials Paul Moore
@ 2021-02-21 12:55   ` John Johansen
  2021-03-08 19:26   ` Richard Guy Briggs
  2021-03-10  3:05   ` John Johansen
  2 siblings, 0 replies; 39+ messages in thread
From: John Johansen @ 2021-02-21 12:55 UTC (permalink / raw)
  To: Paul Moore, Casey Schaufler; +Cc: selinux, linux-security-module, linux-audit

On 2/19/21 3:29 PM, Paul Moore wrote:
> SELinux has a function, task_sid(), which returns the task's
> objective credentials, but unfortunately is used in a few places
> where the subjective task credentials should be used.  Most notably
> in the new security_task_getsecid_subj() LSM hook.
> 
> This patch fixes this and attempts to make things more obvious by
> introducing a new function, task_sid_subj(), and renaming the
> existing task_sid() function to task_sid_obj().
> 
> Signed-off-by: Paul Moore <paul@paul-moore.com>

This looks good, though I have some questions around whether _subj use is correct in selinux_binder_transaction(). So off to learn more binder 



> ---
>  security/selinux/hooks.c |   85 +++++++++++++++++++++++++++-------------------
>  1 file changed, 49 insertions(+), 36 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f311541c4972e..1c53000d28e37 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -229,10 +229,23 @@ static inline u32 cred_sid(const struct cred *cred)
>  	return tsec->sid;
>  }
>  
> +/*
> + * get the subjective security ID of a task
> + */
> +static inline u32 task_sid_subj(const struct task_struct *task)
> +{
> +	u32 sid;
> +
> +	rcu_read_lock();
> +	sid = cred_sid(rcu_dereference(task->cred));
> +	rcu_read_unlock();
> +	return sid;
> +}
> +
>  /*
>   * get the objective security ID of a task
>   */
> -static inline u32 task_sid(const struct task_struct *task)
> +static inline u32 task_sid_obj(const struct task_struct *task)
>  {
>  	u32 sid;
>  
> @@ -2034,11 +2047,8 @@ static inline u32 open_file_to_av(struct file *file)
>  
>  static int selinux_binder_set_context_mgr(struct task_struct *mgr)
>  {
> -	u32 mysid = current_sid();
> -	u32 mgrsid = task_sid(mgr);
> -
>  	return avc_has_perm(&selinux_state,
> -			    mysid, mgrsid, SECCLASS_BINDER,
> +			    current_sid(), task_sid_obj(mgr), SECCLASS_BINDER,
>  			    BINDER__SET_CONTEXT_MGR, NULL);
>  }
>  
> @@ -2046,8 +2056,8 @@ static int selinux_binder_transaction(struct task_struct *from,
>  				      struct task_struct *to)
>  {
>  	u32 mysid = current_sid();
> -	u32 fromsid = task_sid(from);
> -	u32 tosid = task_sid(to);
> +	u32 fromsid = task_sid_subj(from);
> +	u32 tosid = task_sid_subj(to);
>  	int rc;
>  
>  	if (mysid != fromsid) {
> @@ -2066,11 +2076,9 @@ static int selinux_binder_transaction(struct task_struct *from,
>  static int selinux_binder_transfer_binder(struct task_struct *from,
>  					  struct task_struct *to)
>  {
> -	u32 fromsid = task_sid(from);
> -	u32 tosid = task_sid(to);
> -
>  	return avc_has_perm(&selinux_state,
> -			    fromsid, tosid, SECCLASS_BINDER, BINDER__TRANSFER,
> +			    task_sid_subj(from), task_sid_obj(to),
> +			    SECCLASS_BINDER, BINDER__TRANSFER,
>  			    NULL);
>  }
>  
> @@ -2078,7 +2086,7 @@ static int selinux_binder_transfer_file(struct task_struct *from,
>  					struct task_struct *to,
>  					struct file *file)
>  {
> -	u32 sid = task_sid(to);
> +	u32 sid = task_sid_subj(to);
>  	struct file_security_struct *fsec = selinux_file(file);
>  	struct dentry *dentry = file->f_path.dentry;
>  	struct inode_security_struct *isec;
> @@ -2114,10 +2122,10 @@ static int selinux_binder_transfer_file(struct task_struct *from,
>  }
>  
>  static int selinux_ptrace_access_check(struct task_struct *child,
> -				     unsigned int mode)
> +				       unsigned int mode)
>  {
>  	u32 sid = current_sid();
> -	u32 csid = task_sid(child);
> +	u32 csid = task_sid_obj(child);
>  
>  	if (mode & PTRACE_MODE_READ)
>  		return avc_has_perm(&selinux_state,
> @@ -2130,15 +2138,15 @@ static int selinux_ptrace_access_check(struct task_struct *child,
>  static int selinux_ptrace_traceme(struct task_struct *parent)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    task_sid(parent), current_sid(), SECCLASS_PROCESS,
> -			    PROCESS__PTRACE, NULL);
> +			    task_sid_subj(parent), task_sid_obj(current),
> +			    SECCLASS_PROCESS, PROCESS__PTRACE, NULL);
>  }
>  
>  static int selinux_capget(struct task_struct *target, kernel_cap_t *effective,
>  			  kernel_cap_t *inheritable, kernel_cap_t *permitted)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(target), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(target), SECCLASS_PROCESS,
>  			    PROCESS__GETCAP, NULL);
>  }
>  
> @@ -2263,7 +2271,7 @@ static u32 ptrace_parent_sid(void)
>  	rcu_read_lock();
>  	tracer = ptrace_parent(current);
>  	if (tracer)
> -		sid = task_sid(tracer);
> +		sid = task_sid_obj(tracer);
>  	rcu_read_unlock();
>  
>  	return sid;
> @@ -3916,7 +3924,7 @@ static int selinux_file_send_sigiotask(struct task_struct *tsk,
>  				       struct fown_struct *fown, int signum)
>  {
>  	struct file *file;
> -	u32 sid = task_sid(tsk);
> +	u32 sid = task_sid_obj(tsk);
>  	u32 perm;
>  	struct file_security_struct *fsec;
>  
> @@ -4135,47 +4143,52 @@ static int selinux_kernel_load_data(enum kernel_load_data_id id, bool contents)
>  static int selinux_task_setpgid(struct task_struct *p, pid_t pgid)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__SETPGID, NULL);
>  }
>  
>  static int selinux_task_getpgid(struct task_struct *p)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__GETPGID, NULL);
>  }
>  
>  static int selinux_task_getsid(struct task_struct *p)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__GETSESSION, NULL);
>  }
>  
> -static void selinux_task_getsecid(struct task_struct *p, u32 *secid)
> +static void selinux_task_getsecid_subj(struct task_struct *p, u32 *secid)
> +{
> +	*secid = task_sid_subj(p);
> +}
> +
> +static void selinux_task_getsecid_obj(struct task_struct *p, u32 *secid)
>  {
> -	*secid = task_sid(p);
> +	*secid = task_sid_obj(p);
>  }
>  
>  static int selinux_task_setnice(struct task_struct *p, int nice)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__SETSCHED, NULL);
>  }
>  
>  static int selinux_task_setioprio(struct task_struct *p, int ioprio)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__SETSCHED, NULL);
>  }
>  
>  static int selinux_task_getioprio(struct task_struct *p)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__GETSCHED, NULL);
>  }
>  
> @@ -4206,7 +4219,7 @@ static int selinux_task_setrlimit(struct task_struct *p, unsigned int resource,
>  	   upon context transitions.  See selinux_bprm_committing_creds. */
>  	if (old_rlim->rlim_max != new_rlim->rlim_max)
>  		return avc_has_perm(&selinux_state,
> -				    current_sid(), task_sid(p),
> +				    current_sid(), task_sid_obj(p),
>  				    SECCLASS_PROCESS, PROCESS__SETRLIMIT, NULL);
>  
>  	return 0;
> @@ -4215,21 +4228,21 @@ static int selinux_task_setrlimit(struct task_struct *p, unsigned int resource,
>  static int selinux_task_setscheduler(struct task_struct *p)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__SETSCHED, NULL);
>  }
>  
>  static int selinux_task_getscheduler(struct task_struct *p)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__GETSCHED, NULL);
>  }
>  
>  static int selinux_task_movememory(struct task_struct *p)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__SETSCHED, NULL);
>  }
>  
> @@ -4248,14 +4261,14 @@ static int selinux_task_kill(struct task_struct *p, struct kernel_siginfo *info,
>  	else
>  		secid = cred_sid(cred);
>  	return avc_has_perm(&selinux_state,
> -			    secid, task_sid(p), SECCLASS_PROCESS, perm, NULL);
> +			    secid, task_sid_obj(p), SECCLASS_PROCESS, perm, NULL);
>  }
>  
>  static void selinux_task_to_inode(struct task_struct *p,
>  				  struct inode *inode)
>  {
>  	struct inode_security_struct *isec = selinux_inode(inode);
> -	u32 sid = task_sid(p);
> +	u32 sid = task_sid_obj(p);
>  
>  	spin_lock(&isec->lock);
>  	isec->sclass = inode_mode_to_security_class(inode->i_mode);
> @@ -6148,7 +6161,7 @@ static int selinux_msg_queue_msgrcv(struct kern_ipc_perm *msq, struct msg_msg *m
>  	struct ipc_security_struct *isec;
>  	struct msg_security_struct *msec;
>  	struct common_audit_data ad;
> -	u32 sid = task_sid(target);
> +	u32 sid = task_sid_subj(target);
>  	int rc;
>  
>  	isec = selinux_ipc(msq);
> @@ -7143,8 +7156,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_subj, selinux_task_getsecid),
> -	LSM_HOOK_INIT(task_getsecid_obj, selinux_task_getsecid),
> +	LSM_HOOK_INIT(task_getsecid_subj, selinux_task_getsecid_subj),
> +	LSM_HOOK_INIT(task_getsecid_obj, selinux_task_getsecid_obj),
>  	LSM_HOOK_INIT(task_setnice, selinux_task_setnice),
>  	LSM_HOOK_INIT(task_setioprio, selinux_task_setioprio),
>  	LSM_HOOK_INIT(task_getioprio, selinux_task_getioprio),
> 

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 3/4] smack: differentiate between subjective and objective task credentials
  2021-02-19 23:29 ` [RFC PATCH 3/4] smack: differentiate between subjective and objective task credentials Paul Moore
@ 2021-02-21 12:56   ` John Johansen
  2021-03-08 19:26   ` Richard Guy Briggs
  2021-03-10  1:04   ` John Johansen
  2 siblings, 0 replies; 39+ messages in thread
From: John Johansen @ 2021-02-21 12:56 UTC (permalink / raw)
  To: Paul Moore, Casey Schaufler; +Cc: selinux, linux-security-module, linux-audit

On 2/19/21 3:29 PM, Paul Moore wrote:
> With the split of the security_task_getsecid() into subjective and
> objective variants it's time to update Smack to ensure it is using
> the correct task creds.
> 
> Signed-off-by: Paul Moore <paul@paul-moore.com>

first pass looks good to me

> ---
>  security/smack/smack.h     |   18 +++++++++++++++++-
>  security/smack/smack_lsm.c |   40 +++++++++++++++++++++++++++-------------
>  2 files changed, 44 insertions(+), 14 deletions(-)
> 
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index a9768b12716bf..08f9cb80655ce 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -383,7 +383,23 @@ static inline struct smack_known *smk_of_task(const struct task_smack *tsp)
>  	return tsp->smk_task;
>  }
>  
> -static inline struct smack_known *smk_of_task_struct(
> +static inline struct smack_known *smk_of_task_struct_subj(
> +						const struct task_struct *t)
> +{
> +	struct smack_known *skp;
> +	const struct cred *cred;
> +
> +	rcu_read_lock();
> +
> +	cred = rcu_dereference(t->cred);
> +	skp = smk_of_task(smack_cred(cred));
> +
> +	rcu_read_unlock();
> +
> +	return skp;
> +}
> +
> +static inline struct smack_known *smk_of_task_struct_obj(
>  						const struct task_struct *t)
>  {
>  	struct smack_known *skp;
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 2bb354ef2c4a9..ea1a82742e8ba 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -159,7 +159,7 @@ static int smk_bu_current(char *note, struct smack_known *oskp,
>  static int smk_bu_task(struct task_struct *otp, int mode, int rc)
>  {
>  	struct task_smack *tsp = smack_cred(current_cred());
> -	struct smack_known *smk_task = smk_of_task_struct(otp);
> +	struct smack_known *smk_task = smk_of_task_struct_obj(otp);
>  	char acc[SMK_NUM_ACCESS_TYPE + 1];
>  
>  	if (rc <= 0)
> @@ -479,7 +479,7 @@ static int smack_ptrace_access_check(struct task_struct *ctp, unsigned int mode)
>  {
>  	struct smack_known *skp;
>  
> -	skp = smk_of_task_struct(ctp);
> +	skp = smk_of_task_struct_obj(ctp);
>  
>  	return smk_ptrace_rule_check(current, skp, mode, __func__);
>  }
> @@ -2031,7 +2031,7 @@ static int smk_curacc_on_task(struct task_struct *p, int access,
>  				const char *caller)
>  {
>  	struct smk_audit_info ad;
> -	struct smack_known *skp = smk_of_task_struct(p);
> +	struct smack_known *skp = smk_of_task_struct_subj(p);
>  	int rc;
>  
>  	smk_ad_init(&ad, caller, LSM_AUDIT_DATA_TASK);
> @@ -2076,15 +2076,29 @@ static int smack_task_getsid(struct task_struct *p)
>  }
>  
>  /**
> - * smack_task_getsecid - get the secid of the task
> - * @p: the object task
> + * smack_task_getsecid_subj - get the subjective secid of the task
> + * @p: the task
>   * @secid: where to put the result
>   *
> - * Sets the secid to contain a u32 version of the smack label.
> + * Sets the secid to contain a u32 version of the task's subjective smack label.
> + */
> +static void smack_task_getsecid_subj(struct task_struct *p, u32 *secid)
> +{
> +	struct smack_known *skp = smk_of_task_struct_subj(p);
> +
> +	*secid = skp->smk_secid;
> +}
> +
> +/**
> + * smack_task_getsecid_obj - get the objective secid of the task
> + * @p: the task
> + * @secid: where to put the result
> + *
> + * Sets the secid to contain a u32 version of the task's objective smack label.
>   */
> -static void smack_task_getsecid(struct task_struct *p, u32 *secid)
> +static void smack_task_getsecid_obj(struct task_struct *p, u32 *secid)
>  {
> -	struct smack_known *skp = smk_of_task_struct(p);
> +	struct smack_known *skp = smk_of_task_struct_obj(p);
>  
>  	*secid = skp->smk_secid;
>  }
> @@ -2172,7 +2186,7 @@ static int smack_task_kill(struct task_struct *p, struct kernel_siginfo *info,
>  {
>  	struct smk_audit_info ad;
>  	struct smack_known *skp;
> -	struct smack_known *tkp = smk_of_task_struct(p);
> +	struct smack_known *tkp = smk_of_task_struct_obj(p);
>  	int rc;
>  
>  	if (!sig)
> @@ -2210,7 +2224,7 @@ static int smack_task_kill(struct task_struct *p, struct kernel_siginfo *info,
>  static void smack_task_to_inode(struct task_struct *p, struct inode *inode)
>  {
>  	struct inode_smack *isp = smack_inode(inode);
> -	struct smack_known *skp = smk_of_task_struct(p);
> +	struct smack_known *skp = smk_of_task_struct_obj(p);
>  
>  	isp->smk_inode = skp;
>  	isp->smk_flags |= SMK_INODE_INSTANT;
> @@ -3481,7 +3495,7 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
>   */
>  static int smack_getprocattr(struct task_struct *p, char *name, char **value)
>  {
> -	struct smack_known *skp = smk_of_task_struct(p);
> +	struct smack_known *skp = smk_of_task_struct_subj(p);
>  	char *cp;
>  	int slen;
>  
> @@ -4755,8 +4769,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_subj, smack_task_getsecid),
> -	LSM_HOOK_INIT(task_getsecid_obj, smack_task_getsecid),
> +	LSM_HOOK_INIT(task_getsecid_subj, smack_task_getsecid_subj),
> +	LSM_HOOK_INIT(task_getsecid_obj, smack_task_getsecid_obj),
>  	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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 4/4] apparmor: differentiate between subjective and objective task credentials
  2021-02-19 23:29 ` [RFC PATCH 4/4] apparmor: " Paul Moore
@ 2021-02-21 12:57   ` John Johansen
  2021-02-21 22:12     ` Paul Moore
  0 siblings, 1 reply; 39+ messages in thread
From: John Johansen @ 2021-02-21 12:57 UTC (permalink / raw)
  To: Paul Moore, Casey Schaufler; +Cc: selinux, linux-security-module, linux-audit

On 2/19/21 3:29 PM, Paul Moore wrote:
> With the split of the security_task_getsecid() into subjective and
> objective variants it's time to update AppArmor to ensure it is
> using the correct task creds.
> 
> Signed-off-by: Paul Moore <paul@paul-moore.com>

This has a couple problems, that I will work on addressing


> ---
>  security/apparmor/domain.c       |    2 +-
>  security/apparmor/include/cred.h |   19 ++++++++++++++++---
>  security/apparmor/include/task.h |    3 ++-
>  security/apparmor/lsm.c          |   23 +++++++++++++++--------
>  security/apparmor/task.c         |   23 ++++++++++++++++++++---
>  5 files changed, 54 insertions(+), 16 deletions(-)
> 
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index f919ebd042fd2..9ed00b8dcdf0c 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -67,7 +67,7 @@ static int may_change_ptraced_domain(struct aa_label *to_label,
>  	tracer = ptrace_parent(current);
>  	if (tracer)
>  		/* released below */
> -		tracerl = aa_get_task_label(tracer);
> +		tracerl = aa_get_task_label_subj(tracer);
>  
>  	/* not ptraced */
>  	if (!tracer || unconfined(tracerl))
> diff --git a/security/apparmor/include/cred.h b/security/apparmor/include/cred.h
> index 0b9ae4804ef73..43c21ef5568ab 100644
> --- a/security/apparmor/include/cred.h
> +++ b/security/apparmor/include/cred.h
> @@ -64,14 +64,27 @@ static inline struct aa_label *aa_get_newest_cred_label(const struct cred *cred)
>  }
>  
>  /**
> - * __aa_task_raw_label - retrieve another task's label
> + * __aa_task_raw_label_subj - retrieve another task's subjective label
>   * @task: task to query  (NOT NULL)
>   *
> - * Returns: @task's label without incrementing its ref count
> + * Returns: @task's subjective label without incrementing its ref count
>   *
>   * If @task != current needs to be called in RCU safe critical section
>   */
> -static inline struct aa_label *__aa_task_raw_label(struct task_struct *task)
> +static inline struct aa_label *__aa_task_raw_label_subj(struct task_struct *task)
> +{
> +	return aa_cred_raw_label(rcu_dereference(task->cred));
> +}
> +
> +/**
> + * __aa_task_raw_label_obj - retrieve another task's objective label
> + * @task: task to query  (NOT NULL)
> + *
> + * Returns: @task's objective label without incrementing its ref count
> + *
> + * If @task != current needs to be called in RCU safe critical section
> + */
> +static inline struct aa_label *__aa_task_raw_label_obj(struct task_struct *task)
>  {
>  	return aa_cred_raw_label(__task_cred(task));
>  }
> diff --git a/security/apparmor/include/task.h b/security/apparmor/include/task.h
> index f13d12373b25e..27a2961558555 100644
> --- a/security/apparmor/include/task.h
> +++ b/security/apparmor/include/task.h
> @@ -33,7 +33,8 @@ int aa_replace_current_label(struct aa_label *label);
>  int aa_set_current_onexec(struct aa_label *label, bool stack);
>  int aa_set_current_hat(struct aa_label *label, u64 token);
>  int aa_restore_previous_label(u64 cookie);
> -struct aa_label *aa_get_task_label(struct task_struct *task);
> +struct aa_label *aa_get_task_label_subj(struct task_struct *task);
> +struct aa_label *aa_get_task_label_obj(struct task_struct *task);
>  
>  /**
>   * aa_free_task_ctx - free a task_ctx
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 15e37b9132679..38430851675b9 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -119,7 +119,7 @@ static int apparmor_ptrace_access_check(struct task_struct *child,
>  	int error;
>  
>  	tracer = __begin_current_label_crit_section();
> -	tracee = aa_get_task_label(child);
> +	tracee = aa_get_task_label_obj(child);
>  	error = aa_may_ptrace(tracer, tracee,
>  			(mode & PTRACE_MODE_READ) ? AA_PTRACE_READ
>  						  : AA_PTRACE_TRACE);
> @@ -135,7 +135,7 @@ static int apparmor_ptrace_traceme(struct task_struct *parent)
>  	int error;
>  
>  	tracee = __begin_current_label_crit_section();
> -	tracer = aa_get_task_label(parent);
> +	tracer = aa_get_task_label_subj(parent);
>  	error = aa_may_ptrace(tracer, tracee, AA_PTRACE_TRACE);
>  	aa_put_label(tracer);
>  	__end_current_label_crit_section(tracee);
> @@ -719,9 +719,16 @@ static void apparmor_bprm_committed_creds(struct linux_binprm *bprm)
>  	return;
>  }
>  
> -static void apparmor_task_getsecid(struct task_struct *p, u32 *secid)
> +static void apparmor_task_getsecid_subj(struct task_struct *p, u32 *secid)
>  {
> -	struct aa_label *label = aa_get_task_label(p);
> +	struct aa_label *label = aa_get_task_label_subj(p);
> +	*secid = label->secid;
> +	aa_put_label(label);
> +}
> +
> +static void apparmor_task_getsecid_obj(struct task_struct *p, u32 *secid)
> +{
> +	struct aa_label *label = aa_get_task_label_obj(p);
>  	*secid = label->secid;
>  	aa_put_label(label);
>  }
> @@ -750,7 +757,7 @@ static int apparmor_task_kill(struct task_struct *target, struct kernel_siginfo
>  		 * Dealing with USB IO specific behavior
>  		 */
>  		cl = aa_get_newest_cred_label(cred);
> -		tl = aa_get_task_label(target);
> +		tl = aa_get_task_label_obj(target);
>  		error = aa_may_signal(cl, tl, sig);
>  		aa_put_label(cl);
>  		aa_put_label(tl);
> @@ -758,7 +765,7 @@ static int apparmor_task_kill(struct task_struct *target, struct kernel_siginfo
>  	}
>  
>  	cl = __begin_current_label_crit_section();
> -	tl = aa_get_task_label(target);
> +	tl = aa_get_task_label_obj(target);
>  	error = aa_may_signal(cl, tl, sig);
>  	aa_put_label(tl);
>  	__end_current_label_crit_section(cl);
> @@ -1243,8 +1250,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_subj, apparmor_task_getsecid),
> -	LSM_HOOK_INIT(task_getsecid_obj, apparmor_task_getsecid),
> +	LSM_HOOK_INIT(task_getsecid_subj, apparmor_task_getsecid_subj),
> +	LSM_HOOK_INIT(task_getsecid_obj, apparmor_task_getsecid_obj),
>  	LSM_HOOK_INIT(task_setrlimit, apparmor_task_setrlimit),
>  	LSM_HOOK_INIT(task_kill, apparmor_task_kill),
>  
> diff --git a/security/apparmor/task.c b/security/apparmor/task.c
> index d17130ee6795d..c03c8e3928055 100644
> --- a/security/apparmor/task.c
> +++ b/security/apparmor/task.c
> @@ -16,17 +16,34 @@
>  #include "include/task.h"
>  
>  /**
> - * aa_get_task_label - Get another task's label
> + * aa_get_task_label_subj - Get another task's subjective label
>   * @task: task to query  (NOT NULL)
>   *
>   * Returns: counted reference to @task's label
>   */
> -struct aa_label *aa_get_task_label(struct task_struct *task)
> +struct aa_label *aa_get_task_label_subj(struct task_struct *task)
>  {
>  	struct aa_label *p;
>  
>  	rcu_read_lock();
> -	p = aa_get_newest_label(__aa_task_raw_label(task));
> +	p = aa_get_newest_label(__aa_task_raw_label_subj(task));
> +	rcu_read_unlock();
> +
> +	return p;
> +}
> +
> +/**
> + * aa_get_task_label_obj - Get another task's objective label
> + * @task: task to query  (NOT NULL)
> + *
> + * Returns: counted reference to @task's label
> + */
> +struct aa_label *aa_get_task_label_obj(struct task_struct *task)
> +{
> +	struct aa_label *p;
> +
> +	rcu_read_lock();
> +	p = aa_get_newest_label(__aa_task_raw_label_obj(task));
>  	rcu_read_unlock();
>  
>  	return p;
> 

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants
  2021-02-21 12:51   ` John Johansen
@ 2021-02-21 22:09     ` Paul Moore
  2021-03-04  0:44     ` Paul Moore
  1 sibling, 0 replies; 39+ messages in thread
From: Paul Moore @ 2021-02-21 22:09 UTC (permalink / raw)
  To: John Johansen; +Cc: selinux, linux-security-module, linux-audit

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.

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 4/4] apparmor: differentiate between subjective and objective task credentials
  2021-02-21 12:57   ` John Johansen
@ 2021-02-21 22:12     ` Paul Moore
  0 siblings, 0 replies; 39+ messages in thread
From: Paul Moore @ 2021-02-21 22:12 UTC (permalink / raw)
  To: John Johansen; +Cc: selinux, linux-security-module, linux-audit

On Sun, Feb 21, 2021 at 7:57 AM John Johansen
<john.johansen@canonical.com> wrote:
> On 2/19/21 3:29 PM, Paul Moore wrote:
> > With the split of the security_task_getsecid() into subjective and
> > objective variants it's time to update AppArmor to ensure it is
> > using the correct task creds.
> >
> > Signed-off-by: Paul Moore <paul@paul-moore.com>
>
> This has a couple problems, that I will work on addressing

Yes, I figured that might be the case; I don't have enough
understanding of the AppArmor internals to do anything more than a ham
fisted approach - my apologies on that.  Let me know if there is
anything I can do to help.

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 0/4] Split security_task_getsecid() into subj and obj variants
  2021-02-20 14:41   ` Paul Moore
@ 2021-02-22 23:58     ` Casey Schaufler
  2021-02-23 14:14       ` Mimi Zohar
  2021-03-04  0:46       ` Paul Moore
  0 siblings, 2 replies; 39+ messages in thread
From: Casey Schaufler @ 2021-02-22 23:58 UTC (permalink / raw)
  To: Paul Moore
  Cc: John Johansen, selinux, Mimi Zohar, linux-security-module, linux-audit

On 2/20/2021 6:41 AM, Paul Moore wrote:
> On Fri, Feb 19, 2021 at 8:49 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 2/19/2021 3:28 PM, Paul Moore wrote:
>>> As discussed briefly on the list (lore link below), we are a little
>>> sloppy when it comes to using task credentials, mixing both the
>>> subjective and object credentials.  This patch set attempts to fix
>>> this by replacing security_task_getsecid() with two new hooks that
>>> return either the subjective (_subj) or objective (_obj) credentials.
>>>
>>> https://lore.kernel.org/linux-security-module/806848326.0ifERbkFSE@x2/T/
>>>
>>> Casey and John, I made a quick pass through the Smack and AppArmor
>>> code in an effort to try and do the right thing, but I will admit
>>> that I haven't tested those changes, just the SELinux code.  I
>>> would really appreciate your help in reviewing those changes.  If
>>> you find it easier, feel free to wholesale replace my Smack/AppArmor
>>> patch with one of your own.
>> A quick test pass didn't show up anything obviously
>> amiss with the Smack changes. I have will do some more
>> through inspection, but they look fine so far.
> Thanks for testing it out and giving it a look.  Beyond the Smack
> specific changes, I'm also interested in making sure all the hook
> callers are correct; I believe I made the correct substitutions, but a
> second (or third (or fourth ...)) set of eyes is never a bad idea.

I'm still not seeing anything that looks wrong. I'd suggest that Mimi
have a look at the IMA bits.

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 0/4] Split security_task_getsecid() into subj and obj variants
  2021-02-22 23:58     ` Casey Schaufler
@ 2021-02-23 14:14       ` Mimi Zohar
  2021-02-24  0:03         ` Paul Moore
  2021-03-04  0:46       ` Paul Moore
  1 sibling, 1 reply; 39+ messages in thread
From: Mimi Zohar @ 2021-02-23 14:14 UTC (permalink / raw)
  To: Casey Schaufler, Paul Moore
  Cc: selinux, linux-security-module, John Johansen, linux-audit

On Mon, 2021-02-22 at 15:58 -0800, Casey Schaufler wrote:
> On 2/20/2021 6:41 AM, Paul Moore wrote:
> > On Fri, Feb 19, 2021 at 8:49 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 2/19/2021 3:28 PM, Paul Moore wrote:
> >>> As discussed briefly on the list (lore link below), we are a little
> >>> sloppy when it comes to using task credentials, mixing both the
> >>> subjective and object credentials.  This patch set attempts to fix
> >>> this by replacing security_task_getsecid() with two new hooks that
> >>> return either the subjective (_subj) or objective (_obj) credentials.
> >>>
> >>> https://lore.kernel.org/linux-security-module/806848326.0ifERbkFSE@x2/T/
> >>>
> >>> Casey and John, I made a quick pass through the Smack and AppArmor
> >>> code in an effort to try and do the right thing, but I will admit
> >>> that I haven't tested those changes, just the SELinux code.  I
> >>> would really appreciate your help in reviewing those changes.  If
> >>> you find it easier, feel free to wholesale replace my Smack/AppArmor
> >>> patch with one of your own.
> >> A quick test pass didn't show up anything obviously
> >> amiss with the Smack changes. I have will do some more
> >> through inspection, but they look fine so far.
> > Thanks for testing it out and giving it a look.  Beyond the Smack
> > specific changes, I'm also interested in making sure all the hook
> > callers are correct; I believe I made the correct substitutions, but a
> > second (or third (or fourth ...)) set of eyes is never a bad idea.
> 
> I'm still not seeing anything that looks wrong. I'd suggest that Mimi
> have a look at the IMA bits.

Thanks, Casey, Paul.  The IMA changes look fine.  IMA policy rules are
normally written in terms of a file's LSM labels, the obj_type, so
hopefully this change has minimal, if any, impact.

Mimi

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 0/4] Split security_task_getsecid() into subj and obj variants
  2021-02-23 14:14       ` Mimi Zohar
@ 2021-02-24  0:03         ` Paul Moore
  0 siblings, 0 replies; 39+ messages in thread
From: Paul Moore @ 2021-02-24  0:03 UTC (permalink / raw)
  To: Mimi Zohar; +Cc: selinux, linux-security-module, John Johansen, linux-audit

On Tue, Feb 23, 2021 at 9:14 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> On Mon, 2021-02-22 at 15:58 -0800, Casey Schaufler wrote:
> > On 2/20/2021 6:41 AM, Paul Moore wrote:
> > > On Fri, Feb 19, 2021 at 8:49 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > >> On 2/19/2021 3:28 PM, Paul Moore wrote:
> > >>> As discussed briefly on the list (lore link below), we are a little
> > >>> sloppy when it comes to using task credentials, mixing both the
> > >>> subjective and object credentials.  This patch set attempts to fix
> > >>> this by replacing security_task_getsecid() with two new hooks that
> > >>> return either the subjective (_subj) or objective (_obj) credentials.
> > >>>
> > >>> https://lore.kernel.org/linux-security-module/806848326.0ifERbkFSE@x2/T/
> > >>>
> > >>> Casey and John, I made a quick pass through the Smack and AppArmor
> > >>> code in an effort to try and do the right thing, but I will admit
> > >>> that I haven't tested those changes, just the SELinux code.  I
> > >>> would really appreciate your help in reviewing those changes.  If
> > >>> you find it easier, feel free to wholesale replace my Smack/AppArmor
> > >>> patch with one of your own.
> > >> A quick test pass didn't show up anything obviously
> > >> amiss with the Smack changes. I have will do some more
> > >> through inspection, but they look fine so far.
> > > Thanks for testing it out and giving it a look.  Beyond the Smack
> > > specific changes, I'm also interested in making sure all the hook
> > > callers are correct; I believe I made the correct substitutions, but a
> > > second (or third (or fourth ...)) set of eyes is never a bad idea.
> >
> > I'm still not seeing anything that looks wrong. I'd suggest that Mimi
> > have a look at the IMA bits.
>
> Thanks, Casey, Paul.  The IMA changes look fine.  IMA policy rules are
> normally written in terms of a file's LSM labels, the obj_type, so
> hopefully this change has minimal, if any, impact.

Thanks Mimi I appreciate the additional review.  Would you mind
sending your ACK for the IMA related patches in the patchset?

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants
  2021-02-19 23:29 ` [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants Paul Moore
  2021-02-20  2:55   ` James Morris
  2021-02-21 12:51   ` John Johansen
@ 2021-02-24 16:49   ` Mimi Zohar
  2021-03-08 19:25   ` Richard Guy Briggs
  2021-03-10  1:03   ` John Johansen
  4 siblings, 0 replies; 39+ messages in thread
From: Mimi Zohar @ 2021-02-24 16:49 UTC (permalink / raw)
  To: Paul Moore, Casey Schaufler, John Johansen
  Cc: selinux, linux-security-module, linux-audit

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)

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants
  2021-02-21 12:51   ` John Johansen
  2021-02-21 22:09     ` Paul Moore
@ 2021-03-04  0:44     ` Paul Moore
  2021-03-10  0:28       ` Paul Moore
  1 sibling, 1 reply; 39+ messages in thread
From: Paul Moore @ 2021-03-04  0:44 UTC (permalink / raw)
  To: John Johansen; +Cc: selinux, linux-security-module, linux-audit

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?

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 0/4] Split security_task_getsecid() into subj and obj variants
  2021-02-22 23:58     ` Casey Schaufler
  2021-02-23 14:14       ` Mimi Zohar
@ 2021-03-04  0:46       ` Paul Moore
  2021-03-04  2:21         ` Casey Schaufler
  1 sibling, 1 reply; 39+ messages in thread
From: Paul Moore @ 2021-03-04  0:46 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: selinux, linux-security-module, John Johansen, Mimi Zohar, linux-audit

On Mon, Feb 22, 2021 at 6:59 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 2/20/2021 6:41 AM, Paul Moore wrote:
> > On Fri, Feb 19, 2021 at 8:49 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 2/19/2021 3:28 PM, Paul Moore wrote:
> >>> As discussed briefly on the list (lore link below), we are a little
> >>> sloppy when it comes to using task credentials, mixing both the
> >>> subjective and object credentials.  This patch set attempts to fix
> >>> this by replacing security_task_getsecid() with two new hooks that
> >>> return either the subjective (_subj) or objective (_obj) credentials.
> >>>
> >>> https://lore.kernel.org/linux-security-module/806848326.0ifERbkFSE@x2/T/
> >>>
> >>> Casey and John, I made a quick pass through the Smack and AppArmor
> >>> code in an effort to try and do the right thing, but I will admit
> >>> that I haven't tested those changes, just the SELinux code.  I
> >>> would really appreciate your help in reviewing those changes.  If
> >>> you find it easier, feel free to wholesale replace my Smack/AppArmor
> >>> patch with one of your own.
> >> A quick test pass didn't show up anything obviously
> >> amiss with the Smack changes. I have will do some more
> >> through inspection, but they look fine so far.
> > Thanks for testing it out and giving it a look.  Beyond the Smack
> > specific changes, I'm also interested in making sure all the hook
> > callers are correct; I believe I made the correct substitutions, but a
> > second (or third (or fourth ...)) set of eyes is never a bad idea.
>
> I'm still not seeing anything that looks wrong. I'd suggest that Mimi
> have a look at the IMA bits.

Assuming you are still good with these changes Casey, any chance I can
get an ACK on the LSM and Smack patches?

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 0/4] Split security_task_getsecid() into subj and obj variants
  2021-03-04  0:46       ` Paul Moore
@ 2021-03-04  2:21         ` Casey Schaufler
  2021-03-04 23:41           ` Paul Moore
  0 siblings, 1 reply; 39+ messages in thread
From: Casey Schaufler @ 2021-03-04  2:21 UTC (permalink / raw)
  To: Paul Moore
  Cc: John Johansen, selinux, Mimi Zohar, linux-security-module, linux-audit

On 3/3/2021 4:46 PM, Paul Moore wrote:
> On Mon, Feb 22, 2021 at 6:59 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 2/20/2021 6:41 AM, Paul Moore wrote:
>>> On Fri, Feb 19, 2021 at 8:49 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> On 2/19/2021 3:28 PM, Paul Moore wrote:
>>>>> As discussed briefly on the list (lore link below), we are a little
>>>>> sloppy when it comes to using task credentials, mixing both the
>>>>> subjective and object credentials.  This patch set attempts to fix
>>>>> this by replacing security_task_getsecid() with two new hooks that
>>>>> return either the subjective (_subj) or objective (_obj) credentials.
>>>>>
>>>>> https://lore.kernel.org/linux-security-module/806848326.0ifERbkFSE@x2/T/
>>>>>
>>>>> Casey and John, I made a quick pass through the Smack and AppArmor
>>>>> code in an effort to try and do the right thing, but I will admit
>>>>> that I haven't tested those changes, just the SELinux code.  I
>>>>> would really appreciate your help in reviewing those changes.  If
>>>>> you find it easier, feel free to wholesale replace my Smack/AppArmor
>>>>> patch with one of your own.
>>>> A quick test pass didn't show up anything obviously
>>>> amiss with the Smack changes. I have will do some more
>>>> through inspection, but they look fine so far.
>>> Thanks for testing it out and giving it a look.  Beyond the Smack
>>> specific changes, I'm also interested in making sure all the hook
>>> callers are correct; I believe I made the correct substitutions, but a
>>> second (or third (or fourth ...)) set of eyes is never a bad idea.
>> I'm still not seeing anything that looks wrong. I'd suggest that Mimi
>> have a look at the IMA bits.
> Assuming you are still good with these changes Casey, any chance I can
> get an ACK on the LSM and Smack patches?

Yes. You can add my:

Acked-by: Casey Schaufler <casey@schaufler-ca.com>

to both.

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants
  2021-02-20 14:44     ` Paul Moore
@ 2021-03-04 10:04       ` Jeffrey Vander Stoep
  2021-03-04 23:43         ` Paul Moore
  0 siblings, 1 reply; 39+ messages in thread
From: Jeffrey Vander Stoep @ 2021-03-04 10:04 UTC (permalink / raw)
  To: Paul Moore
  Cc: John Johansen, SElinux list, James Morris, LSM List, linux-audit

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

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 0/4] Split security_task_getsecid() into subj and obj variants
  2021-03-04  2:21         ` Casey Schaufler
@ 2021-03-04 23:41           ` Paul Moore
  0 siblings, 0 replies; 39+ messages in thread
From: Paul Moore @ 2021-03-04 23:41 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: selinux, linux-security-module, John Johansen, Mimi Zohar, linux-audit

On Wed, Mar 3, 2021 at 9:21 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 3/3/2021 4:46 PM, Paul Moore wrote:

...

> > Assuming you are still good with these changes Casey, any chance I can
> > get an ACK on the LSM and Smack patches?
>
> Yes. You can add my:
>
> Acked-by: Casey Schaufler <casey@schaufler-ca.com>
>
> to both.

Done, thanks Casey.

I talked to John and he is working on the AppArmor tweaks so I'll hold
off reposting until I see an update from him (nothing beyond the ACKs
has changed anyway).

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants
  2021-03-04 10:04       ` Jeffrey Vander Stoep
@ 2021-03-04 23:43         ` Paul Moore
  2021-03-10  8:21           ` Jeffrey Vander Stoep
  0 siblings, 1 reply; 39+ messages in thread
From: Paul Moore @ 2021-03-04 23:43 UTC (permalink / raw)
  To: Jeffrey Vander Stoep
  Cc: John Johansen, SElinux list, James Morris, LSM List, linux-audit

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

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants
  2021-02-19 23:29 ` [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants Paul Moore
                     ` (2 preceding siblings ...)
  2021-02-24 16:49   ` Mimi Zohar
@ 2021-03-08 19:25   ` Richard Guy Briggs
  2021-03-10  0:23     ` Paul Moore
  2021-03-10  1:03   ` John Johansen
  4 siblings, 1 reply; 39+ messages in thread
From: Richard Guy Briggs @ 2021-03-08 19:25 UTC (permalink / raw)
  To: Paul Moore; +Cc: selinux, linux-security-module, John Johansen, linux-audit

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

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 2/4] selinux: clarify task subjective and objective credentials
  2021-02-19 23:29 ` [RFC PATCH 2/4] selinux: clarify task subjective and objective credentials Paul Moore
  2021-02-21 12:55   ` John Johansen
@ 2021-03-08 19:26   ` Richard Guy Briggs
  2021-03-10  3:05   ` John Johansen
  2 siblings, 0 replies; 39+ messages in thread
From: Richard Guy Briggs @ 2021-03-08 19:26 UTC (permalink / raw)
  To: Paul Moore; +Cc: selinux, linux-security-module, John Johansen, linux-audit

On 2021-02-19 18:29, Paul Moore wrote:
> SELinux has a function, task_sid(), which returns the task's
> objective credentials, but unfortunately is used in a few places
> where the subjective task credentials should be used.  Most notably
> in the new security_task_getsecid_subj() LSM hook.
> 
> This patch fixes this and attempts to make things more obvious by
> introducing a new function, task_sid_subj(), and renaming the
> existing task_sid() function to task_sid_obj().
> 
> Signed-off-by: Paul Moore <paul@paul-moore.com>

FWIW Reviewed-by: Richard Guy Briggs <rgb@redhat.com>

> ---
>  security/selinux/hooks.c |   85 +++++++++++++++++++++++++++-------------------
>  1 file changed, 49 insertions(+), 36 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f311541c4972e..1c53000d28e37 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -229,10 +229,23 @@ static inline u32 cred_sid(const struct cred *cred)
>  	return tsec->sid;
>  }
>  
> +/*
> + * get the subjective security ID of a task
> + */
> +static inline u32 task_sid_subj(const struct task_struct *task)
> +{
> +	u32 sid;
> +
> +	rcu_read_lock();
> +	sid = cred_sid(rcu_dereference(task->cred));
> +	rcu_read_unlock();
> +	return sid;
> +}
> +
>  /*
>   * get the objective security ID of a task
>   */
> -static inline u32 task_sid(const struct task_struct *task)
> +static inline u32 task_sid_obj(const struct task_struct *task)
>  {
>  	u32 sid;
>  
> @@ -2034,11 +2047,8 @@ static inline u32 open_file_to_av(struct file *file)
>  
>  static int selinux_binder_set_context_mgr(struct task_struct *mgr)
>  {
> -	u32 mysid = current_sid();
> -	u32 mgrsid = task_sid(mgr);
> -
>  	return avc_has_perm(&selinux_state,
> -			    mysid, mgrsid, SECCLASS_BINDER,
> +			    current_sid(), task_sid_obj(mgr), SECCLASS_BINDER,
>  			    BINDER__SET_CONTEXT_MGR, NULL);
>  }
>  
> @@ -2046,8 +2056,8 @@ static int selinux_binder_transaction(struct task_struct *from,
>  				      struct task_struct *to)
>  {
>  	u32 mysid = current_sid();
> -	u32 fromsid = task_sid(from);
> -	u32 tosid = task_sid(to);
> +	u32 fromsid = task_sid_subj(from);
> +	u32 tosid = task_sid_subj(to);
>  	int rc;
>  
>  	if (mysid != fromsid) {
> @@ -2066,11 +2076,9 @@ static int selinux_binder_transaction(struct task_struct *from,
>  static int selinux_binder_transfer_binder(struct task_struct *from,
>  					  struct task_struct *to)
>  {
> -	u32 fromsid = task_sid(from);
> -	u32 tosid = task_sid(to);
> -
>  	return avc_has_perm(&selinux_state,
> -			    fromsid, tosid, SECCLASS_BINDER, BINDER__TRANSFER,
> +			    task_sid_subj(from), task_sid_obj(to),
> +			    SECCLASS_BINDER, BINDER__TRANSFER,
>  			    NULL);
>  }
>  
> @@ -2078,7 +2086,7 @@ static int selinux_binder_transfer_file(struct task_struct *from,
>  					struct task_struct *to,
>  					struct file *file)
>  {
> -	u32 sid = task_sid(to);
> +	u32 sid = task_sid_subj(to);
>  	struct file_security_struct *fsec = selinux_file(file);
>  	struct dentry *dentry = file->f_path.dentry;
>  	struct inode_security_struct *isec;
> @@ -2114,10 +2122,10 @@ static int selinux_binder_transfer_file(struct task_struct *from,
>  }
>  
>  static int selinux_ptrace_access_check(struct task_struct *child,
> -				     unsigned int mode)
> +				       unsigned int mode)
>  {
>  	u32 sid = current_sid();
> -	u32 csid = task_sid(child);
> +	u32 csid = task_sid_obj(child);
>  
>  	if (mode & PTRACE_MODE_READ)
>  		return avc_has_perm(&selinux_state,
> @@ -2130,15 +2138,15 @@ static int selinux_ptrace_access_check(struct task_struct *child,
>  static int selinux_ptrace_traceme(struct task_struct *parent)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    task_sid(parent), current_sid(), SECCLASS_PROCESS,
> -			    PROCESS__PTRACE, NULL);
> +			    task_sid_subj(parent), task_sid_obj(current),
> +			    SECCLASS_PROCESS, PROCESS__PTRACE, NULL);
>  }
>  
>  static int selinux_capget(struct task_struct *target, kernel_cap_t *effective,
>  			  kernel_cap_t *inheritable, kernel_cap_t *permitted)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(target), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(target), SECCLASS_PROCESS,
>  			    PROCESS__GETCAP, NULL);
>  }
>  
> @@ -2263,7 +2271,7 @@ static u32 ptrace_parent_sid(void)
>  	rcu_read_lock();
>  	tracer = ptrace_parent(current);
>  	if (tracer)
> -		sid = task_sid(tracer);
> +		sid = task_sid_obj(tracer);
>  	rcu_read_unlock();
>  
>  	return sid;
> @@ -3916,7 +3924,7 @@ static int selinux_file_send_sigiotask(struct task_struct *tsk,
>  				       struct fown_struct *fown, int signum)
>  {
>  	struct file *file;
> -	u32 sid = task_sid(tsk);
> +	u32 sid = task_sid_obj(tsk);
>  	u32 perm;
>  	struct file_security_struct *fsec;
>  
> @@ -4135,47 +4143,52 @@ static int selinux_kernel_load_data(enum kernel_load_data_id id, bool contents)
>  static int selinux_task_setpgid(struct task_struct *p, pid_t pgid)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__SETPGID, NULL);
>  }
>  
>  static int selinux_task_getpgid(struct task_struct *p)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__GETPGID, NULL);
>  }
>  
>  static int selinux_task_getsid(struct task_struct *p)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__GETSESSION, NULL);
>  }
>  
> -static void selinux_task_getsecid(struct task_struct *p, u32 *secid)
> +static void selinux_task_getsecid_subj(struct task_struct *p, u32 *secid)
> +{
> +	*secid = task_sid_subj(p);
> +}
> +
> +static void selinux_task_getsecid_obj(struct task_struct *p, u32 *secid)
>  {
> -	*secid = task_sid(p);
> +	*secid = task_sid_obj(p);
>  }
>  
>  static int selinux_task_setnice(struct task_struct *p, int nice)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__SETSCHED, NULL);
>  }
>  
>  static int selinux_task_setioprio(struct task_struct *p, int ioprio)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__SETSCHED, NULL);
>  }
>  
>  static int selinux_task_getioprio(struct task_struct *p)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__GETSCHED, NULL);
>  }
>  
> @@ -4206,7 +4219,7 @@ static int selinux_task_setrlimit(struct task_struct *p, unsigned int resource,
>  	   upon context transitions.  See selinux_bprm_committing_creds. */
>  	if (old_rlim->rlim_max != new_rlim->rlim_max)
>  		return avc_has_perm(&selinux_state,
> -				    current_sid(), task_sid(p),
> +				    current_sid(), task_sid_obj(p),
>  				    SECCLASS_PROCESS, PROCESS__SETRLIMIT, NULL);
>  
>  	return 0;
> @@ -4215,21 +4228,21 @@ static int selinux_task_setrlimit(struct task_struct *p, unsigned int resource,
>  static int selinux_task_setscheduler(struct task_struct *p)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__SETSCHED, NULL);
>  }
>  
>  static int selinux_task_getscheduler(struct task_struct *p)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__GETSCHED, NULL);
>  }
>  
>  static int selinux_task_movememory(struct task_struct *p)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__SETSCHED, NULL);
>  }
>  
> @@ -4248,14 +4261,14 @@ static int selinux_task_kill(struct task_struct *p, struct kernel_siginfo *info,
>  	else
>  		secid = cred_sid(cred);
>  	return avc_has_perm(&selinux_state,
> -			    secid, task_sid(p), SECCLASS_PROCESS, perm, NULL);
> +			    secid, task_sid_obj(p), SECCLASS_PROCESS, perm, NULL);
>  }
>  
>  static void selinux_task_to_inode(struct task_struct *p,
>  				  struct inode *inode)
>  {
>  	struct inode_security_struct *isec = selinux_inode(inode);
> -	u32 sid = task_sid(p);
> +	u32 sid = task_sid_obj(p);
>  
>  	spin_lock(&isec->lock);
>  	isec->sclass = inode_mode_to_security_class(inode->i_mode);
> @@ -6148,7 +6161,7 @@ static int selinux_msg_queue_msgrcv(struct kern_ipc_perm *msq, struct msg_msg *m
>  	struct ipc_security_struct *isec;
>  	struct msg_security_struct *msec;
>  	struct common_audit_data ad;
> -	u32 sid = task_sid(target);
> +	u32 sid = task_sid_subj(target);
>  	int rc;
>  
>  	isec = selinux_ipc(msq);
> @@ -7143,8 +7156,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_subj, selinux_task_getsecid),
> -	LSM_HOOK_INIT(task_getsecid_obj, selinux_task_getsecid),
> +	LSM_HOOK_INIT(task_getsecid_subj, selinux_task_getsecid_subj),
> +	LSM_HOOK_INIT(task_getsecid_obj, selinux_task_getsecid_obj),
>  	LSM_HOOK_INIT(task_setnice, selinux_task_setnice),
>  	LSM_HOOK_INIT(task_setioprio, selinux_task_setioprio),
>  	LSM_HOOK_INIT(task_getioprio, selinux_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

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 3/4] smack: differentiate between subjective and objective task credentials
  2021-02-19 23:29 ` [RFC PATCH 3/4] smack: differentiate between subjective and objective task credentials Paul Moore
  2021-02-21 12:56   ` John Johansen
@ 2021-03-08 19:26   ` Richard Guy Briggs
  2021-03-10  1:04   ` John Johansen
  2 siblings, 0 replies; 39+ messages in thread
From: Richard Guy Briggs @ 2021-03-08 19:26 UTC (permalink / raw)
  To: Paul Moore; +Cc: selinux, linux-security-module, John Johansen, linux-audit

On 2021-02-19 18:29, Paul Moore wrote:
> With the split of the security_task_getsecid() into subjective and
> objective variants it's time to update Smack to ensure it is using
> the correct task creds.
> 
> Signed-off-by: Paul Moore <paul@paul-moore.com>

FWIW Reviewed-by: Richard Guy Briggs <rgb@redhat.com>

> ---
>  security/smack/smack.h     |   18 +++++++++++++++++-
>  security/smack/smack_lsm.c |   40 +++++++++++++++++++++++++++-------------
>  2 files changed, 44 insertions(+), 14 deletions(-)
> 
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index a9768b12716bf..08f9cb80655ce 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -383,7 +383,23 @@ static inline struct smack_known *smk_of_task(const struct task_smack *tsp)
>  	return tsp->smk_task;
>  }
>  
> -static inline struct smack_known *smk_of_task_struct(
> +static inline struct smack_known *smk_of_task_struct_subj(
> +						const struct task_struct *t)
> +{
> +	struct smack_known *skp;
> +	const struct cred *cred;
> +
> +	rcu_read_lock();
> +
> +	cred = rcu_dereference(t->cred);
> +	skp = smk_of_task(smack_cred(cred));
> +
> +	rcu_read_unlock();
> +
> +	return skp;
> +}
> +
> +static inline struct smack_known *smk_of_task_struct_obj(
>  						const struct task_struct *t)
>  {
>  	struct smack_known *skp;
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 2bb354ef2c4a9..ea1a82742e8ba 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -159,7 +159,7 @@ static int smk_bu_current(char *note, struct smack_known *oskp,
>  static int smk_bu_task(struct task_struct *otp, int mode, int rc)
>  {
>  	struct task_smack *tsp = smack_cred(current_cred());
> -	struct smack_known *smk_task = smk_of_task_struct(otp);
> +	struct smack_known *smk_task = smk_of_task_struct_obj(otp);
>  	char acc[SMK_NUM_ACCESS_TYPE + 1];
>  
>  	if (rc <= 0)
> @@ -479,7 +479,7 @@ static int smack_ptrace_access_check(struct task_struct *ctp, unsigned int mode)
>  {
>  	struct smack_known *skp;
>  
> -	skp = smk_of_task_struct(ctp);
> +	skp = smk_of_task_struct_obj(ctp);
>  
>  	return smk_ptrace_rule_check(current, skp, mode, __func__);
>  }
> @@ -2031,7 +2031,7 @@ static int smk_curacc_on_task(struct task_struct *p, int access,
>  				const char *caller)
>  {
>  	struct smk_audit_info ad;
> -	struct smack_known *skp = smk_of_task_struct(p);
> +	struct smack_known *skp = smk_of_task_struct_subj(p);
>  	int rc;
>  
>  	smk_ad_init(&ad, caller, LSM_AUDIT_DATA_TASK);
> @@ -2076,15 +2076,29 @@ static int smack_task_getsid(struct task_struct *p)
>  }
>  
>  /**
> - * smack_task_getsecid - get the secid of the task
> - * @p: the object task
> + * smack_task_getsecid_subj - get the subjective secid of the task
> + * @p: the task
>   * @secid: where to put the result
>   *
> - * Sets the secid to contain a u32 version of the smack label.
> + * Sets the secid to contain a u32 version of the task's subjective smack label.
> + */
> +static void smack_task_getsecid_subj(struct task_struct *p, u32 *secid)
> +{
> +	struct smack_known *skp = smk_of_task_struct_subj(p);
> +
> +	*secid = skp->smk_secid;
> +}
> +
> +/**
> + * smack_task_getsecid_obj - get the objective secid of the task
> + * @p: the task
> + * @secid: where to put the result
> + *
> + * Sets the secid to contain a u32 version of the task's objective smack label.
>   */
> -static void smack_task_getsecid(struct task_struct *p, u32 *secid)
> +static void smack_task_getsecid_obj(struct task_struct *p, u32 *secid)
>  {
> -	struct smack_known *skp = smk_of_task_struct(p);
> +	struct smack_known *skp = smk_of_task_struct_obj(p);
>  
>  	*secid = skp->smk_secid;
>  }
> @@ -2172,7 +2186,7 @@ static int smack_task_kill(struct task_struct *p, struct kernel_siginfo *info,
>  {
>  	struct smk_audit_info ad;
>  	struct smack_known *skp;
> -	struct smack_known *tkp = smk_of_task_struct(p);
> +	struct smack_known *tkp = smk_of_task_struct_obj(p);
>  	int rc;
>  
>  	if (!sig)
> @@ -2210,7 +2224,7 @@ static int smack_task_kill(struct task_struct *p, struct kernel_siginfo *info,
>  static void smack_task_to_inode(struct task_struct *p, struct inode *inode)
>  {
>  	struct inode_smack *isp = smack_inode(inode);
> -	struct smack_known *skp = smk_of_task_struct(p);
> +	struct smack_known *skp = smk_of_task_struct_obj(p);
>  
>  	isp->smk_inode = skp;
>  	isp->smk_flags |= SMK_INODE_INSTANT;
> @@ -3481,7 +3495,7 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
>   */
>  static int smack_getprocattr(struct task_struct *p, char *name, char **value)
>  {
> -	struct smack_known *skp = smk_of_task_struct(p);
> +	struct smack_known *skp = smk_of_task_struct_subj(p);
>  	char *cp;
>  	int slen;
>  
> @@ -4755,8 +4769,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_subj, smack_task_getsecid),
> -	LSM_HOOK_INIT(task_getsecid_obj, smack_task_getsecid),
> +	LSM_HOOK_INIT(task_getsecid_subj, smack_task_getsecid_subj),
> +	LSM_HOOK_INIT(task_getsecid_obj, smack_task_getsecid_obj),
>  	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

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants
  2021-03-08 19:25   ` Richard Guy Briggs
@ 2021-03-10  0:23     ` Paul Moore
  0 siblings, 0 replies; 39+ messages in thread
From: Paul Moore @ 2021-03-10  0:23 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: selinux, linux-security-module, John Johansen, linux-audit

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.

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants
  2021-03-04  0:44     ` Paul Moore
@ 2021-03-10  0:28       ` Paul Moore
  2021-03-10  3:09         ` John Johansen
  0 siblings, 1 reply; 39+ messages in thread
From: Paul Moore @ 2021-03-10  0:28 UTC (permalink / raw)
  To: John Johansen; +Cc: selinux, linux-security-module, linux-audit

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.

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants
  2021-02-19 23:29 ` [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants Paul Moore
                     ` (3 preceding siblings ...)
  2021-03-08 19:25   ` Richard Guy Briggs
@ 2021-03-10  1:03   ` John Johansen
  2021-03-11  1:55     ` Paul Moore
  4 siblings, 1 reply; 39+ messages in thread
From: John Johansen @ 2021-03-10  1:03 UTC (permalink / raw)
  To: Paul Moore, Casey Schaufler; +Cc: selinux, linux-security-module, linux-audit

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),
> 

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 3/4] smack: differentiate between subjective and objective task credentials
  2021-02-19 23:29 ` [RFC PATCH 3/4] smack: differentiate between subjective and objective task credentials Paul Moore
  2021-02-21 12:56   ` John Johansen
  2021-03-08 19:26   ` Richard Guy Briggs
@ 2021-03-10  1:04   ` John Johansen
  2 siblings, 0 replies; 39+ messages in thread
From: John Johansen @ 2021-03-10  1:04 UTC (permalink / raw)
  To: Paul Moore, Casey Schaufler; +Cc: selinux, linux-security-module, linux-audit

On 2/19/21 3:29 PM, Paul Moore wrote:
> With the split of the security_task_getsecid() into subjective and
> objective variants it's time to update Smack to ensure it is using
> the correct task creds.
> 
> Signed-off-by: Paul Moore <paul@paul-moore.com>

Reviewed-by: John Johansen <john.johansen@canonical.com>


> ---
>  security/smack/smack.h     |   18 +++++++++++++++++-
>  security/smack/smack_lsm.c |   40 +++++++++++++++++++++++++++-------------
>  2 files changed, 44 insertions(+), 14 deletions(-)
> 
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index a9768b12716bf..08f9cb80655ce 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -383,7 +383,23 @@ static inline struct smack_known *smk_of_task(const struct task_smack *tsp)
>  	return tsp->smk_task;
>  }
>  
> -static inline struct smack_known *smk_of_task_struct(
> +static inline struct smack_known *smk_of_task_struct_subj(
> +						const struct task_struct *t)
> +{
> +	struct smack_known *skp;
> +	const struct cred *cred;
> +
> +	rcu_read_lock();
> +
> +	cred = rcu_dereference(t->cred);
> +	skp = smk_of_task(smack_cred(cred));
> +
> +	rcu_read_unlock();
> +
> +	return skp;
> +}
> +
> +static inline struct smack_known *smk_of_task_struct_obj(
>  						const struct task_struct *t)
>  {
>  	struct smack_known *skp;
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 2bb354ef2c4a9..ea1a82742e8ba 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -159,7 +159,7 @@ static int smk_bu_current(char *note, struct smack_known *oskp,
>  static int smk_bu_task(struct task_struct *otp, int mode, int rc)
>  {
>  	struct task_smack *tsp = smack_cred(current_cred());
> -	struct smack_known *smk_task = smk_of_task_struct(otp);
> +	struct smack_known *smk_task = smk_of_task_struct_obj(otp);
>  	char acc[SMK_NUM_ACCESS_TYPE + 1];
>  
>  	if (rc <= 0)
> @@ -479,7 +479,7 @@ static int smack_ptrace_access_check(struct task_struct *ctp, unsigned int mode)
>  {
>  	struct smack_known *skp;
>  
> -	skp = smk_of_task_struct(ctp);
> +	skp = smk_of_task_struct_obj(ctp);
>  
>  	return smk_ptrace_rule_check(current, skp, mode, __func__);
>  }
> @@ -2031,7 +2031,7 @@ static int smk_curacc_on_task(struct task_struct *p, int access,
>  				const char *caller)
>  {
>  	struct smk_audit_info ad;
> -	struct smack_known *skp = smk_of_task_struct(p);
> +	struct smack_known *skp = smk_of_task_struct_subj(p);
>  	int rc;
>  
>  	smk_ad_init(&ad, caller, LSM_AUDIT_DATA_TASK);
> @@ -2076,15 +2076,29 @@ static int smack_task_getsid(struct task_struct *p)
>  }
>  
>  /**
> - * smack_task_getsecid - get the secid of the task
> - * @p: the object task
> + * smack_task_getsecid_subj - get the subjective secid of the task
> + * @p: the task
>   * @secid: where to put the result
>   *
> - * Sets the secid to contain a u32 version of the smack label.
> + * Sets the secid to contain a u32 version of the task's subjective smack label.
> + */
> +static void smack_task_getsecid_subj(struct task_struct *p, u32 *secid)
> +{
> +	struct smack_known *skp = smk_of_task_struct_subj(p);
> +
> +	*secid = skp->smk_secid;
> +}
> +
> +/**
> + * smack_task_getsecid_obj - get the objective secid of the task
> + * @p: the task
> + * @secid: where to put the result
> + *
> + * Sets the secid to contain a u32 version of the task's objective smack label.
>   */
> -static void smack_task_getsecid(struct task_struct *p, u32 *secid)
> +static void smack_task_getsecid_obj(struct task_struct *p, u32 *secid)
>  {
> -	struct smack_known *skp = smk_of_task_struct(p);
> +	struct smack_known *skp = smk_of_task_struct_obj(p);
>  
>  	*secid = skp->smk_secid;
>  }
> @@ -2172,7 +2186,7 @@ static int smack_task_kill(struct task_struct *p, struct kernel_siginfo *info,
>  {
>  	struct smk_audit_info ad;
>  	struct smack_known *skp;
> -	struct smack_known *tkp = smk_of_task_struct(p);
> +	struct smack_known *tkp = smk_of_task_struct_obj(p);
>  	int rc;
>  
>  	if (!sig)
> @@ -2210,7 +2224,7 @@ static int smack_task_kill(struct task_struct *p, struct kernel_siginfo *info,
>  static void smack_task_to_inode(struct task_struct *p, struct inode *inode)
>  {
>  	struct inode_smack *isp = smack_inode(inode);
> -	struct smack_known *skp = smk_of_task_struct(p);
> +	struct smack_known *skp = smk_of_task_struct_obj(p);
>  
>  	isp->smk_inode = skp;
>  	isp->smk_flags |= SMK_INODE_INSTANT;
> @@ -3481,7 +3495,7 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
>   */
>  static int smack_getprocattr(struct task_struct *p, char *name, char **value)
>  {
> -	struct smack_known *skp = smk_of_task_struct(p);
> +	struct smack_known *skp = smk_of_task_struct_subj(p);
>  	char *cp;
>  	int slen;
>  
> @@ -4755,8 +4769,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_subj, smack_task_getsecid),
> -	LSM_HOOK_INIT(task_getsecid_obj, smack_task_getsecid),
> +	LSM_HOOK_INIT(task_getsecid_subj, smack_task_getsecid_subj),
> +	LSM_HOOK_INIT(task_getsecid_obj, smack_task_getsecid_obj),
>  	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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 2/4] selinux: clarify task subjective and objective credentials
  2021-02-19 23:29 ` [RFC PATCH 2/4] selinux: clarify task subjective and objective credentials Paul Moore
  2021-02-21 12:55   ` John Johansen
  2021-03-08 19:26   ` Richard Guy Briggs
@ 2021-03-10  3:05   ` John Johansen
  2021-03-11  4:32     ` Paul Moore
  2 siblings, 1 reply; 39+ messages in thread
From: John Johansen @ 2021-03-10  3:05 UTC (permalink / raw)
  To: Paul Moore, Casey Schaufler; +Cc: selinux, linux-security-module, linux-audit

On 2/19/21 3:29 PM, Paul Moore wrote:
> SELinux has a function, task_sid(), which returns the task's
> objective credentials, but unfortunately is used in a few places
> where the subjective task credentials should be used.  Most notably
> in the new security_task_getsecid_subj() LSM hook.
> 
> This patch fixes this and attempts to make things more obvious by
> introducing a new function, task_sid_subj(), and renaming the
> existing task_sid() function to task_sid_obj().
> 
> Signed-off-by: Paul Moore <paul@paul-moore.com>

I have a couple of questions below but the rest looks good

> ---
>  security/selinux/hooks.c |   85 +++++++++++++++++++++++++++-------------------
>  1 file changed, 49 insertions(+), 36 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f311541c4972e..1c53000d28e37 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -229,10 +229,23 @@ static inline u32 cred_sid(const struct cred *cred)
>  	return tsec->sid;
>  }
>  
> +/*
> + * get the subjective security ID of a task
> + */
> +static inline u32 task_sid_subj(const struct task_struct *task)
> +{
> +	u32 sid;
> +
> +	rcu_read_lock();
> +	sid = cred_sid(rcu_dereference(task->cred));
> +	rcu_read_unlock();
> +	return sid;
> +}
> +
>  /*
>   * get the objective security ID of a task
>   */
> -static inline u32 task_sid(const struct task_struct *task)
> +static inline u32 task_sid_obj(const struct task_struct *task)
>  {
>  	u32 sid;
>  
> @@ -2034,11 +2047,8 @@ static inline u32 open_file_to_av(struct file *file)
>  
>  static int selinux_binder_set_context_mgr(struct task_struct *mgr)
>  {
> -	u32 mysid = current_sid();
> -	u32 mgrsid = task_sid(mgr);
> -
>  	return avc_has_perm(&selinux_state,
> -			    mysid, mgrsid, SECCLASS_BINDER,
> +			    current_sid(), task_sid_obj(mgr), SECCLASS_BINDER,
>  			    BINDER__SET_CONTEXT_MGR, NULL);
>  }
>  
> @@ -2046,8 +2056,8 @@ static int selinux_binder_transaction(struct task_struct *from,
>  				      struct task_struct *to)
>  {
>  	u32 mysid = current_sid();
> -	u32 fromsid = task_sid(from);
> -	u32 tosid = task_sid(to);
> +	u32 fromsid = task_sid_subj(from);

fromsid potentially gets used as both the subject and the object the following
permission checks. It makes sense to use the same cred for both checks but
what I am not sure about yet is whether its actually safe to use the subject
sid when the task isn't current.

ie. I am still trying to determine if there is a race here between the transaction
request and the permission check.

> +	u32 tosid = task_sid_subj(to);
its not clear to me that using the subj for to is correct

>  	int rc;
>  
>  	if (mysid != fromsid) {
> @@ -2066,11 +2076,9 @@ static int selinux_binder_transaction(struct task_struct *from,
>  static int selinux_binder_transfer_binder(struct task_struct *from,
>  					  struct task_struct *to)
>  {
> -	u32 fromsid = task_sid(from);
> -	u32 tosid = task_sid(to);
> -
>  	return avc_has_perm(&selinux_state,
> -			    fromsid, tosid, SECCLASS_BINDER, BINDER__TRANSFER,
> +			    task_sid_subj(from), task_sid_obj(to),
> +			    SECCLASS_BINDER, BINDER__TRANSFER,
>  			    NULL);
>  }
>  
> @@ -2078,7 +2086,7 @@ static int selinux_binder_transfer_file(struct task_struct *from,
>  					struct task_struct *to,
>  					struct file *file)
>  {
> -	u32 sid = task_sid(to);
> +	u32 sid = task_sid_subj(to);

same question about safety here

>  	struct file_security_struct *fsec = selinux_file(file);
>  	struct dentry *dentry = file->f_path.dentry;
>  	struct inode_security_struct *isec;
> @@ -2114,10 +2122,10 @@ static int selinux_binder_transfer_file(struct task_struct *from,
>  }
>  
>  static int selinux_ptrace_access_check(struct task_struct *child,
> -				     unsigned int mode)
> +				       unsigned int mode)
>  {
>  	u32 sid = current_sid();
> -	u32 csid = task_sid(child);
> +	u32 csid = task_sid_obj(child);
>  
>  	if (mode & PTRACE_MODE_READ)
>  		return avc_has_perm(&selinux_state,
> @@ -2130,15 +2138,15 @@ static int selinux_ptrace_access_check(struct task_struct *child,
>  static int selinux_ptrace_traceme(struct task_struct *parent)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    task_sid(parent), current_sid(), SECCLASS_PROCESS,
> -			    PROCESS__PTRACE, NULL);
> +			    task_sid_subj(parent), task_sid_obj(current),
> +			    SECCLASS_PROCESS, PROCESS__PTRACE, NULL);
>  }
>  
>  static int selinux_capget(struct task_struct *target, kernel_cap_t *effective,
>  			  kernel_cap_t *inheritable, kernel_cap_t *permitted)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(target), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(target), SECCLASS_PROCESS,
>  			    PROCESS__GETCAP, NULL);
>  }
>  
> @@ -2263,7 +2271,7 @@ static u32 ptrace_parent_sid(void)
>  	rcu_read_lock();
>  	tracer = ptrace_parent(current);
>  	if (tracer)
> -		sid = task_sid(tracer);
> +		sid = task_sid_obj(tracer);
>  	rcu_read_unlock();
>  
>  	return sid;
> @@ -3916,7 +3924,7 @@ static int selinux_file_send_sigiotask(struct task_struct *tsk,
>  				       struct fown_struct *fown, int signum)
>  {
>  	struct file *file;
> -	u32 sid = task_sid(tsk);
> +	u32 sid = task_sid_obj(tsk);
>  	u32 perm;
>  	struct file_security_struct *fsec;
>  
> @@ -4135,47 +4143,52 @@ static int selinux_kernel_load_data(enum kernel_load_data_id id, bool contents)
>  static int selinux_task_setpgid(struct task_struct *p, pid_t pgid)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__SETPGID, NULL);
>  }
>  
>  static int selinux_task_getpgid(struct task_struct *p)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__GETPGID, NULL);
>  }
>  
>  static int selinux_task_getsid(struct task_struct *p)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__GETSESSION, NULL);
>  }
>  
> -static void selinux_task_getsecid(struct task_struct *p, u32 *secid)
> +static void selinux_task_getsecid_subj(struct task_struct *p, u32 *secid)
> +{
> +	*secid = task_sid_subj(p);
> +}
> +
> +static void selinux_task_getsecid_obj(struct task_struct *p, u32 *secid)
>  {
> -	*secid = task_sid(p);
> +	*secid = task_sid_obj(p);
>  }
>  
>  static int selinux_task_setnice(struct task_struct *p, int nice)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__SETSCHED, NULL);
>  }
>  
>  static int selinux_task_setioprio(struct task_struct *p, int ioprio)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__SETSCHED, NULL);
>  }
>  
>  static int selinux_task_getioprio(struct task_struct *p)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__GETSCHED, NULL);
>  }
>  
> @@ -4206,7 +4219,7 @@ static int selinux_task_setrlimit(struct task_struct *p, unsigned int resource,
>  	   upon context transitions.  See selinux_bprm_committing_creds. */
>  	if (old_rlim->rlim_max != new_rlim->rlim_max)
>  		return avc_has_perm(&selinux_state,
> -				    current_sid(), task_sid(p),
> +				    current_sid(), task_sid_obj(p),
>  				    SECCLASS_PROCESS, PROCESS__SETRLIMIT, NULL);
>  
>  	return 0;
> @@ -4215,21 +4228,21 @@ static int selinux_task_setrlimit(struct task_struct *p, unsigned int resource,
>  static int selinux_task_setscheduler(struct task_struct *p)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__SETSCHED, NULL);
>  }
>  
>  static int selinux_task_getscheduler(struct task_struct *p)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__GETSCHED, NULL);
>  }
>  
>  static int selinux_task_movememory(struct task_struct *p)
>  {
>  	return avc_has_perm(&selinux_state,
> -			    current_sid(), task_sid(p), SECCLASS_PROCESS,
> +			    current_sid(), task_sid_obj(p), SECCLASS_PROCESS,
>  			    PROCESS__SETSCHED, NULL);
>  }
>  
> @@ -4248,14 +4261,14 @@ static int selinux_task_kill(struct task_struct *p, struct kernel_siginfo *info,
>  	else
>  		secid = cred_sid(cred);
>  	return avc_has_perm(&selinux_state,
> -			    secid, task_sid(p), SECCLASS_PROCESS, perm, NULL);
> +			    secid, task_sid_obj(p), SECCLASS_PROCESS, perm, NULL);
>  }
>  
>  static void selinux_task_to_inode(struct task_struct *p,
>  				  struct inode *inode)
>  {
>  	struct inode_security_struct *isec = selinux_inode(inode);
> -	u32 sid = task_sid(p);
> +	u32 sid = task_sid_obj(p);
>  
>  	spin_lock(&isec->lock);
>  	isec->sclass = inode_mode_to_security_class(inode->i_mode);
> @@ -6148,7 +6161,7 @@ static int selinux_msg_queue_msgrcv(struct kern_ipc_perm *msq, struct msg_msg *m
>  	struct ipc_security_struct *isec;
>  	struct msg_security_struct *msec;
>  	struct common_audit_data ad;
> -	u32 sid = task_sid(target);
> +	u32 sid = task_sid_subj(target);
>  	int rc;
>  
>  	isec = selinux_ipc(msq);
> @@ -7143,8 +7156,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_subj, selinux_task_getsecid),
> -	LSM_HOOK_INIT(task_getsecid_obj, selinux_task_getsecid),
> +	LSM_HOOK_INIT(task_getsecid_subj, selinux_task_getsecid_subj),
> +	LSM_HOOK_INIT(task_getsecid_obj, selinux_task_getsecid_obj),
>  	LSM_HOOK_INIT(task_setnice, selinux_task_setnice),
>  	LSM_HOOK_INIT(task_setioprio, selinux_task_setioprio),
>  	LSM_HOOK_INIT(task_getioprio, selinux_task_getioprio),
> 

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants
  2021-03-10  0:28       ` Paul Moore
@ 2021-03-10  3:09         ` John Johansen
  0 siblings, 0 replies; 39+ messages in thread
From: John Johansen @ 2021-03-10  3:09 UTC (permalink / raw)
  To: Paul Moore; +Cc: selinux, linux-security-module, linux-audit

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

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants
  2021-03-04 23:43         ` Paul Moore
@ 2021-03-10  8:21           ` Jeffrey Vander Stoep
  2021-03-11  1:56             ` Paul Moore
  0 siblings, 1 reply; 39+ messages in thread
From: Jeffrey Vander Stoep @ 2021-03-10  8:21 UTC (permalink / raw)
  To: Paul Moore
  Cc: John Johansen, SElinux list, James Morris, LSM List, linux-audit

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

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants
  2021-03-10  1:03   ` John Johansen
@ 2021-03-11  1:55     ` Paul Moore
  0 siblings, 0 replies; 39+ messages in thread
From: Paul Moore @ 2021-03-11  1:55 UTC (permalink / raw)
  To: John Johansen; +Cc: selinux, linux-security-module, linux-audit

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.

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants
  2021-03-10  8:21           ` Jeffrey Vander Stoep
@ 2021-03-11  1:56             ` Paul Moore
  0 siblings, 0 replies; 39+ messages in thread
From: Paul Moore @ 2021-03-11  1:56 UTC (permalink / raw)
  To: Jeffrey Vander Stoep
  Cc: John Johansen, SElinux list, James Morris, LSM List, linux-audit

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.

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 2/4] selinux: clarify task subjective and objective credentials
  2021-03-10  3:05   ` John Johansen
@ 2021-03-11  4:32     ` Paul Moore
  2021-03-17 22:56       ` Paul Moore
  0 siblings, 1 reply; 39+ messages in thread
From: Paul Moore @ 2021-03-11  4:32 UTC (permalink / raw)
  To: John Johansen, Jeffrey Vander Stoep
  Cc: selinux, linux-security-module, linux-audit

On Tue, Mar 9, 2021 at 10:06 PM John Johansen
<john.johansen@canonical.com> wrote:
> On 2/19/21 3:29 PM, Paul Moore wrote:
> > SELinux has a function, task_sid(), which returns the task's
> > objective credentials, but unfortunately is used in a few places
> > where the subjective task credentials should be used.  Most notably
> > in the new security_task_getsecid_subj() LSM hook.
> >
> > This patch fixes this and attempts to make things more obvious by
> > introducing a new function, task_sid_subj(), and renaming the
> > existing task_sid() function to task_sid_obj().
> >
> > Signed-off-by: Paul Moore <paul@paul-moore.com>
>
> I have a couple of questions below but the rest looks good
>
> > ---
> >  security/selinux/hooks.c |   85 +++++++++++++++++++++++++++-------------------
> >  1 file changed, 49 insertions(+), 36 deletions(-)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index f311541c4972e..1c53000d28e37 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -229,10 +229,23 @@ static inline u32 cred_sid(const struct cred *cred)
> >       return tsec->sid;
> >  }
> >
> > +/*
> > + * get the subjective security ID of a task
> > + */
> > +static inline u32 task_sid_subj(const struct task_struct *task)
> > +{
> > +     u32 sid;
> > +
> > +     rcu_read_lock();
> > +     sid = cred_sid(rcu_dereference(task->cred));
> > +     rcu_read_unlock();
> > +     return sid;
> > +}
> > +
> >  /*
> >   * get the objective security ID of a task
> >   */
> > -static inline u32 task_sid(const struct task_struct *task)
> > +static inline u32 task_sid_obj(const struct task_struct *task)
> >  {
> >       u32 sid;
> >
> > @@ -2034,11 +2047,8 @@ static inline u32 open_file_to_av(struct file *file)
> >
> >  static int selinux_binder_set_context_mgr(struct task_struct *mgr)
> >  {
> > -     u32 mysid = current_sid();
> > -     u32 mgrsid = task_sid(mgr);
> > -
> >       return avc_has_perm(&selinux_state,
> > -                         mysid, mgrsid, SECCLASS_BINDER,
> > +                         current_sid(), task_sid_obj(mgr), SECCLASS_BINDER,
> >                           BINDER__SET_CONTEXT_MGR, NULL);
> >  }
> >
> > @@ -2046,8 +2056,8 @@ static int selinux_binder_transaction(struct task_struct *from,
> >                                     struct task_struct *to)
> >  {
> >       u32 mysid = current_sid();
> > -     u32 fromsid = task_sid(from);
> > -     u32 tosid = task_sid(to);
> > +     u32 fromsid = task_sid_subj(from);
>
> fromsid potentially gets used as both the subject and the object the following
> permission checks. It makes sense to use the same cred for both checks but
> what I am not sure about yet is whether its actually safe to use the subject
> sid when the task isn't current.
>
> ie. I am still trying to determine if there is a race here between the transaction
> request and the permission check.

Okay, I see what you are concerned about now ... and unfortunately I'm
not seeing a lot of precedence in the kernel for this type of usage
either; the closest I can find is something like task_lock(), but that
doesn't seem to cover the subjective creds.  In fact, looking at
override_creds(), there is nothing preventing a task from changing
it's subjective creds at any point in time.

Beyond the task_sid_subj() code here, looking back at patch 1 and the
use of security_task_getsecid_subj() we look to be mostly safe (where
safe means we are only inspecting the current task) with the exception
of the binder code once again.  There are some other exceptions but
they are in the ptrace and audit code, both of which should be okay
given the nature and calling context of the code.

The problem really does seem to be just binder, and as I look at
binder userspace example code, I'm starting to wonder if binder is
setup properly to operate sanely in a situation where a process
overrides its subject creds.  It may be that we always need to use the
objective/real creds with binder.  Jeff, any binder insight here you
can share with us?

> > +     u32 tosid = task_sid_subj(to);
> its not clear to me that using the subj for to is correct

Yes, I believe you are correct.  Jeff, I know you looked at this code
already, but I'm guessing you may have missed this (just as I did when
I wrote it); are you okay with changing 'tosid' in
selinux_binder_transaction() to the task's objective credentials?

--
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 2/4] selinux: clarify task subjective and objective credentials
  2021-03-11  4:32     ` Paul Moore
@ 2021-03-17 22:56       ` Paul Moore
  0 siblings, 0 replies; 39+ messages in thread
From: Paul Moore @ 2021-03-17 22:56 UTC (permalink / raw)
  To: John Johansen, Jeffrey Vander Stoep
  Cc: selinux, linux-security-module, linux-audit

On Wed, Mar 10, 2021 at 11:32 PM Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Mar 9, 2021 at 10:06 PM John Johansen
> <john.johansen@canonical.com> wrote:
> > On 2/19/21 3:29 PM, Paul Moore wrote:

...

> > > @@ -2034,11 +2047,8 @@ static inline u32 open_file_to_av(struct file *file)
> > >
> > >  static int selinux_binder_set_context_mgr(struct task_struct *mgr)
> > >  {
> > > -     u32 mysid = current_sid();
> > > -     u32 mgrsid = task_sid(mgr);
> > > -
> > >       return avc_has_perm(&selinux_state,
> > > -                         mysid, mgrsid, SECCLASS_BINDER,
> > > +                         current_sid(), task_sid_obj(mgr), SECCLASS_BINDER,
> > >                           BINDER__SET_CONTEXT_MGR, NULL);
> > >  }
> > >
> > > @@ -2046,8 +2056,8 @@ static int selinux_binder_transaction(struct task_struct *from,
> > >                                     struct task_struct *to)
> > >  {
> > >       u32 mysid = current_sid();
> > > -     u32 fromsid = task_sid(from);
> > > -     u32 tosid = task_sid(to);
> > > +     u32 fromsid = task_sid_subj(from);
> >
> > fromsid potentially gets used as both the subject and the object the following
> > permission checks. It makes sense to use the same cred for both checks but
> > what I am not sure about yet is whether its actually safe to use the subject
> > sid when the task isn't current.
> >
> > ie. I am still trying to determine if there is a race here between the transaction
> > request and the permission check.
>
> Okay, I see what you are concerned about now ... and unfortunately I'm
> not seeing a lot of precedence in the kernel for this type of usage
> either; the closest I can find is something like task_lock(), but that
> doesn't seem to cover the subjective creds.  In fact, looking at
> override_creds(), there is nothing preventing a task from changing
> it's subjective creds at any point in time.
>
> Beyond the task_sid_subj() code here, looking back at patch 1 and the
> use of security_task_getsecid_subj() we look to be mostly safe (where
> safe means we are only inspecting the current task) with the exception
> of the binder code once again.  There are some other exceptions but
> they are in the ptrace and audit code, both of which should be okay
> given the nature and calling context of the code.
>
> The problem really does seem to be just binder, and as I look at
> binder userspace example code, I'm starting to wonder if binder is
> setup properly to operate sanely in a situation where a process
> overrides its subject creds.  It may be that we always need to use the
> objective/real creds with binder.  Jeff, any binder insight here you
> can share with us?
>
> > > +     u32 tosid = task_sid_subj(to);
> > its not clear to me that using the subj for to is correct
>
> Yes, I believe you are correct.  Jeff, I know you looked at this code
> already, but I'm guessing you may have missed this (just as I did when
> I wrote it); are you okay with changing 'tosid' in
> selinux_binder_transaction() to the task's objective credentials?

Hearing no comments from the Android/binder folks, I'm in the process
of switching this patchset to always use the objective creds in the
case of binder.  It's safe and I'm not sure binder is really prepared
for the idea of a task changing it's creds anyway.

Once the kernel builds and passes some basic sanity checks I'll repost
the patches for review and inclusion, minus the AppArmor patch.

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 39+ messages in thread

end of thread, other threads:[~2021-03-17 22:57 UTC | newest]

Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-19 23:28 [RFC PATCH 0/4] Split security_task_getsecid() into subj and obj variants Paul Moore
2021-02-19 23:29 ` [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants Paul Moore
2021-02-20  2:55   ` James Morris
2021-02-20 14:44     ` Paul Moore
2021-03-04 10:04       ` Jeffrey Vander Stoep
2021-03-04 23:43         ` Paul Moore
2021-03-10  8:21           ` Jeffrey Vander Stoep
2021-03-11  1:56             ` Paul Moore
2021-02-21 12:51   ` John Johansen
2021-02-21 22:09     ` Paul Moore
2021-03-04  0:44     ` Paul Moore
2021-03-10  0:28       ` Paul Moore
2021-03-10  3:09         ` John Johansen
2021-02-24 16:49   ` Mimi Zohar
2021-03-08 19:25   ` Richard Guy Briggs
2021-03-10  0:23     ` Paul Moore
2021-03-10  1:03   ` John Johansen
2021-03-11  1:55     ` Paul Moore
2021-02-19 23:29 ` [RFC PATCH 2/4] selinux: clarify task subjective and objective credentials Paul Moore
2021-02-21 12:55   ` John Johansen
2021-03-08 19:26   ` Richard Guy Briggs
2021-03-10  3:05   ` John Johansen
2021-03-11  4:32     ` Paul Moore
2021-03-17 22:56       ` Paul Moore
2021-02-19 23:29 ` [RFC PATCH 3/4] smack: differentiate between subjective and objective task credentials Paul Moore
2021-02-21 12:56   ` John Johansen
2021-03-08 19:26   ` Richard Guy Briggs
2021-03-10  1:04   ` John Johansen
2021-02-19 23:29 ` [RFC PATCH 4/4] apparmor: " Paul Moore
2021-02-21 12:57   ` John Johansen
2021-02-21 22:12     ` Paul Moore
2021-02-20  1:49 ` [RFC PATCH 0/4] Split security_task_getsecid() into subj and obj variants Casey Schaufler
2021-02-20 14:41   ` Paul Moore
2021-02-22 23:58     ` Casey Schaufler
2021-02-23 14:14       ` Mimi Zohar
2021-02-24  0:03         ` Paul Moore
2021-03-04  0:46       ` Paul Moore
2021-03-04  2:21         ` Casey Schaufler
2021-03-04 23:41           ` Paul Moore

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).