linux-kernel - Re: [PATCH] security: selinux: allow per-file labeling for bpffs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 12 Feb 2020 13:09:04 -0500
From:   Stephen Smalley <sds@...ho.nsa.gov>
To:     Steven Moreland <smoreland@...gle.com>,
        Paul Moore <paul@...l-moore.com>
Cc:     Colin Cross <ccross@...roid.com>,
        "Connor O'Brien" <connoro@...gle.com>, kernel-team@...roid.com,
        Eric Paris <eparis@...isplace.org>,
        Kees Cook <keescook@...omium.org>, anton@...msg.org,
        tony.luck@...el.com, selinux@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] security: selinux: allow per-file labeling for bpffs

On 2/12/20 12:46 PM, Steven Moreland wrote:
> And I strongly encourage our downstream in the same way :) I try, I
> try. However, I am a n00b here (thanks for merging "my" first linux
> patch).
> 
> Looking at this code, I was wondering, why isn't SELinux labelling
> completely orthogonal to the fs type? Is this a measurable
> memory/performance thing?

If you just mean why don't we turn on SE_SBGENFS for all filesystem 
types, that's discussed in
https://github.com/SELinuxProject/selinux-kernel/issues/2

It isn't always safe so we have been whitelisting the filesystem types 
that are supported.

More generally, labeling in SELinux goes beyond just GENFS; there are 
the SECURITY_FS_USE_* filesystem labeling behaviors defined by policy 
and those are also based on fstype, just not hardcoded in the kernel.

> 
> 
> On Tue, Feb 11, 2020 at 7:17 PM Paul Moore <paul@...l-moore.com> wrote:
>>
>> On Thu, Feb 6, 2020 at 1:12 PM Stephen Smalley <sds@...ho.nsa.gov> wrote:
>>> On 2/6/20 12:41 PM, Steven Moreland wrote:
>>>> On Thu, Feb 6, 2020 at 9:35 AM Stephen Smalley <sds@...ho.nsa.gov> wrote:
>>>>>
>>>>> On 2/6/20 12:21 PM, Stephen Smalley wrote:
>>>>>> On 2/6/20 11:55 AM, Steven Moreland wrote:
>>>>>>> From: Connor O'Brien <connoro@...gle.com>
>>>>>>>
>>>>>>> Add support for genfscon per-file labeling of bpffs files. This allows
>>>>>>> for separate permissions for different pinned bpf objects, which may
>>>>>>> be completely unrelated to each other.
>>>>>>
>>>>>> Do you want bpf fs to also support userspace labeling of files via
>>>>>> setxattr()?  If so, you'll want to also add it to
>>>>>> selinux_is_genfs_special_handling() as well.
>>>>>>
>>>>
>>>> Android doesn't currently have this use case.
>>>>
>>>>>> The only caveat I would note here is that it appears that bpf fs
>>>>>> supports rename, link, unlink, rmdir etc by userspace, which means that
>>>>>> name-based labeling via genfscon isn't necessarily safe/stable.  See
>>>>>> https://github.com/SELinuxProject/selinux-kernel/issues/2
>>>>>>
>>>>
>>>> Android restricts ownership of these files to a single process (bpfloader) and
>>>> so this isn't a concern in our architecture. Is it a concern in general?
>>>
>>> I guess if the inodes are pinned in memory, then only the original name
>>> under which the file is created will be relevant to determining the
>>> label and subsequent rename/link operations won't have any effect. So as
>>> long as the bpfloader creates the files with the same names being
>>> specified in policy, that should line up and be stable for the lifecycle
>>> of the inode.
>>>
>>> The alternative model is to have bpfloader look up a context from the
>>> userspace file_contexts configuration via selabel_lookup(3) and friends,
>>> and set it on the file explicitly.  That's what e.g. ueventd does for
>>> device nodes.  However, one difference here is that you could currently
>>> only do this via setxattr()/setfilecon() after creating the file so that
>>> the file would temporarily exist in the default label for bpf fs, if
>>> that matters.  ueventd can instead use setfscreatecon(3) before creating
>>> the file so that it is originally created in the right label but that
>>> requires the filesystem to call security_inode_init_security() from its
>>> function that originally creates the inode, which tmpfs/devtmpfs does
>>> but bpf does not.  So you'd have to add that to the bpf filesystem code
>>> if you wanted to support setfscreatecon(3) on it.
>>
>> Considering the relative maturity of bpf, and bpffs, I think it's okay
>> to take this small step right now, with the understanding that more
>> work may need to be done, depending on how this is generally adopted
>> by distros and users (for those of you not following the other thread,
>> I've merged the v3 draft of this patch).
>>
>> However, I've been noticing a trend from the Android folks of tossing
>> patches over the wall without much thought beyond the Android use
>> case.  I understand the Android devs have a job to do, and products to
>> focus on, but I would strongly encourage them to think a bit longer
>> about more general use cases before submitting patches upstream.
>>
>> --
>> paul moore
>> www.paul-moore.com