Hi Simon,
On Tue, 2021-07-27 at 10:23 +0000, THOBY Simon wrote:
> This patch defines a new IMA policy rule option "appraise_hash=",
> that restricts the hash algorithms accepted for the extended attribute
> security.ima when appraising.
"Define ..."
> When a policy rule uses the 'appraise_hash' option, appraisal of a
> file referenced by that rule will now fail if the digest algorithm
> employed to hash the file was not one of those explicitly listed
> in the option. In its absence, any hash algorithm compiled in the
> kernel will be accepted.
>
> For example, on a system where SELinux is properly deployed, the rule
> appraise func=BPRM_CHECK obj_type=iptables_exec_t appraise_hash=sha256,sha384
> will block the execution of iptables if the xattr security.ima of its
> executables were not hashed with either sha256 or sha384.
>
> Signed-off-by: Simon Thoby <simon.thoby@xxxxxxxxxx>
> ---
> Documentation/ABI/testing/ima_policy | 6 ++-
> security/integrity/ima/ima_policy.c | 72 ++++++++++++++++++++++++++--
> 2 files changed, 74 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index 070779e8d836..365e4c91719e 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -27,7 +27,7 @@ Description:
> lsm: [[subj_user=] [subj_role=] [subj_type=]
> [obj_user=] [obj_role=] [obj_type=]]
> option: [[appraise_type=]] [template=] [permit_directio]
> - [appraise_flag=] [keyrings=]
> + [appraise_flag=] [keyrings=] [appraise_hash=]
Nit:
Probably nicer to keep the "appraise_" options together. How about
placing it after "appraise_flag", instead of at the end.
> base:
> func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
> [FIRMWARE_CHECK]
> @@ -55,6 +55,10 @@ Description:
> label:= [selinux]|[kernel_info]|[data_label]
> data_label:= a unique string used for grouping and limiting critical data.
> For example, "selinux" to measure critical data for SELinux.
> + appraise_hash:= comma-separated list of hash algorithms
> + For example, "sha256,sha512" to only accept to appraise
> + files where the security.ima xattr was hashed with one
> + of these two algorithms.
>
> default policy:
> # PROC_SUPER_MAGIC
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 344b5b0dc1a1..a7f110cbbff0 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -92,6 +92,7 @@ struct ima_rule_entry {
> struct ima_template_desc *template;
> };
>
> +
Is this extra blank line intentional?
> /*
> * sanity check in case the kernels gains more hash algorithms that can
> * fit in an unsigned int
> @@ -962,7 +963,7 @@ enum {
> Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
> Opt_appraise_type, Opt_appraise_flag,
> Opt_permit_directio, Opt_pcr, Opt_template, Opt_keyrings,
> - Opt_label, Opt_err
> + Opt_label, Opt_appraise_hash, Opt_err
Nit: move Opt_appraise_hash after Opt_appraise_type.
> };
>
> static const match_table_t policy_tokens = {
> @@ -1000,6 +1001,7 @@ static const match_table_t policy_tokens = {
> {Opt_template, "template=%s"},
> {Opt_keyrings, "keyrings=%s"},
> {Opt_label, "label=%s"},
> + {Opt_appraise_hash, "appraise_hash=%s"},
ditto
> {Opt_err, NULL}
> };
>
> @@ -1125,7 +1127,7 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> IMA_UID | IMA_FOWNER | IMA_FSUUID |
> IMA_INMASK | IMA_EUID | IMA_PCR |
> IMA_FSNAME | IMA_DIGSIG_REQUIRED |
> - IMA_PERMIT_DIRECTIO))
> + IMA_PERMIT_DIRECTIO | IMA_VALIDATE_HASH))
> return false;
>
> break;
> @@ -1137,7 +1139,7 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> IMA_INMASK | IMA_EUID | IMA_PCR |
> IMA_FSNAME | IMA_DIGSIG_REQUIRED |
> IMA_PERMIT_DIRECTIO | IMA_MODSIG_ALLOWED |
> - IMA_CHECK_BLACKLIST))
> + IMA_CHECK_BLACKLIST | IMA_VALIDATE_HASH))
> return false;
>
> break;
> @@ -1187,6 +1189,27 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> return true;
> }
>
> +static unsigned int ima_parse_appraise_hash(char *arg)
> +{
> + unsigned int res = 0;
> + char *token;
> +
> + while ((token = strsep(&arg, ",")) != NULL) {
> + int idx = match_string(hash_algo_name, HASH_ALGO__LAST, token);
Move the variable definition to the beginning of the function.
> +
> + if (idx < 0) {
> + pr_err("unknown hash algorithm \"%s\", ignoring",
> + token);
> + continue;
> + }
> +
> + /* Add the hash algorithm to the 'allowed' bitfield */
> + res |= (1U << idx);
> + }
> +
> + return res;
> +}
> +
> static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> {
> struct audit_buffer *ab;
> @@ -1204,6 +1227,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> entry->uid_op = &uid_eq;
> entry->fowner_op = &uid_eq;
> entry->action = UNKNOWN;
> + entry->allowed_hashes = 0;
> while ((p = strsep(&rule, " \t")) != NULL) {
> substring_t args[MAX_OPT_ARGS];
> int token;
> @@ -1556,6 +1580,23 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> &(template_desc->fields),
> &(template_desc->num_fields));
> entry->template = template_desc;
> + break;
> + case Opt_appraise_hash:
ditto
"appraise_hash=" should be limited to appraise rules. Please update
ima_validate_rule().
thanks,
Mimi
> + ima_log_string(ab, "appraise_hash", args[0].from);
> +
> + if (entry->allowed_hashes) {
> + result = -EINVAL;
> + break;
> + }
> +
> + entry->allowed_hashes = ima_parse_appraise_hash(args[0].from);
> + if (!entry->allowed_hashes) {
> + result = -EINVAL;
> + break;
> + }
> +
> + entry->flags |= IMA_VALIDATE_HASH;
> +
> break;
> case Opt_err:
> ima_log_string(ab, "UNKNOWN", p);
>
<trim>
