| Message ID | 20250225131255.154826-1-enrico.bravi@polito.it (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] ima: add measurement for first unverified write on ima policy file | expand |
On Tue, 2025-02-25 at 14:12 +0100, Enrico Bravi wrote: > The first write on the ima policy file permits to override the default > policy defined with the ima_policy= boot parameter. This can be done > by adding the /etc/ima/ima-policy which allows loading the custom policy > during boot. It is also possible to load custom policy at runtime through > file operations: > > cp custom_ima_policy /sys/kernel/security/ima/policy > cat custom_ima_policy > /sys/kernel/security/ima/policy > > or by writing the absolute path of the file containing the custom policy: > > echo /path/of/custom_ima_policy > /sys/kernel/security/ima/policy > > In these cases, file signature can be necessary to load the policy > (func=POLICY_CHECK). Custom policy can also be set at runtime by directly > writing the policy stream on the ima policy file: > > echo -e "measure func=BPRM_CHECK mask=MAY_EXEC\n" \ > "audit func=BPRM_CHECK mask=MAY_EXEC\n" \ > > /sys/kernel/security/ima/policy > > In this case, there is no mechanism to verify the integrity of the new > policy. > > Add a new entry in the ima measurements list containing the ascii custom > ima policy buffer when not verified at load time. > > Signed-off-by: Enrico Bravi <enrico.bravi@polito.it> Hi Enrico, This patch set hard codes measuring the initial custom IMA policy rules that replace the builtin policies specified on the boot command line. IMA shouldn't hard code policy. I'm not quite sure why you're differentiating between measuring the initial and subsequent custom IMA policy rules. Consider defining a new critical-data record to measure the current IMA policy rules. Also consider including the new critical-data rule in the arch specific policy rules. thanks, Mimi
On Tue, 2025-02-25 at 20:53 -0500, Mimi Zohar wrote: > On Tue, 2025-02-25 at 14:12 +0100, Enrico Bravi wrote: > > The first write on the ima policy file permits to override the default > > policy defined with the ima_policy= boot parameter. This can be done > > by adding the /etc/ima/ima-policy which allows loading the custom policy > > during boot. It is also possible to load custom policy at runtime through > > file operations: > > > > cp custom_ima_policy /sys/kernel/security/ima/policy > > cat custom_ima_policy > /sys/kernel/security/ima/policy > > > > or by writing the absolute path of the file containing the custom policy: > > > > echo /path/of/custom_ima_policy > /sys/kernel/security/ima/policy > > > > In these cases, file signature can be necessary to load the policy > > (func=POLICY_CHECK). Custom policy can also be set at runtime by directly > > writing the policy stream on the ima policy file: > > > > echo -e "measure func=BPRM_CHECK mask=MAY_EXEC\n" \ > > "audit func=BPRM_CHECK mask=MAY_EXEC\n" \ > > > /sys/kernel/security/ima/policy > > > > In this case, there is no mechanism to verify the integrity of the new > > policy. > > > > Add a new entry in the ima measurements list containing the ascii custom > > ima policy buffer when not verified at load time. > > > > Signed-off-by: Enrico Bravi <enrico.bravi@polito.it> > > Hi Enrico, Hi Mimi, thank you for the quick response. > This patch set hard codes measuring the initial custom IMA policy rules that > replace the builtin policies specified on the boot command line. IMA > shouldn't hard code policy. My first approach was to define a new critical-data record, but performing the measurement after the custom policy becomes effective, the measurement could be bypassed omitting func=CRITICAL_DATA in the custom policy. > I'm not quite sure why you're differentiating between > measuring the initial and subsequent custom IMA policy rules. My intention is to measure the first direct write (line by line) on the policy file, without loading the initial custom policy from a file. This case, if I'm not wrong, is not covered by func=POLICY_CHECK. > Consider defining a new critical-data record to measure the current IMA policy > rules. Also consider including the new critical-data rule in the arch > specific policy rules. I think that your suggestion, to add the critical-data rule in the arch policy rules, solves the problems of bypassing the measurement and hard coding policy. Thank you very much for your feedback. Enrico > thanks, > > Mimi >
On Wed, 2025-02-26 at 22:53 +0000, Enrico Bravi wrote: > On Tue, 2025-02-25 at 20:53 -0500, Mimi Zohar wrote: > > On Tue, 2025-02-25 at 14:12 +0100, Enrico Bravi wrote: > > > The first write on the ima policy file permits to override the default > > > policy defined with the ima_policy= boot parameter. This can be done > > > by adding the /etc/ima/ima-policy which allows loading the custom policy > > > during boot. It is also possible to load custom policy at runtime through > > > file operations: > > > > > > cp custom_ima_policy /sys/kernel/security/ima/policy > > > cat custom_ima_policy > /sys/kernel/security/ima/policy > > > > > > or by writing the absolute path of the file containing the custom policy: > > > > > > echo /path/of/custom_ima_policy > /sys/kernel/security/ima/policy > > > > > > In these cases, file signature can be necessary to load the policy > > > (func=POLICY_CHECK). Custom policy can also be set at runtime by directly > > > writing the policy stream on the ima policy file: > > > > > > echo -e "measure func=BPRM_CHECK mask=MAY_EXEC\n" \ > > > "audit func=BPRM_CHECK mask=MAY_EXEC\n" \ > > > > /sys/kernel/security/ima/policy > > > > > > In this case, there is no mechanism to verify the integrity of the new > > > policy. > > > > > > Add a new entry in the ima measurements list containing the ascii custom > > > ima policy buffer when not verified at load time. > > > > > > Signed-off-by: Enrico Bravi <enrico.bravi@polito.it> > > > > Hi Enrico, > > Hi Mimi, > > thank you for the quick response. > > > This patch set hard codes measuring the initial custom IMA policy rules that > > replace the builtin policies specified on the boot command line. IMA > > shouldn't hard code policy. > > My first approach was to define a new critical-data record, Hopefully the new critical-data will be of the entire IMA policy. > but performing the > measurement after the custom policy becomes effective, the measurement could be > bypassed omitting func=CRITICAL_DATA in the custom policy. > > > I'm not quite sure why you're differentiating between > > measuring the initial and subsequent custom IMA policy rules. > > My intention is to measure the first direct write (line by line) on the policy > file, without loading the initial custom policy from a file. This case, if I'm > not wrong, is not covered by func=POLICY_CHECK. When secure boot is enabled, the arch specific policy rules require the IMA policy to be signed. Without secure boot enabled, you're correct. The custom policy rules may directly be loaded without being measured. Why only measure "the first direct write"? Additional custom policy rules may be directly appended without being measured. > > > Consider defining a new critical-data record to measure the current IMA policy > > rules. Also consider including the new critical-data rule in the arch > > specific policy rules. > > I think that your suggestion, to add the critical-data rule in the arch policy > rules, solves the problems of bypassing the measurement and hard coding policy. > > Thank you very much for your feedback. You're welcome. Mimi >
On Wed, 2025-02-26 at 22:05 -0500, Mimi Zohar wrote: > On Wed, 2025-02-26 at 22:53 +0000, Enrico Bravi wrote: > > On Tue, 2025-02-25 at 20:53 -0500, Mimi Zohar wrote: > > > On Tue, 2025-02-25 at 14:12 +0100, Enrico Bravi wrote: > > > > The first write on the ima policy file permits to override the default > > > > policy defined with the ima_policy= boot parameter. This can be done > > > > by adding the /etc/ima/ima-policy which allows loading the custom policy > > > > during boot. It is also possible to load custom policy at runtime > > > > through > > > > file operations: > > > > > > > > cp custom_ima_policy /sys/kernel/security/ima/policy > > > > cat custom_ima_policy > /sys/kernel/security/ima/policy > > > > > > > > or by writing the absolute path of the file containing the custom > > > > policy: > > > > > > > > echo /path/of/custom_ima_policy > /sys/kernel/security/ima/policy > > > > > > > > In these cases, file signature can be necessary to load the policy > > > > (func=POLICY_CHECK). Custom policy can also be set at runtime by > > > > directly > > > > writing the policy stream on the ima policy file: > > > > > > > > echo -e "measure func=BPRM_CHECK mask=MAY_EXEC\n" \ > > > > "audit func=BPRM_CHECK mask=MAY_EXEC\n" \ > > > > > /sys/kernel/security/ima/policy > > > > > > > > In this case, there is no mechanism to verify the integrity of the new > > > > policy. > > > > > > > > Add a new entry in the ima measurements list containing the ascii custom > > > > ima policy buffer when not verified at load time. > > > > > > > > Signed-off-by: Enrico Bravi <enrico.bravi@polito.it> > > > > > > Hi Enrico, > > > > Hi Mimi, > > > > thank you for the quick response. > > > > > This patch set hard codes measuring the initial custom IMA policy rules > > > that > > > replace the builtin policies specified on the boot command line. IMA > > > shouldn't hard code policy. > > > > My first approach was to define a new critical-data record, Hi Mimi, > Hopefully the new critical-data will be of the entire IMA policy. yes, absolutely. > > but performing the > > measurement after the custom policy becomes effective, the measurement could > > be > > bypassed omitting func=CRITICAL_DATA in the custom policy. > > > > > I'm not quite sure why you're differentiating between > > > measuring the initial and subsequent custom IMA policy rules. > > > > My intention is to measure the first direct write (line by line) on the > > policy > > file, without loading the initial custom policy from a file. This case, if > > I'm > > not wrong, is not covered by func=POLICY_CHECK. > > When secure boot is enabled, the arch specific policy rules require the IMA > policy to be signed. Without secure boot enabled, you're correct. The custom > policy rules may directly be loaded without being measured. > > Why only measure "the first direct write"? Additional custom policy rules may > be directly appended without being measured. Yes, you right. The aim was to measure (at least) the first one, because it substitutes the boot policy, but if you are ok with adding a critical-data record, it would be definitely better. Thank you, Enrico > > > > > Consider defining a new critical-data record to measure the current IMA > > > policy > > > rules. Also consider including the new critical-data rule in the arch > > > specific policy rules. > > > > I think that your suggestion, to add the critical-data rule in the arch > > policy > > rules, solves the problems of bypassing the measurement and hard coding > > policy. > > > > Thank you very much for your feedback. > > You're welcome. > > Mimi > >
On Thu, 2025-02-27 at 11:36 +0000, Enrico Bravi wrote: > On Wed, 2025-02-26 at 22:05 -0500, Mimi Zohar wrote: > > On Wed, 2025-02-26 at 22:53 +0000, Enrico Bravi wrote: > > > On Tue, 2025-02-25 at 20:53 -0500, Mimi Zohar wrote: > > > > On Tue, 2025-02-25 at 14:12 +0100, Enrico Bravi wrote: > > > > > The first write on the ima policy file permits to override the default > > > > > policy defined with the ima_policy= boot parameter. This can be done > > > > > by adding the /etc/ima/ima-policy which allows loading the custom policy > > > > > during boot. It is also possible to load custom policy at runtime > > > > > through > > > > > file operations: > > > > > > > > > > cp custom_ima_policy /sys/kernel/security/ima/policy > > > > > cat custom_ima_policy > /sys/kernel/security/ima/policy > > > > > > > > > > or by writing the absolute path of the file containing the custom > > > > > policy: > > > > > > > > > > echo /path/of/custom_ima_policy > /sys/kernel/security/ima/policy > > > > > > > > > > In these cases, file signature can be necessary to load the policy > > > > > (func=POLICY_CHECK). Custom policy can also be set at runtime by > > > > > directly > > > > > writing the policy stream on the ima policy file: > > > > > > > > > > echo -e "measure func=BPRM_CHECK mask=MAY_EXEC\n" \ > > > > > "audit func=BPRM_CHECK mask=MAY_EXEC\n" \ > > > > > > /sys/kernel/security/ima/policy > > > > > > > > > > In this case, there is no mechanism to verify the integrity of the new > > > > > policy. > > > > > > > > > > Add a new entry in the ima measurements list containing the ascii custom > > > > > ima policy buffer when not verified at load time. > > > > > > > > > > Signed-off-by: Enrico Bravi <enrico.bravi@polito.it> > > > > > > > > Hi Enrico, > > > > > > Hi Mimi, > > > > > > thank you for the quick response. > > > > > > > This patch set hard codes measuring the initial custom IMA policy rules > > > > that > > > > replace the builtin policies specified on the boot command line. IMA > > > > shouldn't hard code policy. > > > > > > My first approach was to define a new critical-data record, > > Hi Mimi, > > > Hopefully the new critical-data will be of the entire IMA policy. > > yes, absolutely. > > > > but performing the > > > measurement after the custom policy becomes effective, the measurement could > > > be > > > bypassed omitting func=CRITICAL_DATA in the custom policy. > > > > > > > I'm not quite sure why you're differentiating between > > > > measuring the initial and subsequent custom IMA policy rules. > > > > > > My intention is to measure the first direct write (line by line) on the > > > policy > > > file, without loading the initial custom policy from a file. This case, if > > > I'm > > > not wrong, is not covered by func=POLICY_CHECK. > > > > When secure boot is enabled, the arch specific policy rules require the IMA > > policy to be signed. Without secure boot enabled, you're correct. The custom > > policy rules may directly be loaded without being measured. > > > > Why only measure "the first direct write"? Additional custom policy rules may > > be directly appended without being measured. > > Yes, you right. The aim was to measure (at least) the first one, because it > substitutes the boot policy, but if you are ok with adding a critical-data > record, it would be definitely better. Hi Enrico in addition to what Mimi suggested, I also like to idea that the POLICY_CHECK hook catches the direct policy loading. That would mean that those updates would be seen if the 'tcb' IMA policy is selected. I would have recommended to try to add a process_measurement() call in ima_write_policy(), where the buffer to be processed is. However, I guess you need to have a valid file descriptor in order to use that function (maybe an anonymous inode?). ima_collect_measurement() should be already able to handle buffers, passed by ima_post_read_file(). Thanks Roberto > Thank you, > > Enrico > > > > > > > > Consider defining a new critical-data record to measure the current IMA > > > > policy > > > > rules. Also consider including the new critical-data rule in the arch > > > > specific policy rules. > > > > > > I think that your suggestion, to add the critical-data rule in the arch > > > policy > > > rules, solves the problems of bypassing the measurement and hard coding > > > policy. > > > > > > Thank you very much for your feedback. > > > > You're welcome. > > > > Mimi > > > >
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index c0d3b716d11f..27cba2e612a5 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -46,6 +46,8 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8, TPM_PCR10 = 10 }; /* current content of the policy */ extern int ima_policy_flag; +extern bool override_ima_policy; + /* bitset of digests algorithms allowed in the setxattr hook */ extern atomic_t ima_setxattr_allowed_hash_algorithms; @@ -414,6 +416,7 @@ void *ima_policy_start(struct seq_file *m, loff_t *pos); void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos); void ima_policy_stop(struct seq_file *m, void *v); int ima_policy_show(struct seq_file *m, void *v); +void ima_measure_override_policy(size_t file_len); /* Appraise integrity measurements */ #define IMA_APPRAISE_ENFORCE 0x01 diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c index e4a79a9b2d58..8c516de4aebe 100644 --- a/security/integrity/ima/ima_fs.c +++ b/security/integrity/ima/ima_fs.c @@ -309,6 +309,9 @@ static const struct file_operations ima_ascii_measurements_ops = { .release = seq_release, }; +static size_t text_policy_len; +bool override_ima_policy; + static ssize_t ima_read_policy(char *path) { void *data = NULL; @@ -383,6 +386,7 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf, result = -EACCES; } else { result = ima_parse_add_rule(data); + text_policy_len += (result + 1); } mutex_unlock(&ima_write_mutex); out_free: @@ -532,6 +536,10 @@ static int ima_release_policy(struct inode *inode, struct file *file) } ima_update_policy(); + if (unlikely(override_ima_policy && text_policy_len)) { + ima_measure_override_policy(text_policy_len); + override_ima_policy = false; + } #if !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY) securityfs_remove(ima_policy); ima_policy = NULL; @@ -558,6 +566,9 @@ int __init ima_fs_init(void) ascii_securityfs_measurement_lists = NULL; binary_securityfs_measurement_lists = NULL; + text_policy_len = 0; + override_ima_policy = false; + ima_dir = securityfs_create_dir("ima", integrity_dir); if (IS_ERR(ima_dir)) return PTR_ERR(ima_dir); diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 21a8e54c383f..34753bce4668 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -17,6 +17,7 @@ #include <linux/slab.h> #include <linux/rculist.h> #include <linux/seq_file.h> +#include <linux/vmalloc.h> #include <linux/ima.h> #include "ima.h" @@ -1044,6 +1045,8 @@ void ima_update_policy(void) if (ima_rules != (struct list_head __rcu *)policy) { ima_policy_flag = 0; + override_ima_policy = true; + rcu_assign_pointer(ima_rules, policy); /* * IMA architecture specific policy rules are specified @@ -1982,7 +1985,6 @@ const char *const func_tokens[] = { __ima_hooks(__ima_hook_stringify) }; -#ifdef CONFIG_IMA_READ_POLICY enum { mask_exec = 0, mask_write, mask_read, mask_append }; @@ -1994,6 +1996,7 @@ static const char *const mask_tokens[] = { "^MAY_APPEND" }; +#ifdef CONFIG_IMA_READ_POLICY void *ima_policy_start(struct seq_file *m, loff_t *pos) { loff_t l = *pos; @@ -2028,6 +2031,7 @@ void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos) void ima_policy_stop(struct seq_file *m, void *v) { } +#endif /* CONFIG_IMA_READ_POLICY */ #define pt(token) policy_tokens[token].pattern #define mt(token) mask_tokens[token] @@ -2276,7 +2280,6 @@ int ima_policy_show(struct seq_file *m, void *v) seq_puts(m, "\n"); return 0; } -#endif /* CONFIG_IMA_READ_POLICY */ #if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING) /* @@ -2333,3 +2336,77 @@ bool ima_appraise_signature(enum kernel_read_file_id id) return found; } #endif /* CONFIG_IMA_APPRAISE && CONFIG_INTEGRITY_TRUSTED_KEYRING */ + +void ima_measure_override_policy(size_t file_len) +{ + struct ima_iint_cache iint = {}; + const char event_name[] = "ima-policy-override"; + struct ima_event_data event_data = {.iint = &iint, + .filename = event_name}; + struct ima_max_digest_data hash; + struct ima_digest_data *hash_hdr = container_of(&hash.hdr, + struct ima_digest_data, hdr); + static const char op[] = "measure_ima_policy_override"; + struct ima_template_entry *entry = NULL; + static char *audit_cause = "ENOMEM"; + struct ima_template_desc *template; + struct ima_rule_entry *rule_entry; + struct list_head *ima_rules_tmp; + struct seq_file file; + int result = -ENOMEM; + int violation = 0; + + file.buf = vmalloc(file_len); + if (!file.buf) + goto out; + + file.read_pos = 0; + file.size = file_len; + file.count = 0; + + rcu_read_lock(); + ima_rules_tmp = rcu_dereference(ima_rules); + list_for_each_entry_rcu(rule_entry, ima_rules_tmp, list) { + ima_policy_show(&file, rule_entry); + } + rcu_read_unlock(); + + event_data.buf = file.buf; + event_data.buf_len = file.count; + + template = ima_template_desc_buf(); + if (!template) { + audit_cause = "ima_template_desc_buf"; + goto out_free; + } + + iint.ima_hash = hash_hdr; + iint.ima_hash->algo = ima_hash_algo; + iint.ima_hash->length = hash_digest_size[ima_hash_algo]; + + result = ima_calc_buffer_hash(file.buf, file.count, iint.ima_hash); + if (result < 0) { + audit_cause = "hashing_error"; + goto out_free; + } + + result = ima_alloc_init_template(&event_data, &entry, template); + if (result < 0) { + audit_cause = "alloc_entry"; + goto out_free; + } + + result = ima_store_template(entry, violation, NULL, event_data.buf, + CONFIG_IMA_MEASURE_PCR_IDX); + if (result < 0) { + audit_cause = "store_entry"; + ima_free_template_entry(entry); + } + +out_free: + kvfree(file.buf); +out: + if (result < 0) + integrity_audit_msg(AUDIT_INTEGRITY_PCR, NULL, event_name, + op, audit_cause, result, 1); +}