Leaking Path in XFS's ioctl interface(missing LSM check)

Leaking Path in XFS's ioctl interface(missing LSM check)

[TongZhang]

Hi,

I'm bringing up this issue again to let of LSM developers know the situation, and would like to know your thoughts.
Several weeks ago I sent an email to the security list to discuss the issue where
XFS's ioctl interface can do things like vfs_readlink without asking LSM's
permission, which we think is kind of weird and this kind of operation should be
audited by LSM.

see the original post below:

>We noticed a use of vfs_readlink() in xfs_file_ioctl(), which should have been checked by 
>security_inode_readlink().
>The callgraph is:
>	xfs_file_ioctl()->xfs_readlink_by_handle()->vfs_readlink()
>
>This path allows user to do things similar to SyS_readlinkat(), and the parameters
>are user controllable.

security_inode_readlink() is not used inside vfs_readlink()

- Tong

Re: Leaking Path in XFS's ioctl interface(missing LSM check)

[Dave Chinner]

On Tue, Sep 25, 2018 at 08:51:50PM -0400, TongZhang wrote:
> Hi,
> 
> I'm bringing up this issue again to let of LSM developers know the situation, and would like to know your thoughts.
> Several weeks ago I sent an email to the security list to discuss the issue where
> XFS's ioctl interface can do things like vfs_readlink without asking LSM's
> permission, which we think is kind of weird and this kind of operation should be
> audited by LSM.

These aren't user interfaces. They are filesystem maintenance and
extension interfaces.  They are intended for low level filesystem
utilities that require complete, unrestricted access to the
underlying filesystem via holding CAP_SYSADMIN in the initns.

i.e.  they are used to perform filesystem maintenance and extension
operations that need to be completely invisible to users from
userspace. e.g.  online file defragmentation (xfs_fsr), data
migration (e.g. HSM products), efficient backup of data (xfsdump),
metadata and data scrubbing, online repair, etc.

IOWs, I really don't think these interfaces are something the LSMs
should be trying to intercept or audit, because they are essentially
internal filesystem interfaces used by trusted code and not general
user application facing APIs.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: Leaking Path in XFS's ioctl interface(missing LSM check)

[Stephen Smalley]

On 09/25/2018 09:33 PM, Dave Chinner wrote:
> On Tue, Sep 25, 2018 at 08:51:50PM -0400, TongZhang wrote:
>> Hi,
>>
>> I'm bringing up this issue again to let of LSM developers know the situation, and would like to know your thoughts.
>> Several weeks ago I sent an email to the security list to discuss the issue where
>> XFS's ioctl interface can do things like vfs_readlink without asking LSM's
>> permission, which we think is kind of weird and this kind of operation should be
>> audited by LSM.
> 
> These aren't user interfaces. They are filesystem maintenance and
> extension interfaces.  They are intended for low level filesystem
> utilities that require complete, unrestricted access to the
> underlying filesystem via holding CAP_SYSADMIN in the initns.
> 
> i.e.  they are used to perform filesystem maintenance and extension
> operations that need to be completely invisible to users from
> userspace. e.g.  online file defragmentation (xfs_fsr), data
> migration (e.g. HSM products), efficient backup of data (xfsdump),
> metadata and data scrubbing, online repair, etc.
> 
> IOWs, I really don't think these interfaces are something the LSMs
> should be trying to intercept or audit, because they are essentially
> internal filesystem interfaces used by trusted code and not general
> user application facing APIs.

If they are interfaces exposed to userspace, then they should be 
mediated via LSM.  We only omit the LSM hook when the usage is purely 
kernel-internal.  Security modules are often used to limit even 
"trusted" applications to least privilege and protect them from 
untrustworthy inputs, moving from binary trust notions to only trusting 
them for what they must be trusted to do.  CAP_SYS_ADMIN doesn't 
necessarily indicate that they are trusted to override any given MAC 
policy restrictions.

Wondering why we don't perform the security_inode_readlink() call inside 
of vfs_readlink() currently.  The general pattern is that we do perform 
security_inode_*() calls inside the other vfs_*() helpers, so 
vfs_readlink() is an exception currently.

Re: Leaking Path in XFS's ioctl interface(missing LSM check)

[Dave Chinner]

