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 <[email protected]>
---
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;
}
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();
--
2.29.0
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 <[email protected]>
> ---
> 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 <[email protected]>
>> ---
>> 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();