| Message ID | 5FEB204B.9090109@tlinx.org (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | suggested patch to allow user to access their own file... | expand |
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;
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; >
> 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
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.
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.
--- 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;