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

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

[L.A. Walsh]

[-- Attachment #1: Type: text/plain, Size: 351 bytes --]

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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: xfs_ioctl-perm.diff --]
[-- Type: text/x-diff; name="xfs_ioctl-perm.diff", Size: 768 bytes --]

--- 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;
 
-	if (!capable(CAP_SYS_ADMIN))
-		return -EPERM;
+	if (!capable(CAP_SYS_ADMIN)) conditional_perm=1;
 
 	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;
+	}
+
 	/* Restrict xfs_open_by_handle to directories & regular files. */
 	if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode))) {
 		error = -EPERM;

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

[Brian Foster]

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

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

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

[Darrick J. Wong]

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

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

[Darrick J. Wong]

> Cc: bfoster@redhat.com, xfs-oss <xfs@e29208.dscx.akamaiedge.net>

akamaiedge.net ??

Er, did my mailer do that, or did yours?

[re-adding linux-xfs to cc]

On Mon, Jan 04, 2021 at 12:24:14PM -0800, L A Walsh wrote:
> 
> 
> On 2021/01/04 10:44, Darrick J. Wong wrote:
> > This would open a huge security hole because users can use it to bypass
> > directory access checks.
> ---
> 	Can't say I entirely disagree.  Though given the prevalence of that
> behavior being "normal" on NT due to the "Bypass Traverse Checking" "right"
> being on by default in a standard Windows setup,

That might be true on Windows, but CAP_DAC_* isn't given out by default
on Linux.

> I would question it being a 'huge' security hole, though it would be
> an unwanted change in behavior.

I think people would consider it a hole of /some/ kind, since this patch
wouldn't even require the process to hold CAP_DAC_* privilege.

> 	If a user has a shell open to a directory that is made
> inaccessible in the way you describe, though, simply staying connected
> would seem to allow opening FD's that would be otherwise inaccessible.
> 
> 	Further, can't a user pass an open file descriptor through
> some type of IPC call for the other side to use?  I may be misremembering
> something similar, so I'd have to dig unless someone else remembers.

Yes, they can do both of those things, since the Unix DAC only checks
access at open time.

> 	Though, in the following case:
> > 
> >  have a file /home/djwong/bin/pwnme, r/w by EBM (evil Bitcom minor).
> > then someone issues chmod 0000 on a dir above it...
> > Now I cannot access pwnme anymore, because I've been cut off from ~/bin.
> ----
> 	Oh...but if they hard linked it to someone else's open dir,
> they still could.  It seems if you want to secure the object, you really
> need to alter the perms on object, not on what might be 1 of
> several paths to it.  It might be bind-mounted elsewhere as well.

I /did/ say that the BOFH omitted -r... ;)

