Re: Missing security_inode_readlink() in xfs_file_ioctl()

Re: Missing security_inode_readlink() in xfs_file_ioctl()

[Dan Carpenter]

Hi XFS devs,

We received this email on security@kernel.org.  This is under
CAP_SYS_ADMIN, but it maybe should also check with selinux?

regards,
dan carpenter

On Thu, Aug 09, 2018 at 05:59:50PM -0700, TongZhang wrote:
> [1.] One line summary of the problem:
> 
> Possible missing security_inode_readlink() in xfs_file_ioctl()
> 
> [2.] Full description of the problem/report:
> 
> 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.
> 
> 
> [3.] Keywords: LSM check
> [4.] Kernel information
> [4.1] Kernel Version: 4.14.61
> 
> 
> - Tong

Re: Missing security_inode_readlink() in xfs_file_ioctl()

[Darrick J. Wong]

On Fri, Aug 10, 2018 at 12:22:29PM +0300, Dan Carpenter wrote:
> Hi XFS devs,
> 
> We received this email on security@kernel.org.  This is under
> CAP_SYS_ADMIN, but it maybe should also check with selinux?

Hmm, so the point of adding a security_inode_readlink call would be to
restrict userland access xfs_readlink_by_handle further in case the
system has a policy whereby even possessing CAP_SYS_ADMIN is not by
itself sufficient to be able to read a symlink?

IOWs, are there security policies where CAP_SYS_ADMIN isn't a "get
access to everything" wildcard?  I imagine the answer is "yes" and
therefore xfs needs the call, but I thought I'd ask first.

--D

> regards,
> dan carpenter
> 
> On Thu, Aug 09, 2018 at 05:59:50PM -0700, TongZhang wrote:
> > [1.] One line summary of the problem:
> > 
> > Possible missing security_inode_readlink() in xfs_file_ioctl()
> > 
> > [2.] Full description of the problem/report:
> > 
> > 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.
> > 
> > 
> > [3.] Keywords: LSM check
> > [4.] Kernel information
> > [4.1] Kernel Version: 4.14.61
> > 
> > 
> > - Tong

Re: Missing security_inode_readlink() in xfs_file_ioctl()

[Linus Torvalds]

On Fri, Aug 10, 2018 at 9:09 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> IOWs, are there security policies where CAP_SYS_ADMIN isn't a "get
> access to everything" wildcard?  I imagine the answer is "yes" and
> therefore xfs needs the call, but I thought I'd ask first.

I think the answer is "no", at least for filesystems where it allows
you to just remount the filesystem entirely.

These reports are from some automated logic that doesn't take "some
capabilities are more equal than others" into account.

                 Linus

Re: Missing security_inode_readlink() in xfs_file_ioctl()

[Dan Carpenter]

On Fri, Aug 10, 2018 at 09:09:31AM -0700, Darrick J. Wong wrote:
> On Fri, Aug 10, 2018 at 12:22:29PM +0300, Dan Carpenter wrote:
> > Hi XFS devs,
> > 
> > We received this email on security@kernel.org.  This is under
> > CAP_SYS_ADMIN, but it maybe should also check with selinux?
> 
> Hmm, so the point of adding a security_inode_readlink call would be to
> restrict userland access xfs_readlink_by_handle further in case the
> system has a policy whereby even possessing CAP_SYS_ADMIN is not by
> itself sufficient to be able to read a symlink?
> 
> IOWs, are there security policies where CAP_SYS_ADMIN isn't a "get
> access to everything" wildcard?  I imagine the answer is "yes" and
> therefore xfs needs the call, but I thought I'd ask first.
>

Yeah...  Forget about it.  I pushed this out to you without really
thinking about it, just to get it off my todo list and that wasn't the
right thing.

regards,
dan carpenter

Re: Missing security_inode_readlink() in xfs_file_ioctl()

[Carlos Maiolino]

On Fri, Aug 10, 2018 at 10:02:17PM +0300, Dan Carpenter wrote:
> On Fri, Aug 10, 2018 at 09:09:31AM -0700, Darrick J. Wong wrote:
> > On Fri, Aug 10, 2018 at 12:22:29PM +0300, Dan Carpenter wrote:
> > > Hi XFS devs,
> > > 
> > > We received this email on security@kernel.org.  This is under
> > > CAP_SYS_ADMIN, but it maybe should also check with selinux?
> > 
> > Hmm, so the point of adding a security_inode_readlink call would be to
> > restrict userland access xfs_readlink_by_handle further in case the
> > system has a policy whereby even possessing CAP_SYS_ADMIN is not by
> > itself sufficient to be able to read a symlink?
> > 
> > IOWs, are there security policies where CAP_SYS_ADMIN isn't a "get
> > access to everything" wildcard?  I imagine the answer is "yes" and
> > therefore xfs needs the call, but I thought I'd ask first.
> >
> 
> Yeah...  Forget about it.  I pushed this out to you without really
> thinking about it, just to get it off my todo list and that wasn't the
> right thing.
> 

Just thought it was worth to mention...

A long time ago, I've seen implementations where the system administration was
split between a sys admin and a 'security admin', where the security admin
removed some root permissions, and so, some very specific tasks could only be
done by the security admin.
All these were enforced by selinux.
Although, I don't see such implementations in ages, I still think they are out
there.

Cheers

> regards,
> dan carpenter

-- 
Carlos

