Re: [PATCH review 08/12] quota: Ensure qids map to the filesystem

On Wed, Jul 13, 2016 at 11:34:36AM +1000, Dave Chinner wrote:

On Mon, Jul 11, 2016 at 01:12:49PM -0500, Eric W. Biederman wrote:

quoted

The place where I am concerned about thorough review and testing is
someone poisoning quota files and then the kernel trying to use them.
In the preliminary work we have done in other places in the kernel and
for other filesystems there almost always winds up being some way to
confuse the kernel and get it to misbave if you can poison the disk
based inputs.  As poison disk based inputs is not something filesystems
are stronlgy concerned about.  In most cases the disk the filesystem
resides on is in the box and therefore under control of the OS at all
times.  Dave Chinner has even said he will never consider handling
poisoned disk based inputs for XFS as the run time cost is too high.

I didn't say that. I said that comprehensive checks to catch all
possible malicious inputs is too expensive to consider a viable
solution for allowing user-mounts of arbitrary filesystem images
through the kernel.

[.....]

To bring this back to quota files, the only way to validate that a
quota file has not been tampered with is to run a quotacheck on the
filesystem once it has been mounted. This requires visiting every
inode in the filesystem, so it an expensive operation. Only XFS has
this functionality in kernel, so for untrusted mounts we could
simply run it on every mount that has quotas enabled. Of course,
users won't care that mounting their filesystem now takes several
minutes (hours, even, when we have millions of inodes in the fs)
while these checks are run...

So, over lunch I realised the problem with this. quotacheck is
verifying the contents of the quota file, but we haven't verified
the structure of the quota file to begin with. Hence just enabling
quotas could cause the filesystem to do bad things in the kernel on
mount if the quota file metadata has been tampered with.

IOWs, it's not just quota data parsing that we have to be concerned
with here - parsing the quota file structure itself could be an
attack vector that triggers on mount.

Cheers,

Dave.
-- 
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org