On Wed, Sep 26, 2018 at 09:23:03AM -0400, Stephen Smalley wrote:
> On 09/25/2018 09:33 PM, Dave Chinner wrote:
> >On Tue, Sep 25, 2018 at 08:51:50PM -0400, TongZhang wrote:
> >>Hi,
> >>
> >>I'm bringing up this issue again to let of LSM developers know the situation, and would like to know your thoughts.
> >>Several weeks ago I sent an email to the security list to discuss the issue where
> >>XFS's ioctl interface can do things like vfs_readlink without asking LSM's
> >>permission, which we think is kind of weird and this kind of operation should be
> >>audited by LSM.
> >
> >These aren't user interfaces. They are filesystem maintenance and
> >extension interfaces.  They are intended for low level filesystem
> >utilities that require complete, unrestricted access to the
> >underlying filesystem via holding CAP_SYSADMIN in the initns.
> >
> >i.e.  they are used to perform filesystem maintenance and extension
> >operations that need to be completely invisible to users from
> >userspace. e.g.  online file defragmentation (xfs_fsr), data
> >migration (e.g. HSM products), efficient backup of data (xfsdump),
> >metadata and data scrubbing, online repair, etc.
> >
> >IOWs, I really don't think these interfaces are something the LSMs
> >should be trying to intercept or audit, because they are essentially
> >internal filesystem interfaces used by trusted code and not general
> >user application facing APIs.
> 
> If they are interfaces exposed to userspace, then they should be
> mediated via LSM.  We only omit the LSM hook when the usage is
> purely kernel-internal.

/me points to the mass of diverse management ioctls across the
kernel that aren't mediated by LSM hooks.

> Security modules are often used to limit
> even "trusted" applications to least privilege and protect them from
> untrustworthy inputs, moving from binary trust notions to only
> trusting them for what they must be trusted to do.  CAP_SYS_ADMIN
> doesn't necessarily indicate that they are trusted to override any
> given MAC policy restrictions.

Applications that are tightly integrated into the filesystem to
extend it's functionality effectively operate in the same trust
space as the kernel filesystem implementation itself.  i.e. they
interact with the filesystem at levels below the DAC/MAC checks that
are performed on user filesystem accesses, and perform manipluation
of the on-disk filesystem structure that is invisible to users
accessing the filesystem.

As such, there are very few trusted applications have "massive data
loss" as a potential failure mode if an inappropriately configured
LSM is loaded into the kernel. Breaking a HSM application's access
to the filesystem unexpectedly because someone didn't set up a new
security policy correctly brings a whole new level of risk to
administrating sites that mix non-trivial storage solutions with
LSM-based security.

> Wondering why we don't perform the security_inode_readlink() call
> inside of vfs_readlink() currently.  The general pattern is that we
> do perform security_inode_*() calls inside the other vfs_*()
> helpers, so vfs_readlink() is an exception currently.

Pretty sure that was done to mitigate the risk of breaking existing
userspace applications using the handle interfaces to read links.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: Leaking Path in XFS's ioctl interface(missing LSM check)

[Alan Cox]

On Wed, 26 Sep 2018 11:33:29 +1000
Dave Chinner <david@fromorbit.com> wrote:

> On Tue, Sep 25, 2018 at 08:51:50PM -0400, TongZhang wrote:
> > Hi,
> > 
> > I'm bringing up this issue again to let of LSM developers know the situation, and would like to know your thoughts.
> > Several weeks ago I sent an email to the security list to discuss the issue where
> > XFS's ioctl interface can do things like vfs_readlink without asking LSM's
> > permission, which we think is kind of weird and this kind of operation should be
> > audited by LSM.  
> 
> These aren't user interfaces. They are filesystem maintenance and
> extension interfaces.  They are intended for low level filesystem
> utilities that require complete, unrestricted access to the
> underlying filesystem via holding CAP_SYSADMIN in the initns.

CAP_SYS_ADMIN is meaningless in an environment using a strong LSM set up.
So what if I have CAP_SYS_ADMIN ? In a secure environment low level
complete unrestricted access to the file system is most definitely
something that should be mediated.

CAP_SYS_ADMIN is also a bit weird because low level access usually
implies you can bypass access controls so you should also check
CAP_SYS_DAC ?

Alan

