[PATCH 0/2] smack: fix incorrect task context in smack_msg_queue_msgrcv

[PATCH 0/2] smack: fix incorrect task context in smack_msg_queue_msgrcv

[Konstantin Andreev]

The 1st patch in the set is the fix itself,
the 2nd is a followup to resolve an ambiguity in the audit log.

The patch set applies on top of:
https://github.com/cschaufler/smack-next/commits/next
commit b78fede1c69a

Konstantin Andreev (2):
  smack: fix incorrect task context in smack_msg_queue_msgrcv
  smack: show msgrcv() subject task in audit

 security/smack/smack.h        |  1 +
 security/smack/smack_access.c |  9 +++++
 security/smack/smack_lsm.c    | 67 +++++++++++++++++++++++++----------
 3 files changed, 59 insertions(+), 18 deletions(-)

-- 
2.47.3

[PATCH 1/2] smack: fix incorrect task context in smack_msg_queue_msgrcv

[Konstantin Andreev]

The smack_msg_queue_msgrcv() function incorrectly checks
the permissions of the 'current' task instead of the
'target' task.

In the msgsnd() syscall path, if a receiver is already waiting,
the pipelined_send() optimization is used to push the message
directly to the receiver task:

    ipc/msg.c`pipelined_send():
    ` smp_store_release(&msr->r_msg, msg)

In this case, the 'sender' (current) task performs the check
on behalf of the 'receiver' task (msr->r_tsk, passed as the
'target' parameter):

  ipc/msg.c`pipelined_send():
  ` security_msg_queue_msgrcv(,, target := msr->r_tsk,,)

However, smack_msg_queue_msgrcv() ignores the 'target' and
checks 'current':

  smack_msg_queue_msgrcv(…)
  ` smk_curacc_msq(isp, MAY_READWRITE); // current task

'current' MAY satisfy smack_msg_queue_msgrcv r/w requirement,
but 'target' (the receiver task) might NOT;
as a result, an unauthorized receiver gets the message,
violating MAC policy.

Test:
1) create a sysv message queue with label “foo”
2) echo "bar foo r" >/smack/load2
3) msgrcv(,,,0,MSG_NOERROR) in "bar"-labeled task.
    The task is waiting for the messages ...
4) msgsnd() from a "foo"-labeled task:
"bar"-labeled task gets the message.

This patch fixes the issue by checking permission on the
'target' task instead of 'current'.

(2008-02-04, Casey Schaufler)
Fixes: e114e473771c ("Smack: Simplified Mandatory Access Control Kernel")

Signed-off-by: Konstantin Andreev <andreev@swemel.ru>
---
 security/smack/smack_lsm.c | 65 +++++++++++++++++++++++++++-----------
 1 file changed, 47 insertions(+), 18 deletions(-)

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index f4ef840b203e..3146fa83c2f1 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -130,12 +130,13 @@ static int smk_bu_note(char *note, struct smack_known *sskp,
 #define smk_bu_note(note, sskp, oskp, mode, RC) (RC)
 #endif
 
-#ifdef CONFIG_SECURITY_SMACK_BRINGUP
-static int smk_bu_current(char *note, struct smack_known *oskp,
-			  int mode, int rc)
+static int
+smk_bu_tsk_to_obj(struct task_struct *tsk, const struct task_smack *tsp,
+		  char *note, struct smack_known *oskp, int mode, int rc)
 {
-	struct task_smack *tsp = smack_cred(current_cred());
+#ifdef CONFIG_SECURITY_SMACK_BRINGUP
 	char acc[SMK_NUM_ACCESS_TYPE + 1];
+	char comm[TASK_COMM_LEN];
 
 	if (rc <= 0)
 		return rc;
@@ -143,14 +144,22 @@ static int smk_bu_current(char *note, struct smack_known *oskp,
 		rc = 0;
 
 	smk_bu_mode(mode, acc);
+
 	pr_info("Smack %s: (%s %s %s) %s %s\n", smk_bu_mess[rc],
-		tsp->smk_task->smk_known, oskp->smk_known,
-		acc, current->comm, note);
+		smk_of_task(tsp)->smk_known, oskp->smk_known,
+		acc, get_task_comm(comm, tsk), note);
 	return 0;
-}
 #else
-#define smk_bu_current(note, oskp, mode, RC) (RC)
+	return rc;
 #endif
+}
+
+static int smk_bu_current(char *note, struct smack_known *oskp,
+			  int mode, int rc)
+{
+	return smk_bu_tsk_to_obj(current, smack_cred(current_cred()),
+				 note, oskp, mode, rc);
+}
 
 #ifdef CONFIG_SECURITY_SMACK_BRINGUP
 static int smk_bu_task(struct task_struct *otp, int mode, int rc)
@@ -3353,14 +3362,20 @@ static int smack_sem_semop(struct kern_ipc_perm *isp, struct sembuf *sops,
 }
 
 /**
- * smk_curacc_msq : helper to check if current has access on msq
- * @isp : the msq
+ * smk_tskacc_msq : helper to check if tsk has access on msq
+ * @tsk: the task that requests access
+ * @isp : the sysv msg queue permissions
  * @access : access requested
  *
- * return 0 if current has access, error otherwise
+ * return 0 if tsk has access, error otherwise
  */
-static int smk_curacc_msq(struct kern_ipc_perm *isp, int access)
+static int
+smk_tskacc_msq(struct task_struct *tsk, struct kern_ipc_perm *isp, int access)
 {
+	const bool tsk_is_current = (tsk == current);
+	const struct cred * const tsk_cred =
+			(tsk_is_current ? current_cred() : get_task_cred(tsk));
+	struct task_smack * const tsp = smack_cred(tsk_cred);
 	struct smack_known *msp = smack_of_ipc(isp);
 	struct smk_audit_info ad;
 	int rc;
@@ -3369,11 +3384,25 @@ static int smk_curacc_msq(struct kern_ipc_perm *isp, int access)
 	smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_IPC);
 	ad.a.u.ipc_id = isp->id;
 #endif
-	rc = smk_curacc(msp, access, &ad);
-	rc = smk_bu_current("msq", msp, access, rc);
+	rc = smk_tskacc(tsp, msp, access, &ad);
+	rc = smk_bu_tsk_to_obj(tsk, tsp, "msq", msp, access, rc);
+	if (!tsk_is_current)
+		put_cred(tsk_cred);
 	return rc;
 }
 
+/**
+ * smk_curacc_msq : helper to check if current has access on msq
+ * @isp : the sysv msg queue permissions
+ * @access : access requested
+ *
+ * return 0 if current has access, error otherwise
+ */
+static int smk_curacc_msq(struct kern_ipc_perm *isp, int access)
+{
+	return smk_tskacc_msq(current, isp, access);
+}
+
 /**
  * smack_msg_queue_associate - Smack access check for msg_queue
  * @isp: the object
@@ -3441,21 +3470,21 @@ static int smack_msg_queue_msgsnd(struct kern_ipc_perm *isp, struct msg_msg *msg
 }
 
 /**
- * smack_msg_queue_msgrcv - Smack access check for msg_queue
+ * smack_msg_queue_msgrcv - check it target has r/w access to msg_queue
  * @isp: the object
  * @msg: unused
- * @target: unused
+ * @target: the task that msgrcv() from the queue
  * @type: unused
  * @mode: unused
  *
- * Returns 0 if current has read and write access, error code otherwise
+ * Returns 0 if target has read and write access, error code otherwise
  */
 static int smack_msg_queue_msgrcv(struct kern_ipc_perm *isp,
 				  struct msg_msg *msg,
 				  struct task_struct *target, long type,
 				  int mode)
 {
-	return smk_curacc_msq(isp, MAY_READWRITE);
+	return smk_tskacc_msq(target, isp, MAY_READWRITE);
 }
 
 /**
-- 
2.47.3

[PATCH 2/2] smack: show msgrcv() subject task in audit

[Konstantin Andreev]

When a task msgrcv()'es some message the SMACK audit log message
looks like:

  fn=smk_tskacc_msq action=denied subject="bar" object="foo" requested=rw
	pid=456 comm="mrcv" ipc_key=2
  fn=smk_tskacc_msq action=granted subject="bar" object="foo" requested=rw
	pid=519 comm="mrcv" ipc_key=2

where pid= is a pid of a “current” task which calls smk_tskacc_msq().
Usually, the caller of smk_tskacc_msq() is also a subject task
which determines its own permission. In the example above
the 'mrcv' process has label 'bar' and wants "rw" for label "foo".

However, when sender task delivers message using

  ipc/msg.c`pipelined_send():
  ` security_msg_queue_msgrcv(,, msr->r_tsk,,)
  ` smp_store_release(&msr->r_msg, msg)

