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

[Mimi Zohar]

On Wed, 2008-12-03 at 10:50 -0800, Dave Hansen wrote: 
> 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.

The original IMA maintained its own readcount, but we thought the code
would be simpler if we used existing kernel reference counts.  Based on
both Christoph's and your concerns, we probably should go back to
maintaining our own readcount for user level files.

Mimi

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/