Re: Leaking Path in XFS's ioctl interface(missing LSM check)

[Dave Chinner]

On Wed, Sep 26, 2018 at 07:24:26PM +0100, Alan Cox wrote:
> On Wed, 26 Sep 2018 11:33:29 +1000
> Dave Chinner <david@fromorbit.com> wrote:
> 
> > On Tue, Sep 25, 2018 at 08:51:50PM -0400, TongZhang wrote:
> > > Hi,
> > > 
> > > I'm bringing up this issue again to let of LSM developers know the situation, and would like to know your thoughts.
> > > Several weeks ago I sent an email to the security list to discuss the issue where
> > > XFS's ioctl interface can do things like vfs_readlink without asking LSM's
> > > permission, which we think is kind of weird and this kind of operation should be
> > > audited by LSM.  
> > 
> > These aren't user interfaces. They are filesystem maintenance and
> > extension interfaces.  They are intended for low level filesystem
> > utilities that require complete, unrestricted access to the
> > underlying filesystem via holding CAP_SYSADMIN in the initns.
> 
> CAP_SYS_ADMIN is meaningless in an environment using a strong LSM set up.

Sure, but there are so many CAP_SYS_ADMIN-only ioctls in the kernel
that have no LSM coverage that this is not an isolated problem that
people setting up such systems have to deal with. the LSM hooks are
already complex enough without adding hundreds more hooks to control
individual ioctl behaviour for every filesystem, device, etc.

Unless you are going to add LSM hooks to all ioctls, I don't see
much point in singling out one set of ioctls in a way that will
break existing configurations. It's just a knee jerk reaction (like
ariport security) because it doesn't meaningfully address the
problem of all the other management ioctls in the kernel being
completely unconstrainted by LSMs.

> CAP_SYS_ADMIN is also a bit weird because low level access usually
> implies you can bypass access controls so you should also check
> CAP_SYS_DAC ?

Do you mean CAP_DAC_READ_SEARCH as per the newer handle syscalls?
But that only allows bypassing directory search operations, so maybe
you mean CAP_DAC_OVERRIDE?

Regardless, this horse bolted long before those syscalls were
introduced.  The time to address this issue was when XFS was merged
into linux all those years ago, back when the apps that run in
highly secure restricted environments that use these interfaces were
being ported to linux. We can't change this now without breaking
userspace....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: Leaking Path in XFS's ioctl interface(missing LSM check)

[James Morris]

On Thu, 27 Sep 2018, Dave Chinner wrote:

> Sure, but there are so many CAP_SYS_ADMIN-only ioctls in the kernel
> that have no LSM coverage that this is not an isolated problem that
> people setting up such systems have to deal with. 

I could be missing something here, but all ioctls are mediated by LSM at a 
high level (security_file_ioctl). Some problematic ones are singled out at 
that point by LSMs for special handling.


-- 
James Morris
<jmorris@namei.org>

Re: Leaking Path in XFS's ioctl interface(missing LSM check)

[Dave Chinner]

On Fri, Sep 28, 2018 at 07:23:42AM +1000, James Morris wrote:
> On Thu, 27 Sep 2018, Dave Chinner wrote:
> 
> > Sure, but there are so many CAP_SYS_ADMIN-only ioctls in the kernel
> > that have no LSM coverage that this is not an isolated problem that
> > people setting up such systems have to deal with. 
> 
> I could be missing something here, but all ioctls are mediated by LSM at a 
> high level (security_file_ioctl). Some problematic ones are singled out at 
> that point by LSMs for special handling.

selinux_file_ioctl() checks whether a specific /file/ has
permissions to execute ioctls, but it doesn't know anything about
what that ioctl actually does. It has special cases for a handful of
ioctls, but misses a large number of ioctls that perform similar
functionality (e.g. file getattr/setattr type ioctls).

smack just checks for read/write permissions depending on the
IOW/IOR/IORW definition of the ioctl.  Tomoyo does a path check
based on the path associated with the file passed to it to see if an
ioctl can be run.

IOWs, none of there really know anything about what the ioctl does,
and they sure as hell don't check any of the arguments for other
file descriptors that the ioctl might be accessing. It's basically a
"does we allow ioctls to this inode/path from this user?" check and
nothing more.

