[LTP] [PATCH linux rebase] ima: add gid support
Mimi Zohar
zohar@linux.ibm.com
Tue Oct 5 00:30:55 CEST 2021
On Thu, 2021-09-09 at 10:51 -0600, Alex Henrie wrote:
> From: Curtis Veit <veit@vpieng.com>
>
> 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@vpieng.com>
> Co-developed-by: Alex Henrie <alexh@vpitech.com>
> Signed-off-by: Alex Henrie <alexh@vpitech.com>
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@linux.ibm.com>
> ---
> 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,
More information about the ltp
mailing list