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

SourceForge Headquarters 1320 Columbia Street Suite 310 San Diego, CA 92101 +1 (858) 422-6466

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