[PATCH V3 1/2] security: Add a cred_getsecid hook

[PATCH V3 1/2] security: Add a cred_getsecid hook

[Matthew Garrett]

For IMA purposes, we want to be able to obtain the prepared secid in the
bprm structure before the credentials are committed. Add a cred_getsecid
hook that makes this possible.

Signed-off-by: Matthew Garrett <mjg59@google.com>
Acked-by: Paul Moore <paul@paul-moore.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Stephen Smalley <sds@tycho.nsa.gov>
Cc: Eric Paris <eparis@parisplace.org>
Cc: selinux@tycho.nsa.gov
Cc: Casey Schaufler <casey@schaufler-ca.com>
Cc: linux-security-module@vger.kernel.org
Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
Cc: linux-integrity@vger.kernel.org
---
 V3: Fix smack_cred_getsecid()

 include/linux/lsm_hooks.h  |  6 ++++++
 include/linux/security.h   |  1 +
 security/security.c        |  7 +++++++
 security/selinux/hooks.c   |  8 ++++++++
 security/smack/smack_lsm.c | 18 ++++++++++++++++++
 5 files changed, 40 insertions(+)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index c9258124e417..c28c6f8b65dc 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -554,6 +554,10 @@
  *	@new points to the new credentials.
  *	@old points to the original credentials.
  *	Transfer data from original creds to new creds
+ * @cred_getsecid:
+ *	Retrieve the security identifier of the cred structure @c
+ *	@c contains the credentials, secid will be placed into @secid.
+ *	In case of failure, @secid will be set to zero.
  * @kernel_act_as:
  *	Set the credentials for a kernel service to act as (subjective context).
  *	@new points to the credentials to be modified.
@@ -1507,6 +1511,7 @@ union security_list_options {
 	int (*cred_prepare)(struct cred *new, const struct cred *old,
 				gfp_t gfp);
 	void (*cred_transfer)(struct cred *new, const struct cred *old);
+	void (*cred_getsecid)(const struct cred *c, u32 *secid);
 	int (*kernel_act_as)(struct cred *new, u32 secid);
 	int (*kernel_create_files_as)(struct cred *new, struct inode *inode);
 	int (*kernel_module_request)(char *kmod_name);
@@ -1779,6 +1784,7 @@ struct security_hook_heads {
 	struct list_head cred_free;
 	struct list_head cred_prepare;
 	struct list_head cred_transfer;
+	struct list_head cred_getsecid;
 	struct list_head kernel_act_as;
 	struct list_head kernel_create_files_as;
 	struct list_head kernel_read_file;
diff --git a/include/linux/security.h b/include/linux/security.h
index ce6265960d6c..14848fef8f62 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -324,6 +324,7 @@ int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
 void security_cred_free(struct cred *cred);
 int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp);
 void security_transfer_creds(struct cred *new, const struct cred *old);
+void security_cred_getsecid(const struct cred *c, u32 *secid);
 int security_kernel_act_as(struct cred *new, u32 secid);
 int security_kernel_create_files_as(struct cred *new, struct inode *inode);
 int security_kernel_module_request(char *kmod_name);
diff --git a/security/security.c b/security/security.c
index 4bf0f571b4ef..02d217597400 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1004,6 +1004,13 @@ void security_transfer_creds(struct cred *new, const struct cred *old)
 	call_void_hook(cred_transfer, new, old);
 }
 
+void security_cred_getsecid(const struct cred *c, u32 *secid)
+{
+	*secid = 0;
+	call_void_hook(cred_getsecid, c, secid);
+}
+EXPORT_SYMBOL(security_cred_getsecid);
+
 int security_kernel_act_as(struct cred *new, u32 secid)
 {
 	return call_int_hook(kernel_act_as, 0, new, secid);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f5d304736852..1d11679674a6 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3836,6 +3836,13 @@ static void selinux_cred_transfer(struct cred *new, const struct cred *old)
 	*tsec = *old_tsec;
 }
 
+static void selinux_cred_getsecid(const struct cred *c, u32 *secid)
+{
+	rcu_read_lock();
+	*secid = cred_sid(c);
+	rcu_read_unlock();
+}
+
 /*
  * set the security data for a kernel service
  * - all the creation contexts are set to unlabelled
@@ -6338,6 +6345,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(cred_free, selinux_cred_free),
 	LSM_HOOK_INIT(cred_prepare, selinux_cred_prepare),
 	LSM_HOOK_INIT(cred_transfer, selinux_cred_transfer),
+	LSM_HOOK_INIT(cred_getsecid, selinux_cred_getsecid),
 	LSM_HOOK_INIT(kernel_act_as, selinux_kernel_act_as),
 	LSM_HOOK_INIT(kernel_create_files_as, selinux_kernel_create_files_as),
 	LSM_HOOK_INIT(kernel_module_request, selinux_kernel_module_request),
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 286171a16ed2..37c35aaa6955 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2049,6 +2049,23 @@ static void smack_cred_transfer(struct cred *new, const struct cred *old)
 	/* cbs copy rule list */
 }
 
+/**
+ * smack_cred_getsecid - get the secid corresponding to a creds structure
+ * @c: the object creds
+ * @secid: where to put the result
+ *
+ * Sets the secid to contain a u32 version of the smack label.
+ */
+static void smack_cred_getsecid(const struct cred *c, u32 *secid)
+{
+	struct smack_known *skp;
+
+	rcu_read_lock();
+	skp = smk_of_task(c->security);
+	*secid = skp->smk_secid;
+	rcu_read_unlock();
+}
+
 /**
  * smack_kernel_act_as - Set the subjective context in a set of credentials
  * @new: points to the set of credentials to be modified.
@@ -4651,6 +4668,7 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(cred_free, smack_cred_free),
 	LSM_HOOK_INIT(cred_prepare, smack_cred_prepare),
 	LSM_HOOK_INIT(cred_transfer, smack_cred_transfer),
+	LSM_HOOK_INIT(cred_getsecid, smack_cred_getsecid),
 	LSM_HOOK_INIT(kernel_act_as, smack_kernel_act_as),
 	LSM_HOOK_INIT(kernel_create_files_as, smack_kernel_create_files_as),
 	LSM_HOOK_INIT(task_setpgid, smack_task_setpgid),
-- 
2.15.0.rc2.357.g7e34df9404-goog

[PATCH V3 1/2] security: Add a cred_getsecid hook

[Matthew Garrett]

For IMA purposes, we want to be able to obtain the prepared secid in the
bprm structure before the credentials are committed. Add a cred_getsecid
hook that makes this possible.

Signed-off-by: Matthew Garrett <mjg59@google.com>
Acked-by: Paul Moore <paul@paul-moore.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Stephen Smalley <sds@tycho.nsa.gov>
Cc: Eric Paris <eparis@parisplace.org>
Cc: selinux at tycho.nsa.gov
Cc: Casey Schaufler <casey@schaufler-ca.com>
Cc: linux-security-module at vger.kernel.org
Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
Cc: linux-integrity at vger.kernel.org
---
 V3: Fix smack_cred_getsecid()

 include/linux/lsm_hooks.h  |  6 ++++++
 include/linux/security.h   |  1 +
 security/security.c        |  7 +++++++
 security/selinux/hooks.c   |  8 ++++++++
 security/smack/smack_lsm.c | 18 ++++++++++++++++++
 5 files changed, 40 insertions(+)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index c9258124e417..c28c6f8b65dc 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -554,6 +554,10 @@
  *	@new points to the new credentials.
  *	@old points to the original credentials.
  *	Transfer data from original creds to new creds
+ * @cred_getsecid:
+ *	Retrieve the security identifier of the cred structure @c
+ *	@c contains the credentials, secid will be placed into @secid.
+ *	In case of failure, @secid will be set to zero.
  * @kernel_act_as:
  *	Set the credentials for a kernel service to act as (subjective context).
  *	@new points to the credentials to be modified.
@@ -1507,6 +1511,7 @@ union security_list_options {
 	int (*cred_prepare)(struct cred *new, const struct cred *old,
 				gfp_t gfp);
 	void (*cred_transfer)(struct cred *new, const struct cred *old);
+	void (*cred_getsecid)(const struct cred *c, u32 *secid);
 	int (*kernel_act_as)(struct cred *new, u32 secid);
 	int (*kernel_create_files_as)(struct cred *new, struct inode *inode);
 	int (*kernel_module_request)(char *kmod_name);
@@ -1779,6 +1784,7 @@ struct security_hook_heads {
 	struct list_head cred_free;
 	struct list_head cred_prepare;
 	struct list_head cred_transfer;
+	struct list_head cred_getsecid;
 	struct list_head kernel_act_as;
 	struct list_head kernel_create_files_as;
 	struct list_head kernel_read_file;
diff --git a/include/linux/security.h b/include/linux/security.h
index ce6265960d6c..14848fef8f62 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -324,6 +324,7 @@ int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
 void security_cred_free(struct cred *cred);
 int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp);
 void security_transfer_creds(struct cred *new, const struct cred *old);
+void security_cred_getsecid(const struct cred *c, u32 *secid);
 int security_kernel_act_as(struct cred *new, u32 secid);
 int security_kernel_create_files_as(struct cred *new, struct inode *inode);
 int security_kernel_module_request(char *kmod_name);
diff --git a/security/security.c b/security/security.c
index 4bf0f571b4ef..02d217597400 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1004,6 +1004,13 @@ void security_transfer_creds(struct cred *new, const struct cred *old)
 	call_void_hook(cred_transfer, new, old);
 }
 
+void security_cred_getsecid(const struct cred *c, u32 *secid)
+{
+	*secid = 0;
+	call_void_hook(cred_getsecid, c, secid);
+}
+EXPORT_SYMBOL(security_cred_getsecid);
+
 int security_kernel_act_as(struct cred *new, u32 secid)
 {
 	return call_int_hook(kernel_act_as, 0, new, secid);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f5d304736852..1d11679674a6 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3836,6 +3836,13 @@ static void selinux_cred_transfer(struct cred *new, const struct cred *old)
 	*tsec = *old_tsec;
 }
 
+static void selinux_cred_getsecid(const struct cred *c, u32 *secid)
+{
+	rcu_read_lock();
+	*secid = cred_sid(c);
+	rcu_read_unlock();
+}
+
 /*
  * set the security data for a kernel service
  * - all the creation contexts are set to unlabelled
@@ -6338,6 +6345,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(cred_free, selinux_cred_free),
 	LSM_HOOK_INIT(cred_prepare, selinux_cred_prepare),
 	LSM_HOOK_INIT(cred_transfer, selinux_cred_transfer),
+	LSM_HOOK_INIT(cred_getsecid, selinux_cred_getsecid),
 	LSM_HOOK_INIT(kernel_act_as, selinux_kernel_act_as),
 	LSM_HOOK_INIT(kernel_create_files_as, selinux_kernel_create_files_as),
 	LSM_HOOK_INIT(kernel_module_request, selinux_kernel_module_request),
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 286171a16ed2..37c35aaa6955 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2049,6 +2049,23 @@ static void smack_cred_transfer(struct cred *new, const struct cred *old)
 	/* cbs copy rule list */
 }
 
+/**
+ * smack_cred_getsecid - get the secid corresponding to a creds structure
+ * @c: the object creds
+ * @secid: where to put the result
+ *
+ * Sets the secid to contain a u32 version of the smack label.
+ */
+static void smack_cred_getsecid(const struct cred *c, u32 *secid)
+{
+	struct smack_known *skp;
+
+	rcu_read_lock();
+	skp = smk_of_task(c->security);
+	*secid = skp->smk_secid;
+	rcu_read_unlock();
+}
+
 /**
  * smack_kernel_act_as - Set the subjective context in a set of credentials
  * @new: points to the set of credentials to be modified.
@@ -4651,6 +4668,7 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(cred_free, smack_cred_free),
 	LSM_HOOK_INIT(cred_prepare, smack_cred_prepare),
 	LSM_HOOK_INIT(cred_transfer, smack_cred_transfer),
+	LSM_HOOK_INIT(cred_getsecid, smack_cred_getsecid),
 	LSM_HOOK_INIT(kernel_act_as, smack_kernel_act_as),
 	LSM_HOOK_INIT(kernel_create_files_as, smack_kernel_create_files_as),
 	LSM_HOOK_INIT(task_setpgid, smack_task_setpgid),
-- 
2.15.0.rc2.357.g7e34df9404-goog

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V3 2/2] IMA: Support using new creds in appraisal policy

[Matthew Garrett]

The existing BPRM_CHECK functionality in IMA validates against the
credentials of the existing process, not any new credentials that the
child process may transition to. Add an additional CREDS_CHECK target
and refactor IMA to pass the appropriate creds structure. In
ima_bprm_check(), check with both the existing process credentials and
the credentials that will be committed when the new process is started.
This will not change behaviour unless the system policy is extended to
include CREDS_CHECK targets - BPRM_CHECK will continue to check the same
credentials that it did previously.

Signed-off-by: Matthew Garrett <mjg59@google.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Stephen Smalley <sds@tycho.nsa.gov>
Cc: Eric Paris <eparis@parisplace.org>
Cc: selinux@tycho.nsa.gov
Cc: Casey Schaufler <casey@schaufler-ca.com>
Cc: linux-security-module@vger.kernel.org
Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
Cc: linux-integrity@vger.kernel.org
---
 V3: Update description to make it clear that this doesn't alter the behaviour
 of existing policies

 Documentation/ABI/testing/ima_policy  |  2 +-
 security/integrity/iint.c             |  1 +
 security/integrity/ima/ima.h          |  7 ++++---
 security/integrity/ima/ima_api.c      |  8 +++++---
 security/integrity/ima/ima_appraise.c | 10 +++++++++-
 security/integrity/ima/ima_main.c     | 26 +++++++++++++++++---------
 security/integrity/ima/ima_policy.c   | 19 ++++++++++++-------
 security/integrity/integrity.h        |  9 +++++++--
 8 files changed, 56 insertions(+), 26 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index e76432b9954d..5dc9eed035fb 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -25,7 +25,7 @@ Description:
 				 [obj_user=] [obj_role=] [obj_type=]]
 			option:	[[appraise_type=]] [permit_directio]
 
-		base: 	func:= [BPRM_CHECK][MMAP_CHECK][FILE_CHECK][MODULE_CHECK]
+		base: 	func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
 				[FIRMWARE_CHECK]
 				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
 			mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 6fc888ca468e..ad30094a58b4 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -78,6 +78,7 @@ static void iint_free(struct integrity_iint_cache *iint)
 	iint->ima_mmap_status = INTEGRITY_UNKNOWN;
 	iint->ima_bprm_status = INTEGRITY_UNKNOWN;
 	iint->ima_read_status = INTEGRITY_UNKNOWN;
+	iint->ima_creds_status = INTEGRITY_UNKNOWN;
 	iint->evm_status = INTEGRITY_UNKNOWN;
 	iint->measured_pcrs = 0;
 	kmem_cache_free(iint_cache, iint);
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d52b487ad259..0703a96072b5 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -177,6 +177,7 @@ static inline unsigned long ima_hash_key(u8 *digest)
 	hook(FILE_CHECK)		\
 	hook(MMAP_CHECK)		\
 	hook(BPRM_CHECK)		\
+	hook(CREDS_CHECK)		\
 	hook(POST_SETATTR)		\
 	hook(MODULE_CHECK)		\
 	hook(FIRMWARE_CHECK)		\
@@ -191,7 +192,7 @@ enum ima_hooks {
 };
 
 /* LIM API function definitions */