That just doesn't work for ioctls structured like the XFS filehandle
operations. The ioctl is performed on a "fshandle" file descriptor,
which generally points at the root directory of the filesystem the
file handle belongs to. This gives the ioctl the superblock context
that it is to operate on, and nothing more. It then opens a new file
based on the filehandle that was in the ioctl argument, and performs
the required operation on that file it opened itself.

IOWs, the security_file_ioctl() hook is almost completely useless in
cases like this - you can't isolate the ioctl based on the file
argument, because it can point to any file or directory in the
filesystem. And without actually parsing, decoding and instantiating
the the ioctl arguments, you can't tell the ioctl it can't act on
specific targets. And because filehandle to dentry resolution
results in disconnected dentries, the paths are not complete and
hence path based security checks (e.g. tomoyo) are likely to be
broken and unfixable...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: Leaking Path in XFS's ioctl interface(missing LSM check)

[Tetsuo Handa]

On 2018/09/28 7:19, Dave Chinner wrote:> IOWs, the security_file_ioctl() hook is almost completely useless in
> cases like this - you can't isolate the ioctl based on the file
> argument, because it can point to any file or directory in the
> filesystem. And without actually parsing, decoding and instantiating
> the the ioctl arguments, you can't tell the ioctl it can't act on
> specific targets. And because filehandle to dentry resolution
> results in disconnected dentries, the paths are not complete and
> hence path based security checks (e.g. tomoyo) are likely to be
> broken and unfixable...

Though TOMOYO uses pathname as a mandatory parameter, CaitSith
(currently waiting for review) does not.

CaitSith can filter filesystem specific ioctl() using fsmagic
and cmd parameter like:

10 acl ioctl path.fsmagic=0x9FA0
    audit 0
    10 deny cmd=@FORBIDDEN_IOCTLS_ON_PROCFS
    20 allow

CaitSith does ioctl() checks. Missing LSM check is a bug.

Re: Leaking Path in XFS's ioctl interface(missing LSM check)

[Alan Cox]

> > CAP_SYS_ADMIN is also a bit weird because low level access usually
> > implies you can bypass access controls so you should also check
> > CAP_SYS_DAC ?  
> 
> Do you mean CAP_DAC_READ_SEARCH as per the newer handle syscalls?
> But that only allows bypassing directory search operations, so maybe
> you mean CAP_DAC_OVERRIDE?

It depends what the ioctl allows you to do. If it allows me to bypass
DAC and manipulate the file system to move objects around then it's a
serious issue.

The underlying problem is if CAP_SYS_ADMIN is able to move objects around
then I can move modules around. We already have a problem with
CAP_DAC_OVERRIDE giving you CAP_SYS_RAWIO (ie totally owning the machine)
unless the modules are signed, if xfs allows ADMIN as well then
CAP_SYS_ADMIN is much easier to obtain and you'd get total system
ownership from it.

Not good.

> Regardless, this horse bolted long before those syscalls were
> introduced.  The time to address this issue was when XFS was merged
> into linux all those years ago, back when the apps that run in
> highly secure restricted environments that use these interfaces were
> being ported to linux. We can't change this now without breaking
> userspace....

That's what people said about setuid shell scripts.

I'd like to understand better what can be done. We can argue afterwards
about what if anything to do about it and if it is possible to abuse it.

Alan

Re: Leaking Path in XFS's ioctl interface(missing LSM check)

[Dave Chinner]

On Sun, Sep 30, 2018 at 03:16:52PM +0100, Alan Cox wrote:
> > > CAP_SYS_ADMIN is also a bit weird because low level access usually
> > > implies you can bypass access controls so you should also check
> > > CAP_SYS_DAC ?  
> > 
> > Do you mean CAP_DAC_READ_SEARCH as per the newer handle syscalls?
> > But that only allows bypassing directory search operations, so maybe
> > you mean CAP_DAC_OVERRIDE?
> 
> It depends what the ioctl allows you to do. If it allows me to bypass
> DAC and manipulate the file system to move objects around then it's a
> serious issue.

These interfaces have always been allowed to do that. You can't do
transparent online background defragmentation without bypassing DAC
and moving objects around. You can't scrub metadata and data without
bypassing DAC. You can't do dedupe without bypassing /some level/ of
DAC to get access to the filesystem used space map and the raw block
device to hash the data. But the really important access control for
dedupe - avoiding deduping data across files at different security
levels - isn't controlled at all.

