Message-ID: <370A8DB0-5636-4365-8CAC-EF35F196B86F@fb.com>
Date: Mon, 19 Aug 2024 20:35:53 +0000
From: Song Liu <songliubraving@...a.com>
To: Mickaël Salaün <mic@...ikod.net>
CC: Christian Brauner <brauner@...nel.org>,
        Song Liu
	<songliubraving@...a.com>, Song Liu <song@...nel.org>,
        bpf
	<bpf@...r.kernel.org>,
        Linux-Fsdevel <linux-fsdevel@...r.kernel.org>,
        LKML
	<linux-kernel@...r.kernel.org>,
        Kernel Team <kernel-team@...a.com>,
        "andrii@...nel.org" <andrii@...nel.org>,
        "eddyz87@...il.com"
	<eddyz87@...il.com>,
        "ast@...nel.org" <ast@...nel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "martin.lau@...ux.dev"
	<martin.lau@...ux.dev>,
        "viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>,
        "jack@...e.cz" <jack@...e.cz>,
        "kpsingh@...nel.org" <kpsingh@...nel.org>,
        "mattbobrowski@...gle.com" <mattbobrowski@...gle.com>,
        Liam Wisehart
	<liamwisehart@...a.com>, Liang Tang <lltang@...a.com>,
        Shankaran
 Gnanashanmugam <shankaran@...a.com>,
        LSM List
	<linux-security-module@...r.kernel.org>,
        Günther Noack
	<gnoack@...gle.com>
Subject: Re: [PATCH bpf-next 2/2] selftests/bpf: Add tests for
 bpf_get_dentry_xattr

Hi Mickaël, 

> On Aug 19, 2024, at 6:12 AM, Mickaël Salaün <mic@...ikod.net> wrote:

[...]

>> But because landlock works with a deny-by-default security policy this
>> is ok and it takes overmounts into account etc.
> 
> Correct. Another point is that Landlock uses the file's path (i.e.
> dentry + mnt) to walk down to the parent.  Only using the dentry would
> be incorrect for most use cases (i.e. any system with more than one
> mount point).

Thanks for highlighting the difference. Let me see whether we can bridge
the gap for this set. 

[...]

>>> 
>>> 1. Change security_inode_permission to take dentry instead of inode.
>> 
>> Sorry, no.
>> 
>>> 2. Still add bpf_dget_parent. We will use it with security_inode_permission
>>>   so that we can propagate flags from parents to children. We will need
>>>   a bpf_dput as well. 
>>> 3. There are pros and cons with different approaches to implement this
>>>   policy (tags on directory work for all files in it). We probably need 
>>>   the policy writer to decide with one to use. From BPF's POV, dget_parent
>>>   is "safe", because it won't crash the system. It may encourage some bad
>>>   patterns, but it appears to be required in some use cases.
>> 
>> You cannot just walk a path upwards and check permissions and assume
>> that this is safe unless you have a clear idea what makes it safe in
>> this scenario. Landlock has afaict. But so far you only have a vague
>> sketch of checking permissions walking upwards and retrieving xattrs
>> without any notion of the problems involved.
> 
> Something to keep in mind is that relying on xattr to label files
> requires to deny sanboxed processes to change this xattr, otherwise it
> would be trivial to bypass such a sandbox.  Sandboxing must be though as
> a whole and Landlock's design for file system access control takes into
> account all kind of file system operations that could bypass a sandbox
> policy (e.g. mount operations), and also protects from impersonations.

Thanks for sharing these experiences! 

> What is the use case for this patch series?  Couldn't Landlock be used
> for that?

We have multiple use cases. We can use Landlock for some of them. The 
primary goal of this patchset is to add useful building blocks to BPF LSM
so that we can build effective and flexible security policies for various
use cases. These building blocks alone won't be very useful. For example,
as you pointed out, to make xattr labels useful, we need some policies 
for xattr read/write.

Does this make sense?

Thanks,
Song

Powered by blists - more mailing lists