Re: [PATCH] lsm: security_task_getsecid_subj() -> security_current_getsecid_subj()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 11/19/2021 2:52 PM, Paul Moore wrote:
On Wed, Sep 29, 2021 at 3:17 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
The security_task_getsecid_subj() LSM hook invites misuse by allowing
callers to specify a task even though the hook is only safe when the
current task is referenced.  Fix this by removing the task_struct
argument to the hook, requiring LSM implementations to use the
current task.  While we are changing the hook declaration we also
rename the function to security_current_getsecid_subj() in an effort
to reinforce that the hook captures the subjective credentials of the
current task and not an arbitrary task on the system.

Signed-off-by: Paul Moore <paul@xxxxxxxxxxxxxx>
---
  include/linux/lsm_hook_defs.h         |    3 +--
  include/linux/lsm_hooks.h             |    8 +++-----
  include/linux/security.h              |    4 ++--
  kernel/audit.c                        |    4 ++--
  kernel/auditfilter.c                  |    3 +--
  kernel/auditsc.c                      |   10 +++++++++-
  net/netlabel/netlabel_unlabeled.c     |    2 +-
  net/netlabel/netlabel_user.h          |    2 +-
  security/apparmor/lsm.c               |   13 ++++++++++---
  security/integrity/ima/ima_appraise.c |    2 +-
  security/integrity/ima/ima_main.c     |   14 +++++++-------
  security/security.c                   |    6 +++---
  security/selinux/hooks.c              |   19 +++----------------
  security/smack/smack.h                |   16 ----------------
  security/smack/smack_lsm.c            |    9 ++++-----
  15 files changed, 48 insertions(+), 67 deletions(-)
I never saw any comments, positive or negative, on this patch so I'll
plan on merging it early next week.  If you've got objections, now is
the time to speak up.

It's OK by me.

Reviewed-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>


diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 2adeea44c0d5..8d04b18155a3 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -205,8 +205,7 @@ 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_subj,
-        struct task_struct *p, u32 *secid)
+LSM_HOOK(void, LSM_RET_VOID, current_getsecid_subj, 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)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 5c4c5c0602cb..9f5b93011ecb 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -716,11 +716,9 @@
   *     @p.
   *     @p contains the task_struct for the process.
   *     Return 0 if permission is granted.
- * @task_getsecid_subj:
- *     Retrieve the subjective security identifier of the task_struct in @p
- *     and return it in @secid.  Special care must be taken to ensure that @p
- *     is the either the "current" task, or the caller has exclusive access
- *     to @p.
+ * @current_getsecid_subj:
+ *     Retrieve the subjective security identifier of the current task and
+ *     return it in @secid.
   *     In case of failure, @secid will be set to zero.
   * @task_getsecid_obj:
   *     Retrieve the objective security identifier of the task_struct in @p
diff --git a/include/linux/security.h b/include/linux/security.h
index 5b7288521300..24190cf61858 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -417,7 +417,7 @@ 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_subj(struct task_struct *p, u32 *secid);
+void security_current_getsecid_subj(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);
@@ -1112,7 +1112,7 @@ static inline int security_task_getsid(struct task_struct *p)
         return 0;
  }

-static inline void security_task_getsecid_subj(struct task_struct *p, u32 *secid)
+static inline void security_current_getsecid_subj(u32 *secid)
  {
         *secid = 0;
  }
