[Linux-ima-user] need comments on gid and fgroup patch

[Curtis V. <cr...@so...>]

Here is a preliminary patch that I've been using that adds gid and fgroup
functionality to IMA similar to uid and fowner. (as discussed in January,
very sorry about the delay getting this out!)
It seems to test fine but I want to get comments and clean it up a bit before
submitting...
My biggest nitpick is wondering if I should be doing something with
LSM_SUBJ_GROUP (see the commented out line in the patch near 212)

Anyway looking for comments before I sign off on it.

Thanks,
Curtis


6def190 IMA support for gid and fgroup
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index d0d0c57..556a827 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -31,7 +31,9 @@ Description:
 			fsmagic:= hex value
 			fsuuid:= file system UUID (e.g 8bcbe394-4f13-4144-be8e-5aa9ea2ce2f6)
 			uid:= decimal value
+			gid:= decimal value, the group id of the access attempt
 			fowner:=decimal value
+			fgroup:=decimal value, group id of the file owner
 		lsm:  	are LSM specific
 		option:	appraise_type:= [imasig]
 
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index d1eefb9..e1b7864 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -27,6 +27,8 @@
 #define IMA_UID		0x0008
 #define IMA_FOWNER	0x0010
 #define IMA_FSUUID	0x0020
+#define IMA_GID		0x0040
+#define IMA_FGROUP	0x0080
 
 #define UNKNOWN		0
 #define MEASURE		0x0001	/* same as IMA_MEASURE */
@@ -52,6 +54,8 @@ struct ima_rule_entry {
 	u8 fsuuid[16];
 	kuid_t uid;
 	kuid_t fowner;
+	kgid_t gid;
+	kgid_t fgroup;
 	struct {
 		void *rule;	/* LSM file metadata specific */
 		void *args_p;	/* audit value */
@@ -61,7 +65,8 @@ struct ima_rule_entry {
 
 /*
  * Without LSM specific knowledge, the default policy can only be
- * written in terms of .action, .func, .mask, .fsmagic, .uid, and .fowner
+ * written in terms of .action, .func, .mask, .fsmagic, .uid, .fowner
+ * .gid, and .fgroup.
  */
 
 /*
@@ -192,6 +197,10 @@ static bool ima_match_rules(struct ima_rule_entry *rule,
 		return false;
 	if ((rule->flags & IMA_FOWNER) && !uid_eq(rule->fowner, inode->i_uid))
 		return false;
+	if ((rule->flags & IMA_GID) && !gid_eq(rule->gid, cred->gid))
+		return false;
+	if ((rule->flags & IMA_FGROUP) && !gid_eq(rule->fgroup, inode->i_gid))
+		return false;
 	for (i = 0; i < MAX_LSM_RULES; i++) {
 		int rc = 0;
 		u32 osid, sid;
@@ -212,6 +221,7 @@ retry:
 							NULL);
 			break;
 		case LSM_SUBJ_USER:
+		//case LSM_SUBJ_GROUP: //if needed
 		case LSM_SUBJ_ROLE:
 		case LSM_SUBJ_TYPE:
 			security_task_getsecid(tsk, &sid);
@@ -374,7 +384,8 @@ enum {
 	Opt_obj_user, Opt_obj_role, Opt_obj_type,
 	Opt_subj_user, Opt_subj_role, Opt_subj_type,
 	Opt_func, Opt_mask, Opt_fsmagic, Opt_uid, Opt_fowner,
-	Opt_appraise_type, Opt_fsuuid, Opt_permit_directio
+	Opt_appraise_type, Opt_fsuuid, Opt_permit_directio,
+	Opt_gid, Opt_fgroup
 };
 
 static match_table_t policy_tokens = {
@@ -395,6 +406,8 @@ static match_table_t policy_tokens = {
 	{Opt_fsuuid, "fsuuid=%s"},
 	{Opt_uid, "uid=%s"},
 	{Opt_fowner, "fowner=%s"},
+	{Opt_gid, "gid=%s"},
+	{Opt_fgroup, "fgroup=%s"},
 	{Opt_appraise_type, "appraise_type=%s"},
 	{Opt_permit_directio, "permit_directio"},
 	{Opt_err, NULL}
@@ -442,6 +455,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 
 	entry->uid = INVALID_UID;
 	entry->fowner = INVALID_UID;
+	entry->gid = INVALID_GID;
+	entry->fgroup = INVALID_GID;
 	entry->action = UNKNOWN;
 	while ((p = strsep(&rule, " \t")) != NULL) {
 		substring_t args[MAX_OPT_ARGS];
@@ -598,6 +613,40 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 					entry->flags |= IMA_FOWNER;
 			}
 			break;
+		case Opt_gid:
+			ima_log_string(ab, "gid", args[0].from);
+
+			if (gid_valid(entry->gid)) {
+				result = -EINVAL;
+				break;
+			}
+
+			result = kstrtoul(args[0].from, 10, &lnum);
+			if (!result) {
+				entry->gid = make_kgid(current_user_ns(), (gid_t)lnum);
+				if (!gid_valid(entry->gid) || (((gid_t)lnum) != lnum))
+					result = -EINVAL;
+				else
+					entry->flags |= IMA_GID;
+			}
+			break;
+		case Opt_fgroup:
+			ima_log_string(ab, "fgroup", args[0].from);
+
+			if (gid_valid(entry->fgroup)) {
+				result = -EINVAL;
+				break;
+			}
+
+			result = kstrtoul(args[0].from, 10, &lnum);
+			if (!result) {
+				entry->fgroup = make_kgid(current_user_ns(), (gid_t)lnum);
+				if (!gid_valid(entry->fgroup) || (((gid_t)lnum) != lnum))
+					result = -EINVAL;
+				else
+					entry->flags |= IMA_FGROUP;
+			}
+			break;
 		case Opt_obj_user:
 			ima_log_string(ab, "obj_user", args[0].from);
 			result = ima_lsm_rule_init(entry, args,