linux-ima-user — Q&A for users of IMA

Re: [Linux-ima-user] [RFC][PATCH][0/2] IMA

[Eric P. <ep...@pa...>]

On Sat, Apr 17, 2010 at 12:22 PM, Eric Paris <ep...@pa...> wrote:

> You get the same behaviour with your loaded rules if you load them the
> 'right' way (which is non-obvious, IMHO)  You need to
>
> open(sysfs file)
> write(rule one)
> write(rule two)
> write(rule three)
> write(rule four)
> close(sysfs_file)
>
> I'm guessing you used cat file > sysfs_file which resulted in
>
> open(sysfs_file)
> write(rule one rule two rule th);
> write(ree rule four)
> close(sysfs_file)
>
> Which resulted in the crap behaviour you got.  I suggested to Mimi off
> list that maybe the best solution would be to change the parser a
> little such that we stop parsing on \n on return a short write equal
> to the length of the one rule we parsed.  Thus the way cat handles
> things we would get:

I was thinking something like the attached patch.....

-Eric

Re: [Linux-ima-user] [RFC][PATCH][0/2] IMA

[Eric P. <ep...@pa...>]

On Fri, Apr 16, 2010 at 11:12 AM, Roberto Sassu <rob...@po...> wrote:
> On Thursday 15 April 2010 00:20:00 Mimi Zohar wrote:
>> On Wed, 2010-04-14 at 15:32 -0400, Eric Paris wrote:
>> > On Wed, Apr 14, 2010 at 5:51 AM, Roberto Sassu <rob...@po...> wrote:
>> > > Description of the issue:
>> >
>> > > The parsing function is used only when custom rules must be loaded to replace
>> > > the default ones, but a different behavior can be observed in respect to the
>> > > first case: only one element is allocated and it is populated by the parsing
>> > > function that recognizes the tokens ' ' and '\n'.
>> > > So, having multiple lines in the file passed through the exported interface
>> > > doesn't lead to the same number of rules in the list, since only one allocated
>> > > item is available. If a directive spans two different lines, only the last
>> > > value is taken and the first one is overwritten.
>> >
>> > I admit I haven't looked at the loading code before and it does seem
>> > 'special.'   It looks to me on first glance like the kernel accepts
>> > one rule per write() rather than one rule per line.  If you send it
>> > two rules in a single write you are going to get a rather
>> > unpredictable result.
>>
>> Similar to smack, one rule per write.  Keeps it nice and simple.
>>
>> > Your change is a good start to allow single writes which contain
>> > multiple rules.  I'm guessing that's a good idea, I though I think the
>> > parser should get a little smarter.  Does it ever make sense to have 2
>> > actions in a single rule, with the last one winning?  You also added
>> > support for some sort of commenting in the rule set didn't you?  That
>> > probably needs to be a lot stronger too....
>> >
>> > I'm not acking or nacking here, just trying to understand exactly the
>> > problem we are trying to solve.  Loading rules using
>> >
>> > cat rule > /sys/kernel/security/ima/policy
>> >
>> > seems like a good direction to head.
>>
>> hm, I was under the impression we wanted to minimize any
>> policy/configuration parsing done in the kernel.
>>
>
> In my thinking, actually the behaviour of the code that loads the policy through the /sys interface
> is different from the one that handles hardcoded rules. In the last case these are stored as array of structures
> and each element is added to the list by the "list_add_tail" function. I past here the few lines of the code
> involved:
>
> void __init ima_init_policy(void)
> {
>        int i, entries;
>
>        /* if !ima_use_tcb set entries = 0 so we load NO default rules */
>        if (ima_use_tcb)
>                entries = ARRAY_SIZE(default_rules);
>        else
>                entries = 0;
>
>        for (i = 0; i < entries; i++)
>                list_add_tail(&default_rules[i].list, &measure_default_rules);
>        ima_measure = &measure_default_rules;
> }

You get the same behaviour with your loaded rules if you load them the
'right' way (which is non-obvious, IMHO)  You need to

open(sysfs file)
write(rule one)
write(rule two)
write(rule three)
write(rule four)
close(sysfs_file)

I'm guessing you used cat file > sysfs_file which resulted in

open(sysfs_file)
write(rule one rule two rule th);
write(ree rule four)
close(sysfs_file)

Which resulted in the crap behaviour you got.  I suggested to Mimi off
list that maybe the best solution would be to change the parser a
little such that we stop parsing on \n on return a short write equal
to the length of the one rule we parsed.  Thus the way cat handles
things we would get:

open(sysfs_file)
write(rule one rule two rule_th)
which would return 8 (length of "rule one") so cat would next try
write (rule two rule three rul)
which would return 8 (length of "rule two") so cat would next try
write (rule three rule four)
which would return 10 (length of "rule three") so cat would next try
write (rule four)
which would return a full write 9 (length of "rule four)
close (sysfs_file)

I think that solves the problem you were trying to suggest by relying
on a properly implemented userspace.

Mimi suggest we also add parsing for comments, which I'm ok with.
I'm also think if we go this way we should check 2 other things:

1) make sure that we never have the same options set twice (aka can't
set action to both measure and don't measure in the same rule)
2) make sure that rules ALWAYS end in \n, so if userspace doesn't
write the full rule we have some clue and can reject it.

I don't know if #2 is backwards compatible with how whatever utility
IBM uses to loads rules works, but seeing as how noone else has hit
this until now, I'm guess it can't bite many people in the rump.....

-Eric

Re: [Linux-ima-user] [RFC][PATCH][1/1] ima: extending the format of the measurement list

[Mimi Z. <zo...@li...>]

On Fri, 2010-04-16 at 16:44 +0200, Roberto Sassu wrote:
> On Thursday 15 April 2010 01:20:23 Mimi Zohar wrote:
> > On Wed, 2010-04-14 at 11:52 +0200, Roberto Sassu wrote:
> > > The format of the measurement list has been modified to carry additional 
> > > information.
> > > 
> > > Signed-off-by: Roberto Sassu <rob...@po...>
> > > Acked-by: Gianluca Ramunno <ra...@po...>
> > 
> > As this patchset contains only 1 patch, how about putting the
> > cover-letter description, or an abbreviated description, here in the
> > patch instead.
> > 
> Ok, i'll post another mail after applying all comments.
> 
> > > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > > index 47fb65d..7fb66ef 100644
> > > --- a/security/integrity/ima/ima.h
> > > +++ b/security/integrity/ima/ima.h
> > > @@ -13,6 +13,29 @@
> > >   * File: ima.h
> > >   *	internal Integrity Measurement Architecture (IMA) definitions
> > >   */
> > > +/* ====================================================================
> > > + * Copyright (C) 2009-2010 Politecnico di Torino, Italy
> > > + * TORSEC group -- http://security.polito.it
> > > + * Author: Roberto Sassu <rob...@po...>
> > > + *
> > > + * The original IMA source code from IBM, has been modified by Roberto Sassu 
> > > + * to display extra information in the measurement list displayed through the
> > > + * securityfs filesystem.
> > > + *
> > > + * DISCLAIMER of WARRANTY
> > > + *
> > > + * The following software "IMA" with measurement list format enhanced
> > > + * is experimental and is provided "as is", and no guarantee or warranty
> > > + * is given by Politecnico di Torino (TORSEC group), that the software is
> > > + * fit for any particular purpose.  The user thereof uses the software at its
> > > + * sole risk and liability. Politecnico di Torino shall have no obligation to
> > > + * maintain or support this software. Politecnico di Torino MAKES NO
> > > + * EXPRESS OR IMPLIED WARRANTY OF ANY KIND REGARDING THIS SOFTWARE.
> > > + * Politecnico di Torino SHALL NOT BE LIABLE FOR ANY DIRECT, INDIRECT,
> > > + * SPECIAL, INCIDENTAL OR CONSEQUENTIAL DAMAGES, WHETHER BASED ON
> > > + * CONTRACT, TORT OR ANY OTHER LEGAL THEORY, IN CONNECTION WITH OR
> > > + * ARISING OUT OF THE FURNISHING, PERFORMANCE OR USE OF THIS SOFTWARE.
> > > + */
> > 
> > The Linux kernel is the normal GPL v2 license. Instead, how about adding
> > a line like:
> > Copyright (C) 2009-2010 Politecnico di Torino, Italy, 
> > 	Roberto Sassu <rob...@po...>
> > 
> 
> Does this text is correct?
> 
> /*
>  * Copyright (C) 2009-2010 Politecnico di Torino, Italy
>  * TORSEC group -- http://security.polito.it
>  * Author: Roberto Sassu <rob...@po...>
>  */

The one I recommended above is more common. Refer to either
security/smack/smack_lsm.c or security/selinux/hooks.c for examples.

> > >  #ifndef __LINUX_IMA_H
> > >  #define __LINUX_IMA_H
> > > @@ -30,6 +53,7 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 };
> > >  /* digest size for IMA, fits SHA1 or MD5 */
> > >  #define IMA_DIGEST_SIZE		20
> > >  #define IMA_EVENT_NAME_LEN_MAX	255
> > > +#define IMA_LABEL_LEN_MAX	255
> > 
> > The LSMs must have already defined something like this.
> > 
> I searched the variable in the kernel sources, but i didn't found it. I think there's no fixed size since the function 
> "security_secid_to_secctx" returns itself the length of a newly allocated string.

yes, I stand corrected, but adding 510 bytes for each measurement in the
measurement list is a lot.

> > >  #define IMA_HASH_BITS 9
> > >  #define IMA_MEASURE_HTABLE_SIZE (1 << IMA_HASH_BITS)
> > > @@ -43,6 +67,8 @@ extern char *ima_hash;
> > >  struct ima_template_data {
> > >  	u8 digest[IMA_DIGEST_SIZE];	/* sha1/md5 measurement hash */
> > >  	char file_name[IMA_EVENT_NAME_LEN_MAX + 1];	/* name + \0 */
> > > +	char subj_label[IMA_LABEL_LEN_MAX + 1];	/* subj_label + \0 */
> > > +	char obj_label[IMA_LABEL_LEN_MAX + 1];	/* obj_label + \0 */
> > >  };
> > 
> > For backwards compatibility, this probably needs to be a new template,
> > with a runtime option - orig vs. new template.
> > 
> I can implement a new "setup" function to add the new kernel parameter "ima_report_format="
> At this point there are two possibilities:
> 
> - specifying a mask as value: this gives to platform owner the flexibility to specify the custom format it recognizes;
> 						 by default, the format will be the actual one;
> - specifying a format version string: this permits to use an identifier to name a format with well defined fields;
> 
> The first one seems fine to me.

Meaning support would be: ima_report_format=: "ima" | "ima-ng".
Please remember to add it to Documentation/kernel-parameters.txt.