> The underlying problem is if CAP_SYS_ADMIN is able to move objects around
> then I can move modules around.

Yup, anything with direct access to block devices can do that. Many
filesystem and storage utilities are given direct access to the
block device, because that's what they need to work.

e.g. in DM land, the control ioctls (ctl_ioctl()) are protected by:

        /* only root can play with this */
        if (!capable(CAP_SYS_ADMIN))
                return -EACCES;

Think about it - if DM control ioctls only require CAP_SYS_ADMIN,
then if have that cap you can use DM to remap any block in a block
device to any other block. You don't need to the filesystem to move
stuff around, it can be moved around without the filesystem knowing
anything about it.

> We already have a problem with
> CAP_DAC_OVERRIDE giving you CAP_SYS_RAWIO (ie totally owning the machine)
> unless the modules are signed, if xfs allows ADMIN as well then
> CAP_SYS_ADMIN is much easier to obtain and you'd get total system
> ownership from it.

Always been the case, and it's not isolated to XFS.

$ git grep CAP_SYS_ADMIN fs/ |wc -l
139
$ git grep CAP_SYS_ADMIN block/ |wc -l
16
$ git grep CAP_SYS_ADMIN drivers/block/ drivers/scsi |wc -l
88

The "CAP_SYS_ADMIN for ioctls" trust model in the storage stack
extends both above and below the filesystem. If you don't trust
CAP_SYS_ADMIN, then you are basically saying that you cannot trust
your storage management and maintenance utilities at any level.


> Not good.
> 
> > Regardless, this horse bolted long before those syscalls were
> > introduced.  The time to address this issue was when XFS was merged
> > into linux all those years ago, back when the apps that run in
> > highly secure restricted environments that use these interfaces were
> > being ported to linux. We can't change this now without breaking
> > userspace....
> 
> That's what people said about setuid shell scripts.

Completely different. setuid shell scripts got abused as a hack for
the lazy to avoid setting up permissions properly and hence were
easily exploited.

The storage stack is completely dependent on a simplisitic layered
trust model and that root (CAP_SYS_ADMIN) is god. The storage trust
model falls completely apart if we don't have a trusted root user to
administer all layers of the storage stack.

This isn't the first time I've raised this issue - I raised it
back when the user namespace stuff was ram-roaded into the kernel,
and was essentially ignored by the userns people. As a result, we
end up with all the storage management ioctls restricted to the
initns where we have trusted CAP_SYS_ADMIN users.

I've also raised it more recently in the unprivileged mount
discussions (so untrusted root in containers can mount filesystems)
- no solution to the underlying trust model deficiencies was found
in those discussions, either. Instead, filesystems that can
be mounted by untrusted users (i.e. FUSE) have a special flag in
their fstype definition to say this is allowed.

Systems restricted by LSMs to the point where CAP_SYS_ADMIN is not
trusted have exactly the same issues. i.e. there's nobody trusted by
the kernel to administer the storage stack, and nobody has defined a
workable security model that can prevent untrusted users from
violating the existing storage trust model....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: Leaking Path in XFS's ioctl interface(missing LSM check)

[Alan Cox]

>         /* only root can play with this */
>         if (!capable(CAP_SYS_ADMIN))
>                 return -EACCES;
> 
> Think about it - if DM control ioctls only require CAP_SYS_ADMIN,
> then if have that cap you can use DM to remap any block in a block
> device to any other block. You don't need to the filesystem to move
> stuff around, it can be moved around without the filesystem knowing
> anything about it.

Yes - I am not surprised the XFS is not the only problem area. The fact
XFS also isn't going via the security hooks so security hooks can fix it
just makes it worse.

> > That's what people said about setuid shell scripts.  
> 
> Completely different. setuid shell scripts got abused as a hack for
> the lazy to avoid setting up permissions properly and hence were
> easily exploited.

Sounds to me like an accurate description of the current capabilities
mess in the kernel (and not just XFS and not just file systems)

> Systems restricted by LSMs to the point where CAP_SYS_ADMIN is not
> trusted have exactly the same issues. i.e. there's nobody trusted by
> the kernel to administer the storage stack, and nobody has defined a
> workable security model that can prevent untrusted users from
> violating the existing storage trust model....