diff --git a/kernel/audit.c b/kernel/audit.c
index 121d37e700a6..d4084751cfe6 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_subj(current, &sid);
+       security_current_getsecid_subj(&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_subj(current, &audit_sig_sid);
+               security_current_getsecid_subj(&audit_sig_sid);
         }

         return audit_signal_info_syscall(t);
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index db2c6b59dfc3..f4ebd2f1cfca 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -1359,8 +1359,7 @@ int audit_filter(int msgtype, unsigned int listtype)
                         case AUDIT_SUBJ_SEN:
                         case AUDIT_SUBJ_CLR:
                                 if (f->lsm_rule) {
-                                       security_task_getsecid_subj(current,
-                                                                   &sid);
+                                       security_current_getsecid_subj(&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 8dd73a64f921..e0987ee71b8f 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -673,7 +673,15 @@ static int audit_filter_rules(struct task_struct *tsk,
                            logged upon error */
                         if (f->lsm_rule) {
                                 if (need_sid) {
-                                       security_task_getsecid_subj(tsk, &sid);
+                                       /* @tsk should always be equal to
+                                        * @current with the exception of
+                                        * fork()/copy_process() in which case
+                                        * the new @tsk creds are still a dup
+                                        * of @current's creds so we can still
+                                        * use security_current_getsecid_subj()
+                                        * here even though it always refs
+                                        * @current's creds */
+                                       security_current_getsecid_subj(&sid);
                                         need_sid = 0;
                                 }
                                 result = security_audit_rule_match(sid, f->type,
diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
index 566ba4397ee4..8490e46359ae 100644
--- a/net/netlabel/netlabel_unlabeled.c
+++ b/net/netlabel/netlabel_unlabeled.c
@@ -1537,7 +1537,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_subj(current, &audit_info.secid);
+       security_current_getsecid_subj(&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 6190cbf94bf0..d6c5b31eb4eb 100644
--- a/net/netlabel/netlabel_user.h
+++ b/net/netlabel/netlabel_user.h
@@ -32,7 +32,7 @@
   */
  static inline void netlbl_netlink_auditinfo(struct netlbl_audit *audit_info)
  {
-       security_task_getsecid_subj(current, &audit_info->secid);
+       security_current_getsecid_subj(&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 f72406fe1bf2..fc21190c4f4f 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -728,7 +728,14 @@ 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_current_getsecid_subj(u32 *secid)
+{
+       struct aa_label *label = aa_get_task_label(current);
+       *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(p);
         *secid = label->secid;
@@ -1252,8 +1259,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(current_getsecid_subj, apparmor_current_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/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index dbba51583e7c..17232bbfb9f9 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -76,7 +76,7 @@ int ima_must_appraise(struct user_namespace *mnt_userns, struct inode *inode,
         if (!ima_appraise)
                 return 0;

-       security_task_getsecid_subj(current, &secid);
+       security_current_getsecid_subj(&secid);
         return ima_match_policy(mnt_userns, inode, current_cred(), secid,
                                 func, mask, IMA_APPRAISE | IMA_HASH, NULL,
                                 NULL, NULL, NULL);
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 465865412100..8c6e4514d494 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -408,7 +408,7 @@ int ima_file_mmap(struct file *file, unsigned long prot)
         u32 secid;

         if (file && (prot & PROT_EXEC)) {
-               security_task_getsecid_subj(current, &secid);
+               security_current_getsecid_subj(&secid);
                 return process_measurement(file, current_cred(), secid, NULL,
                                            0, MAY_EXEC, MMAP_CHECK);
         }
@@ -446,7 +446,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_subj(current, &secid);
+       security_current_getsecid_subj(&secid);
         inode = file_inode(vma->vm_file);
         action = ima_get_action(file_mnt_user_ns(vma->vm_file), inode,
                                 current_cred(), secid, MAY_EXEC, MMAP_CHECK,
@@ -487,7 +487,7 @@ int ima_bprm_check(struct linux_binprm *bprm)
         int ret;
         u32 secid;

-       security_task_getsecid_subj(current, &secid);
+       security_current_getsecid_subj(&secid);
         ret = process_measurement(bprm->file, current_cred(), secid, NULL, 0,
                                   MAY_EXEC, BPRM_CHECK);
         if (ret)
@@ -512,7 +512,7 @@ int ima_file_check(struct file *file, int mask)
  {
         u32 secid;

-       security_task_getsecid_subj(current, &secid);
+       security_current_getsecid_subj(&secid);
         return process_measurement(file, current_cred(), secid, NULL, 0,
                                    mask & (MAY_READ | MAY_WRITE | MAY_EXEC |
                                            MAY_APPEND), FILE_CHECK);
@@ -709,7 +709,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_subj(current, &secid);
+       security_current_getsecid_subj(&secid);
         return process_measurement(file, current_cred(), secid, NULL,
                                    0, MAY_READ, func);
  }
@@ -752,7 +752,7 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
         }

         func = read_idmap[read_id] ?: FILE_CHECK;
-       security_task_getsecid_subj(current, &secid);
+       security_current_getsecid_subj(&secid);
         return process_measurement(file, current_cred(), secid, buf, size,
                                    MAY_READ, func);
  }
@@ -905,7 +905,7 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
          * buffer measurements.
          */
         if (func) {
-               security_task_getsecid_subj(current, &secid);
+               security_current_getsecid_subj(&secid);
                 action = ima_get_action(mnt_userns, inode, current_cred(),
                                         secid, 0, func, &pcr, &template,
                                         func_data, NULL);
diff --git a/security/security.c b/security/security.c
index 9ffa9e9c5c55..827bd161f8b0 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1807,12 +1807,12 @@ int security_task_getsid(struct task_struct *p)
         return call_int_hook(task_getsid, 0, p);
  }

-void security_task_getsecid_subj(struct task_struct *p, u32 *secid)
+void security_current_getsecid_subj(u32 *secid)
  {
         *secid = 0;
-       call_void_hook(task_getsecid_subj, p, secid);
+       call_void_hook(current_getsecid_subj, secid);
  }
-EXPORT_SYMBOL(security_task_getsecid_subj);
+EXPORT_SYMBOL(security_current_getsecid_subj);

  void security_task_getsecid_obj(struct task_struct *p, u32 *secid)
  {
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index e7ebd45ca345..5c766b901f66 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -229,19 +229,6 @@ 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
   */
@@ -4222,9 +4209,9 @@ static int selinux_task_getsid(struct task_struct *p)
                             PROCESS__GETSESSION, NULL);
  }

-static void selinux_task_getsecid_subj(struct task_struct *p, u32 *secid)
+static void selinux_current_getsecid_subj(u32 *secid)
  {
-       *secid = task_sid_subj(p);
+       *secid = current_sid();
  }

  static void selinux_task_getsecid_obj(struct task_struct *p, u32 *secid)
@@ -7221,7 +7208,7 @@ 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_subj),
+       LSM_HOOK_INIT(current_getsecid_subj, selinux_current_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),
diff --git a/security/smack/smack.h b/security/smack/smack.h
index 99c3422596ab..fc837dcebf96 100644
--- a/security/smack/smack.h
+++ b/security/smack/smack.h
@@ -389,22 +389,6 @@ 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_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)
  {
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 21a0e7c3b8de..40e1d764e616 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2061,15 +2061,14 @@ static int smack_task_getsid(struct task_struct *p)
  }

  /**
- * smack_task_getsecid_subj - get the subjective secid of the task
- * @p: the task
+ * smack_current_getsecid_subj - get the subjective secid of the current task
   * @secid: where to put the result
   *
   * 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)
+static void smack_current_getsecid_subj(u32 *secid)
  {
-       struct smack_known *skp = smk_of_task_struct_subj(p);
+       struct smack_known *skp = smk_of_current();

         *secid = skp->smk_secid;
  }
@@ -4756,7 +4755,7 @@ 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_subj),
+       LSM_HOOK_INIT(current_getsecid_subj, smack_current_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),





[Index of Archives]     [Selinux Refpolicy]     [Linux SGX]     [Fedora Users]     [Fedora Desktop]     [Yosemite Photos]     [Yosemite Camping]     [Yosemite Campsites]     [KDE Users]     [Gnome Users]

  Powered by Linux