> > >  struct ima_template_entry {
> > > diff --git a/security/integrity/ima/ima_api.c 
> > > b/security/integrity/ima/ima_api.c
> > > index 2a5e0bc..e612a49 100644
> > > --- a/security/integrity/ima/ima_api.c
> > > +++ b/security/integrity/ima/ima_api.c
> > > @@ -12,10 +12,34 @@
> > >   *	Implements must_measure, collect_measurement, store_measurement,
> > >   *	and store_template.
> > >   */
> > > +/* ====================================================================
> > > + * Copyright (C) 2009-2010 Politecnico di Torino, Italy
> > > + * TORSEC group -- http://security.polito.it
> > > + * Author: Roberto Sassu <rob...@po...>
> > > + *
> > > + * The original IMA source code from IBM, has been modified by Roberto Sassu 
> > > + * to display extra information in the measurement list displayed through the
> > > + * securityfs filesystem.
> > > + *
> > > + * DISCLAIMER of WARRANTY
> > > + *
> > > + * The following software "IMA" with measurement list format enhanced
> > > + * is experimental and is provided "as is", and no guarantee or warranty
> > > + * is given by Politecnico di Torino (TORSEC group), that the software is
> > > + * fit for any particular purpose.  The user thereof uses the software at its
> > > + * sole risk and liability. Politecnico di Torino shall have no obligation to
> > > + * maintain or support this software. Politecnico di Torino MAKES NO
> > > + * EXPRESS OR IMPLIED WARRANTY OF ANY KIND REGARDING THIS SOFTWARE.
> > > + * Politecnico di Torino SHALL NOT BE LIABLE FOR ANY DIRECT, INDIRECT,
> > > + * SPECIAL, INCIDENTAL OR CONSEQUENTIAL DAMAGES, WHETHER BASED ON
> > > + * CONTRACT, TORT OR ANY OTHER LEGAL THEORY, IN CONNECTION WITH OR
> > > + * ARISING OUT OF THE FURNISHING, PERFORMANCE OR USE OF THIS SOFTWARE.
> > > + */
> > 
> > ditto here
> > 
> > >  #include <linux/module.h>
> > >  
> > >  #include "ima.h"
> > >  static const char *IMA_TEMPLATE_NAME = "ima";
> > > +static const char *IMA_LABEL_FALLBACK = "n/a";
> > 
> > >  /*
> > >   * ima_store_template - store ima template measurements
> > > @@ -171,7 +195,13 @@ void ima_store_measurement(struct ima_iint_cache *iint, 
> > > struct file *file,
> > 
> > When posting patches, please use a mailer that doesn't wrap lines.
> > 
> > >  	struct inode *inode = file->f_dentry->d_inode;
> > >  	struct ima_template_entry *entry;
> > >  	int violation = 0;
> > > -
> > > +	char *subj_label = NULL;
> > > +	char *obj_label = NULL;
> > > +	int obj_label_len;
> > > +	int subj_label_len;
> > > +	struct task_struct *tsk = current;
> > > +	u32 osid, sid;
> > > +	
> > >  	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
> > >  	if (!entry) {
> > >  		integrity_audit_msg(AUDIT_INTEGRITY_PCR, inode, filename,
> > > @@ -181,7 +211,25 @@ void ima_store_measurement(struct ima_iint_cache *iint, 
> > > struct file *file,
> > >  	memset(&entry->template, 0, sizeof(entry->template));
> > >  	memcpy(entry->template.digest, iint->digest, IMA_DIGEST_SIZE);
> > >  	strncpy(entry->template.file_name, filename, IMA_EVENT_NAME_LEN_MAX);
> > > +	
> > > +	if(file->f_dentry->d_inode) {
> > > +		security_inode_getsecid(file->f_dentry->d_inode, &osid);
> > > +		result = security_secid_to_secctx(osid, &obj_label, &obj_label_len);
> > > +	} else
> > > +		result = -1;
> > > +
> > > +	if(result)
> > > +		strncpy(entry->template.obj_label, IMA_LABEL_FALLBACK, 
> > > strlen(IMA_LABEL_FALLBACK));
> > 
> > Why bother? In the display code, check to to see if it is NULL.
> > 
> I think the label must be defined here, because the function "ima_store_template", called after these lines, 
> calculates at this time the digest of the template.

The memset above will zero out memory. Why would not having a label be a
problem?

> > > +	else
> > > +		strncpy(entry->template.obj_label, obj_label, obj_label_len);
> > >  
> > > +	security_task_getsecid(tsk, &sid);
> > > +	result = security_secid_to_secctx(sid, &subj_label, &subj_label_len);
> > > +	if(result)
> > > +		strncpy(entry->template.subj_label, IMA_LABEL_FALLBACK, 
> > > strlen(IMA_LABEL_FALLBACK));
> > > +	else
> > > +		strncpy(entry->template.subj_label, subj_label, subj_label_len);
> > > + 
> > >  	result = ima_store_template(entry, violation, inode);
> > >  	if (!result)
> > >  		iint->flags |= IMA_MEASURED;
> > > diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> > > index 0c72c9c..465b8b3 100644
> > > --- a/security/integrity/ima/ima_fs.c
> > > +++ b/security/integrity/ima/ima_fs.c
> > > @@ -15,6 +15,30 @@
> > >   *	implemenents security file system for reporting
> > >   *	current measurement list and IMA statistics
> > >   */
> > > +/* ====================================================================
> > > + * Copyright (C) 2009-2010 Politecnico di Torino, Italy
> > > + * TORSEC group -- http://security.polito.it
> > > + * Author: Roberto Sassu <rob...@po...>
> > > + *
> > > + * The original IMA source code from IBM, has been modified by Roberto Sassu 
> > > + * to display extra information in the measurement list displayed through the
> > > + * securityfs filesystem.
> > > + *
> > > + * DISCLAIMER of WARRANTY
> > > + *
> > > + * The following software "IMA" with measurement list format enhanced
> > > + * is experimental and is provided "as is", and no guarantee or warranty
> > > + * is given by Politecnico di Torino (TORSEC group), that the software is
> > > + * fit for any particular purpose.  The user thereof uses the software at its
> > > + * sole risk and liability. Politecnico di Torino shall have no obligation to
> > > + * maintain or support this software. Politecnico di Torino MAKES NO
> > > + * EXPRESS OR IMPLIED WARRANTY OF ANY KIND REGARDING THIS SOFTWARE.
> > > + * Politecnico di Torino SHALL NOT BE LIABLE FOR ANY DIRECT, INDIRECT,
> > > + * SPECIAL, INCIDENTAL OR CONSEQUENTIAL DAMAGES, WHETHER BASED ON
> > > + * CONTRACT, TORT OR ANY OTHER LEGAL THEORY, IN CONNECTION WITH OR
> > > + * ARISING OUT OF THE FURNISHING, PERFORMANCE OR USE OF THIS SOFTWARE.
> > > + */
> > > +
> > 
> > ditto here
> > 
> > >  #include <linux/fcntl.h>
> > >  #include <linux/module.h>
> > >  #include <linux/seq_file.h>
> > > @@ -177,11 +201,16 @@ void ima_template_show(struct seq_file *m, void *e, enum 
> > > imha_show_type show)
> > >  {
> > >  	struct ima_template_data *entry = e;
> > >  	int namelen;
> > > -
> > > +	int obj_label_len;
> > > +	int subj_label_len;
> > > +	
> > >  	switch (show) {
> > >  	case IMA_SHOW_ASCII:
> > >  		ima_print_digest(m, entry->digest);
> > > -		seq_printf(m, " %s\n", entry->file_name);
> > > +		seq_printf(m, " %s", entry->file_name);
> > > +		seq_printf(m, " %s", entry->subj_label);
> > > +		seq_printf(m, " %s\n", entry->obj_label);
> > > +	

How about only displaying the label, if it exists?

> 	
> > >  		break;
> > >  	case IMA_SHOW_BINARY:
> > >  		ima_putc(m, entry->digest, IMA_DIGEST_SIZE);
> > > @@ -189,6 +218,13 @@ void ima_template_show(struct seq_file *m, void *e, enum 
> > > ima_show_type show)
> > >  		namelen = strlen(entry->file_name);
> > >  		ima_putc(m, &namelen, sizeof namelen);
> > >  		ima_putc(m, entry->file_name, namelen);
> > > +		subj_label_len = strlen(entry->subj_label);
> > > +		ima_putc(m, &subj_label_len, sizeof subj_label_len);
> > > +		ima_putc(m, entry->subj_label, subj_label_len);
> > > +		obj_label_len = strlen(entry->obj_label);
> > > +		ima_putc(m, &obj_label_len, sizeof obj_label_len);
> > > +		ima_putc(m, entry->obj_label, obj_label_len);
> > > +		
> > >  	default:
> > >  		break;
> > >  	}
> > > 

We want to minimize the size of the measurement list. How about only
sending the label, if the length is greater than 0?

Thanks,

Mimi

Re: [Linux-ima-user] [RFC][PATCH][0/2] IMA

[Mimi Z. <zo...@li...>]

On Fri, 2010-04-16 at 17:12 +0200, Roberto Sassu wrote:
> On Thursday 15 April 2010 00:20:00 Mimi Zohar wrote:
> > On Wed, 2010-04-14 at 15:32 -0400, Eric Paris wrote:
> > > On Wed, Apr 14, 2010 at 5:51 AM, Roberto Sassu <rob...@po...> wrote:
> > > > Description of the issue:
> > > 
> > > > The parsing function is used only when custom rules must be loaded to replace
> > > > the default ones, but a different behavior can be observed in respect to the
> > > > first case: only one element is allocated and it is populated by the parsing
> > > > function that recognizes the tokens ' ' and '\n'.
> > > > So, having multiple lines in the file passed through the exported interface
> > > > doesn't lead to the same number of rules in the list, since only one allocated
> > > > item is available. If a directive spans two different lines, only the last
> > > > value is taken and the first one is overwritten.
> > > 
> > > I admit I haven't looked at the loading code before and it does seem
> > > 'special.'   It looks to me on first glance like the kernel accepts
> > > one rule per write() rather than one rule per line.  If you send it
> > > two rules in a single write you are going to get a rather
> > > unpredictable result.
> > 
> > Similar to smack, one rule per write.  Keeps it nice and simple.
> > 
> > > Your change is a good start to allow single writes which contain
> > > multiple rules.  I'm guessing that's a good idea, I though I think the
> > > parser should get a little smarter.  Does it ever make sense to have 2
> > > actions in a single rule, with the last one winning?  You also added
> > > support for some sort of commenting in the rule set didn't you?  That
> > > probably needs to be a lot stronger too....
> > > 
> > > I'm not acking or nacking here, just trying to understand exactly the
> > > problem we are trying to solve.  Loading rules using
> > > 
> > > cat rule > /sys/kernel/security/ima/policy
> > > 
> > > seems like a good direction to head.
> > 
> > hm, I was under the impression we wanted to minimize any
> > policy/configuration parsing done in the kernel.
> > 
> 
> In my thinking, actually the behaviour of the code that loads the policy through the /sys interface 
> is different from the one that handles hardcoded rules. In the last case these are stored as array of structures
> and each element is added to the list by the "list_add_tail" function. I past here the few lines of the code 
> involved:
> 
> void __init ima_init_policy(void)
> {
> 	int i, entries;
> 
> 	/* if !ima_use_tcb set entries = 0 so we load NO default rules */
> 	if (ima_use_tcb)
> 		entries = ARRAY_SIZE(default_rules);
> 	else
> 		entries = 0;
> 
> 	for (i = 0; i < entries; i++)
> 		list_add_tail(&default_rules[i].list, &measure_default_rules);
> 	ima_measure = &measure_default_rules;
> }

The original IMA code supported parsing multiple rules, but somewhere
along the way I was asked to clean the up code and told to minimize
kernel parsing. I personally don't have a problem with supporting a more
robust, complete policy parser. Perhaps someone could chime in here?

Thanks,

Mimi

Re: [Linux-ima-user] [RFC][PATCH][0/2] IMA

[Roberto S. <rob...@po...>]

On Thursday 15 April 2010 00:20:00 Mimi Zohar wrote:
> On Wed, 2010-04-14 at 15:32 -0400, Eric Paris wrote:
> > On Wed, Apr 14, 2010 at 5:51 AM, Roberto Sassu <rob...@po...> wrote:
> > > Description of the issue:
> > 
> > > The parsing function is used only when custom rules must be loaded to replace
> > > the default ones, but a different behavior can be observed in respect to the
> > > first case: only one element is allocated and it is populated by the parsing
> > > function that recognizes the tokens ' ' and '\n'.
> > > So, having multiple lines in the file passed through the exported interface
> > > doesn't lead to the same number of rules in the list, since only one allocated
> > > item is available. If a directive spans two different lines, only the last
> > > value is taken and the first one is overwritten.
> > 
> > I admit I haven't looked at the loading code before and it does seem
> > 'special.'   It looks to me on first glance like the kernel accepts
> > one rule per write() rather than one rule per line.  If you send it
> > two rules in a single write you are going to get a rather
> > unpredictable result.
> 
> Similar to smack, one rule per write.  Keeps it nice and simple.
> 
> > Your change is a good start to allow single writes which contain
> > multiple rules.  I'm guessing that's a good idea, I though I think the
> > parser should get a little smarter.  Does it ever make sense to have 2
> > actions in a single rule, with the last one winning?  You also added
> > support for some sort of commenting in the rule set didn't you?  That
> > probably needs to be a lot stronger too....
> > 
> > I'm not acking or nacking here, just trying to understand exactly the
> > problem we are trying to solve.  Loading rules using
> > 
> > cat rule > /sys/kernel/security/ima/policy
> > 
> > seems like a good direction to head.
> 
> hm, I was under the impression we wanted to minimize any
> policy/configuration parsing done in the kernel.
> 

In my thinking, actually the behaviour of the code that loads the policy through the /sys interface 
is different from the one that handles hardcoded rules. In the last case these are stored as array of structures
and each element is added to the list by the "list_add_tail" function. I past here the few lines of the code 
involved:

void __init ima_init_policy(void)
{
	int i, entries;

	/* if !ima_use_tcb set entries = 0 so we load NO default rules */
	if (ima_use_tcb)
		entries = ARRAY_SIZE(default_rules);
	else
		entries = 0;

	for (i = 0; i < entries; i++)
		list_add_tail(&default_rules[i].list, &measure_default_rules);
	ima_measure = &measure_default_rules;
}


> Mimi
> 
> 

Re: [Linux-ima-user] [RFC][PATCH][1/1] ima: extending the format of the measurement list

[Roberto S. <rob...@po...>]

On Thursday 15 April 2010 01:20:23 Mimi Zohar wrote:
> On Wed, 2010-04-14 at 11:52 +0200, Roberto Sassu wrote:
> > The format of the measurement list has been modified to carry additional 
> > information.
> > 
> > Signed-off-by: Roberto Sassu <rob...@po...>
> > Acked-by: Gianluca Ramunno <ra...@po...>
> 
> As this patchset contains only 1 patch, how about putting the
> cover-letter description, or an abbreviated description, here in the
> patch instead.
> 
Ok, i'll post another mail after applying all comments.

> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index 47fb65d..7fb66ef 100644
> > --- a/security/integrity/ima/ima.h
> > +++ b/security/integrity/ima/ima.h
> > @@ -13,6 +13,29 @@
> >   * File: ima.h
> >   *	internal Integrity Measurement Architecture (IMA) definitions
> >   */
> > +/* ====================================================================
> > + * Copyright (C) 2009-2010 Politecnico di Torino, Italy
> > + * TORSEC group -- http://security.polito.it
> > + * Author: Roberto Sassu <rob...@po...>
> > + *
> > + * The original IMA source code from IBM, has been modified by Roberto Sassu 
> > + * to display extra information in the measurement list displayed through the
> > + * securityfs filesystem.
> > + *
> > + * DISCLAIMER of WARRANTY
> > + *
> > + * The following software "IMA" with measurement list format enhanced
> > + * is experimental and is provided "as is", and no guarantee or warranty
> > + * is given by Politecnico di Torino (TORSEC group), that the software is
> > + * fit for any particular purpose.  The user thereof uses the software at its
> > + * sole risk and liability. Politecnico di Torino shall have no obligation to
> > + * maintain or support this software. Politecnico di Torino MAKES NO
> > + * EXPRESS OR IMPLIED WARRANTY OF ANY KIND REGARDING THIS SOFTWARE.
> > + * Politecnico di Torino SHALL NOT BE LIABLE FOR ANY DIRECT, INDIRECT,
> > + * SPECIAL, INCIDENTAL OR CONSEQUENTIAL DAMAGES, WHETHER BASED ON
> > + * CONTRACT, TORT OR ANY OTHER LEGAL THEORY, IN CONNECTION WITH OR
> > + * ARISING OUT OF THE FURNISHING, PERFORMANCE OR USE OF THIS SOFTWARE.
> > + */
> 
> The Linux kernel is the normal GPL v2 license. Instead, how about adding
> a line like:
> Copyright (C) 2009-2010 Politecnico di Torino, Italy, 
> 	Roberto Sassu <rob...@po...>
> 

Does this text is correct?

/*
 * Copyright (C) 2009-2010 Politecnico di Torino, Italy
 * TORSEC group -- http://security.polito.it
 * Author: Roberto Sassu <rob...@po...>
 */

> >  #ifndef __LINUX_IMA_H
> >  #define __LINUX_IMA_H
> > @@ -30,6 +53,7 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 };
> >  /* digest size for IMA, fits SHA1 or MD5 */
> >  #define IMA_DIGEST_SIZE		20
> >  #define IMA_EVENT_NAME_LEN_MAX	255
> > +#define IMA_LABEL_LEN_MAX	255
> 
> The LSMs must have already defined something like this.
> 
I searched the variable in the kernel sources, but i didn't found it. I think there's no fixed size since the function 
"security_secid_to_secctx" returns itself the length of a newly allocated string.

> >  #define IMA_HASH_BITS 9
> >  #define IMA_MEASURE_HTABLE_SIZE (1 << IMA_HASH_BITS)
> > @@ -43,6 +67,8 @@ extern char *ima_hash;
> >  struct ima_template_data {
> >  	u8 digest[IMA_DIGEST_SIZE];	/* sha1/md5 measurement hash */
> >  	char file_name[IMA_EVENT_NAME_LEN_MAX + 1];	/* name + \0 */
> > +	char subj_label[IMA_LABEL_LEN_MAX + 1];	/* subj_label + \0 */
> > +	char obj_label[IMA_LABEL_LEN_MAX + 1];	/* obj_label + \0 */
> >  };
> 
> For backwards compatibility, this probably needs to be a new template,
> with a runtime option - orig vs. new template.
> 
I can implement a new "setup" function to add the new kernel parameter "ima_report_format="
At this point there are two possibilities:

- specifying a mask as value: this gives to platform owner the flexibility to specify the custom format it recognizes;
						 by default, the format will be the actual one;
- specifying a format version string: this permits to use an identifier to name a format with well defined fields;

The first one seems fine to me.

> >  struct ima_template_entry {
> > diff --git a/security/integrity/ima/ima_api.c 
> > b/security/integrity/ima/ima_api.c
> > index 2a5e0bc..e612a49 100644
> > --- a/security/integrity/ima/ima_api.c
> > +++ b/security/integrity/ima/ima_api.c
> > @@ -12,10 +12,34 @@
> >   *	Implements must_measure, collect_measurement, store_measurement,
> >   *	and store_template.
> >   */
> > +/* ====================================================================
> > + * Copyright (C) 2009-2010 Politecnico di Torino, Italy
> > + * TORSEC group -- http://security.polito.it
> > + * Author: Roberto Sassu <rob...@po...>
> > + *
> > + * The original IMA source code from IBM, has been modified by Roberto Sassu 
> > + * to display extra information in the measurement list displayed through the
> > + * securityfs filesystem.
> > + *
> > + * DISCLAIMER of WARRANTY
> > + *
> > + * The following software "IMA" with measurement list format enhanced
> > + * is experimental and is provided "as is", and no guarantee or warranty
> > + * is given by Politecnico di Torino (TORSEC group), that the software is
> > + * fit for any particular purpose.  The user thereof uses the software at its
> > + * sole risk and liability. Politecnico di Torino shall have no obligation to
> > + * maintain or support this software. Politecnico di Torino MAKES NO
> > + * EXPRESS OR IMPLIED WARRANTY OF ANY KIND REGARDING THIS SOFTWARE.
> > + * Politecnico di Torino SHALL NOT BE LIABLE FOR ANY DIRECT, INDIRECT,
> > + * SPECIAL, INCIDENTAL OR CONSEQUENTIAL DAMAGES, WHETHER BASED ON
> > + * CONTRACT, TORT OR ANY OTHER LEGAL THEORY, IN CONNECTION WITH OR
> > + * ARISING OUT OF THE FURNISHING, PERFORMANCE OR USE OF THIS SOFTWARE.
> > + */
> 
> ditto here
> 
> >  #include <linux/module.h>
> >  
> >  #include "ima.h"
> >  static const char *IMA_TEMPLATE_NAME = "ima";
> > +static const char *IMA_LABEL_FALLBACK = "n/a";
> 
> >  /*
> >   * ima_store_template - store ima template measurements
> > @@ -171,7 +195,13 @@ void ima_store_measurement(struct ima_iint_cache *iint, 
> > struct file *file,
> 
> When posting patches, please use a mailer that doesn't wrap lines.
> 
> >  	struct inode *inode = file->f_dentry->d_inode;
> >  	struct ima_template_entry *entry;
> >  	int violation = 0;
> > -
> > +	char *subj_label = NULL;
> > +	char *obj_label = NULL;
> > +	int obj_label_len;
> > +	int subj_label_len;
> > +	struct task_struct *tsk = current;
> > +	u32 osid, sid;
> > +	
> >  	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
> >  	if (!entry) {
> >  		integrity_audit_msg(AUDIT_INTEGRITY_PCR, inode, filename,
> > @@ -181,7 +211,25 @@ void ima_store_measurement(struct ima_iint_cache *iint, 
> > struct file *file,
> >  	memset(&entry->template, 0, sizeof(entry->template));
> >  	memcpy(entry->template.digest, iint->digest, IMA_DIGEST_SIZE);
> >  	strncpy(entry->template.file_name, filename, IMA_EVENT_NAME_LEN_MAX);
> > +	
> > +	if(file->f_dentry->d_inode) {
> > +		security_inode_getsecid(file->f_dentry->d_inode, &osid);
> > +		result = security_secid_to_secctx(osid, &obj_label, &obj_label_len);
> > +	} else
> > +		result = -1;
> > +
> > +	if(result)
> > +		strncpy(entry->template.obj_label, IMA_LABEL_FALLBACK, 
> > strlen(IMA_LABEL_FALLBACK));
> 
> Why bother? In the display code, check to to see if it is NULL.
> 
I think the label must be defined here, because the function "ima_store_template", called after these lines, 
calculates at this time the digest of the template.
 
> > +	else
> > +		strncpy(entry->template.obj_label, obj_label, obj_label_len);
> >  
> > +	security_task_getsecid(tsk, &sid);
> > +	result = security_secid_to_secctx(sid, &subj_label, &subj_label_len);
> > +	if(result)
> > +		strncpy(entry->template.subj_label, IMA_LABEL_FALLBACK, 
> > strlen(IMA_LABEL_FALLBACK));
> > +	else
> > +		strncpy(entry->template.subj_label, subj_label, subj_label_len);
> > + 
> >  	result = ima_store_template(entry, violation, inode);
> >  	if (!result)
> >  		iint->flags |= IMA_MEASURED;
> > diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> > index 0c72c9c..465b8b3 100644
> > --- a/security/integrity/ima/ima_fs.c
> > +++ b/security/integrity/ima/ima_fs.c
> > @@ -15,6 +15,30 @@
> >   *	implemenents security file system for reporting
> >   *	current measurement list and IMA statistics
> >   */
> > +/* ====================================================================
> > + * Copyright (C) 2009-2010 Politecnico di Torino, Italy
> > + * TORSEC group -- http://security.polito.it
> > + * Author: Roberto Sassu <rob...@po...>
> > + *
> > + * The original IMA source code from IBM, has been modified by Roberto Sassu 
> > + * to display extra information in the measurement list displayed through the
> > + * securityfs filesystem.
> > + *
> > + * DISCLAIMER of WARRANTY
> > + *
> > + * The following software "IMA" with measurement list format enhanced
> > + * is experimental and is provided "as is", and no guarantee or warranty
> > + * is given by Politecnico di Torino (TORSEC group), that the software is
> > + * fit for any particular purpose.  The user thereof uses the software at its
> > + * sole risk and liability. Politecnico di Torino shall have no obligation to
> > + * maintain or support this software. Politecnico di Torino MAKES NO
> > + * EXPRESS OR IMPLIED WARRANTY OF ANY KIND REGARDING THIS SOFTWARE.
> > + * Politecnico di Torino SHALL NOT BE LIABLE FOR ANY DIRECT, INDIRECT,
> > + * SPECIAL, INCIDENTAL OR CONSEQUENTIAL DAMAGES, WHETHER BASED ON
> > + * CONTRACT, TORT OR ANY OTHER LEGAL THEORY, IN CONNECTION WITH OR
> > + * ARISING OUT OF THE FURNISHING, PERFORMANCE OR USE OF THIS SOFTWARE.
> > + */
> > +
> 
> ditto here
> 
> >  #include <linux/fcntl.h>
> >  #include <linux/module.h>
> >  #include <linux/seq_file.h>
> > @@ -177,11 +201,16 @@ void ima_template_show(struct seq_file *m, void *e, enum 
> > ima_show_type show)
> >  {
> >  	struct ima_template_data *entry = e;
> >  	int namelen;
> > -
> > +	int obj_label_len;
> > +	int subj_label_len;
> > +	
> >  	switch (show) {
> >  	case IMA_SHOW_ASCII:
> >  		ima_print_digest(m, entry->digest);
> > -		seq_printf(m, " %s\n", entry->file_name);
> > +		seq_printf(m, " %s", entry->file_name);
> > +		seq_printf(m, " %s", entry->subj_label);
> > +		seq_printf(m, " %s\n", entry->obj_label);
> > +		
> >  		break;
> >  	case IMA_SHOW_BINARY:
> >  		ima_putc(m, entry->digest, IMA_DIGEST_SIZE);
> > @@ -189,6 +218,13 @@ void ima_template_show(struct seq_file *m, void *e, enum 
> > ima_show_type show)
> >  		namelen = strlen(entry->file_name);
> >  		ima_putc(m, &namelen, sizeof namelen);
> >  		ima_putc(m, entry->file_name, namelen);
> > +		subj_label_len = strlen(entry->subj_label);
> > +		ima_putc(m, &subj_label_len, sizeof subj_label_len);
> > +		ima_putc(m, entry->subj_label, subj_label_len);
> > +		obj_label_len = strlen(entry->obj_label);
> > +		ima_putc(m, &obj_label_len, sizeof obj_label_len);
> > +		ima_putc(m, entry->obj_label, obj_label_len);
> > +		
> >  	default:
> >  		break;
> >  	}
> > 
> 
> 
> 
> ------------------------------------------------------------------------------
> Download Intel&#174; Parallel Studio Eval
> Try the new software tools for yourself. Speed compiling, find bugs
> proactively, and fine-tune applications for parallel performance.
> See why Intel Parallel Studio got high marks during beta.
> http://p.sf.net/sfu/intel-sw-dev
> _______________________________________________
> Linux-ima-user mailing list
> Lin...@li...
> https://lists.sourceforge.net/lists/listinfo/linux-ima-user
> 

Re: [Linux-ima-user] Some build problem of IMA

[Mimi Z. <zo...@li...>]

On Mon, 2010-04-12 at 21:56 +0500, Shaz wrote:
> 
>         
>         There were a lot of IMA changes last December posted by Eric.
>         Parts were
>         picked up, some were entirely left out, and others were
>         incorrectly
>         modified, leaving IMA totally broken for quite a while.  It
>         only got
>         sorted out right before 2.6.33 was released.
>         
>         Commit 6593c8f77db7 removed IMA_IINT_DUMP_STACK, which is
>         causing the
>         initial problems above.
> 
> I am using 2.6.29-rc3 so should I back port the IMA code from the
> mentioned commit to it? Can you please let me know how to extract the
> patches because I suppose there will be others also contributing to
> IMA.
> 
> Thanks Mimi.

Hi Shaz,

Last time around, there were only a few IMA patches that could easily be
pulled using git log.  This time around it wouldn't be so easy, as there
are vfs changes as well.  There aren't all that many IMA bug fixes to
begin with, and those that do exist were back ported to stable.

The only exception is if you're using openAFS, which will emit imbalance
messages.  If that is a problem for you, we could back port a patch that
limits these messages.

Mimi

Re: [Linux-ima-user] [RFC][PATCH][1/1] ima: extending the format of the measurement list

[Mimi Z. <zo...@li...>]

On Wed, 2010-04-14 at 11:52 +0200, Roberto Sassu wrote:
> The format of the measurement list has been modified to carry additional 
> information.
> 
> Signed-off-by: Roberto Sassu <rob...@po...>
> Acked-by: Gianluca Ramunno <ra...@po...>

As this patchset contains only 1 patch, how about putting the
cover-letter description, or an abbreviated description, here in the
patch instead.

> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 47fb65d..7fb66ef 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -13,6 +13,29 @@
>   * File: ima.h
>   *	internal Integrity Measurement Architecture (IMA) definitions
>   */
> +/* ====================================================================
> + * Copyright (C) 2009-2010 Politecnico di Torino, Italy
> + * TORSEC group -- http://security.polito.it
> + * Author: Roberto Sassu <rob...@po...>
> + *
> + * The original IMA source code from IBM, has been modified by Roberto Sassu 
> + * to display extra information in the measurement list displayed through the
> + * securityfs filesystem.
> + *
> + * DISCLAIMER of WARRANTY
> + *
> + * The following software "IMA" with measurement list format enhanced
> + * is experimental and is provided "as is", and no guarantee or warranty
> + * is given by Politecnico di Torino (TORSEC group), that the software is
> + * fit for any particular purpose.  The user thereof uses the software at its
> + * sole risk and liability. Politecnico di Torino shall have no obligation to
> + * maintain or support this software. Politecnico di Torino MAKES NO
> + * EXPRESS OR IMPLIED WARRANTY OF ANY KIND REGARDING THIS SOFTWARE.
> + * Politecnico di Torino SHALL NOT BE LIABLE FOR ANY DIRECT, INDIRECT,
> + * SPECIAL, INCIDENTAL OR CONSEQUENTIAL DAMAGES, WHETHER BASED ON
> + * CONTRACT, TORT OR ANY OTHER LEGAL THEORY, IN CONNECTION WITH OR
> + * ARISING OUT OF THE FURNISHING, PERFORMANCE OR USE OF THIS SOFTWARE.
> + */

The Linux kernel is the normal GPL v2 license. Instead, how about adding
a line like:
Copyright (C) 2009-2010 Politecnico di Torino, Italy, 
	Roberto Sassu <rob...@po...>

>  #ifndef __LINUX_IMA_H
>  #define __LINUX_IMA_H
> @@ -30,6 +53,7 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 };
>  /* digest size for IMA, fits SHA1 or MD5 */
>  #define IMA_DIGEST_SIZE		20
>  #define IMA_EVENT_NAME_LEN_MAX	255
> +#define IMA_LABEL_LEN_MAX	255

The LSMs must have already defined something like this.

>  #define IMA_HASH_BITS 9
>  #define IMA_MEASURE_HTABLE_SIZE (1 << IMA_HASH_BITS)
> @@ -43,6 +67,8 @@ extern char *ima_hash;
>  struct ima_template_data {
>  	u8 digest[IMA_DIGEST_SIZE];	/* sha1/md5 measurement hash */
>  	char file_name[IMA_EVENT_NAME_LEN_MAX + 1];	/* name + \0 */
> +	char subj_label[IMA_LABEL_LEN_MAX + 1];	/* subj_label + \0 */
> +	char obj_label[IMA_LABEL_LEN_MAX + 1];	/* obj_label + \0 */
>  };

For backwards compatibility, this probably needs to be a new template,
with a runtime option - orig vs. new template.

>  struct ima_template_entry {
> diff --git a/security/integrity/ima/ima_api.c 
> b/security/integrity/ima/ima_api.c
> index 2a5e0bc..e612a49 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -12,10 +12,34 @@
>   *	Implements must_measure, collect_measurement, store_measurement,
>   *	and store_template.
>   */
> +/* ====================================================================
> + * Copyright (C) 2009-2010 Politecnico di Torino, Italy
> + * TORSEC group -- http://security.polito.it
> + * Author: Roberto Sassu <rob...@po...>
> + *
> + * The original IMA source code from IBM, has been modified by Roberto Sassu 
> + * to display extra information in the measurement list displayed through the
> + * securityfs filesystem.
> + *
> + * DISCLAIMER of WARRANTY
> + *
> + * The following software "IMA" with measurement list format enhanced
> + * is experimental and is provided "as is", and no guarantee or warranty
> + * is given by Politecnico di Torino (TORSEC group), that the software is
> + * fit for any particular purpose.  The user thereof uses the software at its
> + * sole risk and liability. Politecnico di Torino shall have no obligation to
> + * maintain or support this software. Politecnico di Torino MAKES NO
> + * EXPRESS OR IMPLIED WARRANTY OF ANY KIND REGARDING THIS SOFTWARE.
> + * Politecnico di Torino SHALL NOT BE LIABLE FOR ANY DIRECT, INDIRECT,
> + * SPECIAL, INCIDENTAL OR CONSEQUENTIAL DAMAGES, WHETHER BASED ON
> + * CONTRACT, TORT OR ANY OTHER LEGAL THEORY, IN CONNECTION WITH OR
> + * ARISING OUT OF THE FURNISHING, PERFORMANCE OR USE OF THIS SOFTWARE.
> + */

ditto here

>  #include <linux/module.h>
>  
>  #include "ima.h"
>  static const char *IMA_TEMPLATE_NAME = "ima";
> +static const char *IMA_LABEL_FALLBACK = "n/a";

>  /*
>   * ima_store_template - store ima template measurements
> @@ -171,7 +195,13 @@ void ima_store_measurement(struct ima_iint_cache *iint, 
> struct file *file,

When posting patches, please use a mailer that doesn't wrap lines.

>  	struct inode *inode = file->f_dentry->d_inode;
>  	struct ima_template_entry *entry;
>  	int violation = 0;
> -
> +	char *subj_label = NULL;
> +	char *obj_label = NULL;
> +	int obj_label_len;
> +	int subj_label_len;
> +	struct task_struct *tsk = current;
> +	u32 osid, sid;
> +	
>  	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
>  	if (!entry) {
>  		integrity_audit_msg(AUDIT_INTEGRITY_PCR, inode, filename,
> @@ -181,7 +211,25 @@ void ima_store_measurement(struct ima_iint_cache *iint, 
> struct file *file,
>  	memset(&entry->template, 0, sizeof(entry->template));
>  	memcpy(entry->template.digest, iint->digest, IMA_DIGEST_SIZE);
>  	strncpy(entry->template.file_name, filename, IMA_EVENT_NAME_LEN_MAX);
> +	
> +	if(file->f_dentry->d_inode) {
> +		security_inode_getsecid(file->f_dentry->d_inode, &osid);
> +		result = security_secid_to_secctx(osid, &obj_label, &obj_label_len);
> +	} else
> +		result = -1;
> +
> +	if(result)
> +		strncpy(entry->template.obj_label, IMA_LABEL_FALLBACK, 
> strlen(IMA_LABEL_FALLBACK));

Why bother? In the display code, check to to see if it is NULL.

> +	else
> +		strncpy(entry->template.obj_label, obj_label, obj_label_len);
>  
> +	security_task_getsecid(tsk, &sid);
> +	result = security_secid_to_secctx(sid, &subj_label, &subj_label_len);
> +	if(result)
> +		strncpy(entry->template.subj_label, IMA_LABEL_FALLBACK, 
> strlen(IMA_LABEL_FALLBACK));
> +	else
> +		strncpy(entry->template.subj_label, subj_label, subj_label_len);
> + 
>  	result = ima_store_template(entry, violation, inode);
>  	if (!result)
>  		iint->flags |= IMA_MEASURED;
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index 0c72c9c..465b8b3 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -15,6 +15,30 @@
>   *	implemenents security file system for reporting
>   *	current measurement list and IMA statistics
>   */
> +/* ====================================================================
> + * Copyright (C) 2009-2010 Politecnico di Torino, Italy
> + * TORSEC group -- http://security.polito.it
> + * Author: Roberto Sassu <rob...@po...>
> + *
> + * The original IMA source code from IBM, has been modified by Roberto Sassu 
> + * to display extra information in the measurement list displayed through the
> + * securityfs filesystem.
> + *
> + * DISCLAIMER of WARRANTY
> + *
> + * The following software "IMA" with measurement list format enhanced
> + * is experimental and is provided "as is", and no guarantee or warranty
> + * is given by Politecnico di Torino (TORSEC group), that the software is
> + * fit for any particular purpose.  The user thereof uses the software at its
> + * sole risk and liability. Politecnico di Torino shall have no obligation to
> + * maintain or support this software. Politecnico di Torino MAKES NO
> + * EXPRESS OR IMPLIED WARRANTY OF ANY KIND REGARDING THIS SOFTWARE.
> + * Politecnico di Torino SHALL NOT BE LIABLE FOR ANY DIRECT, INDIRECT,
> + * SPECIAL, INCIDENTAL OR CONSEQUENTIAL DAMAGES, WHETHER BASED ON
> + * CONTRACT, TORT OR ANY OTHER LEGAL THEORY, IN CONNECTION WITH OR
> + * ARISING OUT OF THE FURNISHING, PERFORMANCE OR USE OF THIS SOFTWARE.
> + */
> +

ditto here

>  #include <linux/fcntl.h>
>  #include <linux/module.h>
>  #include <linux/seq_file.h>
> @@ -177,11 +201,16 @@ void ima_template_show(struct seq_file *m, void *e, enum 
> ima_show_type show)
>  {
>  	struct ima_template_data *entry = e;
>  	int namelen;
> -
> +	int obj_label_len;
> +	int subj_label_len;
> +	
>  	switch (show) {
>  	case IMA_SHOW_ASCII:
>  		ima_print_digest(m, entry->digest);
> -		seq_printf(m, " %s\n", entry->file_name);
> +		seq_printf(m, " %s", entry->file_name);
> +		seq_printf(m, " %s", entry->subj_label);
> +		seq_printf(m, " %s\n", entry->obj_label);
> +		
>  		break;
>  	case IMA_SHOW_BINARY:
>  		ima_putc(m, entry->digest, IMA_DIGEST_SIZE);
> @@ -189,6 +218,13 @@ void ima_template_show(struct seq_file *m, void *e, enum 
> ima_show_type show)
>  		namelen = strlen(entry->file_name);
>  		ima_putc(m, &namelen, sizeof namelen);
>  		ima_putc(m, entry->file_name, namelen);
> +		subj_label_len = strlen(entry->subj_label);
> +		ima_putc(m, &subj_label_len, sizeof subj_label_len);
> +		ima_putc(m, entry->subj_label, subj_label_len);
> +		obj_label_len = strlen(entry->obj_label);
> +		ima_putc(m, &obj_label_len, sizeof obj_label_len);
> +		ima_putc(m, entry->obj_label, obj_label_len);
> +		
>  	default:
>  		break;
>  	}
> 

Re: [Linux-ima-user] [RFC][PATCH][0/2] IMA

[Mimi Z. <zo...@li...>]

On Wed, 2010-04-14 at 15:32 -0400, Eric Paris wrote:
> On Wed, Apr 14, 2010 at 5:51 AM, Roberto Sassu <rob...@po...> wrote:
> > Description of the issue:
> 
> > The parsing function is used only when custom rules must be loaded to replace
> > the default ones, but a different behavior can be observed in respect to the
> > first case: only one element is allocated and it is populated by the parsing
> > function that recognizes the tokens ' ' and '\n'.
> > So, having multiple lines in the file passed through the exported interface
> > doesn't lead to the same number of rules in the list, since only one allocated
> > item is available. If a directive spans two different lines, only the last
> > value is taken and the first one is overwritten.
> 
> I admit I haven't looked at the loading code before and it does seem
> 'special.'   It looks to me on first glance like the kernel accepts
> one rule per write() rather than one rule per line.  If you send it
> two rules in a single write you are going to get a rather
> unpredictable result.

Similar to smack, one rule per write.  Keeps it nice and simple.

> Your change is a good start to allow single writes which contain
> multiple rules.  I'm guessing that's a good idea, I though I think the
> parser should get a little smarter.  Does it ever make sense to have 2
> actions in a single rule, with the last one winning?  You also added
> support for some sort of commenting in the rule set didn't you?  That
> probably needs to be a lot stronger too....
> 
> I'm not acking or nacking here, just trying to understand exactly the
> problem we are trying to solve.  Loading rules using
> 
> cat rule > /sys/kernel/security/ima/policy
> 
> seems like a good direction to head.

hm, I was under the impression we wanted to minimize any
policy/configuration parsing done in the kernel.

Mimi

Re: [Linux-ima-user] [RFC][PATCH][0/2] IMA

[Eric P. <ep...@pa...>]

On Wed, Apr 14, 2010 at 5:51 AM, Roberto Sassu <rob...@po...> wrote:
> Description of the issue:

> The parsing function is used only when custom rules must be loaded to replace
> the default ones, but a different behavior can be observed in respect to the
> first case: only one element is allocated and it is populated by the parsing
> function that recognizes the tokens ' ' and '\n'.
> So, having multiple lines in the file passed through the exported interface
> doesn't lead to the same number of rules in the list, since only one allocated
> item is available. If a directive spans two different lines, only the last
> value is taken and the first one is overwritten.

I admit I haven't looked at the loading code before and it does seem
'special.'   It looks to me on first glance like the kernel accepts
one rule per write() rather than one rule per line.  If you send it
two rules in a single write you are going to get a rather
unpredictable result.

Your change is a good start to allow single writes which contain
multiple rules.  I'm guessing that's a good idea, I though I think the
parser should get a little smarter.  Does it ever make sense to have 2
actions in a single rule, with the last one winning?  You also added
support for some sort of commenting in the rule set didn't you?  That
probably needs to be a lot stronger too....

I'm not acking or nacking here, just trying to understand exactly the
problem we are trying to solve.  Loading rules using

cat rule > /sys/kernel/security/ima/policy

seems like a good direction to head.

Re: [Linux-ima-user] [RFC][PATCH][2/2] ima: policy matching decision depends on the overall "ima_measure" list

[Roberto S. <rob...@po...>]

Yes, this definitely solves the issue and probably grants better performance 
since there are less iterations in the list. The only thing needed is that the 
policy writer must be aware to write rules in the right order.
SELinux has the useful feature to modify the policy at runtime: in case of 
future versions of IMA will support dynamic policy loading, we can insert new 
rules using the "action" as sort key.

On Wednesday 14 April 2010 15:26:47 Eric Paris wrote:
> On Wed, Apr 14, 2010 at 5:52 AM, Roberto Sassu <rob...@po...> 
wrote:
> > The "ima_policy_match" function has been modified to handle situations in
> > a different manner: when two or more policies match criteria given, the
> > MEASURE decision is taken if there are no rule with action DONT_MEASURE.
> > 
> > Signed-off-by: Roberto Sassu <rob...@po...>
> > Acked-by: Gianluca Ramunno <ra...@po...>
> 
> Wouldn't it be better to just fix the rule ordering?  If you want the
> DONT_MEASURE rule to win put it first?
> 
> -Eric

Re: [Linux-ima-user] [RFC][PATCH][2/2] ima: policy matching decision depends on the overall "ima_measure" list

[Eric P. <ep...@pa...>]

On Wed, Apr 14, 2010 at 5:52 AM, Roberto Sassu <rob...@po...> wrote:
> The "ima_policy_match" function has been modified to handle situations in a
> different manner: when two or more policies match criteria given, the MEASURE
> decision is taken if there are no rule with action DONT_MEASURE.
>
> Signed-off-by: Roberto Sassu <rob...@po...>
> Acked-by: Gianluca Ramunno <ra...@po...>

Wouldn't it be better to just fix the rule ordering?  If you want the
DONT_MEASURE rule to win put it first?

-Eric

[Linux-ima-user] [RFC][PATCH][1/1] ima: extending the format of the measurement list

[Roberto S. <rob...@po...>]

The format of the measurement list has been modified to carry additional 
information.

Signed-off-by: Roberto Sassu <rob...@po...>
Acked-by: Gianluca Ramunno <ra...@po...>

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 47fb65d..7fb66ef 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -13,6 +13,29 @@
  * File: ima.h
  *	internal Integrity Measurement Architecture (IMA) definitions
  */
+/* ====================================================================
+ * Copyright (C) 2009-2010 Politecnico di Torino, Italy
+ * TORSEC group -- http://security.polito.it
+ * Author: Roberto Sassu <rob...@po...>
+ *
+ * The original IMA source code from IBM, has been modified by Roberto Sassu 
+ * to display extra information in the measurement list displayed through the
+ * securityfs filesystem.
+ *
+ * DISCLAIMER of WARRANTY
+ *
+ * The following software "IMA" with measurement list format enhanced
+ * is experimental and is provided "as is", and no guarantee or warranty
+ * is given by Politecnico di Torino (TORSEC group), that the software is
+ * fit for any particular purpose.  The user thereof uses the software at its
+ * sole risk and liability. Politecnico di Torino shall have no obligation to
+ * maintain or support this software. Politecnico di Torino MAKES NO
+ * EXPRESS OR IMPLIED WARRANTY OF ANY KIND REGARDING THIS SOFTWARE.
+ * Politecnico di Torino SHALL NOT BE LIABLE FOR ANY DIRECT, INDIRECT,
+ * SPECIAL, INCIDENTAL OR CONSEQUENTIAL DAMAGES, WHETHER BASED ON
+ * CONTRACT, TORT OR ANY OTHER LEGAL THEORY, IN CONNECTION WITH OR
+ * ARISING OUT OF THE FURNISHING, PERFORMANCE OR USE OF THIS SOFTWARE.
+ */
 
 #ifndef __LINUX_IMA_H
 #define __LINUX_IMA_H
@@ -30,6 +53,7 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 };
 /* digest size for IMA, fits SHA1 or MD5 */
 #define IMA_DIGEST_SIZE		20
 #define IMA_EVENT_NAME_LEN_MAX	255
+#define IMA_LABEL_LEN_MAX	255
 
 #define IMA_HASH_BITS 9
 #define IMA_MEASURE_HTABLE_SIZE (1 << IMA_HASH_BITS)
@@ -43,6 +67,8 @@ extern char *ima_hash;
 struct ima_template_data {
 	u8 digest[IMA_DIGEST_SIZE];	/* sha1/md5 measurement hash */
 	char file_name[IMA_EVENT_NAME_LEN_MAX + 1];	/* name + \0 */
+	char subj_label[IMA_LABEL_LEN_MAX + 1];	/* subj_label + \0 */
+	char obj_label[IMA_LABEL_LEN_MAX + 1];	/* obj_label + \0 */
 };
 
 struct ima_template_entry {
diff --git a/security/integrity/ima/ima_api.c 
b/security/integrity/ima/ima_api.c
index 2a5e0bc..e612a49 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -12,10 +12,34 @@
  *	Implements must_measure, collect_measurement, store_measurement,
  *	and store_template.
  */
+/* ====================================================================
+ * Copyright (C) 2009-2010 Politecnico di Torino, Italy
+ * TORSEC group -- http://security.polito.it
+ * Author: Roberto Sassu <rob...@po...>
+ *
+ * The original IMA source code from IBM, has been modified by Roberto Sassu 
+ * to display extra information in the measurement list displayed through the
+ * securityfs filesystem.
+ *
+ * DISCLAIMER of WARRANTY
+ *
+ * The following software "IMA" with measurement list format enhanced
+ * is experimental and is provided "as is", and no guarantee or warranty
+ * is given by Politecnico di Torino (TORSEC group), that the software is
+ * fit for any particular purpose.  The user thereof uses the software at its
+ * sole risk and liability. Politecnico di Torino shall have no obligation to
+ * maintain or support this software. Politecnico di Torino MAKES NO
+ * EXPRESS OR IMPLIED WARRANTY OF ANY KIND REGARDING THIS SOFTWARE.
+ * Politecnico di Torino SHALL NOT BE LIABLE FOR ANY DIRECT, INDIRECT,
+ * SPECIAL, INCIDENTAL OR CONSEQUENTIAL DAMAGES, WHETHER BASED ON
+ * CONTRACT, TORT OR ANY OTHER LEGAL THEORY, IN CONNECTION WITH OR
+ * ARISING OUT OF THE FURNISHING, PERFORMANCE OR USE OF THIS SOFTWARE.
+ */
 #include <linux/module.h>
 
 #include "ima.h"
 static const char *IMA_TEMPLATE_NAME = "ima";
+static const char *IMA_LABEL_FALLBACK = "n/a";
 
 /*
  * ima_store_template - store ima template measurements
@@ -171,7 +195,13 @@ void ima_store_measurement(struct ima_iint_cache *iint, 
struct file *file,
 	struct inode *inode = file->f_dentry->d_inode;
 	struct ima_template_entry *entry;
 	int violation = 0;
-
+	char *subj_label = NULL;
+	char *obj_label = NULL;
+	int obj_label_len;
+	int subj_label_len;
+	struct task_struct *tsk = current;
+	u32 osid, sid;
+	
 	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
 	if (!entry) {
 		integrity_audit_msg(AUDIT_INTEGRITY_PCR, inode, filename,
@@ -181,7 +211,25 @@ void ima_store_measurement(struct ima_iint_cache *iint, 
struct file *file,
 	memset(&entry->template, 0, sizeof(entry->template));
 	memcpy(entry->template.digest, iint->digest, IMA_DIGEST_SIZE);
 	strncpy(entry->template.file_name, filename, IMA_EVENT_NAME_LEN_MAX);
+	
+	if(file->f_dentry->d_inode) {
+		security_inode_getsecid(file->f_dentry->d_inode, &osid);
+		result = security_secid_to_secctx(osid, &obj_label, &obj_label_len);
+	} else
+		result = -1;
+
+	if(result)
+		strncpy(entry->template.obj_label, IMA_LABEL_FALLBACK, 
strlen(IMA_LABEL_FALLBACK));
+	else
+		strncpy(entry->template.obj_label, obj_label, obj_label_len);
 
+	security_task_getsecid(tsk, &sid);
+	result = security_secid_to_secctx(sid, &subj_label, &subj_label_len);
+	if(result)
+		strncpy(entry->template.subj_label, IMA_LABEL_FALLBACK, 
strlen(IMA_LABEL_FALLBACK));
+	else
+		strncpy(entry->template.subj_label, subj_label, subj_label_len);
+ 
 	result = ima_store_template(entry, violation, inode);
 	if (!result)
 		iint->flags |= IMA_MEASURED;
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 0c72c9c..465b8b3 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -15,6 +15,30 @@
  *	implemenents security file system for reporting
  *	current measurement list and IMA statistics
  */
+/* ====================================================================
+ * Copyright (C) 2009-2010 Politecnico di Torino, Italy
+ * TORSEC group -- http://security.polito.it
+ * Author: Roberto Sassu <rob...@po...>
+ *
+ * The original IMA source code from IBM, has been modified by Roberto Sassu 
+ * to display extra information in the measurement list displayed through the
+ * securityfs filesystem.
+ *
+ * DISCLAIMER of WARRANTY
+ *
+ * The following software "IMA" with measurement list format enhanced
+ * is experimental and is provided "as is", and no guarantee or warranty
+ * is given by Politecnico di Torino (TORSEC group), that the software is
+ * fit for any particular purpose.  The user thereof uses the software at its
+ * sole risk and liability. Politecnico di Torino shall have no obligation to
+ * maintain or support this software. Politecnico di Torino MAKES NO
+ * EXPRESS OR IMPLIED WARRANTY OF ANY KIND REGARDING THIS SOFTWARE.
+ * Politecnico di Torino SHALL NOT BE LIABLE FOR ANY DIRECT, INDIRECT,
+ * SPECIAL, INCIDENTAL OR CONSEQUENTIAL DAMAGES, WHETHER BASED ON
+ * CONTRACT, TORT OR ANY OTHER LEGAL THEORY, IN CONNECTION WITH OR
+ * ARISING OUT OF THE FURNISHING, PERFORMANCE OR USE OF THIS SOFTWARE.
+ */
+
 #include <linux/fcntl.h>
 #include <linux/module.h>
 #include <linux/seq_file.h>
@@ -177,11 +201,16 @@ void ima_template_show(struct seq_file *m, void *e, enum 
ima_show_type show)
 {
 	struct ima_template_data *entry = e;
 	int namelen;
-
+	int obj_label_len;
+	int subj_label_len;
+	
 	switch (show) {
 	case IMA_SHOW_ASCII:
 		ima_print_digest(m, entry->digest);
-		seq_printf(m, " %s\n", entry->file_name);
+		seq_printf(m, " %s", entry->file_name);
+		seq_printf(m, " %s", entry->subj_label);
+		seq_printf(m, " %s\n", entry->obj_label);
+		
 		break;
 	case IMA_SHOW_BINARY:
 		ima_putc(m, entry->digest, IMA_DIGEST_SIZE);
@@ -189,6 +218,13 @@ void ima_template_show(struct seq_file *m, void *e, enum 
ima_show_type show)
 		namelen = strlen(entry->file_name);
 		ima_putc(m, &namelen, sizeof namelen);
 		ima_putc(m, entry->file_name, namelen);
+		subj_label_len = strlen(entry->subj_label);
+		ima_putc(m, &subj_label_len, sizeof subj_label_len);
+		ima_putc(m, entry->subj_label, subj_label_len);
+		obj_label_len = strlen(entry->obj_label);
+		ima_putc(m, &obj_label_len, sizeof obj_label_len);
+		ima_putc(m, entry->obj_label, obj_label_len);
+		
 	default:
 		break;
 	}

[Linux-ima-user] [RFC][PATCH][0/1] IMA

[Roberto S. <rob...@po...>]

Description of the issue:

There is a variety of criteria that can be used to determine if a file must be 
measured or not; an useful feature of recent versions of IMA is the ability
of take measure decisions depending on the label assigned by a "Mandatory 
Access Control" to a process which is accessing a file or depending on the 
object label itself.
In general these data, i.e. the subject and the object labels, can be used 
during the "Remote Attestation" by a verifier to better evaluate if a trust 
relationship should be established with the remote peer: for example if 
it wants to check the integrity only of the sshd server, it can 
examine the executable and libraries mapped in the process assigned memory by 
selecting, from the list of measurements with enhanced format, the rows 
with subject label "sshd_t". 
This is just a possible check that can be done: to evaluate the integrity 
of a generic software component, the verifier must know what Mandatory Access
Control is being executed on the remote system and the relative policy since 
the latter is where labels and rules are defined. 
The current format of the measurement list is:

| PCR extended | template digest | ima | event digest | event name |


Solution proposed:

The new format proposed adds two new fields:

| PCR extended | template digest | ima | event digest | event name | subject 
label | object label |

The added data is retrieved in "ima_store_measurement" by calling the function
"security_secid_to_secctx".
Since the template digest is obtained by performing the SHA1 of "event digest" 
and "event name", in the patched code even the two added fields have been 
included in the hash operation.

This patch set applies to kernel 2.6.32 series and 2.6.33 series.

Roberto Sassu (1):
        ima: extending the format of the measurement list

[Linux-ima-user] [RFC][PATCH][2/2] ima: policy matching decision depends on the overall "ima_measure" list

[Roberto S. <rob...@po...>]

The "ima_policy_match" function has been modified to handle situations in a 
different manner: when two or more policies match criteria given, the MEASURE 
decision is taken if there are no rule with action DONT_MEASURE.

Signed-off-by: Roberto Sassu <rob...@po...>
Acked-by: Gianluca Ramunno <ra...@po...>

diff --git a/security/integrity/ima/ima_policy.c 
b/security/integrity/ima/ima_policy.c
index 4759d0f..668e40f 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -161,15 +185,19 @@ static bool ima_match_rules(struct 
ima_measure_rule_entry *rule,
 int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask)
 {
 	struct ima_measure_rule_entry *entry;
+	int flag_matched = 0;
 
 	list_for_each_entry(entry, ima_measure, list) {
 		bool rc;
 
 		rc = ima_match_rules(entry, inode, func, mask);
-		if (rc)
-			return entry->action;
+		if (rc)  {
+			if(!entry->action)
+				return 0;
+			flag_matched = 1;
+		}
 	}
-	return 0;
+	return flag_matched;
 }
 
 /**

[Linux-ima-user] [RFC][PATCH][1/2] ima: fix loading multi line policies from userspace

[Roberto S. <rob...@po...>]

A new item in the list pointed by "ima_measure" is allocated when a new line 
is parsed from the policy loaded through the /sys interface.

Signed-off-by: Roberto Sassu <rob...@po...>
Acked-by: Gianluca Ramunno <ra...@po...>

diff --git a/security/integrity/ima/ima_policy.c   
b/security/integrity/ima/ima_policy.c
index 4759d0f..668e40f 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -10,6 +10,30 @@
  * 	- initialize default measure policy rules
  *
  */
+/* ====================================================================
+ * Copyright (C) 2009-2010 Politecnico di Torino, Italy
+ * TORSEC group -- http://security.polito.it
+ * Author: Roberto Sassu <rob...@po...>
+ *
+ * The original IMA source code from IBM, has been modified by Roberto Sassu 
+ * to correctly load a set of policies for measuring files depending on given 
+ * criteria. 
+ *
+ * DISCLAIMER of WARRANTY
+ *
+ * The following software "IMA" with the patch applied by Roberto Sassu
+ * is experimental and is provided "as is", and no guarantee or warranty
+ * is given by Politecnico di Torino (TORSEC group), that the software is
+ * fit for any particular purpose.  The user thereof uses the software at its
+ * sole risk and liability. Politecnico di Torino shall have no obligation to
+ * maintain or support this software. Politecnico di Torino MAKES NO
+ * EXPRESS OR IMPLIED WARRANTY OF ANY KIND REGARDING THIS SOFTWARE.
+ * Politecnico di Torino SHALL NOT BE LIABLE FOR ANY DIRECT, INDIRECT,
+ * SPECIAL, INCIDENTAL OR CONSEQUENTIAL DAMAGES, WHETHER BASED ON
+ * CONTRACT, TORT OR ANY OTHER LEGAL THEORY, IN CONNECTION WITH OR
+ * ARISING OUT OF THE FURNISHING, PERFORMANCE OR USE OF THIS SOFTWARE.
+ */
+
 #include <linux/module.h>
 #include <linux/list.h>
 #include <linux/security.h>
@@ -261,7 +289,7 @@ static int ima_parse_rule(char *rule, struct 
ima_measure_rule_entry *entry)
 	ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_RULE);
 
 	entry->action = -1;
-	while ((p = strsep(&rule, " \n")) != NULL) {
+	while ((p = strsep(&rule, " ")) != NULL) {
 		substring_t args[MAX_OPT_ARGS];
 		int token;
 		unsigned long lnum;
@@ -388,9 +416,11 @@ static int ima_parse_rule(char *rule, struct 
ima_measure_rule_entry *entry)
 int ima_parse_add_rule(char *rule)
 {
 	const char *op = "update_policy";
-	struct ima_measure_rule_entry *entry;
+	struct ima_measure_rule_entry *entry, *tmp;
 	int result = 0;
 	int audit_info = 0;
+	int fail = 0;
+	char *p;
 
 	/* Prevent installed policy from changing */
 	if (ima_measure != &measure_default_rules) {
@@ -400,25 +430,39 @@ int ima_parse_add_rule(char *rule)
 		return -EACCES;
 	}
 
-	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
-	if (!entry) {
-		integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
-				    NULL, op, "-ENOMEM", -ENOMEM, audit_info);
-		return -ENOMEM;
-	}
-
-	INIT_LIST_HEAD(&entry->list);
-
-	result = ima_parse_rule(rule, entry);
-	if (!result) {
-		mutex_lock(&ima_measure_mutex);
-		list_add_tail(&entry->list, &measure_policy_rules);
-		mutex_unlock(&ima_measure_mutex);
-	} else {
-		kfree(entry);
-		integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
+	while ((p = strsep(&rule, "\n")) != NULL) {
+		if(!*p)
+			continue;
+		if(*p == '#')
+			continue;
+		
+		entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+		if (!entry) {
+			integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
+				NULL, op, "-ENOMEM", -ENOMEM, audit_info);
+			return -ENOMEM;
+		}  
+		result = ima_parse_rule(p, entry);
+		if (!result) {
+			mutex_lock(&ima_measure_mutex);
+			list_add_tail(&entry->list, &measure_policy_rules);
+			mutex_unlock(&ima_measure_mutex);
+		} else {
+			kfree(entry);
+			integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
 				    NULL, op, "invalid policy", result,
 				    audit_info);
+			fail = 1;
+			break;
+		}
+	}
+	if(fail) {
+		mutex_lock(&ima_measure_mutex);
+		list_for_each_entry_safe(entry, tmp, &measure_policy_rules, list) {
+			list_del(&entry->list);
+			kfree(entry);
+		}
+		mutex_unlock(&ima_measure_mutex);	  	  
 	}
 	return result;
 }

[Linux-ima-user] [RFC][PATCH][0/2] IMA

[Roberto S. <rob...@po...>]

Description of the issue: 

IMA performs measurement of files depending on a set of policy loaded. There
are two ways to specify the policy to be used: first by adding the string
"ima_tcb=1" to kernel command line (this causes the loading of a set of 
hard coded rules); the second by passing the content of a file containing a set
of custom rules through a special file exported by IMA in the "securityfs".
In both cases rules are stored in memory in a double linked list, which is 
used by the function "ima_match_policy" to determine if the file must be 
measured.
If the first method is used, then the list is populated when the initialization
function of IMA is called by the kernel; if not, custom policies can be loaded 
at any time and this operation is allowed only once.
Hard coded rules are stored in a static vector and the initialization function 
adds each item to the list by calling the function "list_add_tail". In the 
version of IMA shipped with the kernel 2.6.33 the list contains 9 items.
The parsing function is used only when custom rules must be loaded to replace 
the default ones, but a different behavior can be observed in respect to the 
first case: only one element is allocated and it is populated by the parsing 
function that recognizes the tokens ' ' and '\n'. 
So, having multiple lines in the file passed through the exported interface 
doesn't lead to the same number of rules in the list, since only one allocated
item is available. If a directive spans two different lines, only the last 
value is taken and the first one is overwritten.

Another issue is how IMA determines if a file must be measured or not: 
the behavior is to return the action expressed in the first rule of the list 
that matches criteria given.
This may cause unattended measurement operations: if there are two rules, 
one generic to measure all files with uid 0 and another not to measure files 
in "tmpfs" filesystem and these are written in this order, only the first one 
is considered for root owned files, because the first policy always matches. 
So, this means that all files with uid 0 in a "tmpfs" filesystem will 
be measured.


Solution proposed:

We decided to handle tokens ' ' and '\n' in a different way: first a new 
policy item is allocated each time the second token is encountered: 
so, each line of the file will correspond to a different element; second the 
parsing function will handle only the ' ' token and store the parsing result 
in the passed item. If an error happens in the middle of the procedure, the 
list is cleaned up as usual.
Another code update has been done to solve the second issue reported: in the 
function "ima_match_policy", instead of stopping at the first rule in the list
that matches the given criteria and returning the corresponding action, 
all items will inspected and the decision to measure will be taken only if 
there are no matching rules with action "don't measure". 
This solves the problem described in the previous section.

This patch set applies to kernel 2.6.32 series and 2.6.33 series.

Roberto Sassu (2):
	ima: fix loading multi line policies from userspace
	ima: policy matching decision depends on the overall "ima_measure" list

Re: [Linux-ima-user] IMA measurement list layout question

[Mimi Z. <zo...@li...>]

On Fri, 2010-04-09 at 14:48 +0200, Roberto Sassu wrote:
> Hi Mimi
> 
> sorry for so late reply, i was so busy last month. 
> I successfully tested the EVM patch on a Fedora 12 system and it works well.

Glad it works well.

> I have created some patches for IMA: one is a bug fix in the procedure which 
> handles loading of custom policies through the /sys interface (i released the 
> patch in another mail but now i further investigated the code); the other one 
> contains a modification of the format of the measurement list.

Did you post the patch on a mailing list?  I must have missed it.  If
you want to upstream the patches, could you re-post the patches here
inline, separating the bug fix from other changes - parsing the policy
from the rule matching?

The current policy is an ordered list with clearly defined results. The
rule matching changes have me a bit concerned, as a less specific policy
could override a more specific one, as they both match.

> The proposal is to add other information in such list to give to verifier the 
> ability to better evaluate the integrity of the remote peer. I think that 
> including MAC labels of subject and object can be useful for example when 
> identifying system components affected by a file corruption: may be possible to 
> successful attest the integrity of a platform if critical processes are 
> isolated from those compromised.
> Patches and relative README files are available at:
> 
> http://security.polito.it/tc/kma
> 

Yes, this is a good start for what needs to be included in the template.
Posting the patch here inline would make reviewing easier.

Thanks!

Mimi

Re: [Linux-ima-user] Some build problem of IMA

[Shaz <sha...@gm...>]

> There were a lot of IMA changes last December posted by Eric. Parts were
> picked up, some were entirely left out, and others were incorrectly
> modified, leaving IMA totally broken for quite a while.  It only got
> sorted out right before 2.6.33 was released.
>
> Commit 6593c8f77db7 removed IMA_IINT_DUMP_STACK, which is causing the
> initial problems above.
>

I am using 2.6.29-rc3 so should I back port the IMA code from the mentioned
commit to it? Can you please let me know how to extract the patches because
I suppose there will be others also contributing to IMA.

Thanks Mimi.

-- 
Shaz

Re: [Linux-ima-user] Some build problem of IMA

[Mimi Z. <zo...@li...>]

On Thu, 2010-04-08 at 15:54 +0500, Shaz wrote:
> Dear list,
> 
> Working on some IMA code that worked for me earlier is now giving me
> problems. I checked the latest code and the code I use seems fine thus
> can't figure what's wrong with it. Need help ....
> 
> Following are the error messages and I am also attaching the
> ima_main.c that I am using.
> 
>   CC      security/integrity/ima/ima_main.o
> security/integrity/ima/ima_main.c: In function ‘ima_file_free’:
> security/integrity/ima/ima_main.c:66: error: expected ‘)’ before
> numeric constant
> security/integrity/ima/ima_main.c:69: error: expected ‘;’ before
> numeric constant
> security/integrity/ima/ima_main.c:82: error: expected expression
> before ‘;’ token
> security/integrity/ima/ima_main.c: In function ‘ima_path_check’:
> security/integrity/ima/ima_main.c:189: error: expected expression
> before ‘)’ token
> security/integrity/ima/ima_main.c:193: error: ‘dentry’ undeclared
> (first use in this function)
> security/integrity/ima/ima_main.c:193: error: (Each undeclared
> identifier is reported only once
> security/integrity/ima/ima_main.c:193: error: for each function it appears in.)
> make[2]: *** [security/integrity/ima/ima_main.o] Error 1

There were a lot of IMA changes last December posted by Eric. Parts were
picked up, some were entirely left out, and others were incorrectly
modified, leaving IMA totally broken for quite a while.  It only got
sorted out right before 2.6.33 was released.

Commit 6593c8f77db7 removed IMA_IINT_DUMP_STACK, which is causing the
initial problems above.

Mimi

Re: [Linux-ima-user] IMA measurement list layout question

[Roberto S. <rob...@po...>]

Hi Mimi

sorry for so late reply, i was so busy last month. 
I successfully tested the EVM patch on a Fedora 12 system and it works well.
I have created some patches for IMA: one is a bug fix in the procedure which 
handles loading of custom policies through the /sys interface (i released the 
patch in another mail but now i further investigated the code); the other one 
contains a modification of the format of the measurement list.
The proposal is to add other information in such list to give to verifier the 
ability to better evaluate the integrity of the remote peer. I think that 
including MAC labels of subject and object can be useful for example when 
identifying system components affected by a file corruption: may be possible to 
successful attest the integrity of a platform if critical processes are 
isolated from those compromised.
Patches and relative README files are available at:

http://security.polito.it/tc/kma


On Monday 15 March 2010 13:43:31 you wrote:
> On Mon, 2010-03-15 at 10:50 +0100, Roberto Sassu wrote:
> > Hello all
> > 
> > i was thinking about remote attestation and the data that IMA makes
> > available. I see in the measurement list the complete path of measured
> > files is not displayed; instead only the dentry name is included. My
> > question is about the reason: i know that calling the d_path() function
> > to retrieve the complete path has impact in the performance, but maybe
> > there are different motivations, like the fact that from the value used
> > to extend the PCR the verificator is able to identify immediately the
> > "type" of the file. For example: we suppose that two clients with
> > different distributions have the same version of the libc but stored in
> > different path; the verificator is able to immediately assure, from the
> > digest used to extend the pcr, that the two clients have loaded the same
> > file, which is a version of the libc.
> > Does the measurement list layout of IMA is that due to performance
> > concerns or there are other motivations like this mentioned in the
> > example?
> > 
> > Thanks in advance for replies.
> 
> Hi Roberto! I wish the reason for not including the full pathname was
> for a good reason. At the time, calling d_path() was unacceptable, but
> many things have changed since, including support for Tomoyo, which is
> path based.  I haven't had a chance to look at the Kconfig
> 'SECURITY_PATH' option. It's probably a good time to revisit this
> issue.
> 
> Now that IMA is template based, the information included in the template
> data is not just a hint anymore, as it is included in the hash. So
> besides for a full pathname, is there anything else that would make file
> identification easier?
> 
> Thanks!
> 
> Mimi

[Linux-ima-user] Some build problem of IMA

[Shaz <sha...@gm...>]

Dear list,

Working on some IMA code that worked for me earlier is now giving me
problems. I checked the latest code and the code I use seems fine thus
can't figure what's wrong with it. Need help ....

Following are the error messages and I am also attaching the
ima_main.c that I am using.

  CC      security/integrity/ima/ima_main.o
security/integrity/ima/ima_main.c: In function ‘ima_file_free’:
security/integrity/ima/ima_main.c:66: error: expected ‘)’ before
numeric constant
security/integrity/ima/ima_main.c:69: error: expected ‘;’ before
numeric constant
security/integrity/ima/ima_main.c:82: error: expected expression
before ‘;’ token
security/integrity/ima/ima_main.c: In function ‘ima_path_check’:
security/integrity/ima/ima_main.c:189: error: expected expression
before ‘)’ token
security/integrity/ima/ima_main.c:193: error: ‘dentry’ undeclared
(first use in this function)
security/integrity/ima/ima_main.c:193: error: (Each undeclared
identifier is reported only once
security/integrity/ima/ima_main.c:193: error: for each function it appears in.)
make[2]: *** [security/integrity/ima/ima_main.o] Error 1