“current” task and “subject” task differ, and
the “subject” task is missed from the audit message.

This patch adds two fields, subj_pid and subj_comm,
into the audit message:

  fn=smk_tskacc_msq action=granted subject="bar" object="foo" requested=rw
	subj_pid=564 subj_comm="mrcv" pid=577 comm="msnd" ipc_key=2

Signed-off-by: Konstantin Andreev <andreev@swemel.ru>
---
 security/smack/smack.h        | 1 +
 security/smack/smack_access.c | 9 +++++++++
 security/smack/smack_lsm.c    | 2 ++
 3 files changed, 12 insertions(+)

diff --git a/security/smack/smack.h b/security/smack/smack.h
index 9b9eb262fe33..551fcf2a1832 100644
--- a/security/smack/smack.h
+++ b/security/smack/smack.h
@@ -261,6 +261,7 @@ struct smack_audit_data {
 	char *subject;
 	char *object;
 	char *request;
+	struct task_struct *subj_tsk;
 	int result;
 };
 
diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c
index 350b88d582b3..fb85356266e5 100644
--- a/security/smack/smack_access.c
+++ b/security/smack/smack_access.c
@@ -331,6 +331,15 @@ static void smack_log_callback(struct audit_buffer *ab, void *a)
 		audit_log_format(ab, " labels_differ");
 	else
 		audit_log_format(ab, " requested=%s", sad->request);
+
+	if (sad->subj_tsk) {
+		char comm[TASK_COMM_LEN];
+
+		audit_log_format(ab, " subj_pid=%d subj_comm=",
+			task_tgid_nr(sad->subj_tsk));
+		audit_log_untrustedstring(ab,
+			get_task_comm(comm, sad->subj_tsk));
+	}
 }
 
 /**
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 3146fa83c2f1..6f6ff9b20981 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3383,6 +3383,8 @@ smk_tskacc_msq(struct task_struct *tsk, struct kern_ipc_perm *isp, int access)
 #ifdef CONFIG_AUDIT
 	smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_IPC);
 	ad.a.u.ipc_id = isp->id;
+	if (!tsk_is_current)
+		ad.sad.subj_tsk = tsk;
 #endif
 	rc = smk_tskacc(tsp, msp, access, &ad);
 	rc = smk_bu_tsk_to_obj(tsk, tsp, "msq", msp, access, rc);
-- 
2.47.3