linux-kernel - Re: [PATCH v5 05/21] ima: move ima_file

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 20 May 2011 08:40:19 -0500
From:	"Serge E. Hallyn" <serge@...lyn.com>
To:	Mimi Zohar <zohar@...ux.vnet.ibm.com>
Cc:	"Serge E. Hallyn" <serge@...lyn.com>,
	linux-security-module@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	James Morris <jmorris@...ei.org>,
	David Safford <safford@...son.ibm.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Greg KH <greg@...ah.com>,
	Dmitry Kasatkin <dmitry.kasatkin@...ia.com>,
	Mimi Zohar <zohar@...ibm.com>
Subject: Re: [PATCH v5 05/21] ima: move ima_file_free before releasing the
 file

Quoting Mimi Zohar (zohar@...ux.vnet.ibm.com):
> On Thu, 2011-05-19 at 17:06 -0500, Serge E. Hallyn wrote:
> > Quoting Mimi Zohar (zohar@...ux.vnet.ibm.com):
> > > Integrity appraisal measures files on file_free and stores the file
> > > measurement as an xattr.  Measure the file before releasing it.
> > 
> > Can you put a bit more in the commit msg about why?  What's magic
> > about the fs-specific release function?
> 
> ima_file_free(), called on __fput(), currently flags files that have
> changed, so that the file is re-measured.  However, for appraising a
> files's integrity, flagging the file is not enough.  The file's
> security.ima xattr must be updated to reflect any changes.  This patch
> moves releasing the file to after calculating the new file hash.

Sorry if I'm being dense, but I still don't understand (even though
apparently I used to :)  why the fs release is magic here.  The
dropping of the writelock comes later, so no file will be able to
open the file for execute or write until that point, meaning that
won't be happening without a re-measure with or without this patch.

So you must be thinking something about general opens(), but I
don't believe that file_operations->release makes a difference to
another tasks's ability to open the file, nor to the writing out
of changed contents.

security_file_free() doesn't appear to be hooked by ima or evm,
and if a security module changes its security.X xattr you'll
end up re-measuring the xattrs anyway.

So I'm still missing something, sorry  :)

-serge

> > > Signed-off-by: Mimi Zohar <zohar@...ibm.com>
> > > Acked-by: Serge Hallyn <serge.hallyn@...ntu.com>
> > > ---
> > >  fs/file_table.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/fs/file_table.c b/fs/file_table.c
> > > index 01e4c1e..33f54a1 100644
> > > --- a/fs/file_table.c
> > > +++ b/fs/file_table.c
> > > @@ -243,10 +243,10 @@ static void __fput(struct file *file)
> > >  		if (file->f_op && file->f_op->fasync)
> > >  			file->f_op->fasync(-1, file, 0);
> > >  	}
> > > +	ima_file_free(file);
> > >  	if (file->f_op && file->f_op->release)
> > >  		file->f_op->release(inode, file);
> > >  	security_file_free(file);
> > > -	ima_file_free(file);
> > >  	if (unlikely(S_ISCHR(inode->i_mode) && inode->i_cdev != NULL &&
> > >  		     !(file->f_mode & FMODE_PATH))) {
> > >  		cdev_put(inode->i_cdev);
> > > -- 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/