With a proper set of LSM checks you can lock the filesystem management
and enforcement to a particular set of objects. You can build that model
where for example only an administrative login from a trusted console may
launch processes to do that management.

Or you could - if things were not going around the LSM hooks.

Alan

Re: Leaking Path in XFS's ioctl interface(missing LSM check)

[Theodore Y. Ts'o]

On Mon, Oct 01, 2018 at 04:04:42PM +0100, Alan Cox wrote:
> > Systems restricted by LSMs to the point where CAP_SYS_ADMIN is not
> > trusted have exactly the same issues. i.e. there's nobody trusted by
> > the kernel to administer the storage stack, and nobody has defined a
> > workable security model that can prevent untrusted users from
> > violating the existing storage trust model....
> 
> With a proper set of LSM checks you can lock the filesystem management
> and enforcement to a particular set of objects. You can build that model
> where for example only an administrative login from a trusted console may
> launch processes to do that management.
> 
> Or you could - if things were not going around the LSM hooks.

It would be useful if anyone actually *wants* to do this thing to
define a formal security model, and detail *everything* that would
need to be changed in order to accomplish it.  Just as we don't
speculatively add code "just in case" someone might want to use it
someday, I don't think we should be adding random LSM hooks just
becausre someone *might* want do something.

Let's see the use case, and let's see how horrible the changes would
need to be, and how credible we think it is that someone will actually
want to *use* it.  I suspect the chagnes will be a really huge number
of places, and not just in XFS....

					- Ted

Re: Leaking Path in XFS's ioctl interface(missing LSM check)

[Dave Chinner]

On Mon, Oct 01, 2018 at 11:25:29AM -0400, Theodore Y. Ts'o wrote:
> On Mon, Oct 01, 2018 at 04:04:42PM +0100, Alan Cox wrote:
> > > Systems restricted by LSMs to the point where CAP_SYS_ADMIN is not
> > > trusted have exactly the same issues. i.e. there's nobody trusted by
> > > the kernel to administer the storage stack, and nobody has defined a
> > > workable security model that can prevent untrusted users from
> > > violating the existing storage trust model....
> > 
> > With a proper set of LSM checks you can lock the filesystem management
> > and enforcement to a particular set of objects. You can build that model
> > where for example only an administrative login from a trusted console may
> > launch processes to do that management.
> > 
> > Or you could - if things were not going around the LSM hooks.
> 
> It would be useful if anyone actually *wants* to do this thing to
> define a formal security model, and detail *everything* that would
> need to be changed in order to accomplish it.  Just as we don't
> speculatively add code "just in case" someone might want to use it
> someday, I don't think we should be adding random LSM hooks just
> becausre someone *might* want do something.

Yeah, that's what I was implying we needed to do - taking the
current model and slapping LSM hooks around randomly will only make
things break and cause admins to curse us....

> Let's see the use case, and let's see how horrible the changes would
> need to be, and how credible we think it is that someone will actually
> want to *use* it.  I suspect the chagnes will be a really huge number
> of places, and not just in XFS....

So do I - the "in root we trust" model is pretty deeply ingrained up
and down the storage stack. I also suspect that most of our hardware
admin (not just storage) has similar assumptions about the security
model they operate in.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: Leaking Path in XFS's ioctl interface(missing LSM check)

[Darrick J. Wong]

On Mon, Oct 01, 2018 at 04:04:42PM +0100, Alan Cox wrote:
> >         /* only root can play with this */
> >         if (!capable(CAP_SYS_ADMIN))
> >                 return -EACCES;
> > 
> > Think about it - if DM control ioctls only require CAP_SYS_ADMIN,
> > then if have that cap you can use DM to remap any block in a block
> > device to any other block. You don't need to the filesystem to move
> > stuff around, it can be moved around without the filesystem knowing
> > anything about it.
> 
> Yes - I am not surprised the XFS is not the only problem area. The fact
> XFS also isn't going via the security hooks so security hooks can fix it
> just makes it worse.
> 
> > > That's what people said about setuid shell scripts.  
> > 
> > Completely different. setuid shell scripts got abused as a hack for
> > the lazy to avoid setting up permissions properly and hence were
> > easily exploited.
> 
> Sounds to me like an accurate description of the current capabilities
> mess in the kernel (and not just XFS and not just file systems)
> 
> > Systems restricted by LSMs to the point where CAP_SYS_ADMIN is not
> > trusted have exactly the same issues. i.e. there's nobody trusted by
> > the kernel to administer the storage stack, and nobody has defined a
> > workable security model that can prevent untrusted users from
> > violating the existing storage trust model....
> 
> With a proper set of LSM checks you can lock the filesystem management
> and enforcement to a particular set of objects.

What would a proper set look like?  I /thought/ CAP_SYS_ADMIN was an
(admittedly overly general) way to do that, but evidently that view is
not considered correct.

Looking at include/linux/security.h, I don't see any hooks that seem
like an obvious fit for a lot of the XFS ioctls.  Do we need to add
some?  How do we do that?

Just looking at XFS, we let CAP_SYS_ADMIN processes do things like...

- Change the size of the filesystem
- Discard all post-EOF speculative preallocations
- Manage reserved block pools (which help us avoid ENOSPC)
- Query the filesystem for detailed space usage information
- Issue DISCARDs on unused space

- Set a new volume label
- Check and repair metadata

- Inject errors for testing
- Emergency shutdowns of the FS

- Bulk stat() of inodes
- Deal with files (open, read xattrs, read link targets) via file handles
- Read system xattrs

Can we create the necessary LSM hooks to check all of those things?  I
could see the following new hooks:

* Query filesystem space information
* Manage filesystem space information
* Set new filesystem label/uuid
* Run metadata integrity operations
* Access testing / debugging hooks
* Reading filesystem internal metadata
* Picking files by handle instead of path

I imagine block devices probably need a few explicit hooks too:

* Raw reads
* Raw writes
* Reconfigure block device

If we /did/ replace CAP_SYS_ADMIN checking with a pile of LSM hooks, how
do we make sure that we (XFS) avoid breaking existing XFS tools?  I
guess the default compatibility handler for all of those new hooks would
be "return capable(CAP_SYS_ADMIN) ? 0 : -EPERM;" and then other LSMs
could restrict that further if so configured.

Seriously, I don't know that much about how LSMs actually perform
security checks -- it looks like a number of them can be active
simultaneously, and we just call each of them in a chain until one of
them denies permission or we run out of LSMs and allow it?

FWIW I didn't have a particular security or threat model in mind when I
made the above list; all I did was break up the existing CAP_SYS_ADMIN
into rough functional areas.  Maybe it makes sense, but maybe I'm
rambling. :)