-- 
Shaz

[Linux-ima-user] [RFC][PATCH 14/14] ima: add ima_inode_setxattr and ima_inode_removexattr

[Mimi Z. <zo...@li...>]

Based on xattr_permission comments, the restriction to modify
'security' xattr is left up to the underlying fs or lsm. Ensure
that not just anyone can modify or remove 'security.ima'.

Signed-off-by: Mimi Zohar <zo...@us...>

diff --git a/include/linux/ima.h b/include/linux/ima.h
index ce82e29..3307420 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -20,6 +20,9 @@ extern void ima_file_free(struct file *file);
 extern int ima_file_mmap(struct file *file, unsigned long prot);
 extern void ima_counts_get(struct file *file);
 extern void ima_inode_post_setattr(struct dentry *dentry);
+extern int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
+		       const void *xattr_value, size_t xattr_value_len);
+extern int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name);
 
 #else
 static inline int ima_bprm_check(struct linux_binprm *bprm)
@@ -52,5 +55,15 @@ static inline void ima_inode_post_setattr(struct dentry *dentry)
 	return;
 }
 
+int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
+		       const void *xattr_value, size_t xattr_value_len)
+{
+	return 0;
+}
+
+int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name)
+{
+	return 0;
+}
 #endif /* CONFIG_IMA_H */
 #endif /* _LINUX_IMA_H */
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 635a3be..e5a52a6 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -370,6 +370,32 @@ void ima_inode_post_setattr(struct dentry *dentry)
 	return;
 }
 
