Commit 13c164b1a186 - regression for LSMs/SELinux?

[Ian Kent]

On Tue, 2020-09-22 at 09:33 +0200, Ondrej Mosnacek wrote:
> On Mon, Sep 21, 2020 at 6:30 PM Al Viro <viro at zeniv.linux.org.uk>
> wrote:
> > On Mon, Sep 21, 2020 at 06:09:22PM +0200, Christoph Hellwig wrote:
> > > [adding Linus and Al]
> > > 
> > > On Mon, Sep 21, 2020 at 04:51:35PM +0200, Ondrej Mosnacek wrote:
> > > > Hi folks,
> > > > 
> > > > It seems that after commit 13c164b1a186 ("autofs: switch to
> > > > kernel_write") there is now an extra LSM permission required
> > > > (for the
> > > > current task to write to the automount pipe) for processes
> > > > accessing
> > > > some yet-to-to-be mounted directory on which an autofs mount is
> > > > set
> > > > up. The call chain is:
> > > > [...]
> > > > autofs_wait() ->
> > > > autofs_notify_daemon() ->
> > > > autofs_write() ->
> > > > kernel_write() ->
> > > > rw_verify_area() ->
> > > > security_file_permission()
> > > > 
> > > > The bug report that led me to this commit is at [1].
> > > > 
> > > > Technically, this is a regression for LSM users, since this is
> > > > a
> > > > kernel-internal operation and an LSM permission for the current
> > > > task
> > > > shouldn't be required. Can this patch be reverted? Perhaps
> > > > __kernel_{read|write}() could instead be renamed to
> > > > kernel_*_nocheck()
> > > > so that the name is more descriptive?
> > > 
> > > So we obviously should not break existing user space and need to
> > > fix
> > > this ASAP.  The trivial "fix" would be to export __kernel_write
> > > again
> > > and switch autofs to use it.  The other option would be a FMODE
> > > flag
> > > to bypass security checks, only to be set if the callers ensures
> > > they've been valided (i.e. in autofs_prepare_pipe).
> 
> IMHO that sounds like an overkill in this scenario. I don't think it
> makes sense to do the LSM check here (or at least not against the
> current task's creds), because it is not the current task that wants
> to communicate with the daemon, it just wants to to access some
> directory on the system that just happens to be special to the
> kernel,
> which needs to do some communication on the side to service this
> request. So if we do want to do any LSM check here, there should at
> least be some "bool internal" flag passed to the LSM, signalizing
> that
> this is an internal read/write operation that wasn't directly
> initiated/requested by the current process. SELinux could then either
> use the kernel secid instead of the current task's secid or skip the
> check completely in such case.

Perhaps, but see below.

> 
> I'd like Stephen to weigh in on this, but it looks he might be on
> vacation right now...
> 
> > > Any opinions?
> > 
> > Reexport for now.  Incidentally, what is LSM doing rejecting writes
> > into a pipe?
> 
> With SELinux at least, what is allowed or denied is defined in the
> policy. And the policy usually defaults to everything denied and then
> you add rules to allow what needs (and makes sense) to be allowed.
> Since until kernel 5.8 random processes didn't need to write to pipes
> created by the automount daemon, it has never been explicitly allowed
> and so the automounting now fails. It is in no way obvious that all
> processes should have the permission to talk to the automount daemon
> just to traverse the filesystem...

I think you might have misunderstood what lead to this, just a bit.

Previously the __kern_write() function was used for this communication
and Christoph's patch changed that to use kern_write() instead.

In theory that's a good idea because kern_write() adds some additional
sanity checks, one being a call to rw_verify_area() which is where the
security_file_permission() call fails.

So previously any random process could avoid these checks by calling
__kern_write() so the change to kern_write() is, in theory, that's a
good thing and simply reverting that hunk in Christoph's patch
probably isn't the best thing to do.

But any random process does need to be able to write to the automount
daemon pipe for trailing path components and the root dentry of autofs
mounts, depending on case.

So it's true that any write to any autofs dentry probably doesn't
need to be allowed but I question what that gets us in terms of
security improvement over allowing pipe writes for automount_t
labelled pipes in selinux policy since they must be within an autofs
mounted file system.

But Stephen has a different recommendation (and that appears to
consider the cause I outlined above) so I'll wait to see what others
think about the recommendations.

Ian