Re: Missing security_inode_readlink() in xfs_file_ioctl()

[Dan Carpenter]

On Mon, Aug 13, 2018 at 09:35:28AM +0200, Carlos Maiolino wrote:
> On Fri, Aug 10, 2018 at 10:02:17PM +0300, Dan Carpenter wrote:
> > On Fri, Aug 10, 2018 at 09:09:31AM -0700, Darrick J. Wong wrote:
> > > On Fri, Aug 10, 2018 at 12:22:29PM +0300, Dan Carpenter wrote:
> > > > Hi XFS devs,
> > > > 
> > > > We received this email on security@kernel.org.  This is under
> > > > CAP_SYS_ADMIN, but it maybe should also check with selinux?
> > > 
> > > Hmm, so the point of adding a security_inode_readlink call would be to
> > > restrict userland access xfs_readlink_by_handle further in case the
> > > system has a policy whereby even possessing CAP_SYS_ADMIN is not by
> > > itself sufficient to be able to read a symlink?
> > > 
> > > IOWs, are there security policies where CAP_SYS_ADMIN isn't a "get
> > > access to everything" wildcard?  I imagine the answer is "yes" and
> > > therefore xfs needs the call, but I thought I'd ask first.
> > >
> > 
> > Yeah...  Forget about it.  I pushed this out to you without really
> > thinking about it, just to get it off my todo list and that wasn't the
> > right thing.
> > 
> 
> Just thought it was worth to mention...
> 
> A long time ago, I've seen implementations where the system administration was
> split between a sys admin and a 'security admin', where the security admin
> removed some root permissions, and so, some very specific tasks could only be
> done by the security admin.
> All these were enforced by selinux.
> Although, I don't see such implementations in ages, I still think they are out
> there.

That seems like a crazy thing.  Sys admin can write to firmware, so you
can't really separate root from sys admin.  There are so many other
things that you can do with sys admin.

Another possibility is that maybe the NSA uses selinux for logging, to
try out what files Snowden accessed.  But it's sort of weird for us to
try support some application which might not exist.  TongZhang, try
emailing these things directly to the selinux devs.
selinux@tycho.nsa.gov  They're really the best qualified to talk about
missing LSM checks.

regards,
dan carpenter

Re: Missing security_inode_readlink() in xfs_file_ioctl()

[Dave Chinner]

On Mon, Aug 13, 2018 at 11:30:48AM +0300, Dan Carpenter wrote:
> On Mon, Aug 13, 2018 at 09:35:28AM +0200, Carlos Maiolino wrote:
> > On Fri, Aug 10, 2018 at 10:02:17PM +0300, Dan Carpenter wrote:
> > > On Fri, Aug 10, 2018 at 09:09:31AM -0700, Darrick J. Wong wrote:
> > > > On Fri, Aug 10, 2018 at 12:22:29PM +0300, Dan Carpenter wrote:
> > > > > Hi XFS devs,
> > > > > 
> > > > > We received this email on security@kernel.org.  This is under
> > > > > CAP_SYS_ADMIN, but it maybe should also check with selinux?
> > > > 
> > > > Hmm, so the point of adding a security_inode_readlink call would be to
> > > > restrict userland access xfs_readlink_by_handle further in case the
> > > > system has a policy whereby even possessing CAP_SYS_ADMIN is not by
> > > > itself sufficient to be able to read a symlink?
> > > > 
> > > > IOWs, are there security policies where CAP_SYS_ADMIN isn't a "get
> > > > access to everything" wildcard?  I imagine the answer is "yes" and
> > > > therefore xfs needs the call, but I thought I'd ask first.
> > > >
> > > 
> > > Yeah...  Forget about it.  I pushed this out to you without really
> > > thinking about it, just to get it off my todo list and that wasn't the
> > > right thing.
> > > 
> > 
> > Just thought it was worth to mention...
> > 
> > A long time ago, I've seen implementations where the system administration was
> > split between a sys admin and a 'security admin', where the security admin
> > removed some root permissions, and so, some very specific tasks could only be
> > done by the security admin.
> > All these were enforced by selinux.
> > Although, I don't see such implementations in ages, I still think they are out
> > there.
> 
> That seems like a crazy thing.  Sys admin can write to firmware, so you
> can't really separate root from sys admin.  There are so many other
> things that you can do with sys admin.
> 
> Another possibility is that maybe the NSA uses selinux for logging, to
> try out what files Snowden accessed.  But it's sort of weird for us to
> try support some application which might not exist.  TongZhang, try
> emailing these things directly to the selinux devs.
> selinux@tycho.nsa.gov  They're really the best qualified to talk about
> missing LSM checks.

I suspect the the piece of the puzzle that is missing here is that
this is a private XFS _filehandle_ interface. This means it is used
to directly access inodes *without permission checks* by highly
trusted applications that are tightly integrated into filesystem
operation.  e.g. HSMs, userspace file servers, online defrag tools,
backup/restore utilities, etc.

These applications are subject to very different access rules to
"normal" users - they are effectively considered an extension of the
kernel filesystem implementation as the filesystem may not function
correctly without those applications being able to do what they do
without interference. Hence the CAP_SYS_ADMIN check and the complete
absence of any other type of permission check.....

IOWs, denying inode access through these interfaces based on LSM
policy checks will almost certainly cause the applications that use
the private XFS filehandle interfaces to misbehave in unpredictable
ways.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com