> 	Additionally you aren't dealing with removing more permissive
> ACL's...  That said, you're still right in that it opens a new
> potential security hole that anyone from MS would be used to/expecting
> (that's not to be taken as a justification to do it, just as context
> for expectations and level of the security hole.
> 
> 	Conversely, while users may have ownership rights in their
> home dir, they may not have write permissions above that -- possibly
> not even read permissions if that 'nt-right' is ever supported.
> 
> 	I'm guessing it's not easy to check if they might have path
> permissions to get there, though the intervening files could be accessible
> through a group ACL, that the user is a member of. Might
> be good to keep such files only executable by owner.
> 
> 	So I'd beg off on supporting that change now, without some
> other way of checking accessibility (which could be np-hard given
> the number of ways its possible to get around a simple directory blockade).
> 
> 	Given the wide use of linux as a file server, I'm wondering
> how one might support the extra 'right's from windows in some context.
> 
> 	Certainly, if the above scenario was used within a Linux-subsystem running
> on windows, the resulting access modes could
> be complicated.
> 
> 	This is way beyond this question (here, don't patch unless you
> check other CAPs), but wouldn't it make sense to have the ability
> to apply an LSM-model (or set of rules) only to some specific domain
> (in this case path traversal/access over diverse file systems that
> have different rules and capabilities)?

Yeah.  As far as I can tell, CAP_DAC_OVERRIDE actually /does/ give you
the security permissions that you want.  The sysadmin can then decide
who gets to have that permission, so you /could/ propose doing that.

> 	If it isn't possible already, I'm sure it soon will be
> the case that users will be on systems that have different file
> systems mounted.  If an xfs file system is mounted under an NT
> file system and the user is running Windows, wouldn't NT-rights
> (like ignoring traversal issues) apply by default, as NT would
> be in charge of enforcing security as it walked through a locally
> mounted XFS file system?

When would NT be walking through a locally mounted XFS filesystem?

--D

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

[L A Walsh]

On 2021/01/04 15:15, Darrick J. Wong wrote:
>> Cc: bfoster@redhat.com, xfs-oss <xfs@e29208.dscx.akamaiedge.net>
> 
> akamaiedge.net ??
---
First I've seen that...

> 
> Er, did my mailer do that, or did yours?
> 
> [re-adding linux-xfs to cc]
> 
> On Mon, Jan 04, 2021 at 12:24:14PM -0800, L A Walsh wrote:
>>
>> On 2021/01/04 10:44, Darrick J. Wong wrote:
>>> This would open a huge security hole because users can use it to bypass
>>> directory access checks.
>> ---
>> 	Can't say I entirely disagree.  Though given the prevalence of that
>> behavior being "normal" on NT due to the "Bypass Traverse Checking" "right"
>> being on by default in a standard Windows setup,
> 
> That might be true on Windows, but CAP_DAC_* isn't given out by default
> on Linux.
----
	Not quite the same as I understand it -- it allows ignoring 
an 'X' permission of directories above the target, I believe, but not other
DAC permissions.  I'd have to experiment to be sure.  In their security
settings dialogue, they warn you against changing it away from the default
as it might cause compatibility problems.


> 
>> I would question it being a 'huge' security hole, though it would be
>> an unwanted change in behavior.
> 
> I think people would consider it a hole of /some/ kind, since this patch
> wouldn't even require the process to hold CAP_DAC_* privilege.
---
	Yeah, though it doesn't allow overriding DAC settings on files
or directories that have them when trying to access them, just bypassing
directory traversal permissions for items lower in the hierarchy.


> 
> Yeah.  As far as I can tell, CAP_DAC_OVERRIDE actually /does/ give you
> the security permissions that you want.  The sysadmin can then decide
> who gets to have that permission, so you /could/ propose doing that.
----
	Except that it does more than just allowing directory traversal
to a set of files you own.  Like I think it might not be uncommon to have
some common home dir set restrictively, so you couldn't read who else had
home directories on the system, perhaps, but you could still cd through
it to your own directory.  It may be more like allowing you to 'x' through
parent directories without being able to read/write to them.  CAP_DAC_OVERRIDE
would allow you to override any DAC permissions on any file -- not just
ignore lack of 'X' on a directory.


> 
>> 	If it isn't possible already, I'm sure it soon will be
>> the case that users will be on systems that have different file
>> systems mounted.  If an xfs file system is mounted under an NT
>> file system and the user is running Windows, wouldn't NT-rights
>> (like ignoring traversal issues) apply by default, as NT would
>> be in charge of enforcing security as it walked through a locally
>> mounted XFS file system?
> 
> When would NT be walking through a locally mounted XFS filesystem?
 
Well, at least when it is mounted in a linux subsystem -- I haven't checked
out what's possible, but am told explorer can then be used to move through 
file systems that are locally mounted through the linux drivers.

The other possibility -- is a stretch of locally mounted, in having
an XFS drive mounted via CIFS as a "local" drive.  I'd think it would
allow bypassing traverse checking in the same way that having
NT-admin privileges can give you root access to exported drives, but
you have to set it up so that your login has those privs on the target server,
but less drastic privs, like bypassing traverse checking and having
a read/write access for a user that has the backup+restore privs are likely
also supported.  

As an example, a Domain administrator on my NT workstation has root access
to my server (not by accident, but because it is meant to be that way).

I use Win as a desktop, but lin as my diskspace+server.  My Win desktop barely
has enough diskspace for the OS + programs I have installed.

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

[Dave Chinner]

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.

No. xfs_open_by_handle() requires root permissions because it
bypasses lots of security checks, such as parent directory
permissions, ACLs and security labels.

e.g. backups under a root-only directory heirarchy should not be
accessible to users because users are not allowed to traverse into
those root:root 0700 backup directories because permissions on the 
directory inodes do not allow non-root users to enter them.

Hence ...

> (i.e. it opens the user's own file).

... the user doesn't actually own that file, even though it has
their own UID in it...

> It gets rid of some unnecessary error messages if you
> run xfs_restore to restore one of your own files.

That's not really a user case xfs_restore is intended to support.
It's an admin tool to be run by admins, not end users....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com