-int ima_get_action(struct inode *inode, int mask,
+int ima_get_action(struct inode *inode, const struct cred *cred, int mask,
 		   enum ima_hooks func, int *pcr);
 int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func);
 int ima_collect_measurement(struct integrity_iint_cache *iint,
@@ -212,8 +213,8 @@ void ima_free_template_entry(struct ima_template_entry *entry);
 const char *ima_d_path(const struct path *path, char **pathbuf, char *filename);
 
 /* IMA policy related functions */
-int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask,
-		     int flags, int *pcr);
+int ima_match_policy(struct inode *inode, const struct cred *cred,
+		     enum ima_hooks func, int mask, int flags, int *pcr);
 void ima_init_policy(void);
 void ima_update_policy(void);
 void ima_update_policy_flag(void);
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index c2edba8de35e..ff33b7e65a07 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -157,6 +157,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
 /**
  * ima_get_action - appraise & measure decision based on policy.
  * @inode: pointer to inode to measure
+ * @cred: pointer to credentials structure to validate
  * @mask: contains the permission mask (MAY_READ, MAY_WRITE, MAY_EXEC,
  *        MAY_APPEND)
  * @func: caller identifier
@@ -165,20 +166,21 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
  * The policy is defined in terms of keypairs:
  *		subj=, obj=, type=, func=, mask=, fsmagic=
  *	subj,obj, and type: are LSM specific.
- *	func: FILE_CHECK | BPRM_CHECK | MMAP_CHECK | MODULE_CHECK
+ *	func: FILE_CHECK | BPRM_CHECK | CREDS_CHECK | MMAP_CHECK | MODULE_CHECK
  *	mask: contains the permission mask
  *	fsmagic: hex value
  *
  * Returns IMA_MEASURE, IMA_APPRAISE mask.
  *
  */
-int ima_get_action(struct inode *inode, int mask, enum ima_hooks func, int *pcr)
+int ima_get_action(struct inode *inode, const struct cred *cred, int mask,
+		   enum ima_hooks func, int *pcr)
 {
 	int flags = IMA_MEASURE | IMA_AUDIT | IMA_APPRAISE;
 
 	flags &= ima_policy_flag;
 
-	return ima_match_policy(inode, func, mask, flags, pcr);
+	return ima_match_policy(inode, cred, func, mask, flags, pcr);
 }
 
 /*
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 809ba70fbbbf..137b8d1708c6 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -53,7 +53,8 @@ int ima_must_appraise(struct inode *inode, int mask, enum ima_hooks func)
 	if (!ima_appraise)
 		return 0;
 
-	return ima_match_policy(inode, func, mask, IMA_APPRAISE, NULL);
+	return ima_match_policy(inode, current_cred(), func, mask,
+				IMA_APPRAISE, NULL);
 }
 
 static int ima_fix_xattr(struct dentry *dentry,
@@ -86,6 +87,8 @@ enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
 		return iint->ima_mmap_status;
 	case BPRM_CHECK:
 		return iint->ima_bprm_status;
+	case CREDS_CHECK:
+		return iint->ima_creds_status;
 	case FILE_CHECK:
 	case POST_SETATTR:
 		return iint->ima_file_status;
@@ -106,6 +109,8 @@ static void ima_set_cache_status(struct integrity_iint_cache *iint,
 	case BPRM_CHECK:
 		iint->ima_bprm_status = status;
 		break;
+	case CREDS_CHECK:
+		iint->ima_creds_status = status;
 	case FILE_CHECK:
 	case POST_SETATTR:
 		iint->ima_file_status = status;
@@ -127,6 +132,9 @@ static void ima_cache_flags(struct integrity_iint_cache *iint,
 	case BPRM_CHECK:
 		iint->flags |= (IMA_BPRM_APPRAISED | IMA_APPRAISED);
 		break;
+	case CREDS_CHECK:
+		iint->flags |= (IMA_CREDS_APPRAISED | IMA_APPRAISED);
+		break;
 	case FILE_CHECK:
 	case POST_SETATTR:
 		iint->flags |= (IMA_FILE_APPRAISED | IMA_APPRAISED);
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2aebb7984437..f41aa427792b 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -155,8 +155,9 @@ void ima_file_free(struct file *file)
 	ima_check_last_writer(iint, inode, file);
 }
 
-static int process_measurement(struct file *file, char *buf, loff_t size,
-			       int mask, enum ima_hooks func, int opened)
+static int process_measurement(struct file *file, const struct cred *cred,
+			       char *buf, loff_t size, int mask,
+			       enum ima_hooks func, int opened)
 {
 	struct inode *inode = file_inode(file);
 	struct integrity_iint_cache *iint = NULL;
@@ -178,7 +179,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 	 * bitmask based on the appraise/audit/measurement policy.
 	 * Included is the appraise submask.
 	 */
-	action = ima_get_action(inode, mask, func, &pcr);
+	action = ima_get_action(inode, cred, mask, func, &pcr);
 	violation_check = ((func == FILE_CHECK || func == MMAP_CHECK) &&
 			   (ima_policy_flag & IMA_MEASURE));
 	if (!action && !violation_check)
@@ -282,8 +283,8 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 int ima_file_mmap(struct file *file, unsigned long prot)
 {
 	if (file && (prot & PROT_EXEC))
-		return process_measurement(file, NULL, 0, MAY_EXEC,
-					   MMAP_CHECK, 0);
+		return process_measurement(file, current_cred(), NULL, 0,
+					   MAY_EXEC, MMAP_CHECK, 0);
 	return 0;
 }
 
@@ -302,8 +303,14 @@ int ima_file_mmap(struct file *file, unsigned long prot)
  */
 int ima_bprm_check(struct linux_binprm *bprm)
 {
-	return process_measurement(bprm->file, NULL, 0, MAY_EXEC,
-				   BPRM_CHECK, 0);
+	int ret;
+
+	ret = process_measurement(bprm->file, current_cred(), NULL, 0,
+				  MAY_EXEC, BPRM_CHECK, 0);
+	if (ret)
+		return ret;
+	return process_measurement(bprm->file, bprm->cred, NULL, 0,
+				   MAY_EXEC, CREDS_CHECK, 0);
 }
 
 /**
@@ -318,7 +325,7 @@ int ima_bprm_check(struct linux_binprm *bprm)
  */
 int ima_file_check(struct file *file, int mask, int opened)
 {
-	return process_measurement(file, NULL, 0,
+	return process_measurement(file, current_cred(), NULL, 0,
 				   mask & (MAY_READ | MAY_WRITE | MAY_EXEC |
 					   MAY_APPEND), FILE_CHECK, opened);
 }
@@ -413,7 +420,8 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
 	}
 
 	func = read_idmap[read_id] ?: FILE_CHECK;
-	return process_measurement(file, buf, size, MAY_READ, func, 0);
+	return process_measurement(file, current_cred(), buf, size, MAY_READ,
+				   func, 0);
 }
 
 static int __init init_ima(void)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 95209a5f8595..c9d5735711eb 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -247,10 +247,9 @@ static void ima_lsm_update_rules(void)
  * Returns true on rule match, false on failure.
  */
 static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
-			    enum ima_hooks func, int mask)
+			    const struct cred *cred, enum ima_hooks func,
+			    int mask)
 {
-	struct task_struct *tsk = current;
-	const struct cred *cred = current_cred();
 	int i;
 
 	if ((rule->flags & IMA_FUNC) &&
@@ -305,7 +304,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 		case LSM_SUBJ_USER:
 		case LSM_SUBJ_ROLE:
 		case LSM_SUBJ_TYPE:
-			security_task_getsecid(tsk, &sid);
+			security_cred_getsecid(cred, &sid);
 			rc = security_filter_rule_match(sid,
 							rule->lsm[i].type,
 							Audit_equal,
@@ -339,6 +338,8 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
 		return IMA_MMAP_APPRAISE;
 	case BPRM_CHECK:
 		return IMA_BPRM_APPRAISE;
+	case CREDS_CHECK:
+		return IMA_CREDS_APPRAISE;
 	case FILE_CHECK:
 	case POST_SETATTR:
 		return IMA_FILE_APPRAISE;
@@ -351,6 +352,8 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
 /**
  * ima_match_policy - decision based on LSM and other conditions
  * @inode: pointer to an inode for which the policy decision is being made
+ * @cred: pointer to a credentials structure for which the policy decision is
+ *        being made
  * @func: IMA hook identifier
  * @mask: requested action (MAY_READ | MAY_WRITE | MAY_APPEND | MAY_EXEC)
  * @pcr: set the pcr to extend
@@ -362,8 +365,8 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
  * list when walking it.  Reads are many orders of magnitude more numerous
  * than writes so ima_match_policy() is classical RCU candidate.
  */
-int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask,
-		     int flags, int *pcr)
+int ima_match_policy(struct inode *inode, const struct cred *cred,
+		     enum ima_hooks func, int mask, int flags, int *pcr)
 {
 	struct ima_rule_entry *entry;
 	int action = 0, actmask = flags | (flags << 1);
@@ -374,7 +377,7 @@ int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask,
 		if (!(entry->action & actmask))
 			continue;
 
-		if (!ima_match_rules(entry, inode, func, mask))
+		if (!ima_match_rules(entry, inode, cred, func, mask))
 			continue;
 
 		action |= entry->flags & IMA_ACTION_FLAGS;
@@ -691,6 +694,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 				entry->func = MMAP_CHECK;
 			else if (strcmp(args[0].from, "BPRM_CHECK") == 0)
 				entry->func = BPRM_CHECK;
+			else if (strcmp(args[0].from, "CREDS_CHECK") == 0)
+				entry->func = CREDS_CHECK;
 			else if (strcmp(args[0].from, "KEXEC_KERNEL_CHECK") ==
 				 0)
 				entry->func = KEXEC_KERNEL_CHECK;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index a53e7e4ab06c..45ba0e4501d6 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -48,10 +48,14 @@
 #define IMA_BPRM_APPRAISED	0x00002000
 #define IMA_READ_APPRAISE	0x00004000
 #define IMA_READ_APPRAISED	0x00008000
+#define IMA_CREDS_APPRAISE	0x00010000
+#define IMA_CREDS_APPRAISED	0x00020000
 #define IMA_APPRAISE_SUBMASK	(IMA_FILE_APPRAISE | IMA_MMAP_APPRAISE | \
-				 IMA_BPRM_APPRAISE | IMA_READ_APPRAISE)
+				 IMA_BPRM_APPRAISE | IMA_READ_APPRAISE | \
+				 IMA_CREDS_APPRAISE)
 #define IMA_APPRAISED_SUBMASK	(IMA_FILE_APPRAISED | IMA_MMAP_APPRAISED | \
-				 IMA_BPRM_APPRAISED | IMA_READ_APPRAISED)
+				 IMA_BPRM_APPRAISED | IMA_READ_APPRAISED | \
+				 IMA_CREDS_APPRAISED)
 
 enum evm_ima_xattr_type {
 	IMA_XATTR_DIGEST = 0x01,
@@ -108,6 +112,7 @@ struct integrity_iint_cache {
 	enum integrity_status ima_mmap_status:4;
 	enum integrity_status ima_bprm_status:4;
 	enum integrity_status ima_read_status:4;
+	enum integrity_status ima_creds_status:4;
 	enum integrity_status evm_status:4;
 	struct ima_digest_data *ima_hash;
 };
-- 
2.15.0.rc2.357.g7e34df9404-goog

[PATCH V3 2/2] IMA: Support using new creds in appraisal policy

[Matthew Garrett]

The existing BPRM_CHECK functionality in IMA validates against the
credentials of the existing process, not any new credentials that the
child process may transition to. Add an additional CREDS_CHECK target
and refactor IMA to pass the appropriate creds structure. In
ima_bprm_check(), check with both the existing process credentials and
the credentials that will be committed when the new process is started.
This will not change behaviour unless the system policy is extended to
include CREDS_CHECK targets - BPRM_CHECK will continue to check the same
credentials that it did previously.

Signed-off-by: Matthew Garrett <mjg59@google.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Stephen Smalley <sds@tycho.nsa.gov>
Cc: Eric Paris <eparis@parisplace.org>
Cc: selinux at tycho.nsa.gov
Cc: Casey Schaufler <casey@schaufler-ca.com>
Cc: linux-security-module at vger.kernel.org
Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
Cc: linux-integrity at vger.kernel.org
---
 V3: Update description to make it clear that this doesn't alter the behaviour
 of existing policies

 Documentation/ABI/testing/ima_policy  |  2 +-
 security/integrity/iint.c             |  1 +
 security/integrity/ima/ima.h          |  7 ++++---
 security/integrity/ima/ima_api.c      |  8 +++++---
 security/integrity/ima/ima_appraise.c | 10 +++++++++-
 security/integrity/ima/ima_main.c     | 26 +++++++++++++++++---------
 security/integrity/ima/ima_policy.c   | 19 ++++++++++++-------
 security/integrity/integrity.h        |  9 +++++++--
 8 files changed, 56 insertions(+), 26 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index e76432b9954d..5dc9eed035fb 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -25,7 +25,7 @@ Description:
 				 [obj_user=] [obj_role=] [obj_type=]]
 			option:	[[appraise_type=]] [permit_directio]
 
