> On Aug 20, 2024, at 5:45 AM, Mickaël Salaün <mic@xxxxxxxxxxx> wrote: > > On Mon, Aug 19, 2024 at 08:35:53PM +0000, Song Liu wrote: >> Hi Mickaël, >> >>> On Aug 19, 2024, at 6:12 AM, Mickaël Salaün <mic@xxxxxxxxxxx> 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? > > 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
