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:

quoted

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?

quoted

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

quoted

 
-	if (!capable(CAP_SYS_ADMIN))
-		return -EPERM;
+	if (!capable(CAP_SYS_ADMIN)) conditional_perm=1;

This should remain two lines..

quoted

 
 	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

quoted

 	/* Restrict xfs_open_by_handle to directories & regular files. */
 	if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode))) {
 		error = -EPERM;