-		base: 	func:= [BPRM_CHECK][MMAP_CHECK][FILE_CHECK][MODULE_CHECK]
+		base: 	func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
 				[FIRMWARE_CHECK]
 				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
 			mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 6fc888ca468e..ad30094a58b4 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -78,6 +78,7 @@ static void iint_free(struct integrity_iint_cache *iint)
 	iint->ima_mmap_status = INTEGRITY_UNKNOWN;
 	iint->ima_bprm_status = INTEGRITY_UNKNOWN;
 	iint->ima_read_status = INTEGRITY_UNKNOWN;
+	iint->ima_creds_status = INTEGRITY_UNKNOWN;
 	iint->evm_status = INTEGRITY_UNKNOWN;
 	iint->measured_pcrs = 0;
 	kmem_cache_free(iint_cache, iint);
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d52b487ad259..0703a96072b5 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -177,6 +177,7 @@ static inline unsigned long ima_hash_key(u8 *digest)
 	hook(FILE_CHECK)		\
 	hook(MMAP_CHECK)		\
 	hook(BPRM_CHECK)		\
+	hook(CREDS_CHECK)		\
 	hook(POST_SETATTR)		\
 	hook(MODULE_CHECK)		\
 	hook(FIRMWARE_CHECK)		\
@@ -191,7 +192,7 @@ enum ima_hooks {
 };
 
 /* LIM API function definitions */
-int ima_get_action(struct inode *inode, int mask,
+int ima_get_action(struct inode *inode, const struct cred *cred, int mask,
 		   enum ima_hooks func, int *pcr);
 int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func);
 int ima_collect_measurement(struct integrity_iint_cache *iint,
@@ -212,8 +213,8 @@ void ima_free_template_entry(struct ima_template_entry *entry);
 const char *ima_d_path(const struct path *path, char **pathbuf, char *filename);
 
 /* IMA policy related functions */
-int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask,
-		     int flags, int *pcr);
+int ima_match_policy(struct inode *inode, const struct cred *cred,
+		     enum ima_hooks func, int mask, int flags, int *pcr);
 void ima_init_policy(void);
 void ima_update_policy(void);
 void ima_update_policy_flag(void);
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index c2edba8de35e..ff33b7e65a07 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -157,6 +157,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
 /**
  * ima_get_action - appraise & measure decision based on policy.
  * @inode: pointer to inode to measure
+ * @cred: pointer to credentials structure to validate
  * @mask: contains the permission mask (MAY_READ, MAY_WRITE, MAY_EXEC,
  *        MAY_APPEND)
  * @func: caller identifier
@@ -165,20 +166,21 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
  * The policy is defined in terms of keypairs:
  *		subj=, obj=, type=, func=, mask=, fsmagic=
  *	subj,obj, and type: are LSM specific.
- *	func: FILE_CHECK | BPRM_CHECK | MMAP_CHECK | MODULE_CHECK
+ *	func: FILE_CHECK | BPRM_CHECK | CREDS_CHECK | MMAP_CHECK | MODULE_CHECK
  *	mask: contains the permission mask
  *	fsmagic: hex value
  *
  * Returns IMA_MEASURE, IMA_APPRAISE mask.
  *
  */
-int ima_get_action(struct inode *inode, int mask, enum ima_hooks func, int *pcr)
+int ima_get_action(struct inode *inode, const struct cred *cred, int mask,
+		   enum ima_hooks func, int *pcr)
 {
 	int flags = IMA_MEASURE | IMA_AUDIT | IMA_APPRAISE;
 
 	flags &= ima_policy_flag;
 
-	return ima_match_policy(inode, func, mask, flags, pcr);
+	return ima_match_policy(inode, cred, func, mask, flags, pcr);
 }
 
 /*
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 809ba70fbbbf..137b8d1708c6 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -53,7 +53,8 @@ int ima_must_appraise(struct inode *inode, int mask, enum ima_hooks func)
 	if (!ima_appraise)
 		return 0;
 
-	return ima_match_policy(inode, func, mask, IMA_APPRAISE, NULL);
+	return ima_match_policy(inode, current_cred(), func, mask,
+				IMA_APPRAISE, NULL);
 }
 
 static int ima_fix_xattr(struct dentry *dentry,
@@ -86,6 +87,8 @@ enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
 		return iint->ima_mmap_status;
 	case BPRM_CHECK:
 		return iint->ima_bprm_status;
+	case CREDS_CHECK:
+		return iint->ima_creds_status;
 	case FILE_CHECK:
 	case POST_SETATTR:
 		return iint->ima_file_status;
@@ -106,6 +109,8 @@ static void ima_set_cache_status(struct integrity_iint_cache *iint,
 	case BPRM_CHECK:
 		iint->ima_bprm_status = status;
 		break;
+	case CREDS_CHECK:
+		iint->ima_creds_status = status;
 	case FILE_CHECK:
 	case POST_SETATTR:
 		iint->ima_file_status = status;
@@ -127,6 +132,9 @@ static void ima_cache_flags(struct integrity_iint_cache *iint,
 	case BPRM_CHECK:
 		iint->flags |= (IMA_BPRM_APPRAISED | IMA_APPRAISED);
 		break;
+	case CREDS_CHECK:
+		iint->flags |= (IMA_CREDS_APPRAISED | IMA_APPRAISED);
+		break;
 	case FILE_CHECK:
 	case POST_SETATTR:
 		iint->flags |= (IMA_FILE_APPRAISED | IMA_APPRAISED);
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2aebb7984437..f41aa427792b 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -155,8 +155,9 @@ void ima_file_free(struct file *file)
 	ima_check_last_writer(iint, inode, file);
 }
 
-static int process_measurement(struct file *file, char *buf, loff_t size,
-			       int mask, enum ima_hooks func, int opened)
+static int process_measurement(struct file *file, const struct cred *cred,
+			       char *buf, loff_t size, int mask,
+			       enum ima_hooks func, int opened)
 {
 	struct inode *inode = file_inode(file);
 	struct integrity_iint_cache *iint = NULL;
@@ -178,7 +179,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 	 * bitmask based on the appraise/audit/measurement policy.
 	 * Included is the appraise submask.
 	 */
-	action = ima_get_action(inode, mask, func, &pcr);
+	action = ima_get_action(inode, cred, mask, func, &pcr);
 	violation_check = ((func == FILE_CHECK || func == MMAP_CHECK) &&
 			   (ima_policy_flag & IMA_MEASURE));
 	if (!action && !violation_check)
@@ -282,8 +283,8 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 int ima_file_mmap(struct file *file, unsigned long prot)
 {
 	if (file && (prot & PROT_EXEC))
-		return process_measurement(file, NULL, 0, MAY_EXEC,
-					   MMAP_CHECK, 0);
+		return process_measurement(file, current_cred(), NULL, 0,
+					   MAY_EXEC, MMAP_CHECK, 0);
 	return 0;
 }
 
@@ -302,8 +303,14 @@ int ima_file_mmap(struct file *file, unsigned long prot)
  */
 int ima_bprm_check(struct linux_binprm *bprm)
 {
-	return process_measurement(bprm->file, NULL, 0, MAY_EXEC,
-				   BPRM_CHECK, 0);
+	int ret;
+
+	ret = process_measurement(bprm->file, current_cred(), NULL, 0,
+				  MAY_EXEC, BPRM_CHECK, 0);
+	if (ret)
+		return ret;
+	return process_measurement(bprm->file, bprm->cred, NULL, 0,
+				   MAY_EXEC, CREDS_CHECK, 0);
 }
 
 /**
@@ -318,7 +325,7 @@ int ima_bprm_check(struct linux_binprm *bprm)
  */
 int ima_file_check(struct file *file, int mask, int opened)
 {
-	return process_measurement(file, NULL, 0,
+	return process_measurement(file, current_cred(), NULL, 0,
 				   mask & (MAY_READ | MAY_WRITE | MAY_EXEC |
 					   MAY_APPEND), FILE_CHECK, opened);
 }
@@ -413,7 +420,8 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
 	}
 
 	func = read_idmap[read_id] ?: FILE_CHECK;
-	return process_measurement(file, buf, size, MAY_READ, func, 0);
+	return process_measurement(file, current_cred(), buf, size, MAY_READ,
+				   func, 0);
 }
 
 static int __init init_ima(void)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 95209a5f8595..c9d5735711eb 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -247,10 +247,9 @@ static void ima_lsm_update_rules(void)
  * Returns true on rule match, false on failure.
  */
 static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
-			    enum ima_hooks func, int mask)
+			    const struct cred *cred, enum ima_hooks func,
+			    int mask)
 {
-	struct task_struct *tsk = current;
-	const struct cred *cred = current_cred();
 	int i;
 
 	if ((rule->flags & IMA_FUNC) &&
@@ -305,7 +304,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 		case LSM_SUBJ_USER:
 		case LSM_SUBJ_ROLE:
 		case LSM_SUBJ_TYPE:
-			security_task_getsecid(tsk, &sid);
+			security_cred_getsecid(cred, &sid);
 			rc = security_filter_rule_match(sid,
 							rule->lsm[i].type,
 							Audit_equal,
@@ -339,6 +338,8 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
 		return IMA_MMAP_APPRAISE;
 	case BPRM_CHECK:
 		return IMA_BPRM_APPRAISE;
+	case CREDS_CHECK:
+		return IMA_CREDS_APPRAISE;
 	case FILE_CHECK:
 	case POST_SETATTR:
 		return IMA_FILE_APPRAISE;
@@ -351,6 +352,8 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
 /**
  * ima_match_policy - decision based on LSM and other conditions
  * @inode: pointer to an inode for which the policy decision is being made
+ * @cred: pointer to a credentials structure for which the policy decision is
+ *        being made
  * @func: IMA hook identifier
  * @mask: requested action (MAY_READ | MAY_WRITE | MAY_APPEND | MAY_EXEC)
  * @pcr: set the pcr to extend
@@ -362,8 +365,8 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
  * list when walking it.  Reads are many orders of magnitude more numerous
  * than writes so ima_match_policy() is classical RCU candidate.
  */
-int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask,
-		     int flags, int *pcr)
+int ima_match_policy(struct inode *inode, const struct cred *cred,
+		     enum ima_hooks func, int mask, int flags, int *pcr)
 {
 	struct ima_rule_entry *entry;
 	int action = 0, actmask = flags | (flags << 1);
@@ -374,7 +377,7 @@ int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask,
 		if (!(entry->action & actmask))
 			continue;
 
-		if (!ima_match_rules(entry, inode, func, mask))
+		if (!ima_match_rules(entry, inode, cred, func, mask))
 			continue;
 
 		action |= entry->flags & IMA_ACTION_FLAGS;
@@ -691,6 +694,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 				entry->func = MMAP_CHECK;
 			else if (strcmp(args[0].from, "BPRM_CHECK") == 0)
 				entry->func = BPRM_CHECK;
+			else if (strcmp(args[0].from, "CREDS_CHECK") == 0)
+				entry->func = CREDS_CHECK;
 			else if (strcmp(args[0].from, "KEXEC_KERNEL_CHECK") ==
 				 0)
 				entry->func = KEXEC_KERNEL_CHECK;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index a53e7e4ab06c..45ba0e4501d6 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -48,10 +48,14 @@
 #define IMA_BPRM_APPRAISED	0x00002000
 #define IMA_READ_APPRAISE	0x00004000
 #define IMA_READ_APPRAISED	0x00008000
+#define IMA_CREDS_APPRAISE	0x00010000
+#define IMA_CREDS_APPRAISED	0x00020000
 #define IMA_APPRAISE_SUBMASK	(IMA_FILE_APPRAISE | IMA_MMAP_APPRAISE | \
-				 IMA_BPRM_APPRAISE | IMA_READ_APPRAISE)
+				 IMA_BPRM_APPRAISE | IMA_READ_APPRAISE | \
+				 IMA_CREDS_APPRAISE)
 #define IMA_APPRAISED_SUBMASK	(IMA_FILE_APPRAISED | IMA_MMAP_APPRAISED | \
-				 IMA_BPRM_APPRAISED | IMA_READ_APPRAISED)
+				 IMA_BPRM_APPRAISED | IMA_READ_APPRAISED | \
+				 IMA_CREDS_APPRAISED)
 
 enum evm_ima_xattr_type {
 	IMA_XATTR_DIGEST = 0x01,
@@ -108,6 +112,7 @@ struct integrity_iint_cache {
 	enum integrity_status ima_mmap_status:4;
 	enum integrity_status ima_bprm_status:4;
 	enum integrity_status ima_read_status:4;
+	enum integrity_status ima_creds_status:4;
 	enum integrity_status evm_status:4;
 	struct ima_digest_data *ima_hash;
 };
-- 
2.15.0.rc2.357.g7e34df9404-goog

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 2/2] IMA: Support using new creds in appraisal policy