+/*
+ * ima_protect_xattr - protect 'security.ima'
+ *
+ * Ensure that not just anyone can modify or remove 'security.ima'.
+ */
+int ima_protect_xattr(struct dentry *dentry, const char *xattr_name,
+		      const void *xattr_value, size_t xattr_value_len)
+{
+	if ((strcmp(xattr_name, XATTR_NAME_IMA) == 0)
+	    && !capable(CAP_MAC_ADMIN))
+		return -EPERM;
+	return 0;
+}
+
+int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
+		       const void *xattr_value, size_t xattr_value_len)
+{
+	return ima_protect_xattr(dentry, xattr_name, xattr_value,
+				 xattr_value_len);
+}
+
+int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name)
+{
+	return ima_protect_xattr(dentry, xattr_name, NULL, 0);
+}
+
 static int __init init_ima(void)
 {
 	int error;
diff --git a/security/security.c b/security/security.c
index 754cb25..6ab38ad 100644
--- a/security/security.c
+++ b/security/security.c
@@ -599,6 +599,9 @@ int security_inode_setxattr(struct dentry *dentry, const char *name,
 	ret = security_ops->inode_setxattr(dentry, name, value, size, flags);
 	if (ret)
 		return ret;
+	ret = ima_inode_setxattr(dentry, name, value, size);
+	if (ret)
+		return ret;
 	return evm_inode_setxattr(dentry, name, value, size);
 }
 
@@ -634,6 +637,9 @@ int security_inode_removexattr(struct dentry *dentry, const char *name)
 	ret = security_ops->inode_removexattr(dentry, name);
 	if (ret)
 		return ret;
+	ret = ima_inode_removexattr(dentry, name);
+	if (ret)
+		return ret;
 	return evm_inode_removexattr(dentry, name);
 }
 
