On Aug 20, 2024, at 5:45 AM, Mickaël Salaün [off-list ref] wrote:

On Mon, Aug 19, 2024 at 08:35:53PM +0000, Song Liu wrote:

quoted

Hi Mickaël, 

quoted

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

[...]

quoted

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

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! 

quoted

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?

Yes, but I think you'll end up with a code pretty close to the Landlock
implementation.

At the moment, I think it is not possible to do full Landlock logic in
BPF. We are learning from other LSMs. 

What about adding BPF hooks to Landlock?  User space could create
Landlock sandboxes that would delegate the denials to a BPF program,
which could then also allow such access, but without directly handling
nor reimplementing filesystem path walks.  The Landlock user space ABI
changes would mainly be a new landlock_ruleset_attr field to explicitly
ask for a (system-wide) BPF program to handle access requests if no
Landlock rule allow them.  We could also tie a BPF data (i.e. blob) to
Landlock domains for consistent sandbox management.  One of the
advantage of this approach is to only run related BPF programs if the
sandbox policy would deny the request.  Another advantage would be to
leverage the Landlock user space interface to let any program partially
define and extend their security policy.

Given there is BPF LSM, I have never thought about adding BPF hooks to 
Landlock or other LSMs. I personally would prefer to have a common API
to walk the path, maybe something like vma_iterator. But I need to read
more code to understand whether this makes sense?

Thanks,
Song

I'm working on implementing audit support for Landlock [1] and I think
these changes could be useful to implement BPF hooks to run a dedicated
BPF program type per event (see landlock_log_denial() and struct
landlock_request).  I'll get back on this patch series in September.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git/log/?h=wip-audit