[James Morris]

On Thu, 26 Oct 2017, Matthew Garrett wrote:

> The existing BPRM_CHECK functionality in IMA validates against the
> credentials of the existing process, not any new credentials that the
> child process may transition to. Add an additional CREDS_CHECK target
> and refactor IMA to pass the appropriate creds structure. In
> ima_bprm_check(), check with both the existing process credentials and
> the credentials that will be committed when the new process is started.
> This will not change behaviour unless the system policy is extended to
> include CREDS_CHECK targets - BPRM_CHECK will continue to check the same
> credentials that it did previously.
> 
> Signed-off-by: Matthew Garrett <mjg59@google.com>


Reviewed-by: James Morris <james.l.morris@oracle.com>


-- 
James Morris
<james.l.morris@oracle.com>

[PATCH V3 2/2] IMA: Support using new creds in appraisal policy

[James Morris]

On Thu, 26 Oct 2017, Matthew Garrett wrote:

> The existing BPRM_CHECK functionality in IMA validates against the
> credentials of the existing process, not any new credentials that the
> child process may transition to. Add an additional CREDS_CHECK target
> and refactor IMA to pass the appropriate creds structure. In
> ima_bprm_check(), check with both the existing process credentials and
> the credentials that will be committed when the new process is started.
> This will not change behaviour unless the system policy is extended to
> include CREDS_CHECK targets - BPRM_CHECK will continue to check the same
> credentials that it did previously.
> 
> Signed-off-by: Matthew Garrett <mjg59@google.com>


Reviewed-by: James Morris <james.l.morris@oracle.com>


-- 
James Morris
<james.l.morris@oracle.com>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 2/2] IMA: Support using new creds in appraisal policy

[Mimi Zohar]

Hi Matthew,

On Thu, 2017-10-26 at 01:40 -0700, Matthew Garrett wrote:
> The existing BPRM_CHECK functionality in IMA validates against the
> credentials of the existing process, not any new credentials that the
> child process may transition to. Add an additional CREDS_CHECK target
> and refactor IMA to pass the appropriate creds structure. In
> ima_bprm_check(), check with both the existing process credentials and
> the credentials that will be committed when the new process is started.
> This will not change behaviour unless the system policy is extended to
> include CREDS_CHECK targets - BPRM_CHECK will continue to check the same
> credentials that it did previously.

< snip >