-- 
1.6.6

[Linux-ima-user] [RFC][PATCH 12/14] ima: appraise default rules

[Mimi Z. <zo...@li...>]

Unlike the IMA measurement policy, the appraise policy can not be
dependent on runtime process information, such as the task uid,
as the 'security.ima' xattr is written on file close and must
be updated each time the file changes, regardless of the current
task uid. The appraise default policy appraises all files owned
by root.

Signed-off-by: Mimi Zohar <zo...@us...>
Acked-by: Serge Hallyn <se...@us...>

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 2d1d99b..12f9975 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -122,6 +122,8 @@ void iint_rcu_free(struct rcu_head *rcu);
 enum ima_hooks { FILE_CHECK = 1, FILE_MMAP, BPRM_CHECK, POST_SETATTR };
 
 int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask);
+int ima_match_appraise_policy(struct inode *inode, enum ima_hooks func,
+			      int mask);
 void ima_init_policy(void);
 void ima_update_policy(void);
 int ima_parse_add_rule(char *);
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index a6acde5..fefbd18 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -28,7 +28,19 @@ __setup("ima_appraise=", default_appraise_setup);
 int ima_must_appraise(struct integrity_iint_cache *iint, struct inode *inode,
 		      enum ima_hooks func, int mask)
 {
-	return 0;
+	int must_appraise, rc = 0;
+
+	if (!ima_appraise || !inode->i_op->getxattr)
+		return 0;
+	else if (iint->flags & IMA_APPRAISED)
+		return 0;
+
+	must_appraise = ima_match_appraise_policy(inode, func, mask);
+	if (must_appraise) {
+		iint->flags |= IMA_APPRAISE;
+		rc = 1;
+	}
+	return rc;
 }
 
 static void ima_fix_xattr(struct dentry *dentry,
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 4759d0f..6f31f7f 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -23,8 +23,11 @@
 #define IMA_MASK 	0x0002
 #define IMA_FSMAGIC	0x0004
 #define IMA_UID		0x0008
+#define IMA_OWNER	0x0010
 
-enum ima_action { UNKNOWN = -1, DONT_MEASURE = 0, MEASURE };
+enum ima_action { UNKNOWN = -1,
+		  DONT_MEASURE = 0, MEASURE,
+		  DONT_APPRAISE, APPRAISE};
 
 #define MAX_LSM_RULES 6
 enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE, LSM_OBJ_TYPE,
@@ -39,6 +42,7 @@ struct ima_measure_rule_entry {
 	int mask;
 	unsigned long fsmagic;
 	uid_t uid;
+	uid_t owner;
 	struct {
 		void *rule;	/* LSM file metadata specific */
 		int type;	/* audit type */
@@ -47,7 +51,7 @@ struct ima_measure_rule_entry {
 
 /*
  * Without LSM specific knowledge, the default policy can only be
- * written in terms of .action, .func, .mask, .fsmagic, and .uid
+ * written in terms of .action, .func, .mask, .fsmagic, .uid, and .owner
  */
 
 /*
@@ -69,6 +73,13 @@ static struct ima_measure_rule_entry default_rules[] = {
 	 .flags = IMA_FUNC | IMA_MASK},
 	{.action = MEASURE,.func = FILE_CHECK,.mask = MAY_READ,.uid = 0,
 	 .flags = IMA_FUNC | IMA_MASK | IMA_UID},
+	{.action = DONT_APPRAISE,.fsmagic = PROC_SUPER_MAGIC,.flags = IMA_FSMAGIC},
+	{.action = DONT_APPRAISE,.fsmagic = SYSFS_MAGIC,.flags = IMA_FSMAGIC},
+	{.action = DONT_APPRAISE,.fsmagic = DEBUGFS_MAGIC,.flags = IMA_FSMAGIC},
+	{.action = DONT_APPRAISE,.fsmagic = TMPFS_MAGIC,.flags = IMA_FSMAGIC},
+	{.action = DONT_APPRAISE,.fsmagic = SECURITYFS_MAGIC,.flags = IMA_FSMAGIC},
+	{.action = DONT_APPRAISE,.fsmagic = SELINUX_MAGIC,.flags = IMA_FSMAGIC},
+	{.action = APPRAISE,.owner = 0,.flags = IMA_OWNER},
 };
 
 static LIST_HEAD(measure_default_rules);
@@ -109,6 +120,8 @@ static bool ima_match_rules(struct ima_measure_rule_entry *rule,
 		return false;
 	if ((rule->flags & IMA_UID) && rule->uid != tsk->cred->uid)
 		return false;
+	if ((rule->flags & IMA_OWNER) && rule->owner != inode->i_uid)
+		return false;
 	for (i = 0; i < MAX_LSM_RULES; i++) {
 		int rc = 0;
 		u32 osid, sid;
@@ -165,6 +178,9 @@ int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask)
 	list_for_each_entry(entry, ima_measure, list) {
 		bool rc;
 
+		if ((entry->action == APPRAISE) ||
+		    (entry->action == DONT_APPRAISE))
+			continue;
 		rc = ima_match_rules(entry, inode, func, mask);
 		if (rc)
 			return entry->action;
@@ -172,6 +188,28 @@ int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask)
 	return 0;
 }
 
+int ima_match_appraise_policy(struct inode *inode, enum ima_hooks func,
+			      int mask)
+{
+	struct ima_measure_rule_entry *entry;
+
+	list_for_each_entry(entry, ima_measure, list) {
+		bool rc;
+
+		if ((entry->action == MEASURE) ||
+		    (entry->action == DONT_MEASURE))
+			continue;
+		rc = ima_match_rules(entry, inode, func, mask);
+		if (rc) {
+			if (entry->action == DONT_APPRAISE)
+				return 0;
+			if (entry->action == APPRAISE)
+				return 1;
+		}
+	}
+	return 0;
+}
+
 /**
  * ima_init_policy - initialize the default measure rules.
  *
@@ -219,6 +257,7 @@ void ima_update_policy(void)
 enum {
 	Opt_err = -1,
 	Opt_measure = 1, Opt_dont_measure,
+	Opt_appraise, Opt_dont_appraise,
 	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_uid
@@ -227,6 +266,8 @@ enum {
 static match_table_t policy_tokens = {
 	{Opt_measure, "measure"},
 	{Opt_dont_measure, "dont_measure"},
+	{Opt_appraise, "appraise"},
+	{Opt_dont_appraise, "dont_appraise"},
 	{Opt_obj_user, "obj_user=%s"},
 	{Opt_obj_role, "obj_role=%s"},
 	{Opt_obj_type, "obj_type=%s"},
@@ -280,6 +321,14 @@ static int ima_parse_rule(char *rule, struct ima_measure_rule_entry *entry)
 			audit_log_format(ab, "%s ", "dont_measure");
 			entry->action = DONT_MEASURE;
 			break;
+		case Opt_appraise:
+			audit_log_format(ab, "%s ", "appraise");
+			entry->action = APPRAISE;
+			break;
+		case Opt_dont_appraise:
+			audit_log_format(ab, "%s ", "dont_appraise");
+			entry->action = DONT_APPRAISE;
+			break;
 		case Opt_func:
 			audit_log_format(ab, "func=%s ", args[0].from);
 			if (strcmp(args[0].from, "FILE_CHECK") == 0)
-- 
1.6.6

[Linux-ima-user] [RFC][PATCH 13/14] ima: inode post_setattr

[Mimi Z. <zo...@li...>]

Changing an inode's metadata may result in our not needing to
appraise the file.  In such cases, we must remove 'security.ima'.

Signed-off-by: Mimi Zohar <zo...@us...>
Acked-by: Serge Hallyn <se...@us...>

diff --git a/fs/attr.c b/fs/attr.c
index 2391242..0683d25 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -15,6 +15,7 @@
 #include <linux/quotaops.h>
 #include <linux/security.h>
 #include <linux/evm.h>
+#include <linux/ima.h>
 
 /* Taken over from the old code... */
 
@@ -228,6 +229,7 @@ int notify_change(struct dentry * dentry, struct iattr * attr)
 
 	if (!error) {
 		fsnotify_change(dentry, ia_valid);
+		ima_inode_post_setattr(dentry);
 		evm_inode_post_setattr(dentry, ia_valid);
 	}
 
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 4dce900..ce82e29 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -19,6 +19,7 @@ extern int ima_file_check(struct file *file, int mask);
 extern void ima_file_free(struct file *file);
 extern int ima_file_mmap(struct file *file, unsigned long prot);
 extern void ima_counts_get(struct file *file);
+extern void ima_inode_post_setattr(struct dentry *dentry);
 
 #else
 static inline int ima_bprm_check(struct linux_binprm *bprm)
@@ -46,5 +47,10 @@ static inline void ima_counts_get(struct file *file)
 	return;
 }
 
+static inline void ima_inode_post_setattr(struct dentry *dentry)
+{
+	return;
+}
+
 #endif /* CONFIG_IMA_H */
 #endif /* _LINUX_IMA_H */
-- 
1.6.6