Re: [Linux-ima-user] IMA overwrites xattr even if it has got correct signature

[Dmitry R. <dmi...@li...>]

On Thu, 2016-02-25 at 07:43 -0500, Mimi Zohar wrote:
> On Thu, 2016-02-25 at 12:30 +0200, Dmitry Rozhkov wrote:
> > On Wed, 2016-02-24 at 11:31 -0500, Mimi Zohar wrote:
> > > On Wed, 2016-02-24 at 15:23 +0200, Dmitry Rozhkov wrote:
> > > > Hi!
> > > > 
> > > > I'm working on integrating IMA and swupd (Clear Linux software
> > > > updates)
> > > > and I'm experiencing problems with updating or installing new
> > > > files
> > > > on
> > > > systems with IMA enabled.
> > > > 
> > > > The problem comes from the fact that the IMA kernel module
> > > > unconditionally overwrites the security.ima extended attribute
> > > > upon
> > > > closing a file:
> > > > 1. the swupd client downloads a tarball with updates to
> > > > /var/lib/swupd;
> > > > 2. then unpacks the updated files preserving xattrs including
> > > > security.ima with file signatures;
> > > > 3. as soon as tar closes the unpacked files the kernel wipes
> > > > out
> > > > the
> > > > content of security.ima and puts new value (files' hashes
> > > > without
> > > > signatures).
> > > > 
> > > > AFAIU this happens in the kernel hook ima_file_free() called as
> > > > a
> > > > final
> > > > step of __dput() upon closing a file handle and freeing its
> > > > structure.
> > > > So there is no way to intervene and to prevent this xattr
> > > > reset.
> > > > 
> > > > As result I can't use software updates together with an IMA
> > > > policy
> > > > where all executables must be signed.
> > > > 
> > > > Is it possible not to overwrite a file's security.ima if upon
> > > > closing
> > > > the file its security.ima contains a correct signed hash
> > > > already? I
> > > > saw
> > > > Mimi mentioned the patch doing exactly that a month ago.
> > > 
> > > Commit c68ed80 "ima: limit file hash setting by user to fix and
> > > log
> > > modes" prevents writing a file hash as the security.ima xattr,
> > > which
> > > is
> > > a bit different than the use case that you described.  In that
> > > case,
> > > userspace is attempting to write the xattr.
> > > 
> > > In your usecase scenario, ima_check_last_writer() calls
> > > ima_update_xattr() to write the xattr.  It should prevent writing
> > > a
> > > hash, when the IMA_DIGSIG flag is set.  
> > > 
> > > Some questions for debugging this.  setxattr sets the IMA_DIGSIG
> > > flag.
> > > How is swupd writing out security.ima?   Why isn't IMA_DIGSIG
> > > being
> > > set?
> > 
> > swupd just untars files from a tarball to a staging direcotry.
> > Currently I'm using the libarchive-based bsdtar as tar replacement:
> > it
> > opens a file for writing, writes its content, then sets its
> > attributes
> > including xattrs and finaly closes the file (NB: GNU tar works
> > differently - creates an empty file, sets attributes, closes it,
> > tries
> > to reopen for writing its content, but fails because of failing
> > appraisal).
> 
> True, the xattrs are written first and then the file data, but at
> least
> as of a couple of years ago the file wasn't re-opened in between
> writing
> the xattrs and the file data.  I posted a patch to defer writing the
> security.ima xattr.*   With the IMA_NEW_FILE flag it might not be
> needed. 
> 

Thanks! I've just patched bsdtar to set xattrs after file closing and
it fixes my problem.

> > I suspect that the newly created file's inode doesn't have S_IMA
> > flag
> > since the file hasn't been measured yet (neither it has iint
> > associated
> > with the inode). So setting IMA_DIGSIG is never reached for the
> > file.
> 
> If security.ima isn't being written because the file is not in
> policy,
> that is a different problem.   Perhaps try writing a file that is in
> the
> IMA-appraisal policy.
> 

Didn't get it. The file is in the IMA-appraisal policy.

AFAIU upon file closing ima_inode_setxattr()
calls ima_reset_appraise_flags() where IMA_DIGSIG is supposed to be
set. But right before setting the flag there's the condition

	iint = integrity_iint_find(inode);
	if (!iint)
		return;

which is true for newly created (and never closed yet) files.

BR,
Dima