| Message ID | 20201111205946.503-1-nramas@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: select ima-buf template for buffer measurement | expand |
Hi Lakshmi, On Wed, 2020-11-11 at 12:59 -0800, Lakshmi Ramasubramanian wrote: > The default IMA template for measuring buffer should be 'ima-buf' - so > that the measured buffer is correctly included in the IMA measurement > log entry. But the default IMA template used for all policy rules is > the value set for CONFIG_IMA_DEFAULT_TEMPLATE if the policy rule does > not specify a template. The second sentence defines the current status. The first sentence describes the problem. I would reverse the sentence order. ^measuring buffer -> buffer measurements > IMA does not take into account the template > requirements of different rules when choosing a default template for > a given policy rule. This breaks the buffer measurement if the template > is not provided as part of the rule because the default template could > be different than 'ima-buf'. Does the above paragraph add anything new? Instead describe the problem. Perhaps something like: With the default template format, buffer measurements are added to the measurement list, but do not include the buffer data, making it difficult, if not impossible, to validate. Including "ima-buf" template records in the measurement list by default, should not impact existing attestation servers without "ima-buf" template support. > > For example, the following IMA policy rule enables measuring > the command line arguments passed to the new kernel on kexec system call. > > measure func=KEXEC_CMDLINE > > The IMA template selected should be 'ima-buf' to have the measured > command line arguments included in the IMA measurement log entry. > Instead the default IMA template is selected, which could be different > than 'ima-buf'. When upstreaming a new type of measurement, including an example provides how to validate the new template data. Not every patch description requires an example. > > Initialize a global 'ima-buf' template and select that template, > by default, for measuring buffer. Good. > > Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> > --- > security/integrity/ima/ima.h | 1 + > security/integrity/ima/ima_main.c | 17 +++++------------ > security/integrity/ima/ima_policy.c | 2 +- > security/integrity/ima/ima_template.c | 25 +++++++++++++++++++++++++ > 4 files changed, 32 insertions(+), 13 deletions(-) > > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > index 6ebefec616e4..8e8b1e3cb847 100644 > --- a/security/integrity/ima/ima.h > +++ b/security/integrity/ima/ima.h > @@ -156,6 +156,7 @@ int template_desc_init_fields(const char *template_fmt, > const struct ima_template_field ***fields, > int *num_fields); > struct ima_template_desc *ima_template_desc_current(void); > +struct ima_template_desc *ima_template_desc_buf(void); > struct ima_template_desc *lookup_template_desc(const char *name); > bool ima_template_has_modsig(const struct ima_template_desc *ima_template); > int ima_restore_measurement_entry(struct ima_template_entry *entry); > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index a962b23e0429..3646ae763ba9 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -413,7 +413,7 @@ int ima_file_mmap(struct file *file, unsigned long prot) > */ > int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot) > { > - struct ima_template_desc *template; > + struct ima_template_desc *template = NULL; > struct file *file = vma->vm_file; > char filename[NAME_MAX]; > char *pathbuf = NULL; > @@ -802,7 +802,7 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size, > .filename = eventname, > .buf = buf, > .buf_len = size}; > - struct ima_template_desc *template = NULL; > + struct ima_template_desc *template = ima_template_desc_buf(); > struct { > struct ima_digest_data hdr; > char digest[IMA_MAX_DIGEST_SIZE]; > @@ -833,16 +833,9 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size, > pcr = CONFIG_IMA_MEASURE_PCR_IDX; > > if (!template) { > - template = lookup_template_desc("ima-buf"); > - ret = template_desc_init_fields(template->fmt, > - &(template->fields), > - &(template->num_fields)); > - if (ret < 0) { > - pr_err("template %s init failed, result: %d\n", > - (strlen(template->name) ? > - template->name : template->fmt), ret); > - return; > - } > + ret = -EINVAL; > + audit_cause = "ima_template_desc_buf"; > + goto out; Normally a test follows the variable assignment, but in this case, the check is being deferred in case there isn't a policy rule. > } > > iint.ima_hash = &hash.hdr; > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > index 9b5adeaa47fc..823a0c1379cb 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > @@ -628,7 +628,7 @@ int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid, > struct ima_rule_entry *entry; > int action = 0, actmask = flags | (flags << 1); > > - if (template_desc) > + if (template_desc && !*template_desc) > *template_desc = ima_template_desc_current(); > > rcu_read_lock(); > diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c > index 1e89e2d3851f..e53fce2c1610 100644 > --- a/security/integrity/ima/ima_template.c > +++ b/security/integrity/ima/ima_template.c > @@ -55,6 +55,7 @@ static const struct ima_template_field supported_fields[] = { > #define MAX_TEMPLATE_NAME_LEN sizeof("d-ng|n-ng|sig|buf|d-modisg|modsig") > > static struct ima_template_desc *ima_template; > +static struct ima_template_desc *ima_buf_template; > > /** > * ima_template_has_modsig - Check whether template has modsig-related fields. > @@ -252,6 +253,30 @@ struct ima_template_desc *ima_template_desc_current(void) > return ima_template; > } > > +struct ima_template_desc *ima_template_desc_buf(void) > +{ > + struct ima_template_desc *template = NULL; > + int ret = 0; > + > + if (ima_buf_template) > + return ima_buf_template; > + > + ima_init_template_list(); > + template = lookup_template_desc("ima-buf"); > + if (!template) > + return NULL; > + > + ret = template_desc_init_fields(template->fmt, > + &(template->fields), > + &(template->num_fields)); > + if (ret) > + return NULL; Instead of initializing the fields here, maybe it should be done in ima_init_template()? That would remove the deferred !template test in process_buffer_measurement() and would also simplify this function. thanks, Mimi > + > + ima_buf_template = template; > + > + return ima_buf_template; > +} > + > int __init ima_init_template(void) > { > struct ima_template_desc *template = ima_template_desc_current();
On 11/11/20 4:58 PM, Mimi Zohar wrote: > Hi Lakshmi, > > On Wed, 2020-11-11 at 12:59 -0800, Lakshmi Ramasubramanian wrote: >> The default IMA template for measuring buffer should be 'ima-buf' - so >> that the measured buffer is correctly included in the IMA measurement >> log entry. But the default IMA template used for all policy rules is >> the value set for CONFIG_IMA_DEFAULT_TEMPLATE if the policy rule does >> not specify a template. > > The second sentence defines the current status. The first sentence > describes the problem. I would reverse the sentence order. > > ^measuring buffer -> buffer measurements I'll make this change. > >> IMA does not take into account the template >> requirements of different rules when choosing a default template for >> a given policy rule. This breaks the buffer measurement if the template >> is not provided as part of the rule because the default template could >> be different than 'ima-buf'. > > Does the above paragraph add anything new? Instead describe the > problem. Perhaps something like: > > With the default template format, buffer measurements are added to the > measurement list, but do not include the buffer data, making it > difficult, if not impossible, to validate. Including "ima-buf" > template records in the measurement list by default, should not impact > existing attestation servers without "ima-buf" template support. This sounds better - I'll make this change. > >> >> For example, the following IMA policy rule enables measuring >> the command line arguments passed to the new kernel on kexec system call. >> >> measure func=KEXEC_CMDLINE >> >> The IMA template selected should be 'ima-buf' to have the measured >> command line arguments included in the IMA measurement log entry. >> Instead the default IMA template is selected, which could be different >> than 'ima-buf'. > > When upstreaming a new type of measurement, including an example > provides how to validate the new template data. Not every patch > description requires an example. Will remove the above paragraph. > >> >> Initialize a global 'ima-buf' template and select that template, >> by default, for measuring buffer. > > Good. >> >> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> >> --- >> security/integrity/ima/ima.h | 1 + >> security/integrity/ima/ima_main.c | 17 +++++------------ >> security/integrity/ima/ima_policy.c | 2 +- >> security/integrity/ima/ima_template.c | 25 +++++++++++++++++++++++++ >> 4 files changed, 32 insertions(+), 13 deletions(-) >> >> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h >> index 6ebefec616e4..8e8b1e3cb847 100644 >> --- a/security/integrity/ima/ima.h >> +++ b/security/integrity/ima/ima.h >> @@ -156,6 +156,7 @@ int template_desc_init_fields(const char *template_fmt, >> const struct ima_template_field ***fields, >> int *num_fields); >> struct ima_template_desc *ima_template_desc_current(void); >> +struct ima_template_desc *ima_template_desc_buf(void); >> struct ima_template_desc *lookup_template_desc(const char *name); >> bool ima_template_has_modsig(const struct ima_template_desc *ima_template); >> int ima_restore_measurement_entry(struct ima_template_entry *entry); >> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c >> index a962b23e0429..3646ae763ba9 100644 >> --- a/security/integrity/ima/ima_main.c >> +++ b/security/integrity/ima/ima_main.c >> @@ -413,7 +413,7 @@ int ima_file_mmap(struct file *file, unsigned long prot) >> */ >> int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot) >> { >> - struct ima_template_desc *template; >> + struct ima_template_desc *template = NULL; >> struct file *file = vma->vm_file; >> char filename[NAME_MAX]; >> char *pathbuf = NULL; >> @@ -802,7 +802,7 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size, >> .filename = eventname, >> .buf = buf, >> .buf_len = size}; >> - struct ima_template_desc *template = NULL; >> + struct ima_template_desc *template = ima_template_desc_buf(); >> struct { >> struct ima_digest_data hdr; >> char digest[IMA_MAX_DIGEST_SIZE]; >> @@ -833,16 +833,9 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size, >> pcr = CONFIG_IMA_MEASURE_PCR_IDX; >> >> if (!template) { >> - template = lookup_template_desc("ima-buf"); >> - ret = template_desc_init_fields(template->fmt, >> - &(template->fields), >> - &(template->num_fields)); >> - if (ret < 0) { >> - pr_err("template %s init failed, result: %d\n", >> - (strlen(template->name) ? >> - template->name : template->fmt), ret); >> - return; >> - } >> + ret = -EINVAL; >> + audit_cause = "ima_template_desc_buf"; >> + goto out; > > Normally a test follows the variable assignment, but in this case, the > check is being deferred in case there isn't a policy rule. I will initialize the buf template in ima_init_template() per your comment below. Will move the check for template right after the check for the IMA policy flag. if (!ima_policy_flag) return; template = ima_template_desc_buf(); if (!template) { ret = -EINVAL; audit_cause = "ima_template_desc_buf"; goto out; } > >> } >> >> iint.ima_hash = &hash.hdr; >> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c >> index 9b5adeaa47fc..823a0c1379cb 100644 >> --- a/security/integrity/ima/ima_policy.c >> +++ b/security/integrity/ima/ima_policy.c >> @@ -628,7 +628,7 @@ int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid, >> struct ima_rule_entry *entry; >> int action = 0, actmask = flags | (flags << 1); >> >> - if (template_desc) >> + if (template_desc && !*template_desc) >> *template_desc = ima_template_desc_current(); >> >> rcu_read_lock(); >> diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c >> index 1e89e2d3851f..e53fce2c1610 100644 >> --- a/security/integrity/ima/ima_template.c >> +++ b/security/integrity/ima/ima_template.c >> @@ -55,6 +55,7 @@ static const struct ima_template_field supported_fields[] = { >> #define MAX_TEMPLATE_NAME_LEN sizeof("d-ng|n-ng|sig|buf|d-modisg|modsig") >> >> static struct ima_template_desc *ima_template; >> +static struct ima_template_desc *ima_buf_template; >> >> /** >> * ima_template_has_modsig - Check whether template has modsig-related fields. >> @@ -252,6 +253,30 @@ struct ima_template_desc *ima_template_desc_current(void) >> return ima_template; >> } >> >> +struct ima_template_desc *ima_template_desc_buf(void) >> +{ >> + struct ima_template_desc *template = NULL; >> + int ret = 0; >> + >> + if (ima_buf_template) >> + return ima_buf_template; >> + >> + ima_init_template_list(); >> + template = lookup_template_desc("ima-buf"); >> + if (!template) >> + return NULL; >> + >> + ret = template_desc_init_fields(template->fmt, >> + &(template->fields), >> + &(template->num_fields)); >> + if (ret) >> + return NULL; > > Instead of initializing the fields here, maybe it should be done in > ima_init_template()? That would remove the deferred !template test in > process_buffer_measurement() and would also simplify this function. > Will do. Thanks Mimi. -lakshmi > >> + >> + ima_buf_template = template; >> + >> + return ima_buf_template; >> +} >> + >> int __init ima_init_template(void) >> { >> struct ima_template_desc *template = ima_template_desc_current();
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 6ebefec616e4..8e8b1e3cb847 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -156,6 +156,7 @@ int template_desc_init_fields(const char *template_fmt, const struct ima_template_field ***fields, int *num_fields); struct ima_template_desc *ima_template_desc_current(void); +struct ima_template_desc *ima_template_desc_buf(void); struct ima_template_desc *lookup_template_desc(const char *name); bool ima_template_has_modsig(const struct ima_template_desc *ima_template); int ima_restore_measurement_entry(struct ima_template_entry *entry); diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index a962b23e0429..3646ae763ba9 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -413,7 +413,7 @@ int ima_file_mmap(struct file *file, unsigned long prot) */ int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot) { - struct ima_template_desc *template; + struct ima_template_desc *template = NULL; struct file *file = vma->vm_file; char filename[NAME_MAX]; char *pathbuf = NULL; @@ -802,7 +802,7 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size, .filename = eventname, .buf = buf, .buf_len = size}; - struct ima_template_desc *template = NULL; + struct ima_template_desc *template = ima_template_desc_buf(); struct { struct ima_digest_data hdr; char digest[IMA_MAX_DIGEST_SIZE]; @@ -833,16 +833,9 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size, pcr = CONFIG_IMA_MEASURE_PCR_IDX; if (!template) { - template = lookup_template_desc("ima-buf"); - ret = template_desc_init_fields(template->fmt, - &(template->fields), - &(template->num_fields)); - if (ret < 0) { - pr_err("template %s init failed, result: %d\n", - (strlen(template->name) ? - template->name : template->fmt), ret); - return; - } + ret = -EINVAL; + audit_cause = "ima_template_desc_buf"; + goto out; } iint.ima_hash = &hash.hdr; diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 9b5adeaa47fc..823a0c1379cb 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -628,7 +628,7 @@ int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid, struct ima_rule_entry *entry; int action = 0, actmask = flags | (flags << 1); - if (template_desc) + if (template_desc && !*template_desc) *template_desc = ima_template_desc_current(); rcu_read_lock(); diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c index 1e89e2d3851f..e53fce2c1610 100644 --- a/security/integrity/ima/ima_template.c +++ b/security/integrity/ima/ima_template.c @@ -55,6 +55,7 @@ static const struct ima_template_field supported_fields[] = { #define MAX_TEMPLATE_NAME_LEN sizeof("d-ng|n-ng|sig|buf|d-modisg|modsig") static struct ima_template_desc *ima_template; +static struct ima_template_desc *ima_buf_template; /** * ima_template_has_modsig - Check whether template has modsig-related fields. @@ -252,6 +253,30 @@ struct ima_template_desc *ima_template_desc_current(void) return ima_template; } +struct ima_template_desc *ima_template_desc_buf(void) +{ + struct ima_template_desc *template = NULL; + int ret = 0; + + if (ima_buf_template) + return ima_buf_template; + + ima_init_template_list(); + template = lookup_template_desc("ima-buf"); + if (!template) + return NULL; + + ret = template_desc_init_fields(template->fmt, + &(template->fields), + &(template->num_fields)); + if (ret) + return NULL; + + ima_buf_template = template; + + return ima_buf_template; +} + int __init ima_init_template(void) { struct ima_template_desc *template = ima_template_desc_current();
The default IMA template for measuring buffer should be 'ima-buf' - so that the measured buffer is correctly included in the IMA measurement log entry. But the default IMA template used for all policy rules is the value set for CONFIG_IMA_DEFAULT_TEMPLATE if the policy rule does not specify a template. IMA does not take into account the template requirements of different rules when choosing a default template for a given policy rule. This breaks the buffer measurement if the template is not provided as part of the rule because the default template could be different than 'ima-buf'. For example, the following IMA policy rule enables measuring the command line arguments passed to the new kernel on kexec system call. measure func=KEXEC_CMDLINE The IMA template selected should be 'ima-buf' to have the measured command line arguments included in the IMA measurement log entry. Instead the default IMA template is selected, which could be different than 'ima-buf'. Initialize a global 'ima-buf' template and select that template, by default, for measuring buffer. Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> --- security/integrity/ima/ima.h | 1 + security/integrity/ima/ima_main.c | 17 +++++------------ security/integrity/ima/ima_policy.c | 2 +- security/integrity/ima/ima_template.c | 25 +++++++++++++++++++++++++ 4 files changed, 32 insertions(+), 13 deletions(-)