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;
>
