Re: Commit 13c164b1a186 - regression for LSMs/SELinux?

On Tue, Sep 22, 2020 at 3:34 AM Ondrej Mosnacek [off-list ref] wrote:

On Mon, Sep 21, 2020 at 6:30 PM Al Viro [off-list ref] wrote:

quoted

On Mon, Sep 21, 2020 at 06:09:22PM +0200, Christoph Hellwig wrote:

quoted

[adding Linus and Al]

On Mon, Sep 21, 2020 at 04:51:35PM +0200, Ondrej Mosnacek wrote:

quoted

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.

I'd like Stephen to weigh in on this, but it looks he might be on
vacation right now...

No, just wasn't cc'd previously.  I don't think we want any LSM check
here.  As the long term fix, I would suggest moving the
security_file_permission() call up from rw_verify_area() to the
callers (and not call it from kernel_write() at all).

quoted

quoted

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

Yes, controlling pipe writes is just one part of controlling
information flow between processes but the intent here is to control
userspace actions, not kernel-internal operations.