Re: [PATCH] userns: Convert xfs to use kuid/kgid where appropriate

[Dave Chinner <david@fromorbit.com>]

On Wed, Jun 19, 2013 at 01:35:30PM -0700, Eric W. Biederman wrote:
> 
> I am copying my gmail address so I have a chance of seeing replies from
> Dave Chiner.  So far the only way I have been able to read his replies
> has been to read mailling lists.  Which has not be conductive to having
> this code discussed properly.  Hopefully copying my gmail address will
> allow us to have a reasonable and timely conversation.
> 
> 
> Dwight Engen <dwight.engen@oracle.com> writes:
> 
> > Use uint32 from init_user_ns for xfs internal uid/gid representation in
> > acl, xfs_icdinode. 
> 
> From my review of the code earlier that just isn't safe.  It allows all
> kinds of things to slip through.

Such as?

> > Conversion of kuid/gid is done at the vfs boundary,
> > other user visible xfs specific interfaces (bulkstat, eofblocks filter)
> > expect uint32 init_user_ns uid/gid values.
> 
> From my earlier review of the code conversion at the vfs boundary is
> not safe.    

So you've claimed.

> First off kuid_t and kgid_t are not a vfs concepts, they are linux
> kernel concepts, and xfs is in the linux kernel.  What makes this
> relevant is not all filesystem accesses are through the vfs so all of
> the necessary conversions for security and a consistent user experience
> can be had by only performing conversions at the user/kernel boundary.

Right. Boundaries and consistent conversion across them are
important. They are especially important in filesystems for
converting to/from on-disk and kernel-native formats. Blindly
pushing kernel structure down as far are you can make them reach
ignores important boundaries.

You might want to hit XFS with a big hammer but the right solution
is far more nuanced that that.

> In particular by being sloppy and not pushing kuid_t/kgid_t further down
> you did not handle all of the conversions needed at the user/kernel
> boundary in XFS_IOC_FREE_EOFBLOCKS.

The kuid_t/kgid_t is actually pushed down this far - it's in the
struct inode - the code currently uses the on-disk XFS uid/gid,
not the struct inode's kuid_t/kgid_t. That's easily fixable.

Indeed, that's where most of the work needs to be done in XFS -
using VFS(ip)->i_uid instead of ip->i_d.di_uid in places where we
aren't dealing with reading or modifying the on-disk structures of
XFS. Once that is done, we will have driven kuid_t/kgid_t as far
down as the in-memory/on-disk format boundary allows, and we'll end
up catching all the sorts of problems you are worried about.

> Which can be called by an
> unprivileged user.

That's an oversite. Needs fixing.

> I honestly don't think avoiding the push down of kuid_t and kgid_t to
> all of the xfs in-core data structures is safe.  Even if the initial
> patch is safe I expect there will be silent breakage when the next ioctl
> that bypasses the vfs is added.

This problem isn't isolated to XFS - ioctls are added to ext4,
btrfs, gfs2, etc in every release and they all face the same
problems.  Hence trying to paint it as an XFS problem is realy
missing the mark....

I'd really like to see some regressions tests in xfstests that we
can use to confirm that filesystems have implemented namespaces
properly (e.g. quota set/get/report tests). That would go a long way
to ensuring that people don't inadvertantly. I'm sure you have some
test scripts for testing all the changes you made, so sharing them
would help us a lot.

[ Hmmmm - the quota netlink warning interface - it just takes a
warning for a specific kqid and emits it to all listeners on the
netlink interface. There's no namespace awareness there, so what
stops quota warning messages from one namespace being heard in all
other namespaces? It's not network namespace aware.... ]

As it is, I'm far more concerned about the security problems
existing ioctls and interfaces pose for user namespaces. i.e. that
CAP_SYS_ADMIN in any namespace can use bulkstat and then the
fs/fhandle.c interfaces to find and access any file in the
filesystem regardless of namespace restrictions. You can guess
filehandles pretty trivially, so you don't need bulkstat and you
don't need XFS for this to be a problem....

Further, bulkstat is used by backup utilities, so I think it needs
the unmodified uid/gid/prid to be passed out, and the restore
program needs a way to push them all back in unmodified. How would
you propose we go about doing this. Alternatively could you tell us
how a sub-namespace level backup/restore program supposed to
handle/detect/avoid being invoked from inside a restricted,
non-global namespace?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs