On Thu, 2021-09-09 at 10:51 -0600, Alex Henrie wrote: > From: Curtis Veit <veit@xxxxxxxxxx> > > IMA currently supports the concept of rules based on uid where the rule > is based on the uid of the file owner or the uid of the user accessing > the file. It is useful to have similar rules based on gid. This patch > provides that ability. > > Signed-off-by: Curtis Veit <veit@xxxxxxxxxx> > Co-developed-by: Alex Henrie <alexh@xxxxxxxxxxx> > Signed-off-by: Alex Henrie <alexh@xxxxxxxxxxx> Thanks, Alex. There are a couple of places were the code is duplicated, except for the indentation. At least for now, let's keep the line length at 80 chars. Similarly, when extending a comment/documentation, please split the line. A few minor comments below, otherwise: Reviewed-by: Mimi Zohar <zohar@xxxxxxxxxxxxx> > --- > This is the patch that Curtis sent in 2015,[1] rebased and with support > for effective GID and the new greater-than and less-than operators. > Moreover, the policy_opt enum is now in the same order as the > policy_tokens array, which I'm guessing is the bug that prevented the > patch from being accepted before. That sounds right. > > [1] https://sourceforge.net/p/linux-ima/mailman/message/34210250/ > --- > Documentation/ABI/testing/ima_policy | 5 +- > security/integrity/ima/ima_policy.c | 197 +++++++++++++++++++++++---- > 2 files changed, 174 insertions(+), 28 deletions(-) > > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy > index 5c2798534950..f8e5b5598548 100644 > --- a/Documentation/ABI/testing/ima_policy > +++ b/Documentation/ABI/testing/ima_policy > @@ -23,7 +23,7 @@ Description: > audit | hash | dont_hash > condition:= base | lsm [option] > base: [[func=] [mask=] [fsmagic=] [fsuuid=] [uid=] > - [euid=] [fowner=] [fsname=]] > + [euid=] [gid=] [egid=] [fowner=] [fgroup=] [fsname=]] Like in other places where you modified the ordering, let's group uid, euid, git, egid, etc together on the same line. > lsm: [[subj_user=] [subj_role=] [subj_type=] > [obj_user=] [obj_role=] [obj_type=]] > option: [[appraise_type=]] [template=] [permit_directio] > @@ -40,7 +40,10 @@ Description: > fsuuid:= file system UUID (e.g 8bcbe394-4f13-4144-be8e-5aa9ea2ce2f6) > uid:= decimal value > euid:= decimal value > + gid:= decimal value > + egid:= decimal value > fowner:= decimal value > + fgroup:= decimal value > lsm: are LSM specific > option: > appraise_type:= [imasig] [imasig|modsig] > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > index 87b9b71cb820..0f2287699f85 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > @@ -36,6 +36,9 @@ > #define IMA_KEYRINGS 0x0400 > #define IMA_LABEL 0x0800 > #define IMA_VALIDATE_ALGOS 0x1000 > +#define IMA_GID 0x2000 > +#define IMA_EGID 0x4000 > +#define IMA_FGROUP 0x8000 Ok, any additional changes after this will need to increase the "flags" size. > > #define UNKNOWN 0 > #define MEASURE 0x0001 /* same as IMA_MEASURE */ > @@ -78,9 +81,13 @@ struct ima_rule_entry { > unsigned long fsmagic; > uuid_t fsuuid; > kuid_t uid; > + kgid_t gid; > kuid_t fowner; > + kgid_t fgroup; > bool (*uid_op)(kuid_t, kuid_t); /* Handlers for operators */ > + bool (*gid_op)(kgid_t, kgid_t); > bool (*fowner_op)(kuid_t, kuid_t); /* uid_eq(), uid_gt(), uid_lt() */ > + bool (*fgroup_op)(kgid_t, kgid_t); /* gid_eq(), gid_gt(), gid_lt() */ > int pcr; > unsigned int allowed_algos; /* bitfield of allowed hash algorithms */ > struct { > @@ -104,7 +111,8 @@ static_assert( > > /* > * 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, .gid, > + * .fowner, and .fgroup > */ > > /* > @@ -582,10 +590,23 @@ static bool ima_match_rules(struct ima_rule_entry *rule, > } else if (!rule->uid_op(cred->euid, rule->uid)) > return false; > } > - > + if ((rule->flags & IMA_GID) && !rule->gid_op(rule->gid, cred->gid)) > + return false; > + if (rule->flags & IMA_EGID) { > + if (has_capability_noaudit(current, CAP_SETGID)) { > + if (!rule->gid_op(cred->egid, rule->gid) > + && !rule->gid_op(cred->sgid, rule->gid) > + && !rule->gid_op(cred->gid, rule->gid)) > + return false; > + } else if (!rule->gid_op(cred->egid, rule->gid)) > + return false; > + } > if ((rule->flags & IMA_FOWNER) && > !rule->fowner_op(i_uid_into_mnt(mnt_userns, inode), rule->fowner)) > return false; > + if ((rule->flags & IMA_FGROUP) && > + !rule->fgroup_op(i_gid_into_mnt(mnt_userns, inode), rule->fgroup)) > + return false; > for (i = 0; i < MAX_LSM_RULES; i++) { > int rc = 0; > u32 osid; > @@ -987,16 +1008,19 @@ void ima_update_policy(void) > } > > /* Keep the enumeration in sync with the policy_tokens! */ > -enum { > +enum policy_opt { > Opt_measure, Opt_dont_measure, > Opt_appraise, Opt_dont_appraise, > Opt_audit, Opt_hash, Opt_dont_hash, > 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_fsname, > - Opt_fsuuid, Opt_uid_eq, Opt_euid_eq, Opt_fowner_eq, > - Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt, > - Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt, > + Opt_func, Opt_mask, Opt_fsmagic, Opt_fsname, Opt_fsuuid, > + Opt_uid_eq, Opt_euid_eq, Opt_gid_eq, Opt_egid_eq, > + Opt_fowner_eq, Opt_fgroup_eq, > + Opt_uid_gt, Opt_euid_gt, Opt_gid_gt, Opt_egid_gt, > + Opt_fowner_gt, Opt_fgroup_gt, > + Opt_uid_lt, Opt_euid_lt, Opt_gid_lt, Opt_egid_lt, > + Opt_fowner_lt, Opt_fgroup_lt, > Opt_appraise_type, Opt_appraise_flag, Opt_appraise_algos, > Opt_permit_directio, Opt_pcr, Opt_template, Opt_keyrings, > Opt_label, Opt_err > @@ -1023,13 +1047,22 @@ static const match_table_t policy_tokens = { > {Opt_fsuuid, "fsuuid=%s"}, > {Opt_uid_eq, "uid=%s"}, > {Opt_euid_eq, "euid=%s"}, > + {Opt_gid_eq, "gid=%s"}, > + {Opt_egid_eq, "egid=%s"}, > {Opt_fowner_eq, "fowner=%s"}, > + {Opt_fgroup_eq, "fgroup=%s"}, > {Opt_uid_gt, "uid>%s"}, > {Opt_euid_gt, "euid>%s"}, > + {Opt_gid_gt, "gid>%s"}, > + {Opt_egid_gt, "egid>%s"}, > {Opt_fowner_gt, "fowner>%s"}, > + {Opt_fgroup_gt, "fgroup>%s"}, > {Opt_uid_lt, "uid<%s"}, > {Opt_euid_lt, "euid<%s"}, > + {Opt_gid_lt, "gid<%s"}, > + {Opt_egid_lt, "egid<%s"}, > {Opt_fowner_lt, "fowner<%s"}, > + {Opt_fgroup_lt, "fgroup<%s"}, > {Opt_appraise_type, "appraise_type=%s"}, > {Opt_appraise_flag, "appraise_flag=%s"}, > {Opt_appraise_algos, "appraise_algos=%s"}, > @@ -1073,22 +1106,36 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry, > } > > static void ima_log_string_op(struct audit_buffer *ab, char *key, char *value, > - bool (*rule_operator)(kuid_t, kuid_t)) > + enum policy_opt rule_operator) > { > if (!ab) > return; > > - if (rule_operator == &uid_gt) > + switch (rule_operator) { > + case Opt_uid_gt: > + case Opt_euid_gt: > + case Opt_gid_gt: > + case Opt_egid_gt: > + case Opt_fowner_gt: > + case Opt_fgroup_gt: > audit_log_format(ab, "%s>", key); > - else if (rule_operator == &uid_lt) > + break; > + case Opt_uid_lt: > + case Opt_euid_lt: > + case Opt_gid_lt: > + case Opt_egid_lt: > + case Opt_fowner_lt: > + case Opt_fgroup_lt: > audit_log_format(ab, "%s<", key); > - else > + break; > + default: > audit_log_format(ab, "%s=", key); > + } > audit_log_format(ab, "%s ", value); > } > static void ima_log_string(struct audit_buffer *ab, char *key, char *value) > { > - ima_log_string_op(ab, key, value, NULL); > + ima_log_string_op(ab, key, value, Opt_err); > } > > /* > @@ -1163,7 +1210,8 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) > if (entry->flags & ~(IMA_FUNC | IMA_MASK | IMA_FSMAGIC | > IMA_UID | IMA_FOWNER | IMA_FSUUID | > IMA_INMASK | IMA_EUID | IMA_PCR | > - IMA_FSNAME | IMA_DIGSIG_REQUIRED | > + IMA_FSNAME | IMA_GID | IMA_EGID | > + IMA_FGROUP | IMA_DIGSIG_REQUIRED | > IMA_PERMIT_DIRECTIO | IMA_VALIDATE_ALGOS)) > return false; > > @@ -1174,7 +1222,8 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) > if (entry->flags & ~(IMA_FUNC | IMA_MASK | IMA_FSMAGIC | > IMA_UID | IMA_FOWNER | IMA_FSUUID | > IMA_INMASK | IMA_EUID | IMA_PCR | > - IMA_FSNAME | IMA_DIGSIG_REQUIRED | > + IMA_FSNAME | IMA_GID | IMA_EGID | > + IMA_FGROUP | IMA_DIGSIG_REQUIRED | > IMA_PERMIT_DIRECTIO | IMA_MODSIG_ALLOWED | > IMA_CHECK_BLACKLIST | IMA_VALIDATE_ALGOS)) > return false; > @@ -1186,7 +1235,8 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) > > if (entry->flags & ~(IMA_FUNC | IMA_FSMAGIC | IMA_UID | > IMA_FOWNER | IMA_FSUUID | IMA_EUID | > - IMA_PCR | IMA_FSNAME)) > + IMA_PCR | IMA_FSNAME | IMA_GID | IMA_EGID | > + IMA_FGROUP)) > return false; > > break; > @@ -1195,7 +1245,7 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) > return false; > > if (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_PCR | > - IMA_KEYRINGS)) > + IMA_KEYRINGS | IMA_GID)) > return false; > > if (ima_rule_contains_lsm_cond(entry)) > @@ -1207,7 +1257,7 @@ static bool ima_validate_rule(struct ima_rule_entry *entry) > return false; > > if (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_PCR | > - IMA_LABEL)) > + IMA_LABEL | IMA_GID)) How about moving IMA_GID after IMA_UID? > return false; > > if (ima_rule_contains_lsm_cond(entry)) > @@ -1276,7 +1326,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) > struct audit_buffer *ab; > char *from; > char *p; > - bool uid_token; > + bool eid_token; Took a moment to understand the reason for this change. Adding a comment on the line, like "either euid or egid" might have helped. > struct ima_template_desc *template_desc; > int result = 0; > > @@ -1284,9 +1334,13 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) > AUDIT_INTEGRITY_POLICY_RULE); > > entry->uid = INVALID_UID; > + entry->gid = INVALID_GID; > entry->fowner = INVALID_UID; > + entry->fgroup = INVALID_GID; > entry->uid_op = &uid_eq; > + entry->gid_op = &gid_eq; > entry->fowner_op = &uid_eq; > + entry->fgroup_op = &gid_eq; > entry->action = UNKNOWN; > while ((p = strsep(&rule, " \t")) != NULL) { > substring_t args[MAX_OPT_ARGS]; > @@ -1504,12 +1558,12 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) > fallthrough; > case Opt_uid_eq: > case Opt_euid_eq: > - uid_token = (token == Opt_uid_eq) || > - (token == Opt_uid_gt) || > - (token == Opt_uid_lt); > + eid_token = (token == Opt_euid_eq) || > + (token == Opt_euid_gt) || > + (token == Opt_euid_lt); > > - ima_log_string_op(ab, uid_token ? "uid" : "euid", > - args[0].from, entry->uid_op); > + ima_log_string_op(ab, eid_token ? "euid" : "uid", > + args[0].from, token); > > if (uid_valid(entry->uid)) { > result = -EINVAL; > @@ -1524,8 +1578,41 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) > (uid_t)lnum != lnum) > result = -EINVAL; > else > - entry->flags |= uid_token > - ? IMA_UID : IMA_EUID; > + entry->flags |= eid_token > + ? IMA_EUID : IMA_UID; > + } > + break; > + case Opt_gid_gt: > + case Opt_egid_gt: > + entry->gid_op = &gid_gt; > + fallthrough; > + case Opt_gid_lt: > + case Opt_egid_lt: > + if ((token == Opt_gid_lt) || (token == Opt_egid_lt)) > + entry->gid_op = &gid_lt; > + fallthrough; > + case Opt_gid_eq: > + case Opt_egid_eq: > + eid_token = (token == Opt_egid_eq) || > + (token == Opt_egid_gt) || > + (token == Opt_egid_lt); > + > + ima_log_string_op(ab, eid_token ? "egid" : "gid", > + args[0].from, token); > + > + 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)) Example of where the indentation changes from original "uid" code. > + result = -EINVAL; > + else > + entry->flags |= eid_token > + ? IMA_EGID : IMA_GID; > } > break; > case Opt_fowner_gt: > @@ -1536,8 +1623,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) > entry->fowner_op = &uid_lt; > fallthrough; > case Opt_fowner_eq: > - ima_log_string_op(ab, "fowner", args[0].from, > - entry->fowner_op); > + ima_log_string_op(ab, "fowner", args[0].from, token); > > if (uid_valid(entry->fowner)) { > result = -EINVAL; > @@ -1553,6 +1639,30 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry) > entry->flags |= IMA_FOWNER; > } > break; > + case Opt_fgroup_gt: > + entry->fgroup_op = &gid_gt; > + fallthrough; > + case Opt_fgroup_lt: > + if (token == Opt_fgroup_lt) > + entry->fgroup_op = &gid_lt; > + fallthrough; > + case Opt_fgroup_eq: > + ima_log_string_op(ab, "fgroup", args[0].from, token); > + > + 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)) and here > + 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,