> @@ -305,7 +304,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
>  		case LSM_SUBJ_USER:
>  		case LSM_SUBJ_ROLE:
>  		case LSM_SUBJ_TYPE:
> -			security_task_getsecid(tsk, &sid);
> +			security_cred_getsecid(cred, &sid);
>  			rc = security_filter_rule_match(sid,
>  							rule->lsm[i].type,
>  							Audit_equal,

Based on the patch description, I wouldn't expect to see any changes
here, unless this is wrong to begin with.  In which case, it should be
a separate patch.

Mimi

Re: [PATCH V3 2/2] IMA: Support using new creds in appraisal policy

[Mimi Zohar]

Hi Matthew,

On Thu, 2017-10-26 at 01:40 -0700, Matthew Garrett wrote:
> The existing BPRM_CHECK functionality in IMA validates against the
> credentials of the existing process, not any new credentials that the
> child process may transition to. Add an additional CREDS_CHECK target
> and refactor IMA to pass the appropriate creds structure. In
> ima_bprm_check(), check with both the existing process credentials and
> the credentials that will be committed when the new process is started.
> This will not change behaviour unless the system policy is extended to
> include CREDS_CHECK targets - BPRM_CHECK will continue to check the same
> credentials that it did previously.

< snip >

> @@ -305,7 +304,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
>  		case LSM_SUBJ_USER:
>  		case LSM_SUBJ_ROLE:
>  		case LSM_SUBJ_TYPE:
> -			security_task_getsecid(tsk, &sid);
> +			security_cred_getsecid(cred, &sid);
>  			rc = security_filter_rule_match(sid,
>  							rule->lsm[i].type,
>  							Audit_equal,

Based on the patch description, I wouldn't expect to see any changes
here, unless this is wrong to begin with.  In which case, it should be
a separate patch.

Mimi

[PATCH V3 2/2] IMA: Support using new creds in appraisal policy

[Mimi Zohar]

Hi Matthew,

On Thu, 2017-10-26 at 01:40 -0700, Matthew Garrett wrote:
> The existing BPRM_CHECK functionality in IMA validates against the
> credentials of the existing process, not any new credentials that the
> child process may transition to. Add an additional CREDS_CHECK target
> and refactor IMA to pass the appropriate creds structure. In
> ima_bprm_check(), check with both the existing process credentials and
> the credentials that will be committed when the new process is started.
> This will not change behaviour unless the system policy is extended to
> include CREDS_CHECK targets - BPRM_CHECK will continue to check the same
> credentials that it did previously.

< snip >

> @@ -305,7 +304,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
>  		case LSM_SUBJ_USER:
>  		case LSM_SUBJ_ROLE:
>  		case LSM_SUBJ_TYPE:
> -			security_task_getsecid(tsk, &sid);
> +			security_cred_getsecid(cred, &sid);
>  			rc = security_filter_rule_match(sid,
>  							rule->lsm[i].type,
>  							Audit_equal,

Based on the patch description, I wouldn't expect to see any changes
here, unless this is wrong to begin with. ?In which case, it should be
a separate patch.

Mimi

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 2/2] IMA: Support using new creds in appraisal policy

[Matthew Garrett]

On Tue, Nov 28, 2017 at 12:48 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> Hi Matthew,
>
> On Thu, 2017-10-26 at 01:40 -0700, Matthew Garrett wrote:
> > The existing BPRM_CHECK functionality in IMA validates against the
> > credentials of the existing process, not any new credentials that the
> > child process may transition to. Add an additional CREDS_CHECK target
> > and refactor IMA to pass the appropriate creds structure. In
> > ima_bprm_check(), check with both the existing process credentials and
> > the credentials that will be committed when the new process is started.
> > This will not change behaviour unless the system policy is extended to
> > include CREDS_CHECK targets - BPRM_CHECK will continue to check the same
> > credentials that it did previously.
>
> < snip >
>
> > @@ -305,7 +304,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> >               case LSM_SUBJ_USER:
> >               case LSM_SUBJ_ROLE:
> >               case LSM_SUBJ_TYPE:
> > -                     security_task_getsecid(tsk, &sid);
> > +                     security_cred_getsecid(cred, &sid);
> >                       rc = security_filter_rule_match(sid,
> >                                                       rule->lsm[i].type,
> >                                                       Audit_equal,
>
> Based on the patch description, I wouldn't expect to see any changes
> here, unless this is wrong to begin with.  In which case, it should be
> a separate patch.

We need to check against the appropriate credentials structure, and
since we're doing this before commit_creds() has been called we can't
just do it against the one in the task structure. For BPRM_CHECK
that'll be current_cred(), which means there's no change in
functionality, whereas for CREDS_CHECK it'll be the new credentials
structure.

[PATCH V3 2/2] IMA: Support using new creds in appraisal policy

[Matthew Garrett]

On Tue, Nov 28, 2017 at 12:48 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> Hi Matthew,
>
> On Thu, 2017-10-26 at 01:40 -0700, Matthew Garrett wrote:
> > The existing BPRM_CHECK functionality in IMA validates against the
> > credentials of the existing process, not any new credentials that the
> > child process may transition to. Add an additional CREDS_CHECK target
> > and refactor IMA to pass the appropriate creds structure. In
> > ima_bprm_check(), check with both the existing process credentials and
> > the credentials that will be committed when the new process is started.
> > This will not change behaviour unless the system policy is extended to
> > include CREDS_CHECK targets - BPRM_CHECK will continue to check the same
> > credentials that it did previously.
>
> < snip >
>
> > @@ -305,7 +304,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> >               case LSM_SUBJ_USER:
> >               case LSM_SUBJ_ROLE:
> >               case LSM_SUBJ_TYPE:
> > -                     security_task_getsecid(tsk, &sid);
> > +                     security_cred_getsecid(cred, &sid);
> >                       rc = security_filter_rule_match(sid,
> >                                                       rule->lsm[i].type,
> >                                                       Audit_equal,
>
> Based on the patch description, I wouldn't expect to see any changes
> here, unless this is wrong to begin with.  In which case, it should be
> a separate patch.

We need to check against the appropriate credentials structure, and
since we're doing this before commit_creds() has been called we can't
just do it against the one in the task structure. For BPRM_CHECK
that'll be current_cred(), which means there's no change in
functionality, whereas for CREDS_CHECK it'll be the new credentials
structure.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 2/2] IMA: Support using new creds in appraisal policy

[Mimi Zohar]

On Tue, 2017-11-28 at 13:22 -0800, Matthew Garrett wrote:
> On Tue, Nov 28, 2017 at 12:48 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > Hi Matthew,
> >
> > On Thu, 2017-10-26 at 01:40 -0700, Matthew Garrett wrote:
> > > The existing BPRM_CHECK functionality in IMA validates against the
> > > credentials of the existing process, not any new credentials that the
> > > child process may transition to. Add an additional CREDS_CHECK target
> > > and refactor IMA to pass the appropriate creds structure. In
> > > ima_bprm_check(), check with both the existing process credentials and
> > > the credentials that will be committed when the new process is started.
> > > This will not change behaviour unless the system policy is extended to
> > > include CREDS_CHECK targets - BPRM_CHECK will continue to check the same
> > > credentials that it did previously.
> >
> > < snip >
> >
> > > @@ -305,7 +304,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> > >               case LSM_SUBJ_USER:
> > >               case LSM_SUBJ_ROLE:
> > >               case LSM_SUBJ_TYPE:
> > > -                     security_task_getsecid(tsk, &sid);
> > > +                     security_cred_getsecid(cred, &sid);
> > >                       rc = security_filter_rule_match(sid,
> > >                                                       rule->lsm[i].type,
> > >                                                       Audit_equal,
> >
> > Based on the patch description, I wouldn't expect to see any changes
> > here, unless this is wrong to begin with.  In which case, it should be
> > a separate patch.
> 
> We need to check against the appropriate credentials structure, and
> since we're doing this before commit_creds() has been called we can't
> just do it against the one in the task structure.  For BPRM_CHECK
> that'll be current_cred(), which means there's no change in
> functionality, whereas for CREDS_CHECK it'll be the new credentials
> structure.

The existing code calls security_task_getsecid() with "current" not
"current_cred".  Will replacing security_task_getsecid() with
security_cred_getsecid() return the same info for the original
BRPM_CHECK?

Mimi

Re: [PATCH V3 2/2] IMA: Support using new creds in appraisal policy

[Mimi Zohar]

On Tue, 2017-11-28 at 13:22 -0800, Matthew Garrett wrote:
> On Tue, Nov 28, 2017 at 12:48 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > Hi Matthew,
> >
> > On Thu, 2017-10-26 at 01:40 -0700, Matthew Garrett wrote:
> > > The existing BPRM_CHECK functionality in IMA validates against the
> > > credentials of the existing process, not any new credentials that the
> > > child process may transition to. Add an additional CREDS_CHECK target
> > > and refactor IMA to pass the appropriate creds structure. In
> > > ima_bprm_check(), check with both the existing process credentials and
> > > the credentials that will be committed when the new process is started.
> > > This will not change behaviour unless the system policy is extended to
> > > include CREDS_CHECK targets - BPRM_CHECK will continue to check the same
> > > credentials that it did previously.
> >
> > < snip >
> >
> > > @@ -305,7 +304,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> > >               case LSM_SUBJ_USER:
> > >               case LSM_SUBJ_ROLE:
> > >               case LSM_SUBJ_TYPE:
> > > -                     security_task_getsecid(tsk, &sid);
> > > +                     security_cred_getsecid(cred, &sid);
> > >                       rc = security_filter_rule_match(sid,
> > >                                                       rule->lsm[i].type,
> > >                                                       Audit_equal,
> >
> > Based on the patch description, I wouldn't expect to see any changes
> > here, unless this is wrong to begin with.  In which case, it should be
> > a separate patch.
> 
> We need to check against the appropriate credentials structure, and
> since we're doing this before commit_creds() has been called we can't
> just do it against the one in the task structure.  For BPRM_CHECK
> that'll be current_cred(), which means there's no change in
> functionality, whereas for CREDS_CHECK it'll be the new credentials
> structure.

The existing code calls security_task_getsecid() with "current" not
"current_cred".  Will replacing security_task_getsecid() with
security_cred_getsecid() return the same info for the original
BRPM_CHECK?

Mimi

[PATCH V3 2/2] IMA: Support using new creds in appraisal policy

[Mimi Zohar]

On Tue, 2017-11-28 at 13:22 -0800, Matthew Garrett wrote:
> On Tue, Nov 28, 2017 at 12:48 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > Hi Matthew,
> >
> > On Thu, 2017-10-26 at 01:40 -0700, Matthew Garrett wrote:
> > > The existing BPRM_CHECK functionality in IMA validates against the
> > > credentials of the existing process, not any new credentials that the
> > > child process may transition to. Add an additional CREDS_CHECK target
> > > and refactor IMA to pass the appropriate creds structure. In
> > > ima_bprm_check(), check with both the existing process credentials and
> > > the credentials that will be committed when the new process is started.
> > > This will not change behaviour unless the system policy is extended to
> > > include CREDS_CHECK targets - BPRM_CHECK will continue to check the same
> > > credentials that it did previously.
> >
> > < snip >
> >
> > > @@ -305,7 +304,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> > >               case LSM_SUBJ_USER:
> > >               case LSM_SUBJ_ROLE:
> > >               case LSM_SUBJ_TYPE:
> > > -                     security_task_getsecid(tsk, &sid);
> > > +                     security_cred_getsecid(cred, &sid);
> > >                       rc = security_filter_rule_match(sid,
> > >                                                       rule->lsm[i].type,
> > >                                                       Audit_equal,
> >
> > Based on the patch description, I wouldn't expect to see any changes
> > here, unless this is wrong to begin with.  In which case, it should be
> > a separate patch.
> 
> We need to check against the appropriate credentials structure, and
> since we're doing this before commit_creds() has been called we can't
> just do it against the one in the task structure.  For BPRM_CHECK
> that'll be current_cred(), which means there's no change in
> functionality, whereas for CREDS_CHECK it'll be the new credentials
> structure.

The existing code calls security_task_getsecid() with "current" not
"current_cred". ?Will replacing security_task_getsecid() with
security_cred_getsecid() return the same info for the original
BRPM_CHECK?

Mimi

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 2/2] IMA: Support using new creds in appraisal policy

[Matthew Garrett]

On Tue, Nov 28, 2017 at 1:35 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Tue, 2017-11-28 at 13:22 -0800, Matthew Garrett wrote:
>> We need to check against the appropriate credentials structure, and
>> since we're doing this before commit_creds() has been called we can't
>> just do it against the one in the task structure.  For BPRM_CHECK
>> that'll be current_cred(), which means there's no change in
>> functionality, whereas for CREDS_CHECK it'll be the new credentials
>> structure.
>
> The existing code calls security_task_getsecid() with "current" not
> "current_cred".  Will replacing security_task_getsecid() with
> security_cred_getsecid() return the same info for the original
> BRPM_CHECK?

security_task_getsecid(current) will give the same results as
security_cred_getsecid(current_creds())

[PATCH V3 2/2] IMA: Support using new creds in appraisal policy

[Matthew Garrett]

On Tue, Nov 28, 2017 at 1:35 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Tue, 2017-11-28 at 13:22 -0800, Matthew Garrett wrote:
>> We need to check against the appropriate credentials structure, and
>> since we're doing this before commit_creds() has been called we can't
>> just do it against the one in the task structure.  For BPRM_CHECK
>> that'll be current_cred(), which means there's no change in
>> functionality, whereas for CREDS_CHECK it'll be the new credentials
>> structure.
>
> The existing code calls security_task_getsecid() with "current" not
> "current_cred".  Will replacing security_task_getsecid() with
> security_cred_getsecid() return the same info for the original
> BRPM_CHECK?

security_task_getsecid(current) will give the same results as
security_cred_getsecid(current_creds())
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 2/2] IMA: Support using new creds in appraisal policy

[Mimi Zohar]

On Tue, 2017-11-28 at 13:37 -0800, Matthew Garrett wrote:
> On Tue, Nov 28, 2017 at 1:35 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > On Tue, 2017-11-28 at 13:22 -0800, Matthew Garrett wrote:
> >> We need to check against the appropriate credentials structure, and
> >> since we're doing this before commit_creds() has been called we can't
> >> just do it against the one in the task structure.  For BPRM_CHECK
> >> that'll be current_cred(), which means there's no change in
> >> functionality, whereas for CREDS_CHECK it'll be the new credentials
> >> structure.
> >
> > The existing code calls security_task_getsecid() with "current" not
> > "current_cred".  Will replacing security_task_getsecid() with
> > security_cred_getsecid() return the same info for the original
> > BRPM_CHECK?
> 
> security_task_getsecid(current) will give the same results as
> security_cred_getsecid(current_creds())

Unwinding security_task_getsecid(current) looks like it is using
real_cred, while current_cred() is using cred.

selinux_task_getsecid() -> task_sid() -> cred_sid(__task_cred())

#define __task_cred(task)       \
        rcu_dereference((task)->real_cred)

selinux_task_getsecid() -> cred_sid()

#define current_cred() \
        rcu_dereference_protected(current->cred, 1)

Is the change intentional?

Mimi

[PATCH V3 2/2] IMA: Support using new creds in appraisal policy

[Mimi Zohar]

On Tue, 2017-11-28 at 13:37 -0800, Matthew Garrett wrote:
> On Tue, Nov 28, 2017 at 1:35 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > On Tue, 2017-11-28 at 13:22 -0800, Matthew Garrett wrote:
> >> We need to check against the appropriate credentials structure, and
> >> since we're doing this before commit_creds() has been called we can't
> >> just do it against the one in the task structure.  For BPRM_CHECK
> >> that'll be current_cred(), which means there's no change in
> >> functionality, whereas for CREDS_CHECK it'll be the new credentials
> >> structure.
> >
> > The existing code calls security_task_getsecid() with "current" not
> > "current_cred".  Will replacing security_task_getsecid() with
> > security_cred_getsecid() return the same info for the original
> > BRPM_CHECK?
> 
> security_task_getsecid(current) will give the same results as
> security_cred_getsecid(current_creds())

Unwinding security_task_getsecid(current) looks like it is using
real_cred, while current_cred() is using cred.

selinux_task_getsecid() -> task_sid() -> cred_sid(__task_cred())

#define __task_cred(task)       \
        rcu_dereference((task)->real_cred)

selinux_task_getsecid() -> cred_sid()

#define current_cred() \
        rcu_dereference_protected(current->cred, 1)

Is the change intentional?

Mimi

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 2/2] IMA: Support using new creds in appraisal policy

[Matthew Garrett]

Hm, sorry, missed this mail.

On Tue, Nov 28, 2017 at 2:33 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Tue, 2017-11-28 at 13:37 -0800, Matthew Garrett wrote:
>> security_task_getsecid(current) will give the same results as
>> security_cred_getsecid(current_creds())
>
> Unwinding security_task_getsecid(current) looks like it is using
> real_cred, while current_cred() is using cred.

Good question, and there's a current_real_cred() macro, so I should
just use that instead.

[PATCH V3 2/2] IMA: Support using new creds in appraisal policy

[Matthew Garrett]

Hm, sorry, missed this mail.

On Tue, Nov 28, 2017 at 2:33 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Tue, 2017-11-28 at 13:37 -0800, Matthew Garrett wrote:
>> security_task_getsecid(current) will give the same results as
>> security_cred_getsecid(current_creds())
>
> Unwinding security_task_getsecid(current) looks like it is using
> real_cred, while current_cred() is using cred.

Good question, and there's a current_real_cred() macro, so I should
just use that instead.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 2/2] IMA: Support using new creds in appraisal policy

[Matthew Garrett]

On Fri, Dec 15, 2017 at 2:24 PM, Matthew Garrett <mjg59@google.com> wrote:
> Hm, sorry, missed this mail.
>
> On Tue, Nov 28, 2017 at 2:33 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>> On Tue, 2017-11-28 at 13:37 -0800, Matthew Garrett wrote:
>>> security_task_getsecid(current) will give the same results as
>>> security_cred_getsecid(current_creds())
>>
>> Unwinding security_task_getsecid(current) looks like it is using
>> real_cred, while current_cred() is using cred.
>
> Good question, and there's a current_real_cred() macro, so I should
> just use that instead.

Hm. Actually, I'm not sure. For most checks we were using cred, and
only using real_cred for the secid lookup. This feels somewhat
inconsistent.

[PATCH V3 2/2] IMA: Support using new creds in appraisal policy

[Matthew Garrett]

On Fri, Dec 15, 2017 at 2:24 PM, Matthew Garrett <mjg59@google.com> wrote:
> Hm, sorry, missed this mail.
>
> On Tue, Nov 28, 2017 at 2:33 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>> On Tue, 2017-11-28 at 13:37 -0800, Matthew Garrett wrote:
>>> security_task_getsecid(current) will give the same results as
>>> security_cred_getsecid(current_creds())
>>
>> Unwinding security_task_getsecid(current) looks like it is using
>> real_cred, while current_cred() is using cred.
>
> Good question, and there's a current_real_cred() macro, so I should
> just use that instead.

Hm. Actually, I'm not sure. For most checks we were using cred, and
only using real_cred for the secid lookup. This feels somewhat
inconsistent.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 2/2] IMA: Support using new creds in appraisal policy

[Mimi Zohar]

On Fri, 2017-12-15 at 14:35 -0800, Matthew Garrett wrote:
> On Fri, Dec 15, 2017 at 2:24 PM, Matthew Garrett <mjg59@google.com> wrote:
> > Hm, sorry, missed this mail.

I was kind of wondering what happened...

> > On Tue, Nov 28, 2017 at 2:33 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> >> On Tue, 2017-11-28 at 13:37 -0800, Matthew Garrett wrote:
> >>> security_task_getsecid(current) will give the same results as
> >>> security_cred_getsecid(current_creds())
> >>
> >> Unwinding security_task_getsecid(current) looks like it is using
> >> real_cred, while current_cred() is using cred.
> >
> > Good question, and there's a current_real_cred() macro, so I should
> > just use that instead.
> 
> Hm. Actually, I'm not sure. For most checks we were using cred, and
> only using real_cred for the secid lookup. This feels somewhat
> inconsistent.

Even if it is a one line change, it shouldn't be hidden like this.
Please make it a separate patch, with the reason for the change.  We
need to make sure this change doesn't break existing systems.

thanks,

Mimi

Re: [PATCH V3 2/2] IMA: Support using new creds in appraisal policy

[Mimi Zohar]

On Fri, 2017-12-15 at 14:35 -0800, Matthew Garrett wrote:
> On Fri, Dec 15, 2017 at 2:24 PM, Matthew Garrett <mjg59@google.com> wrote:
> > Hm, sorry, missed this mail.

I was kind of wondering what happened...

> > On Tue, Nov 28, 2017 at 2:33 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> >> On Tue, 2017-11-28 at 13:37 -0800, Matthew Garrett wrote:
> >>> security_task_getsecid(current) will give the same results as
> >>> security_cred_getsecid(current_creds())
> >>
> >> Unwinding security_task_getsecid(current) looks like it is using
> >> real_cred, while current_cred() is using cred.
> >
> > Good question, and there's a current_real_cred() macro, so I should
> > just use that instead.
> 
> Hm. Actually, I'm not sure. For most checks we were using cred, and
> only using real_cred for the secid lookup. This feels somewhat
> inconsistent.

Even if it is a one line change, it shouldn't be hidden like this.
Please make it a separate patch, with the reason for the change.  We
need to make sure this change doesn't break existing systems.

thanks,

Mimi

[PATCH V3 2/2] IMA: Support using new creds in appraisal policy

[Mimi Zohar]

On Fri, 2017-12-15 at 14:35 -0800, Matthew Garrett wrote:
> On Fri, Dec 15, 2017 at 2:24 PM, Matthew Garrett <mjg59@google.com> wrote:
> > Hm, sorry, missed this mail.

I was kind of wondering what happened...

> > On Tue, Nov 28, 2017 at 2:33 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> >> On Tue, 2017-11-28 at 13:37 -0800, Matthew Garrett wrote:
> >>> security_task_getsecid(current) will give the same results as
> >>> security_cred_getsecid(current_creds())
> >>
> >> Unwinding security_task_getsecid(current) looks like it is using
> >> real_cred, while current_cred() is using cred.
> >
> > Good question, and there's a current_real_cred() macro, so I should
> > just use that instead.
> 
> Hm. Actually, I'm not sure. For most checks we were using cred, and
> only using real_cred for the secid lookup. This feels somewhat
> inconsistent.

Even if it is a one line change, it shouldn't be hidden like this.
Please make it a separate patch, with the reason for the change. ?We
need to make sure this change doesn't break existing systems.

thanks,

Mimi

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 1/2] security: Add a cred_getsecid hook

[James Morris]

On Thu, 26 Oct 2017, Matthew Garrett wrote:

> For IMA purposes, we want to be able to obtain the prepared secid in the
> bprm structure before the credentials are committed. Add a cred_getsecid
> hook that makes this possible.
> 
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> Acked-by: Paul Moore <paul@paul-moore.com>


Acked-by: James Morris <james.l.morris@oracle.com>


-- 
James Morris
<james.l.morris@oracle.com>

[PATCH V3 1/2] security: Add a cred_getsecid hook

[James Morris]

On Thu, 26 Oct 2017, Matthew Garrett wrote:

> For IMA purposes, we want to be able to obtain the prepared secid in the
> bprm structure before the credentials are committed. Add a cred_getsecid
> hook that makes this possible.
> 
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> Acked-by: Paul Moore <paul@paul-moore.com>


Acked-by: James Morris <james.l.morris@oracle.com>


-- 
James Morris
<james.l.morris@oracle.com>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 1/2] security: Add a cred_getsecid hook

[Casey Schaufler]

On 10/26/2017 1:40 AM, Matthew Garrett wrote:
> For IMA purposes, we want to be able to obtain the prepared secid in the
> bprm structure before the credentials are committed. Add a cred_getsecid
> hook that makes this possible.
>
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> Acked-by: Paul Moore <paul@paul-moore.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: Eric Paris <eparis@parisplace.org>
> Cc: selinux@tycho.nsa.gov
> Cc: Casey Schaufler <casey@schaufler-ca.com>
> Cc: linux-security-module@vger.kernel.org
> Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
> Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> Cc: linux-integrity@vger.kernel.org
> ---
>  V3: Fix smack_cred_getsecid()

Much better. Have you tried this with Smack?