--D

> You can build that model where for example only an administrative
> login from a trusted console may launch processes to do that
> management.
> 
> Or you could - if things were not going around the LSM hooks.
> 
> Alan

Re: Leaking Path in XFS's ioctl interface(missing LSM check)

[James Morris]

On Mon, 1 Oct 2018, Darrick J. Wong wrote:

> If we /did/ replace CAP_SYS_ADMIN checking with a pile of LSM hooks,

Not sure we'd need a pile of hooks, what about just "read" and "write" 
storage admin?

Or even two new capabilities along these lines, which we convert existing 
CAP_SYS_ADMIN etc. to?

> how do we make sure that we (XFS) avoid breaking existing XFS tools?  I 
> guess the default compatibility handler for all of those new hooks would 
> be "return capable(CAP_SYS_ADMIN) ? 0 : -EPERM;" and then other LSMs 
> could restrict that further if so configured.
> 
> Seriously, I don't know that much about how LSMs actually perform
> security checks -- it looks like a number of them can be active
> simultaneously, and we just call each of them in a chain until one of
> them denies permission or we run out of LSMs and allow it?
> 

Correct.

> FWIW I didn't have a particular security or threat model in mind when I
> made the above list; all I did was break up the existing CAP_SYS_ADMIN
> into rough functional areas.  Maybe it makes sense, but maybe I'm
> rambling. :)
> 
> --D
> 
> > You can build that model where for example only an administrative
> > login from a trusted console may launch processes to do that
> > management.
> > 
> > Or you could - if things were not going around the LSM hooks.
> > 
> > Alan
> 

-- 
James Morris
<jmorris@namei.org>

Re: Leaking Path in XFS's ioctl interface(missing LSM check)

