Hi Mickaël, 

On Aug 19, 2024, at 6:12 AM, Mickaël Salaün [off-list ref] wrote:

[...]

quoted

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. 

[...]

quoted

quoted

1. Change security_inode_permission to take dentry instead of inode.

Sorry, no.

quoted

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