Re: [PATCH -next 0/5] landlock: add chmod and chown support

[Mickaël Salaün]

On 22/08/2022 23:53, Casey Schaufler wrote:
> On 8/22/2022 2:21 PM, Mickaël Salaün wrote:
> > On 22/08/2022 23:18, Günther Noack wrote:
> > > On Mon, Aug 22, 2022 at 12:35:18PM -0700, Casey Schaufler wrote:
> > > > On 8/22/2022 12:17 PM, Günther Noack wrote:
> > > > > Hi!
> > > > >
> > > > > Very exciting to see! Thank you for sending this! :)
> > > > >
> > > > > I'm just throwing in some comments based on the very similar truncate
> > > > > patch set, in the hope that it helps. (But obviously, Mickaël Salaün
> > > > > has the last word on this code.)
> > > > >
> > > > > Slightly higher level question: Should we start to group the
> > > > > functionality of multiple LSM hooks under one Landlock flag? (Will it
> > > > > be harder to change the LSM hook interface in the future if we
> > > > > continue to add one flag per hook? Or is this structure already
> > > > > exposed to userspace by other LSMs?)
> > > >
> > > > I'm not a landlock expert. The question is nonsensical, yet somewhat
> > > > frightening nonetheless. Could you put just a touch more context into
> > > > what you're asking for?
> > >
> > > By "Landlock flags", I meant the integer that Landlock uses to
> > > represent the set of possible operations on a file hierarchy:
> > >
> > > Landlock's file system access rights (access_mode_t on the kernel
> > > side) are defined with an integer with flags (LANDLOCK_ACCESS_FS_*)
> > > for different operations that one might do with files. They get used
> > > from userspace to control what is permitted on which parts of the file
> > > system. (Docs: https://docs.kernel.org/userspace-api/landlock.html)
> > >
> > > Currently most of the available Landlock flags map pretty closely to
> > > one of the file- and path-related LSM hooks. (See various hook
> > > implementations in security/landlock/fs.c)
> > >
> > > The file system operations that Landlock doesn't cover yet (as of
> > > kernel 5.19) are listed below, and there are potentially a few more
> > > that might be missing. I suspect/hope that there will be more patches
> > > in the style of the truncate/chmod/chown patches, which will add that
> > > coverage.
> > >
> > > The question is basically:
> > > When these patches get added, how should the userspace-exposed
> > > Landlock file system access rights map to the LSM hooks for these
> > > upcoming Landlock features? Should each of the newly covered
> > > operations have its own flag, or is it better to group them?
> > >
> > > (It's well possible that the right answer is "one flag per feature",
> > > but I feel it still makes sense to ask this before all these patches
> > > get written?)
> >
> > Landlock is not strictly tied to the current LSM hooks, but they fit
> > well (because they are designed to be flexible enough to be use by
> > multiple access control systems). In fact, Landlock already uses
> > orthogonal access rights such as LANDLOCK_ACCESS_FS_REFER (using the
> > path_link or path_rename hooks), LANDLOCK_ACCESS_FS_MAKE_* (using the
> > path_mknod and path_mkdir hooks)…
> >
> > Anyway, the LSM framework is evolving, we can add new hooks and modify
> > others (e.g. see the security_path_rename hook modification for
> > FS_REFER) as long as mainline access control systems don't break and
> > subsystem maintainers are OK with such changes. Like any kernel API,
> > the LSM API is not stable, but this is not an issue for mainline code.
> >
> > Landlock's goal is to find the sweet spot between flexibility for
> > different sandboxing use cases and an understandable/simple-enough
> > access control system. The access rights should then be meaningful for
> > users, which are already familiar with the UAPI/syscalls, hence the
> > current Landlock access rights (which are not very original, and that
> > is a good thing). This is why I'm wondering if it is worth it to
> > differentiate between chmod and chgrp (and add a dedicated access
> > right per action or only one for both).
>
> The lesson from capabilities is that differentiating between chmod, chown and chgrp is
> pointless, and CAP_DAC_CHMOD, CAP_DAC_CHOWN and CAP_DAC_CHGRP should have just been
> CAP_DAC_OVERRIDE. On the other hand, those who argue that SELinux proves the value of
> fine granularity would likely have you go with separate rights. What's important is
> that you don't tie your rights too tightly to the underlying implementation. That has
> the potential to expose details of how the code work that user-space has no business
> basing decisions on.

Indeed, for a sandboxing feature like Landlock, it may not be useful to 
duplicate other access rights. From a user point of view, I think it 
would make sense to split the file metadata modification into 
potentially-security related or not. That would means three access rights:
- modify user/informative metadata (e.g. dates, user.* xattr);
- modify security-related metadata (e.g. chown, chmod, chgrp, any other 
xattr);
- read any metadata.

This require some LSM hook changes to handle paths instead of inodes 
(e.g. security_inode_setattr, security_inode_setxattr…).