[Dave Chinner]

On Tue, Oct 02, 2018 at 06:08:16AM +1000, James Morris wrote:
> On Mon, 1 Oct 2018, Darrick J. Wong wrote:
> 
> > If we /did/ replace CAP_SYS_ADMIN checking with a pile of LSM hooks,
> 
> Not sure we'd need a pile of hooks, what about just "read" and "write" 
> storage admin?
> 
> Or even two new capabilities along these lines, which we convert existing 
> CAP_SYS_ADMIN etc. to?

So instead of having hundreds of management ioctls under
CAP_SYS_ADMIN, we'd now have hundreds of non-storage ioctls under
CAP_SYS_ADMIN and hundreds of storage ioctls under
CAP_SYS_STORAGE_ADMIN?

Maybe I'm missing something, but I don't see how that improves the
situation w.r.t. locked down LSM configurations?

Cheers,

Dave.


-- 
Dave Chinner
david@fromorbit.com

Re: Leaking Path in XFS's ioctl interface(missing LSM check)

[James Morris]

On Tue, 2 Oct 2018, Dave Chinner wrote:

> On Tue, Oct 02, 2018 at 06:08:16AM +1000, James Morris wrote:
> > On Mon, 1 Oct 2018, Darrick J. Wong wrote:
> > 
> > > If we /did/ replace CAP_SYS_ADMIN checking with a pile of LSM hooks,
> > 
> > Not sure we'd need a pile of hooks, what about just "read" and "write" 
> > storage admin?
> > 
> > Or even two new capabilities along these lines, which we convert existing 
> > CAP_SYS_ADMIN etc. to?
> 
> So instead of having hundreds of management ioctls under
> CAP_SYS_ADMIN, we'd now have hundreds of non-storage ioctls under
> CAP_SYS_ADMIN and hundreds of storage ioctls under
> CAP_SYS_STORAGE_ADMIN?
> 
> Maybe I'm missing something, but I don't see how that improves the
> situation w.r.t. locked down LSM configurations?

I'm not sure about capabilities, but having two specific LSM hooks for 
storage admin would allow SELinux et al to explicitly control privileged 
access to these interfaces.  Storage admin seems to be a special case of 
its own which we want to be able to mediate as such.



-- 
James Morris
<jmorris@namei.org>

Re: Leaking Path in XFS's ioctl interface(missing LSM check)

[Dave Chinner]

On Wed, Oct 03, 2018 at 05:20:31AM +1000, James Morris wrote:
> On Tue, 2 Oct 2018, Dave Chinner wrote:
> 
> > On Tue, Oct 02, 2018 at 06:08:16AM +1000, James Morris wrote:
> > > On Mon, 1 Oct 2018, Darrick J. Wong wrote:
> > > 
> > > > If we /did/ replace CAP_SYS_ADMIN checking with a pile of LSM hooks,
> > > 
> > > Not sure we'd need a pile of hooks, what about just "read" and "write" 
> > > storage admin?
> > > 
> > > Or even two new capabilities along these lines, which we convert existing 
> > > CAP_SYS_ADMIN etc. to?
> > 
> > So instead of having hundreds of management ioctls under
> > CAP_SYS_ADMIN, we'd now have hundreds of non-storage ioctls under
> > CAP_SYS_ADMIN and hundreds of storage ioctls under
> > CAP_SYS_STORAGE_ADMIN?
> > 
> > Maybe I'm missing something, but I don't see how that improves the
> > situation w.r.t. locked down LSM configurations?
> 
> I'm not sure about capabilities, but having two specific LSM hooks for 
> storage admin would allow SELinux et al to explicitly control privileged 
> access to these interfaces.  Storage admin seems to be a special case of 
> its own which we want to be able to mediate as such.

Perhaps so - as a stepping stone it would allow isolation in
specific cases where no management should be allowed, but there are
cases with modern filesystems where users need access to storage
APIs.

e.g. It's entirely plausible that /home is set up as a subvolume per
user, and that subvols in a fileystem can be independently
snapshotted. Hence it would be completely acceptible to allow users
to have access to snapshot management APIs to be able to snapshot
their home directories for backup/rollback purposes.

Hence I'm not sure that black/white storage admin LSM hooks are a
solution that will end up being particularly useful to the wider
population...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com