Re: [PATCH 3/6] integrity: IMA as an integrity service provider

[Dave Hansen <dave@linux.vnet.ibm.com>]

On Wed, 2008-12-03 at 13:24 -0500, Mimi Zohar wrote:
> On Wed, 2008-12-03 at 08:03 -0500, Christoph Hellwig wrote: 
> > On Tue, Dec 02, 2008 at 03:35:25PM -0800, Dave Hansen wrote:
> > > I have memories of talking about this bit.  I was confused and you
> > > explained it to me, but it still isn't explained in the code. :(  Again,
> > > I'm not convinced that this works.  Can the code convince me, or should
> > > I go digging in my inbox?
> > 
> > I also haven't seen a good explanation for it yet.
> 
> Previous posting:
> "The OS protects against an executable file, already open for write,
> from being executed; and an executable file, open for execute, from
> being modified.  In the same vein, we want to know that the file
> measured is the same file read, that it hasn't been modified.  So, if a
> file already open for read is then opened for write, we log it with a
> "Time of Measure, Time of Use" error (ToMToU) and invalidate the
> PCR.....
> 
> This is important when measuring configuration files, shell scripts (not
> measured in brpm_check_integrity which are protected by the OS), and
> files imported/included by scripts."
> 
> Another posting:
> "From an integrity perspective, a file measurement might be invalidated
> unnecessarily, but it is safe. For any file when opened for write, while
> having an existing reader, will cause the file measurement to be
> invalidated."

Those are all great explanations.  But, some of that needs to get in the
patch somehow.  This is a subtle thing and someone looking at this a
year for now is going to have difficulty understanding why it was done.

> I'm just not seeing a problem. Perhaps because only regular files are
> being measured, and of them, only those defined by the policy, which
> most likely would not include pseudo filesystems (i.e. sysfs, procfs,
> tmpfs, securityfs).

There is no practical problem if you have false-positives on this check
and do extra invalidations.  But, I think both Christoph and I are
nervous that this check is racy and there may be false-negatives and
thus may miss some invalidations (which would be harmful).

The check is racy which is cause for concern by itself.  But, with
careful consideration, it may not be a dangerous or harmful race.  Could
you please consider it carefully and share some of your thoughts in a
comment in the next version?

You need to check very, very carefully that there are no possible ways
for i_writecount to be elevated without a corresponding elevation of
d_count.  I'm especially concerned as I look at some of the mmap() code.
It appears that there are some temporary i_writecount elevations as
VM_DENYWRITE is figured out.  That needs some careful auditing to ensure
it doesn't violate what is being assumed in the integrity code.

-- Dave