Re: suggested patch to allow user to access their own file...

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Mon, Jan 04, 2021 at 12:08:15PM -0500, Brian Foster wrote:
> On Tue, Dec 29, 2020 at 04:25:47AM -0800, L.A. Walsh wrote:
> > xfs_io checks for CAP_SYS_ADMIN in order to open a
> > file_by_inode -- however, if the file one is opening
> > is owned by the user performing the call, the call should
> > not fail.
> > 
> > (i.e. it opens the user's own file).
> > 
> > patch against 5.10.2 is attached.
> > 
> > It gets rid of some unnecessary error messages if you
> > run xfs_restore to restore one of your own files.

No S-o-B on the patch so I was hesitant to reply, but since Brian did,
I'll reply to that.  This message brought to you by the letters Z, F,
and S.

> The current logic seems to go a ways back. Can you or somebody elaborate
> on whether there's any risks with loosening the permissions as such?

This would open a huge security hole because users can use it to bypass
directory access checks.

Let's say I have a file /home/djwong/bin/pwnme that can be read or
written by the evil bitcom miner in my open Firefox process.  (Hey,
browsers can flash USB device firmware now, ~/bin is the least of my
problems!)

Then let's say the BOFH decides I'm too much of a security risk and
issues:

$ sudo chmod 0000 /home/djwong/bin; sudo chown root:root /home/djwong/bin

(Our overworked BOFH forgot -r and only changed ~/bin.)

Now I cannot access pwnme anymore, because I've been cut off from ~/bin.

With the below patch applied I can now bypass that restriction because I
still own ~/bin/pwnme and therefore can (now) open it by file handle.

We /could/ relax the check so that the caller only has to have one of
CAP_{SYS_ADMIN,DAC_READ_SEARCH,DAC_OVERRIDE} and let the sysadmin decide
if they want to bless xfsrestore with any of those capabilities...

--D

> E.g., any reason we might not want to allow regular users to perform
> handle lookups, etc.? If not, should some of the other _by_handle() ops
> get similar treatment?
> 
> > --- fs/xfs/xfs_ioctl.c	2020-12-22 21:11:02.000000000 -0800
> > +++ fs/xfs/xfs_ioctl.c	2020-12-29 04:14:48.681102804 -0800
> > @@ -194,15 +194,21 @@
> >  	struct dentry		*dentry;
> >  	fmode_t			fmode;
> >  	struct path		path;
> > +	bool conditional_perm = 0;
> 
> Variable name alignment and I believe we try to use true/false for
> boolean values.
> 
> >  
> > -	if (!capable(CAP_SYS_ADMIN))
> > -		return -EPERM;
> > +	if (!capable(CAP_SYS_ADMIN)) conditional_perm=1;
> 
> This should remain two lines..
> 
> >  
> >  	dentry = xfs_handlereq_to_dentry(parfilp, hreq);
> >  	if (IS_ERR(dentry))
> >  		return PTR_ERR(dentry);
> >  	inode = d_inode(dentry);
> >  
> > +	/* only allow user access to their own file */
> > +	if (conditional_perm && !inode_owner_or_capable(inode)) {
> > +		error = -EPERM;
> > +		goto out_dput;
> > +	}
> > +
> 
> ... but then again, is there any reason we couldn't just move the
> capable() check down to this hunk and avoid the new variable?
> 
> Brian
> 
> >  	/* Restrict xfs_open_by_handle to directories & regular files. */
> >  	if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode))) {
> >  		error = -EPERM;
>