>
>  include/linux/lsm_hooks.h  |  6 ++++++
>  include/linux/security.h   |  1 +
>  security/security.c        |  7 +++++++
>  security/selinux/hooks.c   |  8 ++++++++
>  security/smack/smack_lsm.c | 18 ++++++++++++++++++
>  5 files changed, 40 insertions(+)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index c9258124e417..c28c6f8b65dc 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -554,6 +554,10 @@
>   *	@new points to the new credentials.
>   *	@old points to the original credentials.
>   *	Transfer data from original creds to new creds
> + * @cred_getsecid:
> + *	Retrieve the security identifier of the cred structure @c
> + *	@c contains the credentials, secid will be placed into @secid.
> + *	In case of failure, @secid will be set to zero.
>   * @kernel_act_as:
>   *	Set the credentials for a kernel service to act as (subjective context).
>   *	@new points to the credentials to be modified.
> @@ -1507,6 +1511,7 @@ union security_list_options {
>  	int (*cred_prepare)(struct cred *new, const struct cred *old,
>  				gfp_t gfp);
>  	void (*cred_transfer)(struct cred *new, const struct cred *old);
> +	void (*cred_getsecid)(const struct cred *c, u32 *secid);
>  	int (*kernel_act_as)(struct cred *new, u32 secid);
>  	int (*kernel_create_files_as)(struct cred *new, struct inode *inode);
>  	int (*kernel_module_request)(char *kmod_name);
> @@ -1779,6 +1784,7 @@ struct security_hook_heads {
>  	struct list_head cred_free;
>  	struct list_head cred_prepare;
>  	struct list_head cred_transfer;
> +	struct list_head cred_getsecid;
>  	struct list_head kernel_act_as;
>  	struct list_head kernel_create_files_as;
>  	struct list_head kernel_read_file;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index ce6265960d6c..14848fef8f62 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -324,6 +324,7 @@ int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
>  void security_cred_free(struct cred *cred);
>  int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp);
>  void security_transfer_creds(struct cred *new, const struct cred *old);
> +void security_cred_getsecid(const struct cred *c, u32 *secid);
>  int security_kernel_act_as(struct cred *new, u32 secid);
>  int security_kernel_create_files_as(struct cred *new, struct inode *inode);
>  int security_kernel_module_request(char *kmod_name);
> diff --git a/security/security.c b/security/security.c
> index 4bf0f571b4ef..02d217597400 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1004,6 +1004,13 @@ void security_transfer_creds(struct cred *new, const struct cred *old)
>  	call_void_hook(cred_transfer, new, old);
>  }
>  
> +void security_cred_getsecid(const struct cred *c, u32 *secid)
> +{
> +	*secid = 0;
> +	call_void_hook(cred_getsecid, c, secid);
> +}
> +EXPORT_SYMBOL(security_cred_getsecid);
> +
>  int security_kernel_act_as(struct cred *new, u32 secid)
>  {
>  	return call_int_hook(kernel_act_as, 0, new, secid);
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f5d304736852..1d11679674a6 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3836,6 +3836,13 @@ static void selinux_cred_transfer(struct cred *new, const struct cred *old)
>  	*tsec = *old_tsec;
>  }
>  
> +static void selinux_cred_getsecid(const struct cred *c, u32 *secid)
> +{
> +	rcu_read_lock();
> +	*secid = cred_sid(c);
> +	rcu_read_unlock();
> +}
> +
>  /*
>   * set the security data for a kernel service
>   * - all the creation contexts are set to unlabelled
> @@ -6338,6 +6345,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>  	LSM_HOOK_INIT(cred_free, selinux_cred_free),
>  	LSM_HOOK_INIT(cred_prepare, selinux_cred_prepare),
>  	LSM_HOOK_INIT(cred_transfer, selinux_cred_transfer),
> +	LSM_HOOK_INIT(cred_getsecid, selinux_cred_getsecid),
>  	LSM_HOOK_INIT(kernel_act_as, selinux_kernel_act_as),
>  	LSM_HOOK_INIT(kernel_create_files_as, selinux_kernel_create_files_as),
>  	LSM_HOOK_INIT(kernel_module_request, selinux_kernel_module_request),
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 286171a16ed2..37c35aaa6955 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -2049,6 +2049,23 @@ static void smack_cred_transfer(struct cred *new, const struct cred *old)
>  	/* cbs copy rule list */
>  }
>  
> +/**
> + * smack_cred_getsecid - get the secid corresponding to a creds structure
> + * @c: the object creds
> + * @secid: where to put the result
> + *
> + * Sets the secid to contain a u32 version of the smack label.
> + */
> +static void smack_cred_getsecid(const struct cred *c, u32 *secid)
> +{
> +	struct smack_known *skp;
> +
> +	rcu_read_lock();
> +	skp = smk_of_task(c->security);
> +	*secid = skp->smk_secid;
> +	rcu_read_unlock();
> +}
> +
>  /**
>   * smack_kernel_act_as - Set the subjective context in a set of credentials
>   * @new: points to the set of credentials to be modified.
> @@ -4651,6 +4668,7 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
>  	LSM_HOOK_INIT(cred_free, smack_cred_free),
>  	LSM_HOOK_INIT(cred_prepare, smack_cred_prepare),
>  	LSM_HOOK_INIT(cred_transfer, smack_cred_transfer),
> +	LSM_HOOK_INIT(cred_getsecid, smack_cred_getsecid),
>  	LSM_HOOK_INIT(kernel_act_as, smack_kernel_act_as),
>  	LSM_HOOK_INIT(kernel_create_files_as, smack_kernel_create_files_as),
>  	LSM_HOOK_INIT(task_setpgid, smack_task_setpgid),

[PATCH V3 1/2] security: Add a cred_getsecid hook

[Casey Schaufler]

On 10/26/2017 1:40 AM, Matthew Garrett wrote:
> For IMA purposes, we want to be able to obtain the prepared secid in the
> bprm structure before the credentials are committed. Add a cred_getsecid
> hook that makes this possible.
>
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> Acked-by: Paul Moore <paul@paul-moore.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: Eric Paris <eparis@parisplace.org>
> Cc: selinux at tycho.nsa.gov
> Cc: Casey Schaufler <casey@schaufler-ca.com>
> Cc: linux-security-module at vger.kernel.org
> Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
> Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> Cc: linux-integrity at vger.kernel.org
> ---
>  V3: Fix smack_cred_getsecid()

Much better. Have you tried this with Smack?

>
>  include/linux/lsm_hooks.h  |  6 ++++++
>  include/linux/security.h   |  1 +
>  security/security.c        |  7 +++++++
>  security/selinux/hooks.c   |  8 ++++++++
>  security/smack/smack_lsm.c | 18 ++++++++++++++++++
>  5 files changed, 40 insertions(+)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index c9258124e417..c28c6f8b65dc 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -554,6 +554,10 @@
>   *	@new points to the new credentials.
>   *	@old points to the original credentials.
>   *	Transfer data from original creds to new creds
> + * @cred_getsecid:
> + *	Retrieve the security identifier of the cred structure @c
> + *	@c contains the credentials, secid will be placed into @secid.
> + *	In case of failure, @secid will be set to zero.
>   * @kernel_act_as:
>   *	Set the credentials for a kernel service to act as (subjective context).
>   *	@new points to the credentials to be modified.
> @@ -1507,6 +1511,7 @@ union security_list_options {
>  	int (*cred_prepare)(struct cred *new, const struct cred *old,
>  				gfp_t gfp);
>  	void (*cred_transfer)(struct cred *new, const struct cred *old);
> +	void (*cred_getsecid)(const struct cred *c, u32 *secid);
>  	int (*kernel_act_as)(struct cred *new, u32 secid);
>  	int (*kernel_create_files_as)(struct cred *new, struct inode *inode);
>  	int (*kernel_module_request)(char *kmod_name);
> @@ -1779,6 +1784,7 @@ struct security_hook_heads {
>  	struct list_head cred_free;
>  	struct list_head cred_prepare;
>  	struct list_head cred_transfer;
> +	struct list_head cred_getsecid;
>  	struct list_head kernel_act_as;
>  	struct list_head kernel_create_files_as;
>  	struct list_head kernel_read_file;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index ce6265960d6c..14848fef8f62 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -324,6 +324,7 @@ int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
>  void security_cred_free(struct cred *cred);
>  int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp);
>  void security_transfer_creds(struct cred *new, const struct cred *old);
> +void security_cred_getsecid(const struct cred *c, u32 *secid);
>  int security_kernel_act_as(struct cred *new, u32 secid);
>  int security_kernel_create_files_as(struct cred *new, struct inode *inode);
>  int security_kernel_module_request(char *kmod_name);
> diff --git a/security/security.c b/security/security.c
> index 4bf0f571b4ef..02d217597400 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1004,6 +1004,13 @@ void security_transfer_creds(struct cred *new, const struct cred *old)
>  	call_void_hook(cred_transfer, new, old);
>  }
>  
> +void security_cred_getsecid(const struct cred *c, u32 *secid)
> +{
> +	*secid = 0;
> +	call_void_hook(cred_getsecid, c, secid);
> +}
> +EXPORT_SYMBOL(security_cred_getsecid);
> +
>  int security_kernel_act_as(struct cred *new, u32 secid)
>  {
>  	return call_int_hook(kernel_act_as, 0, new, secid);
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f5d304736852..1d11679674a6 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3836,6 +3836,13 @@ static void selinux_cred_transfer(struct cred *new, const struct cred *old)
>  	*tsec = *old_tsec;
>  }
>  
> +static void selinux_cred_getsecid(const struct cred *c, u32 *secid)
> +{
> +	rcu_read_lock();
> +	*secid = cred_sid(c);
> +	rcu_read_unlock();
> +}
> +
>  /*
>   * set the security data for a kernel service
>   * - all the creation contexts are set to unlabelled
> @@ -6338,6 +6345,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>  	LSM_HOOK_INIT(cred_free, selinux_cred_free),
>  	LSM_HOOK_INIT(cred_prepare, selinux_cred_prepare),
>  	LSM_HOOK_INIT(cred_transfer, selinux_cred_transfer),
> +	LSM_HOOK_INIT(cred_getsecid, selinux_cred_getsecid),
>  	LSM_HOOK_INIT(kernel_act_as, selinux_kernel_act_as),
>  	LSM_HOOK_INIT(kernel_create_files_as, selinux_kernel_create_files_as),
>  	LSM_HOOK_INIT(kernel_module_request, selinux_kernel_module_request),
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 286171a16ed2..37c35aaa6955 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -2049,6 +2049,23 @@ static void smack_cred_transfer(struct cred *new, const struct cred *old)
>  	/* cbs copy rule list */
>  }
>  
> +/**
> + * smack_cred_getsecid - get the secid corresponding to a creds structure
> + * @c: the object creds
> + * @secid: where to put the result
> + *
> + * Sets the secid to contain a u32 version of the smack label.
> + */
> +static void smack_cred_getsecid(const struct cred *c, u32 *secid)
> +{
> +	struct smack_known *skp;
> +
> +	rcu_read_lock();
> +	skp = smk_of_task(c->security);
> +	*secid = skp->smk_secid;
> +	rcu_read_unlock();
> +}
> +
>  /**
>   * smack_kernel_act_as - Set the subjective context in a set of credentials
>   * @new: points to the set of credentials to be modified.
> @@ -4651,6 +4668,7 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
>  	LSM_HOOK_INIT(cred_free, smack_cred_free),
>  	LSM_HOOK_INIT(cred_prepare, smack_cred_prepare),
>  	LSM_HOOK_INIT(cred_transfer, smack_cred_transfer),
> +	LSM_HOOK_INIT(cred_getsecid, smack_cred_getsecid),
>  	LSM_HOOK_INIT(kernel_act_as, smack_kernel_act_as),
>  	LSM_HOOK_INIT(kernel_create_files_as, smack_kernel_create_files_as),
>  	LSM_HOOK_INIT(task_setpgid, smack_task_setpgid),

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 1/2] security: Add a cred_getsecid hook

[Matthew Garrett]

On Thu, Oct 26, 2017 at 2:21 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 10/26/2017 1:40 AM, Matthew Garrett wrote:
>>  V3: Fix smack_cred_getsecid()
>
> Much better. Have you tried this with Smack?

I'm afraid not - I have zero expertise with Smack and no easy way to
set it up. I can do so later in the week if you like?

[PATCH V3 1/2] security: Add a cred_getsecid hook

[Matthew Garrett]

On Thu, Oct 26, 2017 at 2:21 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 10/26/2017 1:40 AM, Matthew Garrett wrote:
>>  V3: Fix smack_cred_getsecid()
>
> Much better. Have you tried this with Smack?

I'm afraid not - I have zero expertise with Smack and no easy way to
set it up. I can do so later in the week if you like?
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 1/2] security: Add a cred_getsecid hook

[Stephen Smalley]

On Thu, 2017-10-26 at 01:40 -0700, Matthew Garrett via Selinux wrote:
> For IMA purposes, we want to be able to obtain the prepared secid in
> the
> bprm structure before the credentials are committed. Add a
> cred_getsecid
> hook that makes this possible.
> 
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> Acked-by: Paul Moore <paul@paul-moore.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: Eric Paris <eparis@parisplace.org>
> Cc: selinux@tycho.nsa.gov
> Cc: Casey Schaufler <casey@schaufler-ca.com>
> Cc: linux-security-module@vger.kernel.org
> Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
> Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> Cc: linux-integrity@vger.kernel.org
> ---
>  V3: Fix smack_cred_getsecid()
> 
>  include/linux/lsm_hooks.h  |  6 ++++++
>  include/linux/security.h   |  1 +
>  security/security.c        |  7 +++++++
>  security/selinux/hooks.c   |  8 ++++++++
>  security/smack/smack_lsm.c | 18 ++++++++++++++++++
>  5 files changed, 40 insertions(+)
> 
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index c9258124e417..c28c6f8b65dc 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -554,6 +554,10 @@
>   *	@new points to the new credentials.
>   *	@old points to the original credentials.
>   *	Transfer data from original creds to new creds
> + * @cred_getsecid:
> + *	Retrieve the security identifier of the cred structure @c
> + *	@c contains the credentials, secid will be placed into
> @secid.
> + *	In case of failure, @secid will be set to zero.
>   * @kernel_act_as:
>   *	Set the credentials for a kernel service to act as
> (subjective context).
>   *	@new points to the credentials to be modified.
> @@ -1507,6 +1511,7 @@ union security_list_options {
>  	int (*cred_prepare)(struct cred *new, const struct cred
> *old,
>  				gfp_t gfp);
>  	void (*cred_transfer)(struct cred *new, const struct cred
> *old);
> +	void (*cred_getsecid)(const struct cred *c, u32 *secid);
>  	int (*kernel_act_as)(struct cred *new, u32 secid);
>  	int (*kernel_create_files_as)(struct cred *new, struct inode
> *inode);
>  	int (*kernel_module_request)(char *kmod_name);
> @@ -1779,6 +1784,7 @@ struct security_hook_heads {
>  	struct list_head cred_free;
>  	struct list_head cred_prepare;
>  	struct list_head cred_transfer;
> +	struct list_head cred_getsecid;
>  	struct list_head kernel_act_as;
>  	struct list_head kernel_create_files_as;
>  	struct list_head kernel_read_file;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index ce6265960d6c..14848fef8f62 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -324,6 +324,7 @@ int security_cred_alloc_blank(struct cred *cred,
> gfp_t gfp);
>  void security_cred_free(struct cred *cred);
>  int security_prepare_creds(struct cred *new, const struct cred *old,
> gfp_t gfp);
>  void security_transfer_creds(struct cred *new, const struct cred
> *old);
> +void security_cred_getsecid(const struct cred *c, u32 *secid);
>  int security_kernel_act_as(struct cred *new, u32 secid);
>  int security_kernel_create_files_as(struct cred *new, struct inode
> *inode);
>  int security_kernel_module_request(char *kmod_name);
> diff --git a/security/security.c b/security/security.c
> index 4bf0f571b4ef..02d217597400 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1004,6 +1004,13 @@ void security_transfer_creds(struct cred *new,
> const struct cred *old)
>  	call_void_hook(cred_transfer, new, old);
>  }
>  
> +void security_cred_getsecid(const struct cred *c, u32 *secid)
> +{
> +	*secid = 0;
> +	call_void_hook(cred_getsecid, c, secid);
> +}
> +EXPORT_SYMBOL(security_cred_getsecid);
> +
>  int security_kernel_act_as(struct cred *new, u32 secid)
>  {
>  	return call_int_hook(kernel_act_as, 0, new, secid);
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f5d304736852..1d11679674a6 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3836,6 +3836,13 @@ static void selinux_cred_transfer(struct cred
> *new, const struct cred *old)
>  	*tsec = *old_tsec;
>  }
>  
> +static void selinux_cred_getsecid(const struct cred *c, u32 *secid)
> +{
> +	rcu_read_lock();
> +	*secid = cred_sid(c);
> +	rcu_read_unlock();

Is rcu_read_lock() necessary here? Seems like we use cred_sid() in many
places without it.

> +}
> +
>  /*
>   * set the security data for a kernel service
>   * - all the creation contexts are set to unlabelled
> @@ -6338,6 +6345,7 @@ static struct security_hook_list
> selinux_hooks[] __lsm_ro_after_init = {
>  	LSM_HOOK_INIT(cred_free, selinux_cred_free),
>  	LSM_HOOK_INIT(cred_prepare, selinux_cred_prepare),
>  	LSM_HOOK_INIT(cred_transfer, selinux_cred_transfer),
> +	LSM_HOOK_INIT(cred_getsecid, selinux_cred_getsecid),
>  	LSM_HOOK_INIT(kernel_act_as, selinux_kernel_act_as),
>  	LSM_HOOK_INIT(kernel_create_files_as,
> selinux_kernel_create_files_as),
>  	LSM_HOOK_INIT(kernel_module_request,
> selinux_kernel_module_request),
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 286171a16ed2..37c35aaa6955 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -2049,6 +2049,23 @@ static void smack_cred_transfer(struct cred
> *new, const struct cred *old)
>  	/* cbs copy rule list */
>  }
>  
> +/**
> + * smack_cred_getsecid - get the secid corresponding to a creds
> structure
> + * @c: the object creds
> + * @secid: where to put the result
> + *
> + * Sets the secid to contain a u32 version of the smack label.
> + */
> +static void smack_cred_getsecid(const struct cred *c, u32 *secid)
> +{
> +	struct smack_known *skp;
> +
> +	rcu_read_lock();
> +	skp = smk_of_task(c->security);
> +	*secid = skp->smk_secid;
> +	rcu_read_unlock();
> +}
> +
>  /**
>   * smack_kernel_act_as - Set the subjective context in a set of
> credentials
>   * @new: points to the set of credentials to be modified.
> @@ -4651,6 +4668,7 @@ static struct security_hook_list smack_hooks[]
> __lsm_ro_after_init = {
>  	LSM_HOOK_INIT(cred_free, smack_cred_free),
>  	LSM_HOOK_INIT(cred_prepare, smack_cred_prepare),
>  	LSM_HOOK_INIT(cred_transfer, smack_cred_transfer),
> +	LSM_HOOK_INIT(cred_getsecid, smack_cred_getsecid),
>  	LSM_HOOK_INIT(kernel_act_as, smack_kernel_act_as),
>  	LSM_HOOK_INIT(kernel_create_files_as,
> smack_kernel_create_files_as),
>  	LSM_HOOK_INIT(task_setpgid, smack_task_setpgid),

Re: [PATCH V3 1/2] security: Add a cred_getsecid hook

[Stephen Smalley]

On Thu, 2017-10-26 at 01:40 -0700, Matthew Garrett via Selinux wrote:
> For IMA purposes, we want to be able to obtain the prepared secid in
> the
> bprm structure before the credentials are committed. Add a
> cred_getsecid
> hook that makes this possible.
> 
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> Acked-by: Paul Moore <paul@paul-moore.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: Eric Paris <eparis@parisplace.org>
> Cc: selinux@tycho.nsa.gov
> Cc: Casey Schaufler <casey@schaufler-ca.com>
> Cc: linux-security-module@vger.kernel.org
> Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
> Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> Cc: linux-integrity@vger.kernel.org
> ---
>  V3: Fix smack_cred_getsecid()
> 
>  include/linux/lsm_hooks.h  |  6 ++++++
>  include/linux/security.h   |  1 +
>  security/security.c        |  7 +++++++
>  security/selinux/hooks.c   |  8 ++++++++
>  security/smack/smack_lsm.c | 18 ++++++++++++++++++
>  5 files changed, 40 insertions(+)
> 
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index c9258124e417..c28c6f8b65dc 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -554,6 +554,10 @@
>   *	@new points to the new credentials.
>   *	@old points to the original credentials.
>   *	Transfer data from original creds to new creds
> + * @cred_getsecid:
> + *	Retrieve the security identifier of the cred structure @c
> + *	@c contains the credentials, secid will be placed into
> @secid.
> + *	In case of failure, @secid will be set to zero.
>   * @kernel_act_as:
>   *	Set the credentials for a kernel service to act as
> (subjective context).
>   *	@new points to the credentials to be modified.
> @@ -1507,6 +1511,7 @@ union security_list_options {
>  	int (*cred_prepare)(struct cred *new, const struct cred
> *old,
>  				gfp_t gfp);
>  	void (*cred_transfer)(struct cred *new, const struct cred
> *old);
> +	void (*cred_getsecid)(const struct cred *c, u32 *secid);
>  	int (*kernel_act_as)(struct cred *new, u32 secid);
>  	int (*kernel_create_files_as)(struct cred *new, struct inode
> *inode);
>  	int (*kernel_module_request)(char *kmod_name);
> @@ -1779,6 +1784,7 @@ struct security_hook_heads {
>  	struct list_head cred_free;
>  	struct list_head cred_prepare;
>  	struct list_head cred_transfer;
> +	struct list_head cred_getsecid;
>  	struct list_head kernel_act_as;
>  	struct list_head kernel_create_files_as;
>  	struct list_head kernel_read_file;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index ce6265960d6c..14848fef8f62 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -324,6 +324,7 @@ int security_cred_alloc_blank(struct cred *cred,
> gfp_t gfp);
>  void security_cred_free(struct cred *cred);
>  int security_prepare_creds(struct cred *new, const struct cred *old,
> gfp_t gfp);
>  void security_transfer_creds(struct cred *new, const struct cred
> *old);
> +void security_cred_getsecid(const struct cred *c, u32 *secid);
>  int security_kernel_act_as(struct cred *new, u32 secid);
>  int security_kernel_create_files_as(struct cred *new, struct inode
> *inode);
>  int security_kernel_module_request(char *kmod_name);
> diff --git a/security/security.c b/security/security.c
> index 4bf0f571b4ef..02d217597400 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1004,6 +1004,13 @@ void security_transfer_creds(struct cred *new,
> const struct cred *old)
>  	call_void_hook(cred_transfer, new, old);
>  }
>  
> +void security_cred_getsecid(const struct cred *c, u32 *secid)
> +{
> +	*secid = 0;
> +	call_void_hook(cred_getsecid, c, secid);
> +}
> +EXPORT_SYMBOL(security_cred_getsecid);
> +
>  int security_kernel_act_as(struct cred *new, u32 secid)
>  {
>  	return call_int_hook(kernel_act_as, 0, new, secid);
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f5d304736852..1d11679674a6 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3836,6 +3836,13 @@ static void selinux_cred_transfer(struct cred
> *new, const struct cred *old)
>  	*tsec = *old_tsec;
>  }
>  
> +static void selinux_cred_getsecid(const struct cred *c, u32 *secid)
> +{
> +	rcu_read_lock();
> +	*secid = cred_sid(c);
> +	rcu_read_unlock();

Is rcu_read_lock() necessary here? Seems like we use cred_sid() in many
places without it.

> +}
> +
>  /*
>   * set the security data for a kernel service
>   * - all the creation contexts are set to unlabelled
> @@ -6338,6 +6345,7 @@ static struct security_hook_list
> selinux_hooks[] __lsm_ro_after_init = {
>  	LSM_HOOK_INIT(cred_free, selinux_cred_free),
>  	LSM_HOOK_INIT(cred_prepare, selinux_cred_prepare),
>  	LSM_HOOK_INIT(cred_transfer, selinux_cred_transfer),
> +	LSM_HOOK_INIT(cred_getsecid, selinux_cred_getsecid),
>  	LSM_HOOK_INIT(kernel_act_as, selinux_kernel_act_as),
>  	LSM_HOOK_INIT(kernel_create_files_as,
> selinux_kernel_create_files_as),
>  	LSM_HOOK_INIT(kernel_module_request,
> selinux_kernel_module_request),
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 286171a16ed2..37c35aaa6955 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -2049,6 +2049,23 @@ static void smack_cred_transfer(struct cred
> *new, const struct cred *old)
>  	/* cbs copy rule list */
>  }
>  
> +/**
> + * smack_cred_getsecid - get the secid corresponding to a creds
> structure
> + * @c: the object creds
> + * @secid: where to put the result
> + *
> + * Sets the secid to contain a u32 version of the smack label.
> + */
> +static void smack_cred_getsecid(const struct cred *c, u32 *secid)
> +{
> +	struct smack_known *skp;
> +
> +	rcu_read_lock();
> +	skp = smk_of_task(c->security);
> +	*secid = skp->smk_secid;
> +	rcu_read_unlock();
> +}
> +
>  /**
>   * smack_kernel_act_as - Set the subjective context in a set of
> credentials
>   * @new: points to the set of credentials to be modified.
> @@ -4651,6 +4668,7 @@ static struct security_hook_list smack_hooks[]
> __lsm_ro_after_init = {
>  	LSM_HOOK_INIT(cred_free, smack_cred_free),
>  	LSM_HOOK_INIT(cred_prepare, smack_cred_prepare),
>  	LSM_HOOK_INIT(cred_transfer, smack_cred_transfer),
> +	LSM_HOOK_INIT(cred_getsecid, smack_cred_getsecid),
>  	LSM_HOOK_INIT(kernel_act_as, smack_kernel_act_as),
>  	LSM_HOOK_INIT(kernel_create_files_as,
> smack_kernel_create_files_as),
>  	LSM_HOOK_INIT(task_setpgid, smack_task_setpgid),

[PATCH V3 1/2] security: Add a cred_getsecid hook

[Stephen Smalley]

On Thu, 2017-10-26 at 01:40 -0700, Matthew Garrett via Selinux wrote:
> For IMA purposes, we want to be able to obtain the prepared secid in
> the
> bprm structure before the credentials are committed. Add a
> cred_getsecid
> hook that makes this possible.
> 
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> Acked-by: Paul Moore <paul@paul-moore.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: Eric Paris <eparis@parisplace.org>
> Cc: selinux at tycho.nsa.gov
> Cc: Casey Schaufler <casey@schaufler-ca.com>
> Cc: linux-security-module at vger.kernel.org
> Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
> Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
> Cc: linux-integrity at vger.kernel.org
> ---
> ?V3: Fix smack_cred_getsecid()
> 
> ?include/linux/lsm_hooks.h??|??6 ++++++
> ?include/linux/security.h???|??1 +
> ?security/security.c????????|??7 +++++++
> ?security/selinux/hooks.c???|??8 ++++++++
> ?security/smack/smack_lsm.c | 18 ++++++++++++++++++
> ?5 files changed, 40 insertions(+)
> 
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index c9258124e417..c28c6f8b65dc 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -554,6 +554,10 @@
> ? *	@new points to the new credentials.
> ? *	@old points to the original credentials.
> ? *	Transfer data from original creds to new creds
> + * @cred_getsecid:
> + *	Retrieve the security identifier of the cred structure @c
> + *	@c contains the credentials, secid will be placed into
> @secid.
> + *	In case of failure, @secid will be set to zero.
> ? * @kernel_act_as:
> ? *	Set the credentials for a kernel service to act as
> (subjective context).
> ? *	@new points to the credentials to be modified.
> @@ -1507,6 +1511,7 @@ union security_list_options {
> ?	int (*cred_prepare)(struct cred *new, const struct cred
> *old,
> ?				gfp_t gfp);
> ?	void (*cred_transfer)(struct cred *new, const struct cred
> *old);
> +	void (*cred_getsecid)(const struct cred *c, u32 *secid);
> ?	int (*kernel_act_as)(struct cred *new, u32 secid);
> ?	int (*kernel_create_files_as)(struct cred *new, struct inode
> *inode);
> ?	int (*kernel_module_request)(char *kmod_name);
> @@ -1779,6 +1784,7 @@ struct security_hook_heads {
> ?	struct list_head cred_free;
> ?	struct list_head cred_prepare;
> ?	struct list_head cred_transfer;
> +	struct list_head cred_getsecid;
> ?	struct list_head kernel_act_as;
> ?	struct list_head kernel_create_files_as;
> ?	struct list_head kernel_read_file;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index ce6265960d6c..14848fef8f62 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -324,6 +324,7 @@ int security_cred_alloc_blank(struct cred *cred,
> gfp_t gfp);
> ?void security_cred_free(struct cred *cred);
> ?int security_prepare_creds(struct cred *new, const struct cred *old,
> gfp_t gfp);
> ?void security_transfer_creds(struct cred *new, const struct cred
> *old);
> +void security_cred_getsecid(const struct cred *c, u32 *secid);
> ?int security_kernel_act_as(struct cred *new, u32 secid);
> ?int security_kernel_create_files_as(struct cred *new, struct inode
> *inode);
> ?int security_kernel_module_request(char *kmod_name);
> diff --git a/security/security.c b/security/security.c
> index 4bf0f571b4ef..02d217597400 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1004,6 +1004,13 @@ void security_transfer_creds(struct cred *new,
> const struct cred *old)
> ?	call_void_hook(cred_transfer, new, old);
> ?}
> ?
> +void security_cred_getsecid(const struct cred *c, u32 *secid)
> +{
> +	*secid = 0;
> +	call_void_hook(cred_getsecid, c, secid);
> +}
> +EXPORT_SYMBOL(security_cred_getsecid);
> +
> ?int security_kernel_act_as(struct cred *new, u32 secid)
> ?{
> ?	return call_int_hook(kernel_act_as, 0, new, secid);
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f5d304736852..1d11679674a6 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3836,6 +3836,13 @@ static void selinux_cred_transfer(struct cred
> *new, const struct cred *old)
> ?	*tsec = *old_tsec;
> ?}
> ?
> +static void selinux_cred_getsecid(const struct cred *c, u32 *secid)
> +{
> +	rcu_read_lock();
> +	*secid = cred_sid(c);
> +	rcu_read_unlock();

Is rcu_read_lock() necessary here? Seems like we use cred_sid() in many
places without it.

> +}
> +
> ?/*
> ? * set the security data for a kernel service
> ? * - all the creation contexts are set to unlabelled
> @@ -6338,6 +6345,7 @@ static struct security_hook_list
> selinux_hooks[] __lsm_ro_after_init = {
> ?	LSM_HOOK_INIT(cred_free, selinux_cred_free),
> ?	LSM_HOOK_INIT(cred_prepare, selinux_cred_prepare),
> ?	LSM_HOOK_INIT(cred_transfer, selinux_cred_transfer),
> +	LSM_HOOK_INIT(cred_getsecid, selinux_cred_getsecid),
> ?	LSM_HOOK_INIT(kernel_act_as, selinux_kernel_act_as),
> ?	LSM_HOOK_INIT(kernel_create_files_as,
> selinux_kernel_create_files_as),
> ?	LSM_HOOK_INIT(kernel_module_request,
> selinux_kernel_module_request),
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 286171a16ed2..37c35aaa6955 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -2049,6 +2049,23 @@ static void smack_cred_transfer(struct cred
> *new, const struct cred *old)
> ?	/* cbs copy rule list */
> ?}
> ?
> +/**
> + * smack_cred_getsecid - get the secid corresponding to a creds
> structure
> + * @c: the object creds
> + * @secid: where to put the result
> + *
> + * Sets the secid to contain a u32 version of the smack label.
> + */
> +static void smack_cred_getsecid(const struct cred *c, u32 *secid)
> +{
> +	struct smack_known *skp;
> +
> +	rcu_read_lock();
> +	skp = smk_of_task(c->security);
> +	*secid = skp->smk_secid;
> +	rcu_read_unlock();
> +}
> +
> ?/**
> ? * smack_kernel_act_as - Set the subjective context in a set of
> credentials
> ? * @new: points to the set of credentials to be modified.
> @@ -4651,6 +4668,7 @@ static struct security_hook_list smack_hooks[]
> __lsm_ro_after_init = {
> ?	LSM_HOOK_INIT(cred_free, smack_cred_free),
> ?	LSM_HOOK_INIT(cred_prepare, smack_cred_prepare),
> ?	LSM_HOOK_INIT(cred_transfer, smack_cred_transfer),
> +	LSM_HOOK_INIT(cred_getsecid, smack_cred_getsecid),
> ?	LSM_HOOK_INIT(kernel_act_as, smack_kernel_act_as),
> ?	LSM_HOOK_INIT(kernel_create_files_as,
> smack_kernel_create_files_as),
> ?	LSM_HOOK_INIT(task_setpgid, smack_task_setpgid),
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 1/2] security: Add a cred_getsecid hook

[Matthew Garrett]

On Thu, Oct 26, 2017 at 3:20 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On Thu, 2017-10-26 at 01:40 -0700, Matthew Garrett via Selinux wrote:
>> +static void selinux_cred_getsecid(const struct cred *c, u32 *secid)
>> +{
>> +     rcu_read_lock();
>> +     *secid = cred_sid(c);
>> +     rcu_read_unlock();
>
> Is rcu_read_lock() necessary here? Seems like we use cred_sid() in many
> places without it.

Ah, I thought it was based on task_sid(), but I guess that's actually
protecting the __task_cred()?

[PATCH V3 1/2] security: Add a cred_getsecid hook

[Matthew Garrett]

On Thu, Oct 26, 2017 at 3:20 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On Thu, 2017-10-26 at 01:40 -0700, Matthew Garrett via Selinux wrote:
>> +static void selinux_cred_getsecid(const struct cred *c, u32 *secid)
>> +{
>> +     rcu_read_lock();
>> +     *secid = cred_sid(c);
>> +     rcu_read_unlock();
>
> Is rcu_read_lock() necessary here? Seems like we use cred_sid() in many
> places without it.

Ah, I thought it was based on task_sid(), but I guess that's actually
protecting the __task_cred()?
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 1/2] security: Add a cred_getsecid hook

[Stephen Smalley]

On Mon, 2017-10-30 at 10:57 +0000, Matthew Garrett via Selinux wrote:
> On Thu, Oct 26, 2017 at 3:20 PM, Stephen Smalley <sds@tycho.nsa.gov>
> wrote:
> > On Thu, 2017-10-26 at 01:40 -0700, Matthew Garrett via Selinux
> > wrote:
> > > +static void selinux_cred_getsecid(const struct cred *c, u32
> > > *secid)
> > > +{
> > > +     rcu_read_lock();
> > > +     *secid = cred_sid(c);
> > > +     rcu_read_unlock();
> > 
> > Is rcu_read_lock() necessary here? Seems like we use cred_sid() in
> > many
> > places without it.
> 
> Ah, I thought it was based on task_sid(), but I guess that's actually
> protecting the __task_cred()?

It appears to me that in all other cases, we are either dealing with
the current cred, or something in the call chain of cred_sid() is
holding a reference to the cred, or something in the call chain of
cred_sid() has called rcu_read_lock() already.  I might have missed
something though, and I don't know how safe it is to assume that all
future callers will do this.  cc'd David for his thoughts.

Re: [PATCH V3 1/2] security: Add a cred_getsecid hook

[Stephen Smalley]

On Mon, 2017-10-30 at 10:57 +0000, Matthew Garrett via Selinux wrote:
> On Thu, Oct 26, 2017 at 3:20 PM, Stephen Smalley <sds@tycho.nsa.gov>
> wrote:
> > On Thu, 2017-10-26 at 01:40 -0700, Matthew Garrett via Selinux
> > wrote:
> > > +static void selinux_cred_getsecid(const struct cred *c, u32
> > > *secid)
> > > +{
> > > +     rcu_read_lock();
> > > +     *secid = cred_sid(c);
> > > +     rcu_read_unlock();
> > 
> > Is rcu_read_lock() necessary here? Seems like we use cred_sid() in
> > many
> > places without it.
> 
> Ah, I thought it was based on task_sid(), but I guess that's actually
> protecting the __task_cred()?

It appears to me that in all other cases, we are either dealing with
the current cred, or something in the call chain of cred_sid() is
holding a reference to the cred, or something in the call chain of
cred_sid() has called rcu_read_lock() already.  I might have missed
something though, and I don't know how safe it is to assume that all
future callers will do this.  cc'd David for his thoughts.

[PATCH V3 1/2] security: Add a cred_getsecid hook

[Stephen Smalley]

On Mon, 2017-10-30 at 10:57 +0000, Matthew Garrett via Selinux wrote:
> On Thu, Oct 26, 2017 at 3:20 PM, Stephen Smalley <sds@tycho.nsa.gov>
> wrote:
> > On Thu, 2017-10-26 at 01:40 -0700, Matthew Garrett via Selinux
> > wrote:
> > > +static void selinux_cred_getsecid(const struct cred *c, u32
> > > *secid)
> > > +{
> > > +?????rcu_read_lock();
> > > +?????*secid = cred_sid(c);
> > > +?????rcu_read_unlock();
> > 
> > Is rcu_read_lock() necessary here? Seems like we use cred_sid() in
> > many
> > places without it.
> 
> Ah, I thought it was based on task_sid(), but I guess that's actually
> protecting the __task_cred()?

It appears to me that in all other cases, we are either dealing with
the current cred, or something in the call chain of cred_sid() is
holding a reference to the cred, or something in the call chain of
cred_sid() has called rcu_read_lock() already.  I might have missed
something though, and I don't know how safe it is to assume that all
future callers will do this.  cc'd David for his thoughts.

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 1/2] security: Add a cred_getsecid hook

[Matthew Garrett]

On Mon, Oct 30, 2017 at 10:03 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On Mon, 2017-10-30 at 10:57 +0000, Matthew Garrett via Selinux wrote:
>> On Thu, Oct 26, 2017 at 3:20 PM, Stephen Smalley <sds@tycho.nsa.gov>
>> wrote:
>> > On Thu, 2017-10-26 at 01:40 -0700, Matthew Garrett via Selinux
>> > wrote:
>> > > +static void selinux_cred_getsecid(const struct cred *c, u32
>> > > *secid)
>> > > +{
>> > > +     rcu_read_lock();
>> > > +     *secid = cred_sid(c);
>> > > +     rcu_read_unlock();
>> >
>> > Is rcu_read_lock() necessary here? Seems like we use cred_sid() in
>> > many
>> > places without it.
>>
>> Ah, I thought it was based on task_sid(), but I guess that's actually
>> protecting the __task_cred()?
>
> It appears to me that in all other cases, we are either dealing with
> the current cred, or something in the call chain of cred_sid() is
> holding a reference to the cred, or something in the call chain of
> cred_sid() has called rcu_read_lock() already.  I might have missed
> something though, and I don't know how safe it is to assume that all
> future callers will do this.  cc'd David for his thoughts.

Hi David,

Any opinion on this?

Thanks!

[PATCH V3 1/2] security: Add a cred_getsecid hook

[Matthew Garrett]

On Mon, Oct 30, 2017 at 10:03 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On Mon, 2017-10-30 at 10:57 +0000, Matthew Garrett via Selinux wrote:
>> On Thu, Oct 26, 2017 at 3:20 PM, Stephen Smalley <sds@tycho.nsa.gov>
>> wrote:
>> > On Thu, 2017-10-26 at 01:40 -0700, Matthew Garrett via Selinux
>> > wrote:
>> > > +static void selinux_cred_getsecid(const struct cred *c, u32
>> > > *secid)
>> > > +{
>> > > +     rcu_read_lock();
>> > > +     *secid = cred_sid(c);
>> > > +     rcu_read_unlock();
>> >
>> > Is rcu_read_lock() necessary here? Seems like we use cred_sid() in
>> > many
>> > places without it.
>>
>> Ah, I thought it was based on task_sid(), but I guess that's actually
>> protecting the __task_cred()?
>
> It appears to me that in all other cases, we are either dealing with
> the current cred, or something in the call chain of cred_sid() is
> holding a reference to the cred, or something in the call chain of
> cred_sid() has called rcu_read_lock() already.  I might have missed
> something though, and I don't know how safe it is to assume that all
> future callers will do this.  cc'd David for his thoughts.

Hi David,

Any opinion on this?

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html