Messages in this thread | | | Subject | Re: [PATCH 3/6] integrity: IMA as an integrity service provider | From | Dave Hansen <> | Date | Wed, 03 Dec 2008 10:50:55 -0800 |
| |
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
| |